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] merge nameOverride into fullnameOverride #180

Closed jimethn closed 2 years ago

jimethn commented 2 years ago

What this PR does / why we need it:

Eliminate nameOverride setting and migrate its functionality to fullnameOverride.

The difference between nameOverride and fullnameOverride is not semantically clear, with the former adjusting the app labels while the latter adjusts the resource naming. In practice, this means that if you want to change either, you probably want to change both.

Which issue this PR fixes

Special notes for your reviewer:

Checklist

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

jimethn commented 2 years ago

@DandyDeveloper can you please review?

DandyDeveloper commented 2 years ago

I'm hesitant with this change as the impact is pretty big to people using it. Also, there is justifiable reasons for both.

This sounds more like I should investigate why there is no clear syntactical difference between them.

jimethn commented 2 years ago

@mkilchhofer for a detailed description of the problem being solved, see the issue it closes: https://github.com/DandyDeveloper/charts/issues/167

jimethn commented 2 years ago

The chart works fine without this change as long as it's the only installation of the chart in the namespace. If you ever install a second redis in the same namespace, the two clusters will interfere with each other.

Probably most users only ever install a single redis cluster, which is why more people haven't hit this problem.

mkilchhofer commented 2 years ago

Ok. But it can't happen due to the selector. All selectors contains the .Release.Name.

So when you install the chart twice like this

Sectors should be fine?

DandyDeveloper commented 2 years ago

There's a separate PR that covers the "hijacking" issue a little bit better. I'm closing this in favor of that.