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

Fix: uninstall action resource namespace #12783

Conversation

PaulBarrie
Copy link

closes#12766

At init configuration the kubeclient namespace is not set. When the uninstall action is triggered, if the release has been installed in a non-default namespace, the resource without any namespace provided (which are installed in the release's namespace) are not deleted since it tries to delete resources in the namespace "".

@pull-request-size pull-request-size bot added the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label Feb 7, 2024
Signed-off-by: Paul Barrié <paul.barrie.calmels@gmail.com>
@rain-on
Copy link

rain-on commented May 16, 2024

I've run the provided fix and confirmed:

  • the issue is able to be replicated prior to the fix being applied, and
  • provided fix resolves the issue.

Effectively 2 namespace values are passed into the cfg.init:

  1. Via the ClientGetter (aka environment/context defined)
  2. Via the namespace parameter

The namespace parameter is used by the driver, while the KubeClient gets its namespace from the ClientGetter - so this change, to align the values seems reasonable.

All commands (bar "List") force the namespace parameter to be the same as the context/environment provided value - so generally these two values are the same.

There is some code in the Client which manages "use override namespace or value from environment", which maybe able to be removed if an "merged" config was created upfront (rather than managing 2 namespace variables through the codebase).

Copy link

@rain-on rain-on left a comment

Choose a reason for hiding this comment

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

LGTM

@robertsirc
Copy link
Contributor

This is done else where in the code this should be rejected.

Copy link
Collaborator

@mattfarina mattfarina left a comment

Choose a reason for hiding this comment

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

This is a common ask but it's not so simple to set it here. The Initialized configuration can be used in multiple actions in a log running process. Such as a daemon that uses this as an SDK.

This initialization is for where the record of the release is stored instead of the manifests that are generated and uploaded. They can technically be in different places and the SDK allows for that.

You can see an example of how the actions put the namespace in over at

err = resources.Visit(setMetadataVisitor(rel.Name, rel.Namespace, true))

I don't think this setup is ideal but we can't break the intent until Helm v4 where we can make some broader changes.

@mattfarina mattfarina closed this Jun 7, 2024
@PaulBarrie PaulBarrie deleted the fix/uninstall_action_resource_namespace branch June 11, 2024 09:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
size/XS Denotes a PR that changes 0-9 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants