search

DandyDeveloper / charts

Various helm charts migrated from [helm/stable] due to deprecation
https://dandydeveloper.github.io/charts
Apache License 2.0
156 stars 143 forks source link

[charts/redis-ha] add namespace to missing resources #177

Closed jimethn closed 2 years ago

jimethn commented 2 years ago

What this PR does / why we need it:

Not every resource had a namespace set. When installing to a non-default namespace, this caused unexpected behavior.

Special notes for your reviewer:

Have a nice day.

Checklist

[Place an '[x]' (no spaces) in all applicable fields. Please remove unrelated fields.]

DandyDeveloper commented 2 years ago

@jimethn Sorry this is so old and I haven't replied. What's the objective of this change though? What unexpected behaviour are you describing?

I'm closing this for housekeeping. Please reopen if you feel this is still needed.

jimethn commented 2 years ago

@DandyDeveloper if you leave metadata.namespace blank, then kubernetes puts your resource in namespace called "default".

Let's say I use the "redis" namespace instead. If I try to install this chart into the "redis" namespace, then the StatefulSet will go into the "redis" namespace, while the PodSecurityPolicy will go into the default namespace. This is not what the user would ever want.

jimethn commented 2 years ago

/reopen

jimethn commented 2 years ago

/lifecycle reopen

DandyDeveloper commented 2 years ago

@DandyDeveloper if you leave metadata.namespace blank, then kubernetes puts your resource in namespace called "default".

Let's say I use the "redis" namespace instead. If I try to install this chart into the "redis" namespace, then the StatefulSet will go into the "redis" namespace, while the PodSecurityPolicy will go into the default namespace. This is not what the user would ever want.

Are you sure? What version of Helm are you using?

Helm should, by default, install all the resources defined either by the --namespace flag OR whatever your KUBECONFIG has defined as the current namespace you're inside.

jimethn commented 2 years ago

@DandyDeveloper in my case I was using a kustomize ChartRenderer, it probably has to do with some interaction there. To fix it, I had to fork the chart and add the namespace to the resources that were missing it.

(Note that the rest of the resources in this chart already specify the namespace, so consistency might also be a good rationale to merge this.)