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

Support deployment to a namespace #42

Merged
merged 1 commit into from
Dec 5, 2021
Merged

Support deployment to a namespace #42

merged 1 commit into from
Dec 5, 2021

Conversation

mrsimonemms
Copy link
Contributor

@mrsimonemms mrsimonemms commented Dec 3, 2021

The charts didn't specify metadata.namespace anywhere so have added support for this. Have used the {{ .Release.Namespace }} variable which defaults to default if not set. This makes it a non-intrusive change for anyone not wanting to use it, but a useful change for those who do

Fixed #41

Have tested against my own cluster with these commands:

Default namespace

helm upgrade \
  --atomic \
  --cleanup-on-fail \
  --install \
  --wait \
  registry .

This deploys registry to default namespace

Custom namespace

helm upgrade \
  --atomic \
  --cleanup-on-fail \
  --create-namespace \
  --install \
  --namespace registry \
  --wait \
  registry .

This deploys registry to registry namespace

canterberry
canterberry previously approved these changes Dec 4, 2021
Copy link
Member

@canterberry canterberry left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To install a Helm chart into a specific namespace, can you not pass --namespace ... to the helm install/upgrade command? This should, in theory, install all resources to that namespace, as well as the Helm release metadata. If my understanding is correct, the changeset should be a no-op (i.e: since it reflects the default behavior anyway). If the chart currently just installs resources to the default namespace anyway, though, that's certainly undesirable!

In all the Helm charts I have written, I have done as you have proposed in this changeset, so I am inclined to approve. Regardless of whether my assumption above is correct regarding the default behavior, I do think being explicit about the namespace is an improvement.

However, I've read that having the resource namespace match the release metadata namespace is not always desirable. Helm release secrets do accumulate over time, polluting the release namespace. To split that, you'd have essentially the same changeset as here, but {{ .Values.namespace | default .Release.Namespace }}, and introduce an optional namespace value that chart users can set if desired. That solution, I think, would take care of all desired namespacing scenarios.

@mrsimonemms
Copy link
Contributor Author

@canterberry specifying --namespace may well work with you're using it with helm install or helm upgrade, but it doesn't seem to work with helm template which is my specific use-case - see gitpod/installer for more details.

I've not come across what you've said about the resource/release namespace matching not being desirable, but I can see that it could be a problem with a library. I'll make the change you suggested and document it in the values.yaml

canterberry
canterberry previously approved these changes Dec 5, 2021
Signed-off-by: Simon Emms <simon@simonemms.com>
@mrsimonemms
Copy link
Contributor Author

@canterberry apologies, I hadn't signed the commit - done now

@canterberry canterberry merged commit cfb7daa into twuni:main Dec 5, 2021
@canterberry
Copy link
Member

Released in v1.15.0.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

Add namespaces to metadata
2 participants