add options to disable unique keys #105
Conversation
|
|
k8s-ci-robot
added
the
cncf-cla: no
k8s-ci-robot
added
the
sig/api-machinery
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: MeNsaaH The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
Welcome @MeNsaaH! |
k8s-ci-robot
added
size/L
|
/assign @jpbetz |
| @@ -111,6 +110,12 @@ func (dec *Decoder) KnownFields(enable bool) { | |||
| dec.knownFields = enable | |||
| } | |||
|
|
|||
| // DisableUniqueKeys ensure function Decode not to return errors when keys exist | |||
| // more than once inside the given mapping. | |||
| func (dec *Decoder) DisableUniqueKeys(enable bool) { | |||
There was a problem hiding this comment.
I'm confused why we need this. Duplicate keys are not valid yaml. Why would we start accepting them now when we didn't previously?
/hold
There was a problem hiding this comment.
@liggitt Duplicate yamls are not valid yamls spec but not when it comes to kubernetes manifests. With kubernetes manifests, duplicates are used to override what was formerly specified (especially with labels and annotations).
The below yaml, even tho having multiple name label is a valid kubernetes manifests. And since this yaml library aims to adapt this for kubernetes, I believe we need to have this implemented as well.
apiVersion: v1
kind: Namespace
metadata:
labels:
kubernetes.io/metadata.name: default
name: default1
name: default
name: defaultKustomize uses this library for parsing yaml files. So, it makes sense to add this as part of the features which can be enabled on demand as in the PR I earlier sent above.
There was a problem hiding this comment.
My initial feeling was to agree with @liggitt that duplicate keys are not valid yaml so we shouldn't support them (I left the same comment in kubernetes-sigs/kustomize#4096 (comment)). This library is not intended to be adapted specifically for kubernetes. We forked it because the upstream yaml was unmaintained, but IMO it should remain a general-purpose yaml library adhering to the yaml specification. Extending its scope beyond that should be done with care and only for very good reasons, if at all.
That said, I am surprised that the above Namespace yaml is accepted by the apiserver, thank you for pointing that out. @liggitt do you have any context on duplicated keys in upstream k8s? How is k8s able to parse yaml that kustomize cannot, when both repos are depending on the same yaml library?
There was a problem hiding this comment.
@natasha41575 thanks for you inputs on this. That has always been my issue. Waiting for the response as well.
k8s-ci-robot
added
the
do-not-merge/hold
|
The Kubernetes project currently lacks enough contributors to adequately respond to all PRs. This bot triages PRs according to the following rules:
You can:
Please send feedback to sig-contributor-experience at kubernetes/community. /lifecycle stale |
k8s-ci-robot
added
the
lifecycle/stale
This is a follow up for https://github.com/kubernetes-sigs/kustomize/pull/4096/files
As @natasha41575 pointed out in her comment, the PR was pending when go-yaml is moved to a dedicated kubernetes-sigs repo.
kubernetes-sigs/kustomize#4096 (comment)