feat: wire image field through API for custom image pinning per database or node#401
Conversation
|
Warning Review limit reached
Next review available in: 35 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (7)
📒 Files selected for processing (3)
📝 WalkthroughWalkthroughThis PR adds an optional Swarm image override, checks image availability during validation, and propagates validation warnings through workflow aggregation into create and update database responses. ChangesImage Override and Validation Warnings
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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 |
Up to standards ✅🟢 Issues
|
| Metric | Results |
|---|---|
| Complexity | 17 |
| Duplication | 0 |
NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.
|
@coderabbitai review |
✅ Action performedReview finished.
|
jason-lynch
left a comment
jason-lynch
left a comment
There was a problem hiding this comment.
Adding the image field to the API looks good! However, I'm not sure that the validation this PR adds is very helpful, especially since it doesn't prevent you from using an invalid image. Instead of a non-fatal validation, maybe we could check that the image exists by trying to pull it with the Docker client?
Something else to think about for a separate PR: another way that users can get into trouble is if the Postgres and Spock versions in the image don't match the ones in the spec. We should spend a little time thinking about how we can make that less dangerous, such as by enforcing a tag format that contains the Postgres and Spock versions like we do in our official images. I think @mmols had mentioned at one point that it would be reasonable for us to enforce a tag format for custom images.
Thanks @jason-lynch Instead of performing a full The validation behavior is now:
Created place holder ticket to |
|
@tsivaprasad That sounds good! Although I still have a question about this type of validation:
What's the use case for this warning? To me, this looks like any valid usage of the override field will result in a warning. The only time it wouldn't return a warning is when you specify the same image that's in the manifest, in which case the override isn't doing anything. Am I understanding that right or is there a case I'm missing? I'm wondering if we need warnings at all. |
You're right — the warning fires on every intentional use of the override, so it was just noise. We've removed it. The only validation that remains is the hard fail when the image can't be found or reached in the registry. If the image is reachable, it's accepted with no warnings. |
Thanks! Now that it's unused, could you please remove the |
tsivaprasad
force-pushed
the
PLAT-596-add-image-version-fields-to-database-spec
branch
from
94d6615 to
b49d1cd
Compare
tsivaprasad
force-pushed
the
PLAT-599-allow-custom-image-override-per-database-or-node
branch
from
cb8ce47 to
7c3f46b
Compare
![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)
server/internal/orchestrator/swarm/validate_instance_specs_test.go (1)
31-101: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssertions run inside empty loops — these tests are largely vacuous.
In every subtest
Previousis nil,Currenthas no port/volume/network diff, ando.dockeris nil. As a resultValidateInstanceSpecsappends nothing toresults, so thefor _, r := range resultsloops never execute and theassert.Empty(...)calls never run. The only assertion actually exercised isrequire.NoError. The image-override path (CheckImageExists) is never covered because the docker client is nil.Recommend asserting the result count explicitly and, ideally, injecting a fake docker client to cover the failure path.
♻️ Example: assert on results explicitly
results, err := o.ValidateInstanceSpecs(ctx, changes) require.NoError(t, err) - for _, r := range results { - assert.Empty(t, r.Warnings) - assert.Empty(t, r.Errors) - } + assert.Empty(t, results)🤖 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 `@server/internal/orchestrator/swarm/validate_instance_specs_test.go` around lines 31 - 101, The subtests in ValidateInstanceSpecs are vacuous because results is empty and the assertion loop never runs, so only require.NoError is exercised. Update the tests to assert the expected results length explicitly in each subtest, using ValidateInstanceSpecs as the entry point and checking for zero or nonzero items as appropriate. To cover the image-override behavior, inject a fake docker client into o.docker and add a test path that exercises CheckImageExists instead of relying on the nil-client skip. Keep the existing cases for knownVersion and unknownVersion, but make the assertions verify actual returned warnings/errors rather than looping over an empty slice.
🤖 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 `@server/internal/docker/docker.go`:
- Around line 591-597: Update CheckImageExists to avoid relying on anonymous
DistributionInspect, since it can reject private images and valid overrides;
resolve registry credentials first using the Docker client/auth flow before
validating the image. If the lookup may contact an external registry, wrap the
inspection in a context with a timeout, and keep the existing error wrapping in
Docker.CheckImageExists for clear failure messages.
---
Nitpick comments:
In `@server/internal/orchestrator/swarm/validate_instance_specs_test.go`:
- Around line 31-101: The subtests in ValidateInstanceSpecs are vacuous because
results is empty and the assertion loop never runs, so only require.NoError is
exercised. Update the tests to assert the expected results length explicitly in
each subtest, using ValidateInstanceSpecs as the entry point and checking for
zero or nonzero items as appropriate. To cover the image-override behavior,
inject a fake docker client into o.docker and add a test path that exercises
CheckImageExists instead of relying on the nil-client skip. Keep the existing
cases for knownVersion and unknownVersion, but make the assertions verify actual
returned warnings/errors rather than looping over an empty slice.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d9344ed9-e337-41c5-a0a4-7dbcf944da73
⛔ Files ignored due to path filters (11)
api/apiv1/gen/control_plane/service.gois excluded by!**/gen/**api/apiv1/gen/http/cli/control_plane/cli.gois excluded by!**/gen/**api/apiv1/gen/http/control_plane/client/cli.gois excluded by!**/gen/**api/apiv1/gen/http/control_plane/client/encode_decode.gois excluded by!**/gen/**api/apiv1/gen/http/control_plane/client/types.gois excluded by!**/gen/**api/apiv1/gen/http/control_plane/server/encode_decode.gois excluded by!**/gen/**api/apiv1/gen/http/control_plane/server/types.gois excluded by!**/gen/**api/apiv1/gen/http/openapi.jsonis excluded by!**/gen/**api/apiv1/gen/http/openapi.yamlis excluded by!**/gen/**api/apiv1/gen/http/openapi3.jsonis excluded by!**/gen/**api/apiv1/gen/http/openapi3.yamlis excluded by!**/gen/**
📒 Files selected for processing (10)
api/apiv1/design/database.goserver/internal/api/apiv1/convert.goserver/internal/api/apiv1/convert_test.goserver/internal/api/apiv1/post_init_handlers.goserver/internal/database/orchestrator.goserver/internal/docker/docker.goserver/internal/orchestrator/swarm/orchestrator.goserver/internal/orchestrator/swarm/validate_instance_specs_test.goserver/internal/workflows/validate_spec.goserver/internal/workflows/validate_spec_test.go
🚧 Files skipped from review as they are similar to previous changes (7)
- server/internal/workflows/validate_spec_test.go
- server/internal/workflows/validate_spec.go
- server/internal/api/apiv1/convert.go
- server/internal/api/apiv1/post_init_handlers.go
- server/internal/database/orchestrator.go
- api/apiv1/design/database.go
- server/internal/api/apiv1/convert_test.go
@jason-lynch Addressed this comment, pls have a look. |
tsivaprasad
deleted the
PLAT-599-allow-custom-image-override-per-database-or-node
branch
2 participants
Summary
This PR wires the
imagefield through the API, allowing users to pin any container image at either the database or node level viaorchestrator_opts.swarm.image. The Control Plane deploys the specified image without requiring manifest validation, while returning a warning in the API response when the image is not present in the manifest.Changes
imagefield to theSwarmOptsGoa type and regenerated the API layer.Warnings []stringtoValidationResultandValidateSpecOutput, allowing non-fatal validation warnings to be propagated through the validation workflow and returned to API callers.ValidateInstanceSpecsto emit a warning whenSwarmOpts.Imageis explicitly set and differs from the manifest-resolved image for the selected version. The specification remains valid and is still accepted.overridableValue()merge logic inNodeInstances(), requiring no additional changes.Imageis cleared, the Control Plane automatically falls back toResolvedImageduring the next reconcile cycle using the existingresolveInstanceImages()precedence logic.Testing
Verification:
Custom image at database level → warning returned, image deployed, Docker surfaces pull failure for non-existent image
docker service ls | grep storefront
2lyuimxnkbc2 postgres-storefront-manifest-image-n1-9ptayhma replicated 1/1 ghcr.io/pgedge/pgedge-postgres:17.9-spock5.0.6-standard-2
9792vnbo0jwn postgres-storefront-manifest-image-n1-689qacsi replicated 1/1 ghcr.io/pgedge/pgedge-postgres:17.9-spock5.0.6-standard-2
8a3zmtz2hbzq postgres-storefront-manifest-image-n1-ant97dj4 replicated 1/1 ghcr.io/pgedge/pgedge-postgres:17.9-spock5.0.6-standard-2
Checklist
PLAT-599