bugfix: Override subcharts with null values

[ryanhockstad]

This PR closes #12469 and closes #12488

Helm should allow users to not only override default values, but also completely remove any default values by setting a config to null.

This works fine for regular charts, but default values within sub-charts cannot be null-ed. The linked issue has a good example of this created by user "naemono."

The reason this issue is happening is because the coalesce function goes over sub-chart values that are defined in a values file or with a --set flag twice due to the logic here. merge is always set to false in this context, and the first time coalesce gets called, the null value gets removed during the coalesceTablesFullKey function here. This is fine for regular chart values, but for sub-chart values, the coalesceDeps function runs the logic again for every value in the subChart, so we end right back to the spot in the previous link. But the config option that was explicitly set to null was already removed, so we end up at this point two lines down, which overwrites the user defined null config the sub-chart default value.

This problem only occurs because when the coalesce function is run while merge is false.

To prevent this bug from removing sub-chart config values that are explicitly set to null and therefore getting overwritten when coalesceDeps runs through the sub-chart values a second time, I added logic to check if the key was the name of a sub-chart, and if so setting merge to false so the null config option is not deleted.
 

Special notes for your reviewer:
Follow-up if this PR is accepted may be to add a unit-test to confirm sub-chart nulls aren't overwritten.

[messiahUA]

When this could be merged?

@joejulian @mattfarina

[AndrewFarley]

Please merge/release ASAP. All builds from 3.13 onwards are "effectively" broken for many of my clients because of this.

[aabouzaid]

I hope to see this in the next Helm release, there are many workarounds used to overcome this bug.

[duplabe]

I think this is also related #12991

[AndrewFarley]

Beep boop we need this, boop beep

[AndrewFarley]

Ahh, alas, another week goes by, and yet more DevOps engineers are fraught with this unnecessary pain in having to continue to use such an old version of Helm without this bug. Beep boop, someone merge and release this, boop beep.

[AndrewFarley]

Another week nudge on this. Sorry to be the annoying jerk, but this is important.

[Tawmu]

Sorry to go against etiquette here with comment spam but we're also affected by this bug, and with this PR waiting for review for almost 3 months now I think some of us might be losing hope :-)

[pisto]

Just collecting all the duplicate bugs about this over the years, for fun I guess...

#12991 #12741 #12730 #12637 #12594 #12522 #12511 #12490 #12488 #12469 #12441 #12417 #11567 #9806 #9804 #9696 #9136 #9027 #6277 #5184

I'm so happy that the helm devs keep this bug alive so that I can charge my clients hours in working around it. Thanks! Tonight I eat steak thanks to you!

On a more serious note, who can we ping about this? I see @mattfarina worked on #12480 , sorry for the rude ping but maybe you were at least aware of the issue at some point in time.

[MrBlaise]

I'd like to join the ping party, even though I know it won't help and normally I'd consider it rude as well, but this is getting ridiculous. Even if this is merged it will be some time until tools like argo-cd picks it up so that things can finally go back to the way they were.

Anyone with merge rights on this repo please take a look, lots of people would appreciate it ❤️

[AndrewFarley]

Hmm, can we just start aggressively @-ing owners and/or higher tier contributors on this repo? I feel like maybe this thread has been muted, and maybe we have no choice. I don't mention brute force lightly, but this is a serious issue which has gone unchecked for too long.

[sylwit]

This PR fixes a really inconvenient bug and is highly expected to be merge.
@joejulian do you know who we should ping to have a look at it or if this PR isn't ready can we have some feedback please ?
Thank you ❤️

[ryanhockstad]

@scottrigby I'm not sure the added complexity of adding logic to nullify certain keys after the merge/coalesce recursion has occurred once is worth dealing with edge cases that users are able to handle themselves anyway.

Also, the way that this should be handled is a bit ambiguous anyway. A user may want to actually nullify a parent global key without nullifying all of the children globals.

[scottrigby]

OK so I think we should keep the scope of this PR to a fix for known issues users are having. It addresses them very well with many new tests, and as such is a valuable improvement. Let's move consideration for fixing possible issues with globals and null values to a separate issue. I'm running the tests now—let's see where they land.

[scottrigby]

lgtm 👍

[joejulian]

lgtm

[scottrigby]

Thanks @joejulian and everyone 👏 Merging

@ryanhockstad do you want to open a v3 backport PR from your same branch while the code applies as-is, and link to this one?

[AndrewFarley]

10 months later, amazing, thank you team. Now just waiting for the backport and release of Helm 3 with this patch

[ryanhockstad]

Here is the backport PR: #13654

[pierluigilenoci]

@ryanhockstad thank you!

[scottrigby]

📣 Backport PR #13654 merged 👏 This fix is scheduled to be in Helm 3.17.1