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

Default to helm release's namespace for templates where ns unspecified #1733

Merged
merged 5 commits into from
Sep 30, 2021

Conversation

viveklak
Copy link
Contributor

@viveklak viveklak commented Sep 28, 2021

Fix #1710

Curious to hear feedback on this approach.

Here we essentially set the kubeconfig used by helm to use the Helm Release's namespace. This means for namespaced objects created where namespace is not specified, the Helm Release's namespace is used. This is essentially the equivalent of helm template <chart> | kubectl create -n <helmrelease.namespace> -f -. This avoids having to use post render magic to manipulate templates. This also mirrors the behavior that Flux provides.

@github-actions
Copy link

Does the PR have any schema changes?

Looking good! No breaking changes found.
No new resources/functions.

1 similar comment
@github-actions
Copy link

Does the PR have any schema changes?

Looking good! No breaking changes found.
No new resources/functions.

@viveklak
Copy link
Contributor Author

Could it be because get doesn't await the release operation correctly? Do we have no means to fetch resources from the Release resource itself? Or track its completion?

The id interpolate will wait on status.name and namespace which will only be set after the release is created. Its not an await issue since it seems to happen even if I hardcode the namespace/name. I might be doing something stupid like a typo. I will investigate further today.

Release delegates the resource creation to helm and so we don't have a great way to get child resources directly from it.

@github-actions
Copy link

Does the PR have any schema changes?

Looking good! No breaking changes found.
No new resources/functions.

Copy link
Member

@lblackstone lblackstone left a comment

Choose a reason for hiding this comment

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

Provider changes look reasonable. One question about the test.

tests/sdk/nodejs/examples/helm-release-prometheus/index.ts Outdated Show resolved Hide resolved
@gitfool
Copy link

gitfool commented Sep 28, 2021

Curious to hear feedback on this approach.

Sounds like a great idea, especially if that's what flux does.

@github-actions
Copy link

Does the PR have any schema changes?

Looking good! No breaking changes found.
No new resources/functions.

2 similar comments
@github-actions
Copy link

Does the PR have any schema changes?

Looking good! No breaking changes found.
No new resources/functions.

@github-actions
Copy link

Does the PR have any schema changes?

Looking good! No breaking changes found.
No new resources/functions.

@viveklak viveklak merged commit e963a47 into master Sep 30, 2021
@pulumi-bot pulumi-bot deleted the vl/Fix1710 branch September 30, 2021 23:01
@gitfool
Copy link

gitfool commented Oct 1, 2021

@viveklak I took this for a test run and helm install with a namespace override works great! ⭐

However, when destroying the stack helm uninstall deleted the helm release but left resources behind for some charts like fluent bit where I was overriding the namespace. If instead I manually uninstall by specifying the namespace then it removes everything:

helm uninstall --namespace kube-system fluent-bit

So a tweak is needed to override the namespace for helm uninstall too.

@viveklak
Copy link
Contributor Author

viveklak commented Oct 1, 2021

@viveklak I took this for a test run and helm install with a namespace override works great! ⭐

However, when destroying the stack helm uninstall deleted the helm release but left resources behind for some charts like fluent bit where I was overriding the namespace. If instead I manually uninstall by specifying the namespace then it removes everything:

helm uninstall --namespace kube-system fluent-bit

So a tweak is needed to override the namespace for helm uninstall too.

Thanks for the quick feedback @gitfool. Certainly makes sense and sorry for the oversight. Looks like we missed this in the tests since the namespace was created in the stack. Opened #1744 to track this.

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.

[Helm/Release] Resources without explicit namespace are created in default namespace
4 participants