fix: Update-images - #310
Conversation
📝 WalkthroughWalkthroughClickHouse Keeper image selection now uses server manifest values and the operator’s global registry, with a fallback image for older manifests. Manifest fixtures, reconciliation wiring, and Keeper vendor spec tests were updated accordingly. ChangesClickHouse Keeper image resolution
Estimated code review effort: 2 (Simple) | ~10 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 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 |
![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
🤖 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 `@internal/controller/infra/managed/clickhouse/altinity/keeper/spec_test.go`:
- Line 52: Update the assertion in the relevant keeper spec test to compare
container.Image against defaultKeeperImage (or the explicit expected fallback
value) instead of calling KeeperImage, ensuring the test independently validates
fallback wiring.
🪄 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: fa0f273c-495d-4ef4-a9e5-b54b003f245d
📒 Files selected for processing (6)
hack/testing-manifests/server-manifest/0.83.0-clickhouse-keeper.1/manifest.yamlhack/testing-manifests/server-manifest/0.83.0-clickhouse-keeper.2/manifest.yamlinternal/controller/infra/managed/clickhouse/altinity/keeper/spec.gointernal/controller/infra/managed/clickhouse/altinity/keeper/spec_test.gointernal/controller/infra/managed/clickhouse/altinity/keeper/values.gointernal/controller/reconciler/clickhouse.go
| Expect(chk.Spec.Templates.PodTemplates).To(HaveLen(1)) | ||
| container := chk.Spec.Templates.PodTemplates[0].Spec.Containers[0] | ||
| Expect(container.Image).To(Equal(KeeperImage)) | ||
| Expect(container.Image).To(Equal(KeeperImage(manifest.ImageRef{}, ""))) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Assert the fallback independently of the implementation helper.
The production value and expected value both call KeeperImage, so this test cannot detect incorrect fallback wiring. Compare against defaultKeeperImage (or an explicit contract value) instead.
Proposed fix
- Expect(container.Image).To(Equal(KeeperImage(manifest.ImageRef{}, "")))
+ Expect(container.Image).To(Equal(defaultKeeperImage))📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| Expect(container.Image).To(Equal(KeeperImage(manifest.ImageRef{}, ""))) | |
| Expect(container.Image).To(Equal(defaultKeeperImage)) |
🤖 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 `@internal/controller/infra/managed/clickhouse/altinity/keeper/spec_test.go` at
line 52, Update the assertion in the relevant keeper spec test to compare
container.Image against defaultKeeperImage (or the explicit expected fallback
value) instead of calling KeeperImage, ensuring the test independently validates
fallback wiring.
997322c to
5c31ca2
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@internal/controller/infra/managed/clickhouse/altinity/keeper/spec.go`:
- Around line 23-29: Update KeeperImage to detect an empty manifest image
repository before calling img.GetImage, then return defaultKeeperImage with the
configured global registry prefix applied; preserve the existing GetImage result
for non-empty images. Add a regression test covering manifest.ImageRef{} with a
non-empty global registry and verifying the correctly prefixed fallback image.
🪄 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: c84eccee-154f-42fd-803a-c9ca4c98b772
📒 Files selected for processing (6)
hack/testing-manifests/server-manifest/0.83.0-clickhouse-keeper.1/manifest.yamlhack/testing-manifests/server-manifest/0.83.0-clickhouse-keeper.2/manifest.yamlinternal/controller/infra/managed/clickhouse/altinity/keeper/spec.gointernal/controller/infra/managed/clickhouse/altinity/keeper/spec_test.gointernal/controller/infra/managed/clickhouse/altinity/keeper/values.gointernal/controller/reconciler/clickhouse.go
| func KeeperImage(img manifest.ImageRef, globalImageRegistry string) string { | ||
| if out := img.GetImage(globalImageRegistry); out != "" { | ||
| return out | ||
| } | ||
| // Fallback for older manifests that don't supply the image. | ||
| return defaultKeeperImage | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Preserve the fallback when a global registry is configured.
For an older manifest, manifest.ImageRef{} passed to GetImage("myregistry.io") returns "myregistry.io/", so out != "" skips the fallback and produces an invalid image. Even after fixing detection, returning defaultKeeperImage directly would bypass the global registry.
Detect an empty repository before calling GetImage, then apply the same global-registry prefix to the fallback and add a regression test for an empty image with a non-empty global registry.
Proposed fix
func KeeperImage(img manifest.ImageRef, globalImageRegistry string) string {
+ if img.Repository == "" {
+ return (manifest.ImageRef{Repository: defaultKeeperImage}).GetImage(globalImageRegistry)
+ }
if out := img.GetImage(globalImageRegistry); out != "" {
return out
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func KeeperImage(img manifest.ImageRef, globalImageRegistry string) string { | |
| if out := img.GetImage(globalImageRegistry); out != "" { | |
| return out | |
| } | |
| // Fallback for older manifests that don't supply the image. | |
| return defaultKeeperImage | |
| } | |
| func KeeperImage(img manifest.ImageRef, globalImageRegistry string) string { | |
| if img.Repository == "" { | |
| return (manifest.ImageRef{Repository: defaultKeeperImage}).GetImage(globalImageRegistry) | |
| } | |
| if out := img.GetImage(globalImageRegistry); out != "" { | |
| return out | |
| } | |
| // Fallback for older manifests that don't supply the image. | |
| return defaultKeeperImage | |
| } |
🤖 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 `@internal/controller/infra/managed/clickhouse/altinity/keeper/spec.go` around
lines 23 - 29, Update KeeperImage to detect an empty manifest image repository
before calling img.GetImage, then return defaultKeeperImage with the configured
global registry prefix applied; preserve the existing GetImage result for
non-empty images. Add a regression test covering manifest.ImageRef{} with a
non-empty global registry and verifying the correctly prefixed fallback image.
2 participants
Missed hardcoded clickhouse-keeper image when moving image refs to manifest
Summary by CodeRabbit
New Features
Bug Fixes
Tests