Perform deep merge for template values #4668
Conversation
|
Thanks for your pull request. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). 📝 Please follow instructions at https://git.k8s.io/community/CLA.md#the-contributor-license-agreement to sign the CLA. It may take a couple minutes for the CLA signature to be fully registered; after that, please reply here with a new comment and we'll verify. Thanks.
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here. |
k8s-ci-robot
added
cncf-cla: no
|
/assign @geojaz |
| } | ||
| } | ||
|
|
||
| return context, nil | ||
| } | ||
|
|
||
| // Merges source and destination map, preferring values from the source map |
There was a problem hiding this comment.
Can we make a note where we used this code from? It is all OSS under the same language, so it is fine. But lets give credit.
|
/ok-to-test |
k8s-ci-robot
removed
the
needs-ok-to-test
|
CI is not happy. The new tests should give you some incite on what is wrong. |
|
Man I love our new tests! Sorry patting myself on the back |
k8s-ci-robot
added
the
needs-rebase
k8s-ci-robot
removed
the
needs-rebase
cmd/kops/toolbox_template.go
Outdated
| continue | ||
| } | ||
| // If the key doesn't exist already, then just set the key to that value | ||
| if _, exists := dest[k]; !exists { |
There was a problem hiding this comment.
Isn't this a duplicate of the first if?
There was a problem hiding this comment.
Good catch, I'll also make a PR for the Helm project
|
@gambol99 what do you think? Code looks fine, and I think the merge behaviour is more intuitive (but the intricacies of map vs non-map are also pretty tricky, even if they shouldn't happen in practice) |
|
... looks good to me 👍 |
|
I just ran into this and am glad to see @gwkunze already implemented the fix. Soft lgtm from me! |
|
/lgtm |
k8s-ci-robot
added
lgtm
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: gwkunze, justinsb The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
As per #4662 improves the merging of multiple
--valuesarguments by doing a deep merge. This way nested maps will all be merged instead of overwriting each other.NOTE:⚠️ This code is copied from the kubernetes/helm project. Since this is another CNCF project I suspect this is fine, but would like confirmation.