-
Notifications
You must be signed in to change notification settings - Fork 247
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
helm: add namespace override for multi-namespace deployments #831
Conversation
Welcome @jasine! |
Hi @jasine. Thanks for your PR. I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @jasine for the patch! Looks pretty straightforward to me 👍
Just one small nit about the commit message, I didn't get what feat refers to 😊 I'd suggest to change the topic to e.g. helm
helm: add namespace override for multi-namespace deployments
/ok-to-test
Oh, and one more thing about the commit message: you could add the description of this PR in the commit message, too (just start sentences with a Capital letter). We rarely have too long and descriptive commit messages 😉 |
When used as other charts' dependency, helm will install manifests of this chart to parent chart's namespace, if subchart needs to install to another namespace, helm recommend to use namespaceOverride (helm/charts#15202)
@marquiz thanks for your advice, I have changed the commit message and description |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the update and nice contribution @jasine. I think this is good to go
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: jasine, marquiz The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
/assign @adrianchiris |
@marquiz: GitHub didn't allow me to request PR reviews from the following users: eliaskoromilas. Note that only kubernetes-sigs members and repo collaborators can review this PR, and authors cannot review their own PRs. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
Generally, it's not the best practice to escape the Helm namespace. I'm not against providing this as an option though! What I like in this PR is that finally |
is there anything I can do to help merging this pr @marquiz |
Sorry for the silence. Just back from a long summer holiday... Thanks for the comment @eliaskoromilas! I take that as a lgtm(?) (at least a weak signal 😄). I'm not a Helm wizard myself so I like to have a second opinion from someone more educated |
yes, looks good :) |
OK, let's then merge this. Especially as it's not changing any default behavior |
when use this helm chart as other charts' dependency(Helm Dependency), helm will install manifests of this chart to parent chart's namespace, because .Release.Namespace is parent chart's namespace. so, if we want this chart installed to a separated namespace, community of helm project recommend add namespaceOverride (helm/charts#15202), and namespaceOverride is in helm template chart by default now.
many helm charts support namespaceOverride, such as kube-prometheus-stack