Add --take-ownership flag for install and upgrade commands by mayankshah1607 · Pull Request #13439 · helm/helm

mayankshah1607 · 2024-11-12T12:12:48Z

What this PR does / why we need it:

TakeOwnership option was added to the Install and Upgrade actions in #12876. This PR allows setting this option on the CLI commands using a new --take-ownership flag

Special notes for your reviewer:

xref: #10304 (comment)

If applicable:

this PR contains user facing changes (the docs needed label should be applied if so)
this PR contains unit tests
this PR has been tested for backwards compatibility

The `TakeOwnership` setting was added to the install and upgrade actions in helm#12876 This PR allows setting this option on install and upgrade via the CLI using a --take-ownership flag Signed-off-by: Mayank Shah <mayankshah1614@gmail.com>

gjenkins8

very minor fix up of the help text please

cmd/helm/install.go

cmd/helm/upgrade.go

Co-authored-by: George Jenkins <gvjenkins@gmail.com> Signed-off-by: Mayank Shah <mayankshah1614@gmail.com>

banjoh · 2024-12-09T16:06:10Z

@scottrigby / @joejulian are you able to take a look at this one?

banjoh · 2024-12-10T17:51:21Z

@mayankshah1607 since this PR needs to merged into v3, can you please create a new PR to dev-v3 branch with the same changes here?

scottrigby

This PR looks good, however with manual testing this doesn't seem to be working for upgrade, only install. I'm not sure why this would be the case, since it should work according to #12876

Can someone else reproduce this?

# create example chart
helm create foo

# create matching plain template files
helm template foo foo --output-dir foo-template
<remove all helm labels>
<remove tests dir>

# install plan manifests without helm labels or metadata
kubectl apply -f foo-template/foo/templates/

# it makes sense that this wouldn't work
$ helm upgrade foo foo/ --take-ownership
Error: UPGRADE FAILED: "foo" has no deployed releases

# from this PR it seems this should work, but doesn't
$ helm upgrade --install foo foo/ --take-ownership
Error: Unable to continue with install: ServiceAccount "foo" in namespace "default" exists and cannot be imported into the current release: invalid ownership metadata; label validation error: missing key "app.kubernetes.io/managed-by": must be set to "Helm"; annotation validation error: missing key "meta.helm.sh/release-name": must be set to "foo"; annotation validation error: missing key "meta.helm.sh/release-namespace": must be set to "default"

# this works
$ helm install foo foo/ --take-ownership
<expected output>
$ helm ls  
NAME	NAMESPACE	REVISION	UPDATED                             	STATUS  	CHART    	APP VERSION
foo 	default  	1       	2024-12-11 00:21:59.727628 -0500 EST	deployed	foo-0.1.0	1.16.0

PS I'm also updating the description to point to the correct PR (#12876 not #13130), and adding closing keywords to related issues (#10304 and #2730).

scottrigby

For upgrade --install to work, we also need to add this:

diff --git a/cmd/helm/upgrade.go b/cmd/helm/upgrade.go
index 1fe793a92..75788459b 100644
--- a/cmd/helm/upgrade.go
+++ b/cmd/helm/upgrade.go
@@ -151,6 +151,7 @@ func newUpgradeCmd(cfg *action.Configuration, out io.Writer) *cobra.Command {
                                        instClient.Labels = client.Labels
                                        instClient.EnableDNS = client.EnableDNS
                                        instClient.HideSecret = client.HideSecret
+                                       instClient.TakeOwnership = client.TakeOwnership
 
                                        if isReleaseUninstalled(versions) {
                                                instClient.Replace = true

Signed-off-by: Mayank Shah <mayankshah1614@gmail.com>

mayankshah1607 · 2024-12-11T15:27:50Z

Thanks @scottrigby , I've updated the PR

mayankshah1607 · 2024-12-11T15:28:21Z

@mayankshah1607 since this PR needs to merged into v3, can you please create a new PR to dev-v3 branch with the same changes here?

@banjoh can I just change the base of this PR? Or does it need to be created against both main and dev-v3?

banjoh · 2024-12-11T16:09:37Z

@mayankshah1607 since this PR needs to merged into v3, can you please create a new PR to dev-v3 branch with the same changes here?

@banjoh can I just change the base of this PR? Or does it need to be created against both main and dev-v3?

You'll need 2 separate PRs. Changing the PR base will mean your changes go to one branch and not the other.

mayankshah1607 · 2024-12-12T16:28:45Z

Running the tests locally, everything seems to pass. I'm not sure what the issue is, but I pushed a fix anyway.. not sure if that'll do.

Signed-off-by: Mayank Shah <mayankshah1614@gmail.com>

mayankshah1607 · 2024-12-13T07:34:59Z

Ok, tests are fixed. PR against dev-v3 branch is ready: #13531

scottrigby

@mayankshah1607 test fix looks good. Nice work 👏

lucasfcnunes · 2024-12-19T19:09:33Z

@yxxhero take a look at this. Helmfile would greatly benefit from it.

yxxhero · 2024-12-20T01:46:00Z

@yxxhero take a look at this. Helmfile would greatly benefit from it.

ok got it

atsu85 · 2025-01-24T09:00:27Z

if i understand correctly, then official documentation isn't currently updated with this new flag?

Seems just the feature i was looking for - very good timing regarding the release of this feature :)

@mathetake

**Description** As discussed in this slack thread: https://envoyproxy.slack.com/archives/C07Q4N24VAA/p1761235915409479, we want to separate the crd chart and the resources chart. (option 1 of the proposed approaches in #1186) **Related Issues/PRs (if applicable)** #1186 **Special notes for reviewers (if applicable)** Added an extra note on upgrade when done from a previous install of ai-gateway-helm helm chart alone. it uses the `--take-ownership` flag while upgrading: helm/helm#13439, https://helm.sh/docs/helm/helm_upgrade/#options Open Question: - Should we rename `ai-gateway-helm` to `ai-gateway-resources-helm`? It would be more consistent but upgrade is slightly trickier cc: @mathetake @johnugeorge @nagar-ajay Signed-off-by: sailesh duddupudi <saileshradar@gmail.com>

@mathetake

…1431) **Description** As discussed in this slack thread: https://envoyproxy.slack.com/archives/C07Q4N24VAA/p1761235915409479, we want to separate the crd chart and the resources chart. (option 1 of the proposed approaches in envoyproxy#1186) **Related Issues/PRs (if applicable)** envoyproxy#1186 **Special notes for reviewers (if applicable)** Added an extra note on upgrade when done from a previous install of ai-gateway-helm helm chart alone. it uses the `--take-ownership` flag while upgrading: helm/helm#13439, https://helm.sh/docs/helm/helm_upgrade/#options Open Question: - Should we rename `ai-gateway-helm` to `ai-gateway-resources-helm`? It would be more consistent but upgrade is slightly trickier cc: @mathetake @johnugeorge @nagar-ajay Signed-off-by: sailesh duddupudi <saileshradar@gmail.com> Signed-off-by: Erica Hughberg <erica.sundberg.90@gmail.com>

pull-request-size bot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Nov 12, 2024

banjoh approved these changes Nov 12, 2024

View reviewed changes

unclok approved these changes Nov 13, 2024

View reviewed changes

gjenkins8 reviewed Nov 19, 2024

View reviewed changes

cmd/helm/install.go Outdated Show resolved Hide resolved

cmd/helm/upgrade.go Outdated Show resolved Hide resolved

mayankshah1607 and others added 2 commits November 19, 2024 10:03

Update cmd/helm/install.go

7efa286

Co-authored-by: George Jenkins <gvjenkins@gmail.com> Signed-off-by: Mayank Shah <mayankshah1614@gmail.com>

Update cmd/helm/upgrade.go

885e938

Co-authored-by: George Jenkins <gvjenkins@gmail.com> Signed-off-by: Mayank Shah <mayankshah1614@gmail.com>

gjenkins8 previously approved these changes Nov 19, 2024

View reviewed changes

gjenkins8 modified the milestones: 3.17.0, v4 Nov 19, 2024

gjenkins8 mentioned this pull request Dec 2, 2024

Use three way merge for unstructured objects #13411

Open

3 tasks

scottrigby reviewed Dec 11, 2024

View reviewed changes

scottrigby requested changes Dec 11, 2024

View reviewed changes

fix upgrade

2541e46

Signed-off-by: Mayank Shah <mayankshah1614@gmail.com>

mayankshah1607 dismissed gjenkins8’s stale review via 2541e46 December 11, 2024 15:27

fix test output

edf7b66

Signed-off-by: Mayank Shah <mayankshah1614@gmail.com>

pull-request-size bot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Dec 13, 2024

mayankshah1607 mentioned this pull request Dec 13, 2024

v3 backport: Add --take-ownership flag for install and upgrade commands #13531

Merged

banjoh approved these changes Dec 13, 2024

View reviewed changes

gjenkins8 approved these changes Dec 15, 2024

View reviewed changes

scottrigby approved these changes Dec 19, 2024

View reviewed changes

scottrigby removed the Has One Approval This PR has one approval. It still needs a second approval to be merged. label Dec 19, 2024

scottrigby merged commit 9c0d3c4 into helm:main Dec 19, 2024

gjenkins8 removed the Needs v3 backport Label PRs for v4/main, which are still applicable to v3 so need a separate backport PR label Dec 19, 2024

blakepettersson mentioned this pull request Dec 28, 2024

Option to have ArgoCD helm charts with meta.helm.sh annotations argoproj/argo-cd#21250

Closed

FWest98 mentioned this pull request Jan 6, 2025

Allow using --take-ownership flag from Helm helmfile/helmfile#1854

Closed

AnanyaDevGo mentioned this pull request Jan 7, 2025

Improve Test Coverage for --take-ownership Flag in Helm Commands #13610

Closed

3 tasks

This was referenced Jan 8, 2025

Make takeOwnership of conflicting resources configurable fluxcd/helm-controller#1139

Closed

Add disableTakeOwnership to Helm install/upgrade actions fluxcd/helm-controller#1140

Merged

hanseltime mentioned this pull request Jan 14, 2025

(aws-eks): Support --take-ownership flag in new helm version aws/aws-cdk#32918

Open

2 tasks

atsu85 mentioned this pull request Jan 24, 2025

support --take-ownership option like helm install and helm upgrade commands databus23/helm-diff#731

Closed

gjenkins8 mentioned this pull request Feb 1, 2025

Proposal: officially support helm.sh/hook-delete-policy: never #13627

Closed

manno mentioned this pull request Feb 20, 2025

Allow install, update action to adopt existing resources (sdk only) #12876

Merged

karthikmanam mentioned this pull request Mar 12, 2025

Add take ownership tests #30657

Closed

3 tasks

This was referenced Apr 7, 2025

[Helm] Support for --take-ownership flag pulumi/pulumi-kubernetes#3595

Open

Don't delete resources unless field managers match pulumi/pulumi-kubernetes#3407

Open

saileshd1402 mentioned this pull request Oct 24, 2025

refactor: remove /crds folder from ai-gateway-helm chart envoyproxy/ai-gateway#1431

Merged

Conversation

mayankshah1607 commented Nov 12, 2024 • edited by scottrigby Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gjenkins8 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

banjoh commented Dec 9, 2024

Uh oh!

banjoh commented Dec 10, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

scottrigby left a comment

Choose a reason for hiding this comment

Uh oh!

scottrigby left a comment

Choose a reason for hiding this comment

Uh oh!

mayankshah1607 commented Dec 11, 2024

Uh oh!

mayankshah1607 commented Dec 11, 2024

Uh oh!

banjoh commented Dec 11, 2024

Uh oh!

mayankshah1607 commented Dec 12, 2024

Uh oh!

mayankshah1607 commented Dec 13, 2024

Uh oh!

scottrigby left a comment

Choose a reason for hiding this comment

Uh oh!

lucasfcnunes commented Dec 19, 2024

Uh oh!

yxxhero commented Dec 20, 2024

Uh oh!

atsu85 commented Jan 24, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

mayankshah1607 commented Nov 12, 2024 •

edited by scottrigby

Loading

banjoh commented Dec 10, 2024 •

edited

Loading