Fixed controller logic due to clarifications - #8
Conversation
📝 WalkthroughWalkthroughThis PR refactors garbage collection configuration in Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 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 |
Merging this branch will decrease overall coverage
Coverage by fileChanged files (no unit tests)
Please note that the "Total", "Covered", and "Missed" counts above refer to code statements instead of lines of code. The value in brackets refers to the test coverage of that file in the old version of the code. Changed unit test files
|
![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)
controllers/managedcloudprofile_controller_test.go (1)
549-552: These specs still fail before GC runs.
controllers/managedcloudprofile_controller.go:80-82callsupdateMachineImagesbeforereconcileGarbageCollection, andcontrollers/managedcloudprofile_controller.go:283-295consumes the same bad secret/registry during apply. So addingmcp.Spec.GarbageCollectionhere doesn’t make these examples cover GC-specific failures; they still pass on theApplyFailedpath. Either rename them to match that behavior or restructure the fixtures so apply succeeds and GC is the only failing stage.Also applies to: 588-591
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@controllers/managedcloudprofile_controller_test.go` around lines 549 - 552, The test sets mcp.Spec.GarbageCollection but the controller calls updateMachineImages before reconcileGarbageCollection and apply already fails on the bad secret/registry, so the test is not exercising GC failures. Either (A) rename the test(s) to reflect they hit the ApplyFailed path (e.g., include "ApplyFailed" in the test name) or (B) change the test fixtures so updateMachineImages and the apply stage succeed and only reconcileGarbageCollection fails: fix the fake secret/registry values used by updateMachineImages and the apply path (the same secret consumed in controllers/managedcloudprofile_controller.go updateMachineImages and the reconcile/apply logic) so they are valid, leaving only the garbage-collection-specific failure to be triggered by reconcileGarbageCollection; apply this change for the instances around the mcp.Spec.GarbageCollection setup (the blocks at the shown location and the other occurrence noted).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@api/v1alpha1/managedcloudprofile.go`:
- Around line 23-25: The CRD is missing the new top-level spec.garbageCollection
field added to ManagedCloudProfileSpec; update the CRD generation so the schema
includes spec.garbageCollection (GarbageCollection *GarbageCollectionConfig)
alongside the existing machineImageUpdates[].garbageCollection definition,
regenerate the YAML, and commit it in this PR; also provide a migration
note/script to copy existing nested machineImageUpdates[].garbageCollection
values into the new spec.garbageCollection for existing ManagedCloudProfile
objects so the API server won't prune the new field at admission time.
---
Nitpick comments:
In `@controllers/managedcloudprofile_controller_test.go`:
- Around line 549-552: The test sets mcp.Spec.GarbageCollection but the
controller calls updateMachineImages before reconcileGarbageCollection and apply
already fails on the bad secret/registry, so the test is not exercising GC
failures. Either (A) rename the test(s) to reflect they hit the ApplyFailed path
(e.g., include "ApplyFailed" in the test name) or (B) change the test fixtures
so updateMachineImages and the apply stage succeed and only
reconcileGarbageCollection fails: fix the fake secret/registry values used by
updateMachineImages and the apply path (the same secret consumed in
controllers/managedcloudprofile_controller.go updateMachineImages and the
reconcile/apply logic) so they are valid, leaving only the
garbage-collection-specific failure to be triggered by
reconcileGarbageCollection; apply this change for the instances around the
mcp.Spec.GarbageCollection setup (the blocks at the shown location and the other
occurrence noted).
🪄 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
Run ID: d35c0d0a-935c-4432-ab1b-1da0544f98f9
📒 Files selected for processing (4)
api/v1alpha1/managedcloudprofile.goapi/v1alpha1/zz_generated.deepcopy.gocontrollers/managedcloudprofile_controller.gocontrollers/managedcloudprofile_controller_test.go
| // GarbageCollection contains configuration for automated garbage collection | ||
| // +optional | ||
| GarbageCollection *GarbageCollectionConfig `json:"garbageCollection,omitempty"` |
There was a problem hiding this comment.
Regenerate the CRD for this API move.
ManagedCloudProfileSpec now exposes spec.garbageCollection, but crd/cloudprofilesync.cobaltcore.dev_managedcloudprofiles.yaml:519-611 still only defines machineImageUpdates[].garbageCollection and does not add the new top-level property. In-cluster that means the apiserver can keep validating the old shape and prune/reject the new field, so mcp.Spec.GarbageCollection never becomes effective. Please update/regenerate the CRD in the same PR, and call out a migration path if existing objects still rely on the nested field.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@api/v1alpha1/managedcloudprofile.go` around lines 23 - 25, The CRD is missing
the new top-level spec.garbageCollection field added to ManagedCloudProfileSpec;
update the CRD generation so the schema includes spec.garbageCollection
(GarbageCollection *GarbageCollectionConfig) alongside the existing
machineImageUpdates[].garbageCollection definition, regenerate the YAML, and
commit it in this PR; also provide a migration note/script to copy existing
nested machineImageUpdates[].garbageCollection values into the new
spec.garbageCollection for existing ManagedCloudProfile objects so the API
server won't prune the new field at admission time.
2 participants
It was clarified, that we don't need to have GC under each MachineImageUpdate, so made changes to fix controller and tests according to the updates
Summary by CodeRabbit