Skip to content
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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

elezar
Copy link
Contributor

@elezar elezar commented Oct 22, 2024

This closes #235

A sample spec would now output as:

---
cdiVersion: v0.3.0
kind: example.com/class
devices: []
containerEdits:
  deviceNodes:
  - path: /dev/foo

instead of:

---
cdiVersion: v0.3.0
containerEdits:
  deviceNodes:
  - path: /dev/foo
devices: null
kind: example.com/class

@elezar elezar marked this pull request as ready for review October 24, 2024 10:22
@elezar elezar requested review from bart0sh, klihub and marquiz and removed request for bart0sh and klihub October 24, 2024 10:22
Copy link
Contributor

@klihub klihub left a 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:
    1. 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 for gopkg.in/yaml.v2.
    2. 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.
  • 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 ?

@elezar
Copy link
Contributor Author

elezar commented Oct 24, 2024

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 gopkg.in/yaml.v2.

Let me take some time to better address / answer your questions.

@klihub
Copy link
Contributor

klihub commented Oct 25, 2024

Thanks for the points that you raised, @klihub. The main motivation for struct-based ordering is the human readability implied by the ordering.

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Ensure that YAML marshalling maintains field ordering
3 participants