Skip to content

Support helm release upgrade on helm chart name change#1447

Open
MichaelMorrisEst wants to merge 1 commit intofluxcd:mainfrom
Nordix:issue-870
Open

Support helm release upgrade on helm chart name change#1447
MichaelMorrisEst wants to merge 1 commit intofluxcd:mainfrom
Nordix:issue-870

Conversation

@MichaelMorrisEst
Copy link
Contributor

Summary

Closes #870

Proposes to add a new feature gate UninstallOnChartNameChange.

When enabled (default), the current behaviour of uninstalling a Helm Release when the Helm chart name is changed shall remain unchanged. This results in a new Helm Release install.
When disabled, the Helm Release will not be uninstalled when the Helm chart name is changed. This results in a Helm upgrade being performed. This was the behaviour before Flux 2.2.0

This re-introduces support that was lost going from Flux 2.1.x -> 2.2.x but controls the behaviour through a feature gate so either the previous or current behaviour (default) can be achieved

Changes

  • Define UninstallOnChartNameChange feature gate in internal/features/features.go
  • Wire the feature gate from main.go to HelmReleaseReconciler
  • When the flag is false and the chart name has changed do not uninstall the HelmRelease
  • Change the visibility of action.targetChartName to enable it to be referenced in the HelmRelease controller
  • Test added for verify new behaviour

This re-introduces support that was lost going from 2.1.x -> 2.2.x but controls the behaviour through a feature gate so either the previous or current behaviour (default) can be achieved

Signed-off-by: MichaelMorris <michael.morris@est.tech>
Copy link
Member

@hiddeco hiddeco left a comment

Choose a reason for hiding this comment

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

Feedback beyond the contents of the PR: I do wonder if there are not other changes (e.g., release name/namespace or storage namespace) where there could be a desire of control. Given this, I do wonder if what this PR attempts to do isn't too narrow.

// opt-in from v1.5.2
DefaultToRetryOnFailure: false,
// UninstallOnChartNameChange
// opt-in from v1.5.4
Copy link
Member

Choose a reason for hiding this comment

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

The chance of this going into a patch release is unlikely, this should thus indicate 1.6.0.

targetReleaseNamespace = "release namespace"
targetReleaseName = "release name"
targetChartName = "chart name"
TargetChartName = "chart name"
Copy link
Member

Choose a reason for hiding this comment

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

This list of constants was always meant to be a descriptive reason (factored out to a const for code readability), not an identifier. Additionally, only exposing one of them creates a weak contract. I would prefer a solution where:

  1. All identifiers are public
  2. Identifiers do not contain spaces

@hiddeco
Copy link
Member

hiddeco commented Mar 19, 2026

Some additional thoughts:

  • For storage namespace changes, we could actually do the more intelligent thing and attempt to migrate instead of going through an uninstall cycle. This would open a surface for "Secret smuggling" however, and integrity would likely have to be verified as part of this.
  • In hindsight, the biggest design mistake I ever introduced in the helm-controller (and something that still bites me as an operator once in a while) is not having the storageNamespace field mirror the releaseNamespace unless explicitly overwritten. If we are rethinking how this all works, this might be worth addressing as well (behind a feature gate).
  • Some of these changes may actually require an uninstall (or juggling with Helm install options). As changing a release name would make the resources stay put, which could then cause the subsequent Helm install to fail.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Can't change chart name without triggering uninstall (flux 2.2.x)

2 participants