-
Notifications
You must be signed in to change notification settings - Fork 318
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: add the namespace resource within helm templates #1332
Conversation
This is unfortunate workaround due the difference in UX between `helm template` and `helm install` The project recommends `helm install` as a way to install EG which supports a `--create-namespace` flag to create a namespace However we also generate a static YAML using `helm template` as part of the release artficat so a user can install the YAML directly using `kubectl` instead of `helm` . The issue here is `helm template` does not support `--create-namespace`, so instead this commit adds a knob called `createNamespace` to the Helm chart which is `false` by default, but turned on during `make generate-manifests` Fixes: envoyproxy#1307 Signed-off-by: Arko Dasgupta <arko@tetrate.io>
Codecov Report
@@ Coverage Diff @@
## main #1332 +/- ##
==========================================
+ Coverage 61.80% 61.82% +0.01%
==========================================
Files 85 85
Lines 10853 10854 +1
==========================================
+ Hits 6708 6710 +2
Misses 3699 3699
+ Partials 446 445 -1 see 3 files with indirect coverage changes Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
@@ -0,0 +1,6 @@ | |||
{{ if .Values.createNamespace }} |
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.
maybe I'm out of date, I recall it's not a good practice to put namespace in a helm chart.
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.
it is not, but we are using helm
as the source of truth for manifest templates, in the previous release it was kustomize
- for the common case using
helm install
we skip this file (createNamespace
isfalse
) - for the manifest generation case to support install via 1 single YAML, we rely on it
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 for now
we need review the helm chart in v0.5, this and the crds
folder.
Thanks @arkodg |
Add the namespace resource within helm templates This is unfortunate workaround due the difference in UX between `helm template` and `helm install` The project recommends `helm install` as a way to install EG which supports a `--create-namespace` flag to create a namespace However we also generate a static YAML using `helm template` as part of the release artficat so a user can install the YAML directly using `kubectl` instead of `helm` . The issue here is `helm template` does not support `--create-namespace`, so instead this commit adds a knob called `createNamespace` to the Helm chart which is `false` by default, but turned on during `make generate-manifests` Fixes: #1307 Signed-off-by: Arko Dasgupta <arko@tetrate.io> (cherry picked from commit 9d6d699) Signed-off-by: AliceProxy <alicewasko@datawire.io>
* Extension: fix pointer error (#1323) (cherry picked from commit 2bf9607) Signed-off-by: AliceProxy <alicewasko@datawire.io> * fix: add the namespace resource within helm templates (#1332) Add the namespace resource within helm templates This is unfortunate workaround due the difference in UX between `helm template` and `helm install` The project recommends `helm install` as a way to install EG which supports a `--create-namespace` flag to create a namespace However we also generate a static YAML using `helm template` as part of the release artficat so a user can install the YAML directly using `kubectl` instead of `helm` . The issue here is `helm template` does not support `--create-namespace`, so instead this commit adds a knob called `createNamespace` to the Helm chart which is `false` by default, but turned on during `make generate-manifests` Fixes: #1307 Signed-off-by: Arko Dasgupta <arko@tetrate.io> (cherry picked from commit 9d6d699) Signed-off-by: AliceProxy <alicewasko@datawire.io> --------- Signed-off-by: Arko Dasgupta <arko@tetrate.io> Signed-off-by: AliceProxy <alicewasko@datawire.io> Co-authored-by: Arko Dasgupta <arkodg@users.noreply.github.com>
This is an unfortunate workaround due the difference in UX between
helm template
andhelm install
The project recommends
helm install
as a way to install EG which supports a--create-namespace
flag to create a namespace However we also generate a static YAML usinghelm template
as part of the release artifact so a user can install the YAML directly usingkubectl
instead ofhelm
. The issue here ishelm template
does not support--create-namespace
, so instead this commit adds a knob calledcreateNamespace
to the Helm chart which isfalse
by default, but turned on duringmake generate-manifests
Fixes: #1307