-
Notifications
You must be signed in to change notification settings - Fork 7.7k
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
Refine Helm Charts: Add missing namespaces. #4606
Conversation
Codecov Report
@@ Coverage Diff @@
## master #4606 +/- ##
=======================================
- Coverage 75% 72% -3%
=======================================
Files 297 295 -2
Lines 24742 25217 +475
=======================================
- Hits 18524 18099 -425
- Misses 5453 6359 +906
+ Partials 765 759 -6
Continue to review full report at Codecov.
|
.gitignore
Outdated
@@ -48,6 +48,10 @@ install/kubernetes/istio-sidecar-injector-configmap-release.yaml | |||
install/kubernetes/istio-sidecar-injector.yaml | |||
install/kubernetes/istio.yaml | |||
install/kubernetes/istio-all.yaml | |||
install/kubernetes/orig_istio-auth.yaml |
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.
Can we generate files under $OUT and not in the source tree ?
Adding more people - I don't think we have tiller tests - or if it works with tiller. We need to keep it working for both |
@costin the helm charts worked with tiller a few weeks ago. I'll test this PR later tonight against tiller - I've got a few errands to run. I don't think there is harm in hard-coding the namespaces, although typically with helm -n argument is used to specify the namespace (and is automatically set in that condition IIRC). I could be wrong on th eabove - the helm charts went through a large refactor. I'll report back this evening. otherwise lgtm |
Why do we need namespace embedded in the artifacts ? What is the helm best practice here? |
This change is to make sure that the generated yamls from Istio release are correct (i.e. user can use kubectl apply -f istio.yaml without specifying namespaces). Currently some of them are tagged with namespaces and some are not. The kubectl -n and helm -n should still work as before. It is not breaking the e2e tests only because those test are always using kubectl -n to set up the environment. |
Btw, it seems that helm install -n and Helm template -n have different behavior. The previous one will tag all resources with correct namespace while the later won't unless you specifiy {{.Release.Namespace}} explicitly. |
There is a related issue in Helm: helm/helm#3553 |
I think we should follow Helm practices - if it means 'kubectl apply -n istio-system' - it's fine with me. If helm recommends including the namespace in template - good with me as well. |
It seems that a new file is added by previous PR #4001 (install/kubernetes/helm/istio/templates/namespace.yaml), which broke the helm install. Because helm -n is trying to create namespace which already exits. istio$ helm install install/kubernetes/helm/istio --namespace=istio-system After I deleted that file, it passes with this PR. However, we need to find a way to generate yaml from templates correctly. |
Fixed the above namespace issue in #4612 by moving the namespace file out of helm chart. |
Having a Namespace manifest is currently problematic with Helm as you have discovered. Just using |
@jascott1 This PR is not adding the namespace manifest but only to specify the metata.namespace field in k8s resources. Do you think it is OK to do that? |
This namespaces are part of the RBAC rules, so this fix is required. We cannot simply get a yaml file and apply it with -n to a namespace. |
@yusuoh Yes there is no problem with that approach afaik. |
BTW I corrected my above post, just for clarity its |
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.
/lgtm
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: costinm 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 |
/test all [submit-queue is verifying that this PR is safe to merge] |
Automatic merge from submit-queue. |
Automatic merge from submit-queue. Refine Helm Charts: Add missing namespaces.
No description provided.