allow dockerspecs on service restart#2119
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
/run-security-scan |
alexcos20
left a comment
alexcos20
left a comment
There was a problem hiding this comment.
AI automated code review (Gemini 3).
Overall risk: low
Summary:
This PR refactors the serviceRestart method to support updating Docker specifications during a service restart. It introduces ServiceContainerSpec to share fields between start and restart operations. The providers (HttpProvider and P2pProvider) are updated to handle the new params object and properly conditionally-spread properties into the payload. The implementation is clean, though it introduces a breaking API change for library consumers calling serviceRestart directly.
Comments:
• [INFO][style] Excellent refactoring here. Extracting a shared ServiceContainerSpec interface reduces duplication and clearly establishes the common container attributes used by both start and restart actions.
• [WARNING][other] Changing the method signature from positional arguments (userData, dockerCmd, dockerEntrypoint) to a params object constitutes a breaking change in the public API. Ensure this is highlighted in the changelog/release notes so that consumers of ocean.js can update their implementations accordingly.
• [INFO][style] Good use of the conditional spread syntax (...(image !== undefined ? { image } : {})). This guarantees that keys are entirely omitted from the payload when undefined, cleanly satisfying the node's strict 'omitted means reuse' semantics. LGTM!
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/@types/Services.ts (1)
155-184: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDocument the public restart contract consistently. The new breaking restart API needs generated-API-friendly JSDoc that states required versus optional inputs across its shared type and both provider entry points.
src/@types/Services.ts#L155-L184: use TSDoc for the exported container, restart, and start contracts.src/services/providers/BaseProvider.ts#L859-L865: document the consolidatedparamsargument and optional abort signal.src/services/providers/HttpProvider.ts#L2126-L2145: add explicit@paramentries for required and optional arguments.src/services/providers/P2pProvider.ts#L1866-L1872: add matching JSDoc and REUSE/RESPEC semantics.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/`@types/Services.ts around lines 155 - 184, The public restart API lacks consistent generated-API-friendly TSDoc. In src/@types/Services.ts:155-184, document ServiceContainerSpec, ServiceRestartParams, and ServiceStartParams with required versus optional fields and REUSE/RESPEC semantics; in src/services/providers/BaseProvider.ts:859-865, document the consolidated params argument and optional abort signal; in src/services/providers/HttpProvider.ts:2126-2145 and src/services/providers/P2pProvider.ts:1866-1872, add matching JSDoc with explicit `@param` entries for required and optional restart arguments, including REUSE/RESPEC behavior.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/`@types/Services.ts:
- Around line 169-177: Update ServiceRestartParams to model REUSE and RESPEC as
a discriminated union: REUSE must allow no container-spec override fields, while
RESPEC requires image and exactly one of tag, checksum, or dockerfile, with
additionalDockerFiles only valid in RESPEC. Add runtime validation at the
serviceRestart dispatch boundary so untyped JavaScript inputs matching invalid
combinations are rejected before being forwarded to the node.
---
Nitpick comments:
In `@src/`@types/Services.ts:
- Around line 155-184: The public restart API lacks consistent
generated-API-friendly TSDoc. In src/@types/Services.ts:155-184, document
ServiceContainerSpec, ServiceRestartParams, and ServiceStartParams with required
versus optional fields and REUSE/RESPEC semantics; in
src/services/providers/BaseProvider.ts:859-865, document the consolidated params
argument and optional abort signal; in
src/services/providers/HttpProvider.ts:2126-2145 and
src/services/providers/P2pProvider.ts:1866-1872, add matching JSDoc with
explicit `@param` entries for required and optional restart arguments, including
REUSE/RESPEC behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: abe7ed2f-3b7c-49a9-9021-3e058eec169e
📒 Files selected for processing (5)
src/@types/Services.tssrc/services/providers/BaseProvider.tssrc/services/providers/HttpProvider.tssrc/services/providers/P2pProvider.tstest/integration/Services.test.ts
💤 Files with no reviewable changes (1)
- test/integration/Services.test.ts
| // Optional container-spec overrides for serviceRestart. The node treats the restart | ||
| // atomically ("all-or-nothing"): providing ANY of image/tag/checksum/dockerfile/ | ||
| // additionalDockerFiles switches it into RESPEC mode — the container is rebuilt entirely | ||
| // from these values (`image` becomes mandatory, and exactly one of tag/checksum/dockerfile | ||
| // applies) instead of being bounced on the stored spec (REUSE mode). Passing none of them | ||
| // reuses the stored container spec unchanged. `userData`/`dockerCmd`/`dockerEntrypoint` | ||
| // keep their replace-when-supplied semantics: an omitted value reuses the stored one, an | ||
| // explicit value (including `[]`) REPLACES it. | ||
| export type ServiceRestartParams = ServiceContainerSpec |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Make invalid RESPEC combinations unrepresentable.
ServiceRestartParams = ServiceContainerSpec accepts { tag }, { additionalDockerFiles }, or multiple source selectors without image. The documented protocol requires RESPEC requests to include image and exactly one of tag/checksum/dockerfile; both providers forward these invalid shapes to the node. Model REUSE and RESPEC as distinct union members and validate untyped JS inputs before dispatch.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/`@types/Services.ts around lines 169 - 177, Update ServiceRestartParams
to model REUSE and RESPEC as a discriminated union: REUSE must allow no
container-spec override fields, while RESPEC requires image and exactly one of
tag, checksum, or dockerfile, with additionalDockerFiles only valid in RESPEC.
Add runtime validation at the serviceRestart dispatch boundary so untyped
JavaScript inputs matching invalid combinations are rejected before being
forwarded to the node.
2 participants
Closes #2120
Allow Docker specs on
serviceRestart(RESPEC mode)What
Client-side support for the extended
serviceRestartcommand from ocean-node#1429 ("Allow docker specs on
service restart"): a restart can now change the container's image specification while
keeping the same serviceId, payment, resources, duration/expiry and host port(s). This
covers the realistic recovery workflow — start a service, hit a bug, publish a new tag,
then restart onto the new image without losing the paid window or reserved resources.
ProviderInstance.serviceRestartnow takes a single optionalparams: ServiceRestartParamsobject (replacing the previous positional
userData/dockerCmd/dockerEntrypointargs),consistent with how
serviceStarttakesServiceStartParams. Available on both the HTTPand P2P transports.
Semantics (mirrors the node)
The node treats the restart atomically ("all-or-nothing"), so the client just forwards
the fields the caller supplies and lets the node pick the mode:
stored spec, unchanged (the previous behaviour).
image/tag/checksum/dockerfile/additionalDockerFilessupplied → the container is rebuilt entirely from
params.imagebecomes mandatory,exactly one of
tag/checksum/dockerfileapplies, and adockerfilerequiresallowImageBuildon the environment (the node replies403otherwise). Sending acontainer param without
image, or more than one image mode, is a400from the node.userData/dockerCmd/dockerEntrypointkeep their replace-when-supplied semantics:an omitted field reuses the node's stored value, an explicit value (including
[])REPLACES it. As before,
userDatais ECIES-encrypted to the node's key client-side.Each field is only put on the wire when actually supplied, so the client never
accidentally forces the node out of REUSE mode.
Changes
src/@types/Services.ts— factored the container spec shared by start and restart intoa new exported
ServiceContainerSpec(image?,tag?,checksum?,dockerfile?,additionalDockerFiles?,userData?,dockerCmd?,dockerEntrypoint?).ServiceRestartParamsis that spec (all fields optional; atomic REUSE/RESPEC contractdocumented inline);
ServiceStartParamsnowextends ServiceContainerSpec, re-declaringimageas required and addingenvironment/exposedPorts/resources/duration/payment. No structural change toServiceStartParams.src/services/providers/HttpProvider.ts—serviceRestart()now accepts(nodeUri, signerOrAuthToken, serviceId, params?, signal?); unpacksparamsand addseach image-spec field to the request body only when defined.
src/services/providers/P2pProvider.ts— same signature/body change over theserviceRestartP2P command.src/services/providers/BaseProvider.ts— façade signature updated to the newparamsshape (dropped the now-unused
ServiceUserDataimport).Tests
test/unit/Services.test.ts): unchanged — theserviceRestartcommand stringand method exposure assertions still hold.
test/integration/Services.test.ts): the existing REUSE restart stepis updated to the new
paramssignature (passesundefinedforparams).Compatibility
serviceRestart's positionaluserData/dockerCmd/dockerEntrypointarguments are replaced by a singleparams: ServiceRestartParamsobject. Callers on
9.0.0-next.*that passed those positionally must move them into theobject. REUSE-mode restarts (no container spec) behave exactly as before.
older nodes ignore/na the extra fields and restart in REUSE mode. Purely additive on the
wire — new fields are only sent when supplied.
Summary by CodeRabbit
New Features
Bug Fixes
Tests