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

chore: Unify yaml dependency #6599

Closed

Conversation

johnchen-k
Copy link

Why the changes in this PR are needed?

There are 2 yaml dependencies in the repo. This pr 6195 in v0.56.0 replaces gopkg.in/yaml with sigs.k8s.io/yaml, but gopkg.in/yaml is not replaced.

What are the changes in this PR?

replace gopkg.in/yaml.v2 with sigs.k8s.io

Notes to assist PR review:

issue: 6598

Further comments:

@johnchen-k johnchen-k marked this pull request as ready for review February 26, 2024 21:22
@ashutosh-narkar
Copy link
Member

@johnchen-k thanks for working on this. If you could look into the test failures that would be great. Thanks.

replace gopkg.in/yaml.v2 with sigs.k8s.io

fix: 6598
Signed-off-by: JohnChen <johnkim.chen@gmail.com>
@johnchen-k johnchen-k closed this Mar 5, 2024
@anderseknert
Copy link
Member

Looking at the errors, they're all similar to this:

expected error location row 4, got 3

So I guess the "new" marshaller changes the order of how things are printed?
Did you decide to give up on thos @johnchen-k? Let me know if you want me to pick it up.

@johnchen-k
Copy link
Author

Yes, the error of new marshaller dose not contain line number. Parse uses regex to get the line number from error of marshaller. So It always uses the default row.

I can not focus on this issue this week and don't want to be the bottleneck, so I close it. It would be great if you can pick it up.

BTW: the new marshaller returns float64 for number not int, like "a": 1,. It can be fixed by changing to ""a": float64(1),". But I'm not sure if there are any other effects. Could you help to have a look?

@anderseknert
Copy link
Member

Thanks for that great investigation! It sounds more complex than I initially thought, so I don't think I'll be able to pick this up until possibly after KubeCon. If anyone else feels inclined, feel free to take a stab at it!

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.

None yet

3 participants