keps: centralize yaml calls, use UnmarshalStrict #2870
Conversation
Centralize yaml unmarshalling to k8s.io/enhancements/yaml and add an UnmarshalStrict() function implemented using gopkg.in/yaml.v3 Somehow all of gopkg.in/yaml.v2, gopkg.in/yaml.v3 and sigs.k8s.io/yaml were being used across the code. Hopefully this unifies and lets us choose just one. Update KEPs to fix unmarshalling failures. The one controversial change would be to support the KEP that added "deprecated" and "removed" milestones. I opted to add those to the API, but am open to suggestion on how that planned lifecycle should be encoded in the KEP instead.
In service of centralizing the one package that decides which yaml library to use
k8s-ci-robot
added
cncf-cla: yes
k8s-ci-robot
added
kind/api-change
|
This PR may require API review. If so, when the changes are ready, complete the pre-review checklist and request an API review. Status of requested reviews is tracked in the API Review project. |
Fine to move forward with that change and iterate as needed. /lgtm |
k8s-ci-robot
added
the
lgtm
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: justaugustus, spiffxp 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 |
k8s-ci-robot
added
the
approved
Prompted by
#2867 (comment)
I decided it would be worth having tests enforce that fields in
KEP-related yaml are well known (vs. allowing addition of unknown
fields)
This codebase largely used
gopkg.in/yaml.v3but alsosigs.k8s.io/yamlinsome places, which each have different ways of accomplishing "return
error if there are unknown or duplicate fields" functionality. I opted
to pull calls to
yaml.Unmarshalandyaml.Marshalinto a helperpackage,
k8s.io/enhancements/pkg/yaml, so there's only one place tochange for yaml-related changes.
I then converged on a
gopkg.in/yaml.v3implementation ofyaml.UnmarshalStrictto accomplish erroring on unknown fields, andcorrected the KEPs that failed tests as a result. The reasoning for
choice of library was: most everything already used it, and it supports
preserving comments (not that the code as-written does). I don't feel
strongly about switching it back to sigs.k8s.io/yaml though if that's
the preference
The ony controversial change was to support the KEP that added
"deprecated" and "removed" milestones. I opted to add those to the API
but am open to suggestion on how that could be better encoded in the
Proposalstruct that we have today.