Fix nested null value overrides

[aeijdenberg]

Closes #5184

What this PR does / why we need it:

This PR adds tests for setting nested values to null and ensure such values are actually deleted.

Special notes for your reviewer:

I've split this into 4 commits. They do the following:

1. Adds some code to the tests to make it easier to add nested null values tests.
2. Adds tests that fails.
3. Does a fairly minimal fix - this is enough to make the tests pass.
4. The last commit touches a bit more code, and attempts to simplify the coalescence more broadly by consolidating on to a single codepath, and being a bit more careful about which maps are mutated. Tests continue to pass - but I'm a little less confident about the full impact of this commit.

If applicable:

• this PR contains unit tests

[cdenneen]

@aeijdenberg can you fix this by adding metadata to the integration tests? Really want to get this fix in place.

[aeijdenberg]

@cdenneen - I've found the missing metadata, added it, and removed the nil check. Let me know if you'd like that added back in.

(oh, and also re-based on latest master)

[cdenneen]

I’m okay with nil being valid “null” setting but that’s just me

[cdenneen]

@aeijdenberg can you add label bug and awaiting review

[cdenneen]

@aeijdenberg can you please add those labels?

[bacongobbler]

Only admins can attach labels. I've done the needful.

[cdenneen]

Thanks @bacongobbler

[scottrigby]

Hi I just saw this from #5184 👋 I'm late to the party

@cdenneen Quick reply to #5185 (comment): the reason I added these 5 lines in the test initially is that null, Null, NULL, and ~ are all valid when parsed, but different from empty string "". I think it's fine that these are already checked on these lines and don't need to be re-checked in the deeply nested test added by this PR 👍

@aeijdenberg re point 4 in the PR description, I'm also not fully sure about other possible impacts of this (second to last) commit: e69e888. @bacongobbler I'd like another set of eyes on that. But, it does fix the 🐛 (the updated tests are great, and I also manually tested with a few different scenarios I'm aware of from community helm/charts) - everything seems to work 💯

[aeijdenberg]

Hi all, sorry to go quiet on this PR. Am back on deck now - let me know if any changes are desired.

[scottrigby]

I resolved my last comment. This LGTM 👍 but would still like another set of eyes on the reorganizing commit I mentioned here #5185 (comment)

[thomastaylor312]

Hey @aeijdenberg so that refactor commit looks pretty good, but I want to review it a little more in depth with @scottrigby to make sure it doesn't affect other things. Would you be willing to pop that commit off of this PR and open it as a separate PR? It would allow us to merge this fix and address the refactor in another PR.

Thanks for going through the refactoring effort with this code. We really appreciate it!

[aeijdenberg]

@thomastaylor312 - done. I've rebased on latest master, squashed the commits (excluding the refactor), and have opened #5821 for the refactoring.

[scottrigby]

Thanks for splitting this. Lgtm

[thomastaylor312]

Ok, sorry for the delay. I was talking with some of the other maintainers to make sure that deleting a key wouldn't create any side effects. This LGTM. Thanks for the good work!