-
Notifications
You must be signed in to change notification settings - Fork 104
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
Use sigs.k8s.io/yaml rather than pkg.in/yaml.v2. #1438
Conversation
Signed-off-by: Marcin Owsiany <mowsiany@D2iQ.com>
Signed-off-by: Marcin Owsiany <mowsiany@D2iQ.com>
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.
🚢
Also, I'd like to see a test for the #1437 |
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.
Thanks. A unit test for this would be good, apart from that lgtm.
Signed-off-by: Marcin Owsiany <mowsiany@D2iQ.com>
Thanks for the quick reviews. Test added. |
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.
👏 nice work! lgtm
What this PR does / why we need it:
Turns out that
yaml.v2
unmarshals maps intomap[interface{}]interface{}
ratherthan try to insist on a most narrow possible key type (i.e. string). Such values
are then rejected by sigs.k8s.io/yaml.Marshal that
WrapParamValue
uses,because it does a conversion to JSON first, and JSON does not support
non-string keys.
It seems like
yaml.v3
works better in that regard. However comments on theaforementioned issue seem to imply that it's still not all rosy.
Moreover, using the same package for unmarshalling as we do for marshalling
seems like a more stable choice long-term.
This also adds some vlog statements and tidies up go.mod.
Fixes #1437
Fixes #1463