Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[v1.29] Fix: Add Take Ownership #49

Merged
merged 1 commit into from
May 21, 2024

Conversation

krunalhinguu
Copy link

@krunalhinguu krunalhinguu commented May 15, 2024

Updates

  • take-ownership was missing from cmd/install.go an cmd/upgrade.go.

Related

@krunalhinguu krunalhinguu requested a review from a team as a code owner May 15, 2024 02:01
@recena
Copy link
Collaborator

recena commented May 15, 2024

❓ Do we want to continue using a fork?

@@ -195,6 +195,7 @@ func addInstallFlags(cmd *cobra.Command, f *pflag.FlagSet, client *action.Instal
f.BoolVar(&client.SubNotes, "render-subchart-notes", false, "if set, render subchart notes along with the parent")
f.StringToStringVarP(&client.Labels, "labels", "l", nil, "Labels that would be added to release metadata. Should be divided by comma.")
f.BoolVar(&client.EnableDNS, "enable-dns", false, "enable DNS lookups when rendering templates")
f.BoolVar(&client.TakeOwnership, "take-ownership", false, "take ownership will ignore the check for helm annotations and take ownership of the resources")
Copy link
Member

Choose a reason for hiding this comment

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

Why are we adding TakeOwnership ? How is this being used in rancher/rancher codebase ? What change that triggered this PR ?

Copy link
Author

Choose a reason for hiding this comment

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

d8b5b77

ForceAdopt was replaced with TakeOwnership, but there was no support for the tags which was removed for forceAdopt

Copy link
Member

Choose a reason for hiding this comment

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

@krunalhinguu you also mentioned we are running into issues with rancher/shell due to missing vars, could you post the failure/behavior you saw there?

Copy link
Author

Choose a reason for hiding this comment

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

So we are using forceAdopt reference in r/r, which is removed in 3.14.3, so after bumping new rancher/shell image that will become inaccessible during helm upgrade which is failing helm pods

Copy link
Author

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

@krunalhinguu This PR makes sense to me now.

But what I don't understand is how rancher/rancher knows about --take-ownership=true flag since rancher/rancher codebase uses FroceAdopt. Which rancher/rancher branch are you running ?

Copy link
Author

Choose a reason for hiding this comment

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

I have updated the code in r/r and part of 1.29 release

Copy link
Author

Choose a reason for hiding this comment

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

Choose a reason for hiding this comment

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

@rohitsakala can we please get this merged soon. It is blocking work for 1.29 support.
cc: @Sahota1225 @snasovich

@krunalhinguu
Copy link
Author

Didn't get you @recena

@kinarashah
Copy link
Member

@recena This is for rancher/rancher and rancher/shell which are still using forked tags, there are additional changes existing in the fork which we're dependent on so we have to continue using fork until we establish they're no longer needed.

@@ -195,6 +195,7 @@ func addInstallFlags(cmd *cobra.Command, f *pflag.FlagSet, client *action.Instal
f.BoolVar(&client.SubNotes, "render-subchart-notes", false, "if set, render subchart notes along with the parent")
f.StringToStringVarP(&client.Labels, "labels", "l", nil, "Labels that would be added to release metadata. Should be divided by comma.")
f.BoolVar(&client.EnableDNS, "enable-dns", false, "enable DNS lookups when rendering templates")
f.BoolVar(&client.TakeOwnership, "take-ownership", false, "take ownership will ignore the check for helm annotations and take ownership of the resources")
Copy link
Member

Choose a reason for hiding this comment

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

@krunalhinguu This PR makes sense to me now.

But what I don't understand is how rancher/rancher knows about --take-ownership=true flag since rancher/rancher codebase uses FroceAdopt. Which rancher/rancher branch are you running ?

Copy link
Member

@rohitsakala rohitsakala left a comment

Choose a reason for hiding this comment

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

Didn't test the changes, but codebase looks valid.

@kinarashah kinarashah merged commit 075dfd2 into rancher:release-3.14 May 21, 2024
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
5 participants