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

docs/README: add missing "--namespace" flag to "helm" command #486

Merged
merged 1 commit into from
Apr 23, 2020
Merged

Conversation

gyuho
Copy link
Member

@gyuho gyuho commented Apr 22, 2020

No description provided.

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Apr 22, 2020
@k8s-ci-robot k8s-ci-robot requested review from d-nishi and wongma7 April 22, 2020 02:41
@k8s-ci-robot k8s-ci-robot added the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label Apr 22, 2020
@coveralls
Copy link

Pull Request Test Coverage Report for Build 1079

  • 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 78.871%

Totals Coverage Status
Change from base Build 1078: 0.0%
Covered Lines: 1411
Relevant Lines: 1789

💛 - Coveralls

@wongma7
Copy link
Contributor

wongma7 commented Apr 23, 2020

/lgtm
looks like all the templates have namespace: kube-system set...but I don't see the harm in the helm install command being explicit!

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Apr 23, 2020
@wongma7
Copy link
Contributor

wongma7 commented Apr 23, 2020

/approve

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: gyuho, wongma7

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 23, 2020
@k8s-ci-robot k8s-ci-robot merged commit 7b693e9 into kubernetes-sigs:master Apr 23, 2020
@gyuho gyuho deleted the readme branch April 26, 2020 09:03
@TBBle
Copy link

TBBle commented May 4, 2020

Would it be worthwhile putting a check in the NOTES.txt that causes a Helm render error if you don't provide a namespace? With Helm v3, the --namespace parameter also sets the namespace used to hold the secrets with the chart data, and so if you set this differently, helm ls will report a different namespace to where all the resources are.

Conversely, does the EBS-CSI driver have to be in kube-system? The other option would be to fix the templates to use {{.Release.Namespace}} so that they are installed wherever the user specifies, which would allow using, e.g. ebs-csi-system, as a namespace for the driver and all its resources.

@wongma7
Copy link
Contributor

wongma7 commented May 4, 2020

No, it doesn't have to be in kube-system. I think helm best practice is to not specify namespace: in the templates at all then the namespace is determined by the --namespace flag or whatever your current kube context is. But I don't keep up with helm and I have to defer to people who actually use it.

@TBBle
Copy link

TBBle commented May 5, 2020

Ah, you are correct, excluding the namespace does do the right thing, and honours the --namespace flag, and the default Helm chart created by helm create does not have namespace metadata fields in its objects.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. 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.

5 participants