-
Notifications
You must be signed in to change notification settings - Fork 5.6k
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: use sigs.k8s.io/yaml/goyaml.v* instead of gopkg.in/yaml.v* #21273
base: master
Are you sure you want to change the base?
Conversation
🔴 Preview Environment stopped on BunnyshellSee: Environment Details | Pipeline Logs Available commands (reply to this comment):
|
67cb14e
to
2a7671b
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #21273 +/- ##
=======================================
Coverage 55.22% 55.23%
=======================================
Files 337 337
Lines 56945 56945
=======================================
+ Hits 31447 31451 +4
+ Misses 22816 22814 -2
+ Partials 2682 2680 -2 ☔ View full report in Codecov by Sentry. |
2a7671b
to
12ab00e
Compare
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.
Hm, there are some differences in how those handle things. E.g. time.Duration for sigs yaml is expected to be integer in nanoseconds, while for regular yaml it'd be in format like "1h".
12ab00e
to
1bdbccd
Compare
Can you add a test with time duration parsing or point at the existing one, please? |
Does thoses tests fits your expectations or do you need more ? goyaml.v3https://github.com/kubernetes-sigs/yaml/blob/master/goyaml.v3/encode_test.go#L334-L336 goyaml.v2https://github.com/kubernetes-sigs/yaml/blob/master/goyaml.v2/encode_test.go#L306-L309 |
Looks like it won't accept ints in new versions as duration https://github.com/kubernetes-sigs/yaml/blob/master/goyaml.v3/decode_test.go#L926. Looks like sigs yaml version 1 is used in Argo CD now. Overall, this would be a breaking change, e.g. in my case there's already a config that specifies nanoseconds int for a duration. I think this may be a change we want for Argo CD v3. For now, I suggest to only migrate places that use |
This PR only moves from gopkg.in/yaml.v* to sigs.k8s.io/yaml/goyaml.v* . |
fa5dc53
to
ab12e09
Compare
Yeah, I believe so. |
ab12e09
to
5688d45
Compare
5688d45
to
520b8a2
Compare
This one worries me. It's not clear that it makes sense in every case to use the k8s fork. iiuc, the k8s fork is for high-priority fixes for problems that affect core k8s libraries. It's possible that those bugs also affect Argo CD. But it's also possible that the k8s fork trades off things like performance in order to get those bug fixes. |
c7cc717
to
c7fae83
Compare
c7fae83
to
fb7b98a
Compare
Signed-off-by: Matthieu MOREL <[email protected]>
fb7b98a
to
0549978
Compare
Description
gomodguard allows to block some dependencies.
This ensure than
gopkg.in/yaml.v2
andgopkg.in/yaml.v3
are not used anymore assigs.k8s.io/yaml
provides them both