Error on duplicate fields in Kustomization #4929
Conversation
532d1a3 to
5fed0f7
Compare
| } | ||
| assert.Contains(t, err.Error(), | ||
| "kustomization unmarshal error: error converting YAML to JSON: yaml: unmarshal errors:\n"+ | ||
| " line 13: key \"literals\" already set in map\n line 18: key \"files\" already set in map") |
There was a problem hiding this comment.
be an error like this TODO comment.
| th.WriteK("/whatever", ` | ||
| apiVersion: kustomize.config.k8s.io/v1beta1 | ||
| kind: Kustomization |
There was a problem hiding this comment.
remove apiVersion: kustomize.config.k8s.io/v1beta1 and kind: Kustomization,
because th.WriteK is always added above two lines.
|
I think it is related to #4861 /cc @natasha41575 |
If this PR does fix that issue, can you please add a test showing it? As you noticed in our own tests, this can be a breaking change, in that some users who were previously successfully building their Kustomizations will now encounter an error. Since we were already throwing an error on unknown fields, the novel case is duplicate fields, as in your test changes. Per the kubernetes-sigs/yaml docs, previously "Duplicate fields, including in-case-sensitive matches, are ignored in an undefined order." (emphasis mine). That last part in particular makes me lean towards accepting this change for inclusion in the 5.0 release--not only is there a risk of affected users not getting the result they want, but there's also a risk that what they get will be inconsistent (which is obviously not good in general and also against our reproducibility principle for @natasha41575 wdyt? /triage under-consideration |
SGTM. /triage accepted |
api/types/kustomization.go
Outdated
| if err != nil { | ||
| return err | ||
| if err := yaml.UnmarshalStrict(y, &k); err != nil { | ||
| return fmt.Errorf("kustomization unmarshal error: %w", err) |
There was a problem hiding this comment.
Since the message from err itself already mentions unmarshalling, I suggest changing this to
| return fmt.Errorf("kustomization unmarshal error: %w", err) | |
| return errors.WrapPrefixf(err, "invalid Kustomization") |
(and using sigs.k8s.io/kustomize/kyaml/errors for wrapping)
|
@koba1t: This PR has multiple commits, and the default merge method is: merge. 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. |
|
Thanks for the quick follow-up! |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: KnVerey, koba1t 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 |
|
@KnVerey I fix a few points from your comments. |
kustomization.yaml