fix: Behaviour of paused: true when there are empty providerConfig.machineImages - #55
Conversation
There was a problem hiding this comment.
🟡 Changes recommended
The new ProviderConfig restore code can unintentionally drop unrelated ProviderConfig fields by round-tripping through a partial JSON struct, risking configuration loss.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Ensures that when a MachineImageUpdate is marked Paused: true, the reconciler preserves runtime-discovered provider machineImages mappings stored in the existing CloudProfile.Spec.ProviderConfig instead of wiping them when the MCP spec has an empty/omitted provider config machineImages list.
Changes:
- Add logic in the CloudProfile reconciliation path to restore stored ProviderConfig
machineImagesentries for paused image updates. - Extend the paused-update controller test to simulate an already-reconciled CloudProfile with ProviderConfig mappings and assert they remain intact after a paused reconcile.
File summaries
| File | Description |
|---|---|
| controllers/managedcloudprofile_controller_test.go | Adds a regression test verifying paused reconciles preserve existing ProviderConfig image mappings. |
| controllers/cloud_profile.go | Restores stored ProviderConfig machineImages for paused updates via JSON manipulation helpers. |
Review details
- Files reviewed: 2/2 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
17359f2 to
0dbe273
Compare
There was a problem hiding this comment.
🟢 Approval recommended
The change is narrowly scoped, adds coverage for the intended regression scenario, and the reconciliation logic preserves the previously stored ProviderConfig mappings as intended.
Review details
- Files reviewed: 2/2 changed files
- Comments generated: 0 new
- Review effort level: Lite
Signed-off-by: C5421281 <yahor.kurachkin@sap.com>
0dbe273 to
f676bb6
Compare
There was a problem hiding this comment.
🟡 Changes recommended
Critical compatibility and paused-reconciliation issues remain unresolved.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 4/4 changed files
- Comments generated: 3
- Review effort level: Lite
There was a problem hiding this comment.
🟡 Changes recommended
Address the legacy pause-field compatibility break and correctly detect existing CloudProfiles with empty fields.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 5/5 changed files
- Comments generated: 2
- Review effort level: Lite
…images update Signed-off-by: C5421281 <yahor.kurachkin@sap.com>
f69c78f to
3440ed4
Compare
There was a problem hiding this comment.
🔵 Needs a closer look
Address the breaking API migration and paused reconciliation fallback issues before approval.
Review details
Suppressed comments (2)
api/v1alpha1/managedcloudprofile.go:20
- Adding the global flag while removing
MachineImageUpdate.Pausedis a breaking API change: after this CRD is upgraded, Kubernetes will prune existingmachineImageUpdates[].paused: truefields and this controller will silently resume those updates. Please retain/deprecate the old field and map it to the new semantics, or provide an explicit migration before removing it.
// MachineImagesPaused disables automatic machine image updates and keeps the
// existing CloudProfile machine images and provider config unchanged. Other
// updates (e.g. Kubernetes versions and base spec fields) still apply.
// +optional
MachineImagesPaused bool `json:"machineImagesPaused,omitempty"`
controllers/cloud_profile.go:67
- This fallback conflates a newly created CloudProfile with an existing one whose machineImages is empty or whose ProviderConfig is nil. With the pause enabled, such an existing object is overwritten by the MCP base values, despite the field contract saying both values remain unchanged. Detect whether CreateOrPatch found an existing object (for example, via ResourceVersion) and restore both fields, including nil/empty values, in that case.
if len(storedMachineImages) > 0 {
cloudProfile.Spec.MachineImages = storedMachineImages
}
if storedProviderConfig != nil {
cloudProfile.Spec.ProviderConfig = storedProviderConfig
- Files reviewed: 5/5 changed files
- Comments generated: 0 new
- Review effort level: Lite
No description provided.