Skip to content

Conversation

@erikgb
Copy link
Member

@erikgb erikgb commented Dec 29, 2025

Helm v4 contains quite a few breaking changes. In particular, releases are now immutable, forcing me to rewrite the "safe" uninstall introduced in #13.

I am not sure how to test these changes beyond what's covered by the integration tests run in CI. Any help in testing and verifying that this works as expected would be appreciated.

@cert-manager-prow cert-manager-prow bot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. dco-signoff: yes Indicates that all commits in the pull request have the valid DCO sign-off message. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Dec 29, 2025
@erikgb erikgb force-pushed the helm-v4 branch 2 times, most recently from 8882093 to 8bb417b Compare December 29, 2025 19:37
@erikgb erikgb requested a review from Copilot December 29, 2025 19:47
@erikgb erikgb changed the title WIP: Upgrade to Helm v4 Upgrade to Helm v4 Dec 29, 2025
@cert-manager-prow cert-manager-prow bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Dec 29, 2025
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR upgrades cmctl from Helm v3 to Helm v4, addressing breaking changes introduced in the new version. The most significant change is that Helm v4 releases are now immutable, requiring a complete rewrite of the "safe uninstall" functionality that was introduced in PR #13.

Key changes:

  • Updates Helm dependency from v3.19.4 to v4.0.4
  • Rewrites CRD annotation logic to modify CRDs directly via Kubernetes API instead of modifying the Helm release manifest
  • Migrates to Helm v4's new Accessor API pattern for accessing release and chart metadata
  • Replaces deprecated Wait/Atomic/DryRun flags with new WaitStrategy, RollbackOnFailure, and DryRunStrategy patterns

Reviewed changes

Copilot reviewed 8 out of 9 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
go.mod Updates Helm dependency to v4.0.4 and adds new transitive dependencies
go.sum Updates checksums for Helm v4 and its new dependencies
pkg/uninstall/uninstall.go Rewrites addCRDAnnotations to modify CRDs directly using dynamic client instead of modifying release manifest; migrates to WaitStrategy and Accessor APIs
pkg/install/install.go Migrates to Helm v4 chart and release Accessor APIs; replaces DryRun/Wait/Atomic flags with new DryRunStrategy/WaitStrategy/RollbackOnFailure patterns
pkg/install/util.go Updates Helm import paths to v4
pkg/install/helm/settings.go Removes logger callback from ActionConfiguration.Init (no longer supported in v4)
pkg/install/helm/resource.go Updates Helm import path to v4
pkg/install/helm/applycrd.go Migrates to WaitStrategy pattern using GetWaiter method
test/integration/ctl_uninstall_test.go Updates expected test output to match new uninstall behavior

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 8 out of 9 changed files in this pull request and generated 4 comments.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 8 out of 9 changed files in this pull request and generated 3 comments.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 8 out of 9 changed files in this pull request and generated 3 comments.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Signed-off-by: Erik Godding Boye <egboye@gmail.com>
@erikgb
Copy link
Member Author

erikgb commented Dec 29, 2025

/cc @wallrj @inteon

@cert-manager-prow cert-manager-prow bot requested review from inteon and wallrj December 29, 2025 20:56
if err := o.settings.ActionConfiguration.Releases.Update(lastRelease); err != nil {
o.settings.ActionConfiguration.Log("uninstall: Failed to store updated release: %s", err)
for _, name := range nonAnnotatedCRD {
crd, err := crdClient.Get(ctx, name, metav1.GetOptions{})
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this might not work: afaik, Helm looks at the templated resources in the release to determine if it has to keep a resource or not.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

tried to fix this here: 4454638

@inteon
Copy link
Member

inteon commented Jan 9, 2026

@erikgb I did a review a while ago, but might not have submitted it and lost it.
Re-added the most important comment, will take a look at the code now to see if I can replicate/ fix this myself.

inteon added 2 commits January 9, 2026 10:23
Signed-off-by: Tim Ramlot <42113979+inteon@users.noreply.github.com>
Signed-off-by: Tim Ramlot <42113979+inteon@users.noreply.github.com>
Signed-off-by: Tim Ramlot <42113979+inteon@users.noreply.github.com>
@inteon
Copy link
Member

inteon commented Jan 9, 2026

/approve
/lgtm
/hold @erikgb feel free to unhold if you are ok with my changes

@cert-manager-prow cert-manager-prow bot added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. lgtm Indicates that a PR is ready to be merged. labels Jan 9, 2026
@cert-manager-prow
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: inteon

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@cert-manager-prow cert-manager-prow bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jan 9, 2026
@erikgb
Copy link
Member Author

erikgb commented Jan 9, 2026

Thanks a lot, @inteon! I think your changes look good, but I am not a Helm-expert. The build is happy, and if we detect any issues with this, we should just fix it. Do you want to take a look at cert-manager/release#225 while you are in Helm v4 mode? 😉

/unhold

@cert-manager-prow cert-manager-prow bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jan 9, 2026
@cert-manager-prow cert-manager-prow bot merged commit 0bc5fb4 into cert-manager:main Jan 9, 2026
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. dco-signoff: yes Indicates that all commits in the pull request have the valid DCO sign-off message. lgtm Indicates that a PR is ready to be merged. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants