feat: add Image and ResolvedImage fields to SwarmOpts#392
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds Image and ResolvedImage to SwarmOpts (copied by Clone), implements resolveInstanceImages with precedence (Image → ResolvedImage → manifest fetch + lazy backfill), integrates resolution into instanceResources and reconciliation, updates DB/service hooks and systemd stub, and adds tests plus a version-manifest.json. ChangesImage Resolution and SwarmOpts Enhancement
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning Review ran into problems🔥 ProblemsGit: Failed to clone repository. Please run the 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
|
| Category | Results |
|---|---|
| Complexity | 1 medium |
🟢 Metrics 62 complexity · 2 duplication
Metric Results Complexity 62 Duplication 2
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.
|
Actionable comments posted: 0 |
tsivaprasad
force-pushed
the
PLAT-596-add-image-version-fields-to-database-spec
branch
from
44c4a96 to
02de9bd
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
♻️ Duplicate comments (1)
server/internal/orchestrator/swarm/orchestrator.go (1)
214-221:⚠️ Potential issue | 🟠 Major | ⚡ Quick winClear
ResolvedImagefor nil↔explicit version changes too.Line 214 only invalidates the cached image when both specs have a non-nil
PgEdgeVersion. An update fromnil -> 17.xor17.x -> nilleavesResolvedImageintact, and Line 221 will then reuse that stale value instead of resolving against the new effective version. That can deploy the wrong Postgres image after a version change.Suggested fix
func (o *Orchestrator) ReconcileInstanceSpec(old, new *database.InstanceSpec) error { + versionChanged := false + // If the Postgres version changed, the previously resolved image no longer // matches — clear it so resolveInstanceImages fetches the correct one from // the manifest. - if old != nil && old.PgEdgeVersion != nil && new.PgEdgeVersion != nil { - if !old.PgEdgeVersion.Equals(new.PgEdgeVersion) { - if new.OrchestratorOpts != nil && new.OrchestratorOpts.Swarm != nil { - new.OrchestratorOpts.Swarm.ResolvedImage = "" - } - } + if old != nil { + switch { + case old.PgEdgeVersion == nil && new.PgEdgeVersion == nil: + versionChanged = false + case old.PgEdgeVersion == nil || new.PgEdgeVersion == nil: + versionChanged = true + default: + versionChanged = !old.PgEdgeVersion.Equals(new.PgEdgeVersion) + } + } + + if versionChanged && new.OrchestratorOpts != nil && new.OrchestratorOpts.Swarm != nil { + new.OrchestratorOpts.Swarm.ResolvedImage = "" } _, err := o.resolveInstanceImages(new) return err }🤖 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/orchestrator.go` around lines 214 - 221, The cache invalidation only checks when both old.PgEdgeVersion and new.PgEdgeVersion are non-nil; change the condition before calling o.resolveInstanceImages(new) to detect any version change including nil↔non-nil transitions (i.e., old==nil && new!=nil, old!=nil && new==nil, or !old.Equals(new)) and when true, set new.OrchestratorOpts.Swarm.ResolvedImage = "" (guarded by new.OrchestratorOpts != nil && new.OrchestratorOpts.Swarm != nil) so resolveInstanceImages(new) cannot reuse a stale ResolvedImage; update the logic around variables old, new, PgEdgeVersion, OrchestratorOpts.Swarm.ResolvedImage and the subsequent call to resolveInstanceImages to implement this.
🤖 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.
Duplicate comments:
In `@server/internal/orchestrator/swarm/orchestrator.go`:
- Around line 214-221: The cache invalidation only checks when both
old.PgEdgeVersion and new.PgEdgeVersion are non-nil; change the condition before
calling o.resolveInstanceImages(new) to detect any version change including
nil↔non-nil transitions (i.e., old==nil && new!=nil, old!=nil && new==nil, or
!old.Equals(new)) and when true, set new.OrchestratorOpts.Swarm.ResolvedImage =
"" (guarded by new.OrchestratorOpts != nil && new.OrchestratorOpts.Swarm != nil)
so resolveInstanceImages(new) cannot reuse a stale ResolvedImage; update the
logic around variables old, new, PgEdgeVersion,
OrchestratorOpts.Swarm.ResolvedImage and the subsequent call to
resolveInstanceImages to implement this.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 4dbfec80-c34f-4855-98c9-eb20bd943510
📒 Files selected for processing (4)
server/internal/database/orchestrator.goserver/internal/database/service.goserver/internal/orchestrator/swarm/orchestrator.goserver/internal/orchestrator/systemd/orchestrator.go
There was a problem hiding this comment.
🧹 Nitpick comments (1)
Makefile (1)
158-158: ⚡ Quick winAdd explanatory comment for the new exclusion.
The comment block at lines 149-151 documents why dependencies are excluded from NOTICE.txt generation. Following this pattern, add a comment explaining that
dario.cat/mergois manually handled in NOTICE.txt.tmpl.📝 Proposed addition to comment block
# Exclude some dependencies from NOTICE.txt generation # - github.com/pgEdge/control-plane is our own code # - github.com/eclipse/paho.golang is licensed under EDL-1.0 explicitly in # NOTICES.txt.tmpl +# - dario.cat/mergo is licensed under BSD-3-Clause explicitly in NOTICE.txt.tmpl .PHONY: licenses🤖 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 `@Makefile` at line 158, Add a brief explanatory comment above the new Makefile exclusion flag for "dario.cat/mergo" documenting that this dependency is intentionally excluded because it is handled manually in NOTICE.txt.tmpl; update the existing comment block that explains why dependencies are excluded (the block around the --ignore flags) to mention "dario.cat/mergo is manually maintained in NOTICE.txt.tmpl" so future maintainers understand the exception.
🤖 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.
Nitpick comments:
In `@Makefile`:
- Line 158: Add a brief explanatory comment above the new Makefile exclusion
flag for "dario.cat/mergo" documenting that this dependency is intentionally
excluded because it is handled manually in NOTICE.txt.tmpl; update the existing
comment block that explains why dependencies are excluded (the block around the
--ignore flags) to mention "dario.cat/mergo is manually maintained in
NOTICE.txt.tmpl" so future maintainers understand the exception.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 84f6954e-3a43-41cb-aa08-40510574b650
📒 Files selected for processing (3)
MakefileNOTICE.txtNOTICE.txt.tmpl
✅ Files skipped from review due to trivial changes (1)
- NOTICE.txt
tsivaprasad
force-pushed
the
PLAT-596-add-image-version-fields-to-database-spec
branch
3 times, most recently
from
d09cff8 to
b66c3f9
Compare
tsivaprasad
force-pushed
the
PLAT-596-add-image-version-fields-to-database-spec
branch
from
b66c3f9 to
94d6615
Compare
jason-lynch
left a comment
jason-lynch
left a comment
There was a problem hiding this comment.
Awesome! I was little confused for a second because it looks like the ManifestLoader PR got merged into this one, but I can see that it's still unused.
2 participants
Summary
This PR adds
ImageandResolvedImagefields toSwarmOptsand implements precedence-based image resolution in the Swarm orchestrator. This provides the foundation for user-controlled image overrides and stable image pinning.Changes
Image(user override, never managed by CP) andResolvedImage(CP-managed) fields toSwarmOptsinserver/internal/database/spec.go, serialized underorchestrator_opts.docker.SwarmOpts.Clone()to correctly copy both new fields.resolveInstanceImages()in the Swarm orchestrator to implement precedence-based image resolution:Image→ResolvedImage→ manifest lookup (lazy backfill).Imageis explicitly set, the manifest is bypassed entirely, allowing unknown or custom versions to work through user overrides.Testing
Verification:
Checklist
PLAT-596