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(helm): chart should respect -n <namespace> flag #53

Merged
merged 1 commit into from
Oct 25, 2022
Merged

Conversation

zbindenren
Copy link
Member

@zbindenren zbindenren commented Oct 25, 2022

Namespace should be configured via -n helm flag.

I kept the namespace value in values.yaml for backward compatibility reasons. But it would probably be better to set as

namespace: ""

Then, per default, the Releases.Namespace value is taken and you still can overwrite it in values.yaml.

Currently, if someone wants to install the helm chart, it gets installed in kube-system namespace. Even if you specify -n other-namespace . You have to actively set namespace: "" in values.yaml to make it installable in a different namespace via helm flags.

@djboris9 let me know what you think.

Edit:

Best practices is discussed here. I get the feeling that namespace: {{ .Releases.Namespace }} would be best.

@coveralls
Copy link

Pull Request Test Coverage Report for Build 3320802895

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 80.603%

Totals Coverage Status
Change from base Build 3318277956: 0.0%
Covered Lines: 428
Relevant Lines: 531

💛 - Coveralls

@zbindenren zbindenren requested a review from djboris9 October 25, 2022 12:21
@djboris9
Copy link
Collaborator

Nice solution @zbindenren 🍻

@djboris9 djboris9 merged commit a5a3a79 into master Oct 25, 2022
@zbindenren zbindenren deleted the namespace branch December 7, 2022 09:08
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.

3 participants