-
Notifications
You must be signed in to change notification settings - Fork 87
Use 'yaml.v3' (2nd try) #128
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
base: master
Are you sure you want to change the base?
Conversation
Signed-off-by: Tim Ramlot <[email protected]>
Signed-off-by: Tim Ramlot <[email protected]>
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: inteon 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 |
// the go-yaml library uses to convert float to string when | ||
// Marshaling. | ||
s := strconv.FormatFloat(typedKey, 'g', -1, 32) | ||
s := strconv.FormatFloat(typedKey, 'g', -1, 64) |
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.
"Will this result in visibly different output? Any changes have the potential to break consumers so we need to be extremely careful about changes that break byte-for-byte round-tripping" - @liggitt
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.
Afaik, this should just result in float64 values also being supported.
I added a few additional test cases for float marshalling and unmarshalling.
Signed-off-by: Tim Ramlot <[email protected]>
fyi, once #133 lands, you will need to revisit this PR. |
(even once reworked on top of that, it's still not clear to me that the things this changes / breaks will be feasible to overcome and roll out ... the unknowns about switching to v3 are super time-consuming to chase down definitively ... I appreciate the continued effort here, it's just really difficult to prioritize almost unbounded due diligence work to review / approve this) |
PR needs rebase. 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-sigs/prow repository. |
There are current (albeit tentative) plans to make the v4 API configurable to a very granular level. If we can get the v4 API to work exactly the same way given a v2 configuration, then I think it will be possible for users to add very specific new v4 features to their configs. |
I'll mention this PR in the yaml/go-yaml v4 config PR when it lands, so people here can be aware and weigh in... |
Continuation of #100
Switches from the 'yaml.v2' fork to the 'yaml.v3' fork. Tried to do this while maintaining backwards compatibility as much as possible.
Fixes some of the Yes = true and No = false confusion (see tests).