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

Added Release.Namespace to all namespaced resources in the helm chart #119

Merged
merged 1 commit into from
Oct 6, 2021

Conversation

ecojan
Copy link
Contributor

@ecojan ecojan commented Sep 16, 2021

Added {{ .Release.Namespace }} field to all namespaced resources to take into account the namespace value passed at templating.

@ecojan
Copy link
Contributor Author

ecojan commented Oct 1, 2021

Hi @ricoberger , can you have a look at this PR?

@ricoberger
Copy link
Owner

Hi @ecojan, thanks for your contribution and sorry that I missed your PR.

I'm not quite sure why the namespace should be needed in all the files. Isn't it recommended by Helm to not set the namespace?

@ecojan
Copy link
Contributor Author

ecojan commented Oct 5, 2021

Hi @ricoberger ,
I only added the namespace for namespaced resources to allow templating or installs to be able to specify a namespace where one would like the resources to go (mostly important if someone decides to use something like a CI/CD solution and they don't have access to an entire cluster but they wish to deploy the helm chart to a specific namespace).

Regarding the discussions on namespace for helm charts in helm3 I've noticed, there have been a lot of discussions around it.
The main issue was that the feature itself was removed initially in helm3 (design choice so that helm would not create extra namespaces), the community challenged this decision, and --namespace is a valid option again but the creation of a namespace is under an opt-in flag (as it should be).

You can read the discussion on helm/helm#6794

I currently tested out and validated the change with helm version: v3.6.3.

@ricoberger
Copy link
Owner

Hi @ecojan, thanks for the clarification and your contribution 🙂

@ricoberger ricoberger merged commit 21c8c76 into ricoberger:master Oct 6, 2021
@ecojan ecojan deleted the release_namespace_helm branch October 7, 2021 08:35
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.

2 participants