support GEP-31 in place update#38
Open
adziauho wants to merge 1 commit into
Open
Conversation
📝 WalkthroughWalkthroughExports a ChangesUSIFeature In-Place Update Support
Go Toolchain Version Bump
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
cloudprofilesync/imageupdater.go (1)
91-93:⚠️ Potential issue | 🟠 Major | ⚡ Quick winExisting versions are not reconciled for
InPlaceUpdates.Line 89 computes
supportInPlaceUpdate, but existing-version paths only patch architectures. If a version already exists,InPlaceUpdatesstays stale (never added/removed), so spec can drift from source capabilities.Proposed fix pattern
if idx, exists := existingVersions[sourceImage.Version]; exists { image.Versions[idx].Architectures = sourceImage.Architectures + if supportInPlaceUpdate { + image.Versions[idx].InPlaceUpdates = &gardenerv1beta1.InPlaceUpdates{Supported: true} + } else { + image.Versions[idx].InPlaceUpdates = nil + } } else { ... } @@ if idx, exists := existingVersions[sourceImage.CleanVersion]; exists { existing := &image.Versions[idx] for _, arch := range sourceImage.Architectures { if !slices.Contains(existing.Architectures, arch) { existing.Architectures = append(existing.Architectures, arch) } } + if supportInPlaceUpdate { + existing.InPlaceUpdates = &gardenerv1beta1.InPlaceUpdates{Supported: true} + } else { + existing.InPlaceUpdates = nil + } } else { ... }Also applies to: 118-124
🤖 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 `@cloudprofilesync/imageupdater.go` around lines 91 - 93, When an existing version is found in the existingVersions map within the block where `if idx, exists := existingVersions[sourceImage.Version]; exists`, only the Architectures field is being patched but the InPlaceUpdates field is not being reconciled. The supportInPlaceUpdate variable that was computed should also be used to update the InPlaceUpdates field of the existing version to prevent it from drifting from source capabilities. Apply this same reconciliation pattern to both occurrences of this existing version update logic.
🧹 Nitpick comments (1)
cloudprofilesync/source.go (1)
34-35: ⚡ Quick winRename misspelled exported constant before it spreads.
USIDevFeatrueat Line 34 is typoed (should beUSIDevFeature). Since it is exported, this will leak a misspelled API name into consumers and tests.Proposed fix
- USIDevFeatrue = "_usidev" + USIDevFeature = "_usidev" @@ - USIDevFeatrue: {}, + USIDevFeature: {},Also applies to: 45-45
🤖 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 `@cloudprofilesync/source.go` around lines 34 - 35, The exported constant USIDevFeatrue contains a typo (should be USIDevFeature). Rename this constant to the correct spelling USIDevFeature. Since this constant is exported and appears in multiple locations (including line 45 as noted), search for all usages of the misspelled constant name USIDevFeatrue throughout the codebase and update them to use the corrected spelling USIDevFeature to ensure consistency across all references.
🤖 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 `@cloudprofilesync/imageupdater_test.go`:
- Around line 341-358: The test case at the It block for "reflect inplace update
ability to machineimage" needs to be extended to cover the clean-version path.
Modify the mockSource.images SourceImage to include a CleanVersion field set to
a different value than the Version field (e.g., "1.0" while Version is "1.0.0").
After calling updater.Update and verifying the first version entry, add an
additional assertion to check that the clean-version entry also has
InPlaceUpdates.Supported set to true, ensuring the test exercises both the
legacy append path and the new clean-version path in ImageUpdater.Update.
---
Outside diff comments:
In `@cloudprofilesync/imageupdater.go`:
- Around line 91-93: When an existing version is found in the existingVersions
map within the block where `if idx, exists :=
existingVersions[sourceImage.Version]; exists`, only the Architectures field is
being patched but the InPlaceUpdates field is not being reconciled. The
supportInPlaceUpdate variable that was computed should also be used to update
the InPlaceUpdates field of the existing version to prevent it from drifting
from source capabilities. Apply this same reconciliation pattern to both
occurrences of this existing version update logic.
---
Nitpick comments:
In `@cloudprofilesync/source.go`:
- Around line 34-35: The exported constant USIDevFeatrue contains a typo (should
be USIDevFeature). Rename this constant to the correct spelling USIDevFeature.
Since this constant is exported and appears in multiple locations (including
line 45 as noted), search for all usages of the misspelled constant name
USIDevFeatrue throughout the codebase and update them to use the corrected
spelling USIDevFeature to ensure consistency across all references.
🪄 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: dca6ed2c-0edb-47dc-a74f-5d5c785a04d4
📒 Files selected for processing (4)
cloudprofilesync/imageupdater.gocloudprofilesync/imageupdater_test.gocloudprofilesync/source.gogo.mod
Comment on lines
+341
to
+358
| It("reflect inplace update ability to machineimage", func(ctx SpecContext) { | ||
| mockSource.images = []cloudprofilesync.SourceImage{{ | ||
| Version: "1.0.0", | ||
| Architectures: []string{"amd64"}, | ||
| Capabilities: map[string]gardencorev1beta1.CapabilityValues{"feature": {cloudprofilesync.USIFeature}}}, | ||
| } | ||
| updater := cloudprofilesync.ImageUpdater{ | ||
| Log: logr.Discard(), | ||
| Source: &mockSource, | ||
| ImageName: "test", | ||
| EnableCapabilities: true, | ||
| } | ||
| var cpSpec gardencorev1beta1.CloudProfileSpec | ||
| Expect(updater.Update(ctx, &cpSpec)).To(Succeed()) | ||
| Expect(cpSpec.MachineImages[0].Versions).To(HaveLen(1)) | ||
| Expect(cpSpec.MachineImages[0].Versions[0].Version).To(Equal("1.0.0")) | ||
| Expect(cpSpec.MachineImages[0].Versions[0].InPlaceUpdates.Supported).To(Equal(true)) | ||
| }) |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Flag-ON test does not cover the clean-version branch.
This case sets EnableCapabilities: true but omits CleanVersion, so it only exercises the legacy append path again. It misses the new clean-version path at ImageUpdater.Update Lines 126-136.
Please add/adjust this test to set CleanVersion different from Version and assert InPlaceUpdates.Supported on the clean-version entry too.
🤖 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 `@cloudprofilesync/imageupdater_test.go` around lines 341 - 358, The test case
at the It block for "reflect inplace update ability to machineimage" needs to be
extended to cover the clean-version path. Modify the mockSource.images
SourceImage to include a CleanVersion field set to a different value than the
Version field (e.g., "1.0" while Version is "1.0.0"). After calling
updater.Update and verifying the first version entry, add an additional
assertion to check that the clean-version entry also has
InPlaceUpdates.Supported set to true, ensuring the test exercises both the
legacy append path and the new clean-version path in ImageUpdater.Update.
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 by CodeRabbit
New Features
Chores