-
Notifications
You must be signed in to change notification settings - Fork 593
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
Add support for JSON version output #171
Conversation
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 for taking this on, @dakaneye! One minor comment on struct tags.
Also, I'm not sure this is achieving the desired output based on the issue. I could be mistaken, but I believe the intent is for the JSON output to show the full version details even without the verbose switch.
For reference:
syft version -o json
should report all version details in json to aid integrations.
When I run that command right now, I only see an abbreviated JSON output.
I don't think we have a need for any JSON consumers to need to specify less verbosity, so we can just always show the full output.
cmd/version.go
Outdated
if showVerboseVersionInfo { | ||
err = enc.Encode(&struct { | ||
version.Version | ||
Application string `json:"Application"` |
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.
This struct tag won't have an effect on the output, since it's not changing the name of the struct field. Usually you'll see JSON struct tags used like this if you're changing the casing (e.g. making the field name lowercase or something) or the name altogether (less common).
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.
hmm, my lack of Go expertise is showing here. I was working off of the example in Grype, where mainly we want to include the application name in the version output, but it's not part of the original object. The way I see it there are 4 options:
- Add the Application Name field to the Version object
- Create a type that combines the two objects
- Use this struct tag
- Use a map
I kept the struct for now so that the output matches the format of grype's (I think it's good to be consistent?)
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 made all the keys lowercase which I think matches best practice AFA json goes.
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.
Awesome! A few points on this:
-
Re functionality: it looks like you've nailed it! (When I run
go run main.go version -o json
, I see several fields on a single root object. Perfect.) -
Re struct tags: Based on your comment, I'm not sure if what I said was clear. The problem wasn't where the struct field was, it was just that the struct tag itself (the
`json:"Application"`
) was renaming the fieldApplication
toApplication
— so since the rename was a no-op, the struct tag didn't need to be there. -
Fun fact on structs: You mention moving the Application field to the Version struct. For this output implementation, it actually doesn't matter if you do that or not. You might have noticed that although you didn't do that, you see the
version.Version
struct fields appearing in the output at the same level as the outer struct'sApplication
field. This is becauseversion.Version
isn't given a field name in the outer struct, so its fields are what Go calls "promoted fields". When you have this kind of anonymous struct inside another struct, its fields get promoted. This isn't just for encodings like JSON, but for any time the struct is accessed within the code. It's a cool and somewhat unknown feature. 🤓 -
Re casing in JSON: Now that you're changing the field names (by using "kebab" case), the struct tags are no longer useless. Usually with JSON fields, you'll see camel case (e.g.
gitTreeState
,goVersion
). This originates in JS, where JSON was born. Snake case (git_tree_state
, etc.) is also somewhat common. The trouble with kebab case is that it makes object properties more difficult to access when deserialized, since the hyphen (-
) usually breaks apart the field name into multiple parts (incorrectly), which happens because the hyphen isn't a valid character within symbol names in most languages. So I'd recommend changing tocamelCase
orsnake_case
.
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.
@luhring hmm this was also ported over to be consistent with Grype. We should probably change the casing over there too.
512f4d5
to
80a0e74
Compare
Signed-off-by: Samuel Dacanay <sam.dacanay@anchore.com> Make json version output always verbose, cleanup struct tag Signed-off-by: Samuel Dacanay <sam.dacanay@anchore.com> Use camel case for json Signed-off-by: Samuel Dacanay <sam.dacanay@anchore.com>
80a0e74
to
ef64a6f
Compare
Add support for JSON version output
Fixes #122
Signed-off-by: Samuel Dacanay sam.dacanay@anchore.com