[v3 backport] Fix rollback for missing resources (31412)#31982
[v3 backport] Fix rollback for missing resources (31412)#31982fmuyassarov wants to merge 1 commit intohelm:dev-v3from
Conversation
|
/cc @banjoh |
There was a problem hiding this comment.
Pull request overview
Backports Helm upstream fix to allow rollbacks to succeed when a resource exists in the cluster but is missing from the “original” release manifest (e.g., failed upgrade removed it from release state).
Changes:
- Adjust
Client.update()to use live cluster state as the baseline when the resource is missing fromoriginal. - Add a unit test covering the rollback-with-missing-original scenario.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| pkg/kube/client.go | Implements cluster-state baseline behavior for rollback updates when original lacks the resource. |
| pkg/kube/client_test.go | Adds a regression test for the rollback scenario where the original manifest is empty. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Signed-off-by: Feruzjon Muyassarov <feruzjon.muyassarov@est.tech>
e0e87bb to
0a47573
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| helper := resource.NewHelper(info.Client, info.Mapping).WithFieldManager(getManagedFieldsManager()) | ||
| if _, err := helper.Get(info.Namespace, info.Name); err != nil { | ||
| currentObj, err := helper.Get(info.Namespace, info.Name) |
There was a problem hiding this comment.
@fmuyassarov curious about another difference from the v4 PR. Specifically why this code changed here, instead of currentObj being set inside of the if original == nil conditional?
There was a problem hiding this comment.
The v3 code was already calling helper.Get() unconditionally earlier in the function to check if the resource needs to be created. The current patch just captures that result instead of discarding it with _, so it's available later in the original == nil branch - so no extra API call needed. In main branch the code structure was different: there was no prior Get() call, so it was added inside original == nil.
What this PR does / why we need it:
Backport of #31412 as discussed over the slack
Special notes for your reviewer:
Two minor/cosmetic differences compared to the original PR that landed on the main:
updateApplyFunc() callback pattern
If applicable:
docs neededlabel should be applied if so)