ibm-messaging / mq-helm

Apache License 2.0
28 stars 34 forks source link

Add namespace metadata to the templates #28

Closed JGrzybowski closed 1 year ago

JGrzybowski commented 1 year ago

Without this field running helm template --namespace <namespace> will not fill it. See https://github.com/helm/helm/issues/10737

It should not affect any examples using helm. I'm not sure about direct kubectl calls. Should I update examples to use --namespace?

Also, what kind of chart version bump should I add if this PR would get to be accepted?

JGrzybowski commented 1 year ago

I accept the CLA Please add label hacktoberfest or hacktoberfest-accepted

callumpjackson commented 1 year ago

Thanks for raising this pull request and spending the time. Sorry for the delay in responding, but if to explicitly specify a namespace within generated resources or not is a hotly debated topic. I wanted to refresh my memory on the discussion prior to responding. The question of a Helm Best Practice in this area is discussed here in issue 5465. Although @bacongobbler received some push back on his comments, I think he is the defacto owner and therefore gets to declare Good Practice: In general, templates should not define a namespace. This is because Helm installs objects into the namespace provided with the --namespace flag. By omitting this information, it also provides templates with some flexibility for post-render operations (like helm template | kubectl create --namespace foo -f -)

In my head one major reason to explicitly declare a namespace would be if the Helm Chart needed to deploy resources across multiple namespaces, in another GitHub issues 3553, he highlights that charts are generally restricted to one namespace: Compared to kubectl, Helm does not have strict enforcement enabled. We tried to do that for Helm 3 but had to back out as people were deploying apps across multiple namespaces, breaking a long-standing assumption in Helm 2 that charts should be restricted to a namespace. Enforcing it meant breaking user's charts, which we avoided with the Helm 3 update. So we had to remove that constraint in Helm 3.0.0-alpha.2.

Regardless the IBM MQ Helm Chart only deploys into a single namespace, and therefore I don't think that argument holds too much water.

Within the sample script you will see that we set kubectl default namespace to the one we are targeting the deployment for. There are numerous ways to do this which accomplish the same thing, but as the core Helm organization do not appear to support it, explicitly adding a namespace to each resource does not appear to be the best.

Am I missing anything?

JGrzybowski commented 1 year ago

I see, I didn't know there was such official recomendation. I'll close the PR then.