Fix deep merge of ValuesYamlFiles #2963
Conversation
|
PR is now waiting for a maintainer to run the acceptance tests. |
|
PR is now waiting for a maintainer to run the acceptance tests. |
|
/run-acceptance-tests |
|
Please view the PR build: https://github.com/pulumi/pulumi-kubernetes/actions/runs/8742418951 |
|
@KevinFairise2 Thanks for identifying this issue and providing a fix. The overall code looks good to me. It'd be nice to have some tests to exercise this new logic. Would you be able to add a simple test case for this? |
| return nil, err | ||
| } | ||
| mergedValues, err := mergeMaps(values, valuesMap, false) |
There was a problem hiding this comment.
I notice we might still deviate from upstream's behavior here -- this is Helm's logic:
But we're using github.com/imdario/mergo.Merge. I haven't looked at this closely enough to see if there are behavioral difference -- it's quite a bit more complex so I imagine there must be?
I would feel a lot better if we followed upstream's mergeMaps logic directly. It's not public but I wonder if we should just vendor the logic to stay closer to upstream. Or maybe this is an edge case not worth worrying about?
There was a problem hiding this comment.
I would definitely advocate for using Helm itself as much as possible, copying as necessary.
There was a problem hiding this comment.
Thanks for your fast feedbacks on that PR!
Using the same function as Helm to merge the maps sounds look I good idea.
mergo.Merge can do a deep merge on any struct, so the code is indeed way more complex. In our case we only need to deep merge two map[string]any so using the same function as Helm should be enough.
I can replace the call to mergo.Merge here with a call to a copy of the Helm merge maps. If that sounds good to you. As you said it is hard to anticipate any behavior changes seeing how complex the mergoM.Merge implementation is.
Do you think your tests would catch any changes in the behavior of mergeMaps?
Let me know how I should handle License things if I copy the function from Helm. The code they use seems to be standard way to deep merge two map[string]any that can be found elsewhere on the internet
There was a problem hiding this comment.
I pushed a change to use upstream's merge logic. Both projects are Apache 2.0 and they don't include a NOTICE so I think it's sufficient to preserve attribution.
I left the existing mergeMaps implementation alone because I'd like to keep the scope of this change as small as possible for now. I'm not sure how well our tests cover that behavior.
There was a problem hiding this comment.
PR looks good to me like that. I think everything is in your hand to have it merged now. Let me know if I can do anything more to help with that.
What will be the release plan for that? I think this is a breaking change that can impact current users of the provider.
There was a problem hiding this comment.
I think this is a breaking change that can impact current users of the provider.
It's breaking in the sense that behavior will change, yes. In this case it's a bug that this field doesn't agree (and has never agreed) with helm's behavior despite claiming to. I'm comfortable fixing the behavior instead of forcing users to live with it until a much later date for v5 -- or introducing another awkward field valueYamlFilesThisIsTheOneYouReallyWant.
Correctness is more important since the current behavior can potentially cause cascading issues by unexpectedly dropping values. If someone has been impacted by this, they might not realize it (in which case they could be sitting on a time bomb) or they might have worked around it by providing a union of values (in which case the change in behavior is less impactful).
In any case, previews will show a diff with any impacted values and users can choose to modify their values accordingly.
|
PR is now waiting for a maintainer to run the acceptance tests. |
|
PR is now waiting for a maintainer to run the acceptance tests. |
|
/run-acceptance-tests |
|
Please view the PR build: https://github.com/pulumi/pulumi-kubernetes/actions/runs/8743736851 |
|
PR is now waiting for a maintainer to run the acceptance tests. |
|
/run-acceptance-tests |
|
Please view the PR build: https://github.com/pulumi/pulumi-kubernetes/actions/runs/8758245162 |
| return nil, err | ||
| } | ||
| values = helm.MergeMaps(values, valuesMap) |
There was a problem hiding this comment.
Should we also merge the literal values using helm.MergeMaps rather than our own mergeMaps (see line 319)? Perhaps use helm.MergeMaps when AllowNullValues is true (since the flag was intended to improve compatibility). Are there any differences in the logic in that case? Or we could leave it for the v4 implementation of Release.
There was a problem hiding this comment.
This is a good question. I added some test cases and I think this basically boils down to whether we also want to fix #2731 as part of this PR, because I think that's essentially what your suggestion would do.
I'm OK to leave line 319 as-is just to keep scope and risk down. I can follow up with #2731.
|
PR is now waiting for a maintainer to run the acceptance tests. |
|
/run-acceptance-tests |
|
Please view the PR build: https://github.com/pulumi/pulumi-kubernetes/actions/runs/8759550382 |
|
PR is now waiting for a maintainer to run the acceptance tests. |
|
/run-acceptance-tests |
|
Please view the PR build: https://github.com/pulumi/pulumi-kubernetes/actions/runs/8822950135 |
Proposed changes
Stop relying on
yaml.Unmarshalto mergeValuesYamlas it cannot do a deep merge. Instead we Unmarshal the values in a temporary map and use themergeMapfunction that already exists to merge.With this the behavior of the provider is aligned with the deep merge made by Helm helm/helm#1620
Note: This is a breaking change
Related issues (optional)
Should close #2958