Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix deep merge of ValuesYamlFiles #2963

Merged
merged 7 commits into from
Apr 24, 2024
Merged

Fix deep merge of ValuesYamlFiles #2963

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
ebd496e
Fix deep merge of ValuesYamlFiles
KevinFairise2 Apr 18, 2024
85313c3
Update changelog
KevinFairise2 Apr 18, 2024
0435b26
Add a simple regression test
blampe Apr 18, 2024
87b26a9
Reword changelog
blampe Apr 18, 2024
7b7b645
Use upstream mergeMaps
blampe Apr 19, 2024
b2523ab
Test merging with literals
blampe Apr 19, 2024
3ab077b
Merge branch 'master' into kfairise/deep-merge-valuesyalm
blampe Apr 24, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 2 additions & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
## Unreleased

- deep merge values yaml files provided instead of simply relying on yaml.Unmarshall to merge root fields (https://github.com/pulumi/pulumi-kubernetes/pull/2963)

## 4.11.0 (April 17, 2024)

- [dotnet] Unknowns for previews involving an uninitialized provider (https://github.com/pulumi/pulumi-kubernetes/pull/2957)
Expand Down
8 changes: 7 additions & 1 deletion provider/pkg/provider/helm_release.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -299,9 +299,15 @@ func decodeRelease(pm resource.PropertyMap, label string) (*Release, error) {
if err != nil {
return nil, err
}
if err = yaml.Unmarshal(b, &values); err != nil {
valuesMap := map[string]any{}
if err = yaml.Unmarshal(b, &valuesMap); err != nil {
return nil, err
}
mergedValues, err := mergeMaps(values, valuesMap, false)
Copy link
Contributor

@blampe blampe Apr 18, 2024 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I notice we might still deviate from upstream's behavior here -- this is Helm's logic:

https://github.com/helm/helm/blob/14d0c13e9eefff5b4a1b511cf50643529692ec94/pkg/cli/values/options.go#L108-L125

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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would definitely advocate for using Helm itself as much as possible, copying as necessary.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

if err != nil {
return nil, err
}
values = mergedValues
default:
return nil, fmt.Errorf("unsupported type for 'valueYamlFiles' arg: %T", v)
}
Expand Down