Add test for subchart coalesce null values

[aeijdenberg]

Since coalesce can sometimes be called more than once, make sure
that we don't drop keys until right at the end, else they can lose
their meaning (and allow the original value to come through).

More tests would be welcome.

Signed-off-by: Adam Eijdenberg adam@continusec.com

Closes #5184 (at least hopefully improves the situation)

What this PR does / why we need it:

#5184 caused a regress for subchart values. This attempts to fix it.

Special notes for your reviewer:

If applicable:

• this PR contains documentation
• this PR contains unit tests
• this PR has been tested for backwards compatibility

[aeijdenberg]

(I had to force push with slightly amended commit message a few times until I managed to get the build to pass - it seems there might be some unrelated flaky tests or builds?)

[bacongobbler]

Thank you for working on this. We really appreciate it.

@sgillespie would you mind testing to see if this fix works for you?

[sgillespie]

This does not seem to fix it for me.

Consider a default value in a subchart:

livenessProbe:
  tcpSocket:
    port: 5000

Overridden with:

livenessProbe:
    tcpSocket: null

The result (2.14.2):

livenessProbe:                    
  tcpSocket: null

And this patch:

 livenessProbe:            
   tcpSocket:
     port: 5000

[sgillespie]

FWIW I'm testing with helm template

[aeijdenberg]

@sgillespie is right - this still doesn't fix the demo.zip example in the linked issue.

I've had another look today - sadly I don't think I'm going to be able to fix this easily - and I don't have the capacity available to learn the code base to fully fix it.

The underlying problem so far as I can tell is that chartutil.CoalesceValues() is called many times, and there seems to be an assumption that it's safe to call more than once, passing the previous output as input a 2nd time.

However with the behavior now of removing nil keys, that means that if it's called a second time with a nil key removed, then there's no value value to override the source value and thus it comes back again.

I think the right way to fix is likely to stop removing nil keys at all inside of that function, and instead remove them much closer to wherever the final values files are rendered. And that's the part of the code base I'm not going to have the capacity in the near future to tease apart and figure out.

[sgillespie]

This looks to have been fixed in #6080.

[aeijdenberg]

#6080 does appears to have fixed this, which is great! I've amended this PR to just add the test that I'd already written (which #6080 also fixes), as we can never have enough tests. :)

[hickeyma]

In conjunction with PR #6080 and validation in #6146 (comment), LGTM.