feat: add attachment image push support to CLI and control plane#104
Merged
Conversation
…recent Replace single-file .bak overwrite with timestamped backups (.bak.<unix>) and automatic cleanup to prevent config directory pollution. Restore falls back to legacy .bak for backward compatibility.
…nd precision - Backup suffix now validated with strconv.ParseInt to reject non-numeric files - Sort by parsed timestamp instead of lexicographic string comparison - Use UnixNano() instead of Unix() to prevent collisions on rapid saves - Test asserts exact backup count (5) instead of permissive range
Contributor
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdds a new CLI command gordon attachments push, control-plane and HTTP APIs to lookup attachment targets by image, config-service image-matching and attachment resolution, DTOs and tests, docs updates, and rotated backup/cleanup behavior in the config service. Changes
Sequence Diagram(s)sequenceDiagram
participant User as User/CLI
participant CLI as Attachments Push Command
participant CP as ControlPlane (local/remote)
participant Config as ConfigService
participant Builder as Image Builder
participant Registry as Container Registry
User->>CLI: gordon attachments push image[:tag] [--build]
CLI->>CP: FindAttachmentTargetsByImage(imageName)
alt local control plane
CP->>Config: FindAttachmentTargetsByImage(imageName)
Config-->>CP: []targets
else remote control plane
CP->>Registry: GET /admin/attachments/by-image/{imageName}
Registry-->>CP: AttachmentTargetsByImageResponse (targets)
end
CP-->>CLI: targets
CLI->>CLI: determine version, compute refs, validate
alt --build flag set--
CLI->>Builder: BuildAndPush(dockerfile, refs, platform, build-args)
Builder->>Registry: push tags (version & latest)
Registry-->>Builder: success
else tag-only
CLI->>Builder: TagAndPush(sourceImage, refs)
Builder->>Registry: push tags
Registry-->>Builder: success
end
Builder-->>CLI: result
CLI-->>User: "Push complete"
Estimated Code Review Effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
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)
📝 Coding Plan
Comment |
Add gordon attachments push command for building/tagging/pushing attachment images to the Gordon registry. This closes the gap where attachment-based deploys fail because the attachment image has no supported push path. - Add FindAttachmentTargetsByImage config lookup with shared normalization - Expose attachment-by-image lookup via admin API and control plane - Add attachments push CLI command with build/tag/push support - Update push error messaging to suggest attachments push for non-route images - Update CLI docs for attachments, push, bootstrap, and index Closes #103
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/adapters/in/cli/attachments_push.go`:
- Around line 50-109: The function runAttachmentsPush exceeds allowed cyclomatic
complexity; extract the output formatting (the cliWritef/cliWriteLine blocks
that print Attachment image, Targets, Image, Also, and Push complete) into a
helper like formatAndPrintAttachmentInfo(out, imageArg, targets, versionRef,
latestRef, version) and extract the push decision logic into another helper like
performAttachmentPush(ctx, build, version, platform, dockerfile, buildArgs,
registry, imageName, versionRef, latestRef) that encapsulates the buildAndPushFn
vs tagAndPushFn branches; replace the inline blocks in runAttachmentsPush with
calls to these two helpers to reduce branching and complexity while keeping
existing behavior.
In `@internal/usecase/config/service.go`:
- Around line 615-621: The cleanupOldBackups call currently returns an error up
the stack which makes Save fail even though the backup write succeeded; change
the cleanup behavior to log the error instead of returning it. Locate the block
with backupPath := fmt.Sprintf(...) and the subsequent
cleanupOldBackups(configFile, 5) call and replace the "return fmt.Errorf(...)"
path with a non-fatal log statement using the service's logger (e.g.,
s.logger.Errorf or similar) or the package logger, including context (configFile
and backupPath) and the error; keep the original error returned only for the
backup write failure, not for cleanup.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 811212aa-b528-48d8-a4c0-84ffc27335c1
📒 Files selected for processing (3)
internal/adapters/in/cli/attachments_push.gointernal/usecase/config/service.gointernal/usecase/config/service_test.go
Comment on lines
+615
to
+621
| backupPath := fmt.Sprintf("%s.bak.%d", configFile, time.Now().UnixNano()) | ||
| if err := os.WriteFile(backupPath, src, 0600); err != nil { | ||
| return fmt.Errorf("failed to write backup config: %w", err) | ||
| } | ||
| if err := cleanupOldBackups(configFile, 5); err != nil { | ||
| return fmt.Errorf("failed to clean up old backups: %w", err) | ||
| } |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider logging cleanup errors instead of propagating them.
The backup creation succeeds but returns an error if old backup cleanup fails. This could cause unnecessary failures for the caller (e.g., Save operation) when the primary operation succeeded.
🔧 Suggested approach
if err := os.WriteFile(backupPath, src, 0600); err != nil {
return fmt.Errorf("failed to write backup config: %w", err)
}
- if err := cleanupOldBackups(configFile, 5); err != nil {
- return fmt.Errorf("failed to clean up old backups: %w", err)
- }
+ // Cleanup is best-effort; don't fail the backup operation
+ _ = cleanupOldBackups(configFile, 5)
return nilAlternatively, if cleanup errors should be visible, log them without failing the operation.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/usecase/config/service.go` around lines 615 - 621, The
cleanupOldBackups call currently returns an error up the stack which makes Save
fail even though the backup write succeeded; change the cleanup behavior to log
the error instead of returning it. Locate the block with backupPath :=
fmt.Sprintf(...) and the subsequent cleanupOldBackups(configFile, 5) call and
replace the "return fmt.Errorf(...)" path with a non-fatal log statement using
the service's logger (e.g., s.logger.Errorf or similar) or the package logger,
including context (configFile and backupPath) and the error; keep the original
error returned only for the backup write failure, not for cleanup.
bnema
added a commit
that referenced
this pull request
Mar 19, 2026
feat: add attachment image push support to CLI and control plane
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Closes #103
gordon attachments push <image>command for building, tagging, and pushing attachment images to the Gordon registryFindAttachmentTargetsByImageconfig lookup with shared image normalization (reuses route-matching rules)GET /admin/attachments/by-image/{image}admin API endpointgordon pusherror messaging to suggestattachments pushfor non-route imagesWhy
Attachment-based deploys fail when the attachment image does not exist in the registry. Gordon had no native way to push attachment images —
gordon pushonly works for route images. This adds that missing path so first deploys with custom attachments (e.g.pitlane-pgsql:v18) work end-to-end.Test coverage
Summary by CodeRabbit
New Features
gordon attachments pushto build/tag/push custom attachment images prior to deploymentDocumentation
UX
Tests