Replace Shopify/yaml with gopkg.in/yaml.v3 #227
Conversation
ghost
added
| &yaml.Node{Kind: yaml.MappingNode, Tag: mapTag, Content: []*yaml.Node{ | ||
| {Kind: yaml.ScalarNode, Tag: strTag, Value: "K", HeadComment: "Hi"}, | ||
| {Kind: yaml.ScalarNode, Tag: strTag, Value: "V2", LineComment: "Bye"}, | ||
| }}, |
There was a problem hiding this comment.
nit: because this is kind of a big PR, I would recommend trying to keep as many things constant as possible to make it easy for reviewers. In this test case both the structure and the content of the nodes change, which makes things a little more complicated - especially because I'm new to the codebase. If we want to test both comment types and we didn't support them before it might make sense to split out multiple smaller commits that reviewers can consider individually like: "move to yaml.v3 nodes", "add HeadComment to test cases"? Just a suggestion for future PRs.
There was a problem hiding this comment.
Yes, that makes sense! We did support them before, they just looked different. In the old format, aka what the fork uses, it would have looked like this:
yaml.MapSlice{
{Comment: "Hi"},
{Key: "K", Value: "V2", Comment: "Bye"},
},
but I agree that I should have done a direct conversion and kept the test contents the same. I'll keep it in mind for next time!
| - /bin/sh | ||
| - -c | ||
| - ls /etc/config/ | ||
| command: ["/bin/sh", "-c", "ls /etc/config/"] |
There was a problem hiding this comment.
These changes are kind of surprising, since the library is supposed to preserve the structure of the YAML file? Is this caused by the move to yaml.v3?
There was a problem hiding this comment.
Actually this output matches the formatting of the input file https://github.com/Shopify/kubeaudit/blob/master/fixtures/preserve_comments_v2.yml#L35
Our fork used default formatting for all data types so all arrays got marshaled into the multi-line dashed formatting. It wasn't great but it wasn't technically incorrect as far as yaml parsers could tell.
There was a problem hiding this comment.
I like the one liner way more :)
genevieveluyt
changed the title
| @@ -189,7 +189,8 @@ spec: | |||
| spec: | |||
| # Trust me the following line is longer than 80 characters | |||
| containers: | |||
| - image: gcr.io/docker-images-directory/apps/production/fake-image-app:<%= current_sha %> | |||
| - image: gcr.io/docker-images-directory/apps/production/fake-image-app:<%= current_sha | |||
There was a problem hiding this comment.
Is there a way to not do the 80 (or whatever it is) character limit?
There was a problem hiding this comment.
I doesn't look like it's supported right now but I could open a feature request
| @@ -21,5 +21,5 @@ spec: | |||
| podSelector: # chooses pods with type=monitoring | |||
| matchLabels: | |||
| type: monitoring | |||
| - ports: # This is comment 1 | |||
| - port: 3452 #This is comment 2 | |||
| - ports: | |||
There was a problem hiding this comment.
What happened to this and the other comments?
There was a problem hiding this comment.
These caused tests to fail because of the bug in goyaml but since they're covered by the PreserveComments test, I took them out to make sure nothing else was causing the tests to fail. I can put them back but I think the PreserveComments test is thorough enough. What do you think?
| - /bin/sh | ||
| - -c | ||
| - ls /etc/config/ | ||
| command: ["/bin/sh", "-c", "ls /etc/config/"] |
There was a problem hiding this comment.
I like the one liner way more :)
Description
Now that goyaml v3 is out and supports comments, we can use that instead of the Shopify fork of goyaml v2.
There is one test that fails because of go-yaml/yaml#508. Once that is fixed, we need to update the goyaml v3 dependency and then hopefully all the tests will pass!Fixes #223
Type of change
How Has This Been Tested?
test.shChecklist: