-
Notifications
You must be signed in to change notification settings - Fork 38
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Ensure that YAML fields are sorted #236
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm fine with this if others think this is a good way to go forward.
However, I have a few questions/comments:
- We originally chose to go with the K8s spin on yaml for 2 reasons:
- It's explicitly stated design goal is
to enable a better way of handling YAML when marshaling to and from structs
which is pretty much the only thing we use YAML for in CDI. This reference given here provides the original justification/reasoning behind the (predecessor to the) sigs.k8s.io implementation instead of using the stock golang one, and I simply don't know if these are now irrelevant/invalid forgopkg.in/yaml.v2
. - As working in the K8s and runtimes domains, we typically deal with YAML via the sigs.k8s.io package and therefore I think any potential behavioral differences will catch us by surprise.
- It's explicitly stated design goal is
- Since gopkg.in/yaml.v2 does not reuse the JSON struct tags, can we have some linter verifying that we always have either neither or both JSON and YAML tag explicitly defined ?
Thanks for the points that you raised, @klihub. The main motivation for struct-based ordering is the human readability implied by the ordering. Note that the underlying yaml implementation is still Let me take some time to better address / answer your questions. |
Sure, I understood that and I'm all for it. Just wanted to make sure that nothing hits us by surprise as a side-effect. |
This change switches to usining gopkg.in/yaml.v2 when Marshalling data to YAML. This ensures that YAML fields are sorted as they are defined in the structs and not alphabetically. Signed-off-by: Evan Lezar <[email protected]>
@klihub @bart0sh I have updated the implementation to only switch to ordered YAML when outputing the files. This means that for the consumer (containerd, cri-o, etc) workflow we have not changed anything. Another option would be to add a non-default "ordered" option to the producer API from #233 so that we don't have to change the default behaviour at all. |
This closes #235
A sample spec would now output as:
instead of: