Remove non-essential dependency `imdario/mergo` #5044

krzysiekg · 2023-02-10T00:00:03Z

This fixes #4905

There was an issue with overwriting empty values in map[string]interface{} that was fixed a while ago in darccio/mergo#84.

I'm not sure how to test it best. None of the charts used in tests have this issue. I'm thinking about replacing minecraft chart that is used to test valuesInline with istio gateway chart. That way I can test all that these tests already cover plus that empty map issue. But maybe that is too much. I can also just add a separate test just to cover this.

k8s-ci-robot · 2023-02-10T00:00:12Z

Welcome @krzysiekg!

It looks like this is your first PR to kubernetes-sigs/kustomize 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes-sigs/kustomize has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

k8s-ci-robot · 2023-02-10T00:00:13Z

Hi @krzysiekg. Thanks for your PR.

I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

koba1t · 2023-02-14T12:13:53Z

Thanks for your contributing!

I'm thinking about replacing minecraft chart that is used to test valuesInline with istio gateway chart.

Could you try to add the test with istio gateway chart below minecraft chart tests?
I think that is better than rewriting existing test cases.

koba1t · 2023-02-14T12:17:01Z

/ok-to-test

KnVerey · 2023-02-14T19:34:19Z

Agreed, I would prefer a dedicated test case that shows the bug fix specifically (ideally as small and specific as possible). I'm also wondering why we use mergo here instead of our own kyaml/yaml/merge2... if we had that test in place, perhaps we could also try getting rid of the dependency instead of upgrading it?

krzysiekg · 2023-02-14T20:03:55Z

No problem, I will write separate tests for this. Once I have them I'll also try and replace mergo with kyaml/yaml/merge2 and see if it breaks anything.

KnVerey · 2023-03-22T18:59:54Z

/triage under-consideration
/kind bug
/priority backlog

k8s-triage-robot · 2023-06-20T19:01:55Z

The Kubernetes project currently lacks enough contributors to adequately respond to all PRs.

This bot triages PRs according to the following rules:

After 90d of inactivity, lifecycle/stale is applied
After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
After 30d of inactivity since lifecycle/rotten was applied, the PR is closed

You can:

Mark this PR as fresh with /remove-lifecycle stale
Close this PR with /close
Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale

k8s-triage-robot · 2023-07-20T19:27:19Z

The Kubernetes project currently lacks enough active contributors to adequately respond to all PRs.

This bot triages PRs according to the following rules:

After 90d of inactivity, lifecycle/stale is applied
After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
After 30d of inactivity since lifecycle/rotten was applied, the PR is closed

You can:

Mark this PR as fresh with /remove-lifecycle rotten
Close this PR with /close
Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle rotten

vladfr · 2023-08-22T07:37:41Z

@krzysiekg Hi, did you get a chance to write those tests?

superbrothers · 2023-09-24T07:50:27Z

@KnVerey @koba1t I am facing #4905 too. I have created #5342 with an additional small dedicated test case that reproduces the problem. I would like to ask you to check it.

krzysiekg · 2023-09-25T21:23:24Z

Sorry for the long silence.

I did write the test and replaced mergo with kyaml/yaml/merge2. I also looked into both packages a bit. The tests pass but I'm not certain that merge2 is a drop-in replacement for mergo. First, it has no concept of merge with override. I tried merges both with and without $patch: replace but it doesn't make any difference with current tests and I can't really tell which one is closer.

Since kustomize roadmap states that helmCharts field is going to be removed (is that still true?), I'm not sure if changing this and potentially breaking people's configs is worth it. But I would like to listen what maintainers think. WDYT @koba1t @KnVerey?

k8s-ci-robot · 2023-10-04T22:09:18Z

This PR has multiple commits, and the default merge method is: merge.
You can request commits to be squashed using the label: tide/merge-method-squash

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.

koba1t · 2023-12-07T04:52:55Z

/ok-to-test

koba1t · 2023-12-21T14:14:28Z

plugin/builtin/helmchartinflationgenerator/HelmChartInflationGenerator.go

 	switch p.ValuesMerge {
 	case valuesMergeOptionOverride:
-		err = mergo.Merge(
-			&chValues, p.ValuesInline, mergo.WithOverride)
+		outValues, err = merge2.Merge(inlineValues, chValues, kyaml.MergeOptions{})
 	case valuesMergeOptionMerge:
-		err = mergo.Merge(&chValues, p.ValuesInline)
+		outValues, err = merge2.Merge(chValues, inlineValues, kyaml.MergeOptions{})
+	}


Override option with kyaml/yaml/merge2 lib can be achieved without extra parameters, just by passing src and dest in different order.

Could you add any comments about the above information?
I think this code is a little confusing what is doing because the above two lines only have different argument orders.

Good point! I added the explanation in the code.

Unrelated, but I also decided to copy the value before merging, so that the original values are preserved after the merge. Someone might still want to use it.

koba1t · 2023-12-21T14:33:26Z

I think this change looks almost good, and we can merge.
But, I want to get checks from others.

@natasha41575
Hi! I don't have permission to run Actions on this PR.
Could you run CI, and Could you give this PR another check?

plugin/builtin/helmchartinflationgenerator/HelmChartInflationGenerator.go

k8s-triage-robot · 2024-01-20T15:13:46Z

The Kubernetes project currently lacks enough active contributors to adequately respond to all issues and PRs.

This bot triages PRs according to the following rules:

After 90d of inactivity, lifecycle/stale is applied
After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
After 30d of inactivity since lifecycle/rotten was applied, the PR is closed

You can:

Reopen this PR with /reopen
Mark this PR as fresh with /remove-lifecycle rotten
Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/close

k8s-ci-robot · 2024-01-20T15:13:51Z

@k8s-triage-robot: Closed this PR.

In response to this:

The Kubernetes project currently lacks enough active contributors to adequately respond to all issues and PRs.

This bot triages PRs according to the following rules:

After 90d of inactivity, lifecycle/stale is applied

After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied

After 30d of inactivity since lifecycle/rotten was applied, the PR is closed

You can:

Reopen this PR with /reopen

Mark this PR as fresh with /remove-lifecycle rotten

Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/close

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.

krzysiekg · 2024-01-23T19:31:55Z

/reopen

k8s-ci-robot · 2024-01-23T19:32:00Z

@krzysiekg: Failed to re-open PR: state cannot be changed. The fix-empty-map-merge branch was force-pushed or recreated.

In response to this:

/reopen

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.

krzysiekg · 2024-01-23T20:01:53Z

/reopen

k8s-ci-robot · 2024-01-23T20:01:58Z

@krzysiekg: Reopened this PR.

In response to this:

/reopen

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.

plugin/builtin/helmchartinflationgenerator/HelmChartInflationGenerator_test.go

koba1t

Hi @krzysiekg
This PR almost looks good to me!
I plan to merge it.

But, I found a scenario of tests that are better to take.
Could you add any tests?

krzysiekg · 2024-01-28T23:49:29Z

I added the test for valuesMerge: replace. After using replace option myself, I think it either needs some more clarification in the docs or deprecation and removal.

I think it functionality is limited but it is hard to tell how many people actually use it, so maybe it is better to keep it around. It does not replace the default values file from the chart (I would expect that). It only can replace the values file defined in the kustomization file with the values inline. But since it has to be the same kustomization file, people can just pass everything as inline values of as values file. And you can't use it to replace the values when you include the kustomization into another kustomization as resource, because at that point, helm chart is already inflated.

But it is not the kustomize issue either. Afaict helm do not offer a way to ignore the default values file that a chart comes with (other than removing the files from the chart, which is not a good approach imho).

krzysiekg · 2024-01-29T21:27:11Z

Lint check was failing on make check-license. It was passing locally though. I saw that there was some issue with license headers on main branch so I rebased.

koba1t · 2024-01-31T17:03:22Z

Lint check was failing on make check-license. It was passing locally though. I saw that there was some issue with license headers on main branch so I rebased.

Thanks!

koba1t · 2024-01-31T17:56:33Z

I added the test for valuesMerge: replace. After using replace option myself, I think it either needs some more clarification in the docs or deprecation and removal.

I agree with your opinion. I think the valuesMerge: replace field doesn't have enough explanation, and I think it is not used by many people.
We need to consider deprecating that. But I feel we can not delete it with this PR and keep this function now because maybe some users use this field.

koba1t · 2024-01-31T17:58:15Z

Hi @krzysiekg

Thanks for your long working!
I feel this PR looks good!

/lgtm
/approve

k8s-ci-robot · 2024-01-31T17:58:22Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: koba1t, krzysiekg

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:

~~OWNERS~~ [koba1t]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Feb 10, 2023

k8s-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Feb 10, 2023

k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Feb 10, 2023

k8s-ci-robot requested review from koba1t and yuwenma February 10, 2023 00:00

k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Feb 14, 2023

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 14, 2023

k8s-ci-robot added triage/under-consideration kind/bug Categorizes issue or PR as related to a bug. priority/backlog Higher priority than priority/awaiting-more-evidence. labels Mar 22, 2023

k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Jun 20, 2023

k8s-ci-robot added lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. and removed lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. labels Jul 20, 2023

superbrothers mentioned this pull request Sep 24, 2023

Update imdario/mergo to v0.3.13 #5342

Merged

krzysiekg force-pushed the fix-empty-map-merge branch from d88c002 to ff4d8b5 Compare September 25, 2023 21:20

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 26, 2023

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 7, 2023

koba1t reviewed Dec 21, 2023

View reviewed changes

plugin/builtin/helmchartinflationgenerator/HelmChartInflationGenerator.go Outdated Show resolved Hide resolved

k8s-ci-robot closed this Jan 20, 2024

k8s-ci-robot reopened this Jan 23, 2024

koba1t reviewed Jan 28, 2024

View reviewed changes

plugin/builtin/helmchartinflationgenerator/HelmChartInflationGenerator_test.go Show resolved Hide resolved

koba1t requested changes Jan 28, 2024

View reviewed changes

krzysiekg added 3 commits January 29, 2024 22:06

replace mergo with kyaml/yaml/merge2

d768fc3

wrap errors and add comments

bf286dc

add test for values merge replace

d73f0fd

krzysiekg force-pushed the fix-empty-map-merge branch from 919aefb to d73f0fd Compare January 29, 2024 21:06

k8s-ci-robot assigned koba1t Jan 31, 2024

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jan 31, 2024

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jan 31, 2024

k8s-ci-robot merged commit b1b61ad into kubernetes-sigs:master Jan 31, 2024
9 checks passed

dims mentioned this pull request May 11, 2024

rename mergo module in clientcmd kubernetes/kubernetes#124279

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove non-essential dependency `imdario/mergo` #5044

Remove non-essential dependency `imdario/mergo` #5044

krzysiekg commented Feb 10, 2023

k8s-ci-robot commented Feb 10, 2023

k8s-ci-robot commented Feb 10, 2023

koba1t commented Feb 14, 2023 •

edited

Loading

koba1t commented Feb 14, 2023

KnVerey commented Feb 14, 2023

krzysiekg commented Feb 14, 2023

KnVerey commented Mar 22, 2023

k8s-triage-robot commented Jun 20, 2023

k8s-triage-robot commented Jul 20, 2023

vladfr commented Aug 22, 2023

superbrothers commented Sep 24, 2023 •

edited

Loading

krzysiekg commented Sep 25, 2023

k8s-ci-robot commented Oct 4, 2023

koba1t commented Dec 7, 2023

koba1t Dec 21, 2023

krzysiekg Jan 23, 2024

koba1t commented Dec 21, 2023

k8s-triage-robot commented Jan 20, 2024

k8s-ci-robot commented Jan 20, 2024

krzysiekg commented Jan 23, 2024

k8s-ci-robot commented Jan 23, 2024

krzysiekg commented Jan 23, 2024

k8s-ci-robot commented Jan 23, 2024

koba1t left a comment

krzysiekg commented Jan 28, 2024

krzysiekg commented Jan 29, 2024

koba1t commented Jan 31, 2024

koba1t commented Jan 31, 2024

koba1t commented Jan 31, 2024

k8s-ci-robot commented Jan 31, 2024

Remove non-essential dependency imdario/mergo #5044

Remove non-essential dependency imdario/mergo #5044

Conversation

krzysiekg commented Feb 10, 2023

k8s-ci-robot commented Feb 10, 2023

k8s-ci-robot commented Feb 10, 2023

koba1t commented Feb 14, 2023 • edited Loading

koba1t commented Feb 14, 2023

KnVerey commented Feb 14, 2023

krzysiekg commented Feb 14, 2023

KnVerey commented Mar 22, 2023

k8s-triage-robot commented Jun 20, 2023

k8s-triage-robot commented Jul 20, 2023

vladfr commented Aug 22, 2023

superbrothers commented Sep 24, 2023 • edited Loading

krzysiekg commented Sep 25, 2023

k8s-ci-robot commented Oct 4, 2023

koba1t commented Dec 7, 2023

koba1t Dec 21, 2023

Choose a reason for hiding this comment

krzysiekg Jan 23, 2024

Choose a reason for hiding this comment

koba1t commented Dec 21, 2023

k8s-triage-robot commented Jan 20, 2024

k8s-ci-robot commented Jan 20, 2024

krzysiekg commented Jan 23, 2024

k8s-ci-robot commented Jan 23, 2024

krzysiekg commented Jan 23, 2024

k8s-ci-robot commented Jan 23, 2024

koba1t left a comment

Choose a reason for hiding this comment

krzysiekg commented Jan 28, 2024

krzysiekg commented Jan 29, 2024

koba1t commented Jan 31, 2024

koba1t commented Jan 31, 2024

koba1t commented Jan 31, 2024

k8s-ci-robot commented Jan 31, 2024

Remove non-essential dependency `imdario/mergo` #5044

Remove non-essential dependency `imdario/mergo` #5044

koba1t commented Feb 14, 2023 •

edited

Loading

superbrothers commented Sep 24, 2023 •

edited

Loading