-
Notifications
You must be signed in to change notification settings - Fork 306
Add ability to render output in json and yaml #717
Conversation
This adds the ability to select from the default table output, or have more machine readable output in yaml or json format. Signed-off-by: Sean McGinnis <[email protected]>
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.
LGTM
Tested on a standalone cluster, installed the default package repository and listed with table, yaml, and json. All look great! Very neat implementation too
// renderJSON prints output as json | ||
func renderJSON(ow *outputwriter) { | ||
data := ow.dataStruct() | ||
bytesJSON, err := json.MarshalSafeCollections(data) |
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.
I see that github.com/helloeave/json
forwards to github.com/homelight/json
(probably an internal change of ownership). It looks to be a fork of go's encoding/json
library but with a few additional "safe" methods for handling null
, primarily through the MarshalSafeCollections
method. But it hasn't been updated in ~1/2 year. Would it be worth including logic in our own writer in order to handle null cases so we can instead use the core encoding/json
library?
Hmmm. But, on deeper inspection, it seems this isn't really handled well in go at all. If this gets around an imperfection in the go library, makes sense to me 👍
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.
@stmcginnis - Circling back on this one. I think it's reasonable to introduce this library so we don't have to deal with Go's handling of null cases in json. Let me know your thoughts and we can merge
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 bringing this up again. I should mark this as WIP for now.
Based on discussion in Slack, the core
repo really is a more appropriate place for something like this. Then our CLI plugins can just use that common functionality so we're consistent across all plugins.
That change has now merged to core
. We just need to wait to use a more recent commit from there and then I can update this PR to switch over to using the common functionality.
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! Thanks for the update - good to hear it's made it's way into core
!
No longer needed now. Changes have been merged to tanzu-framework, but we no longer have commands that need the different output formats in the TCE repo. |
What this PR does / why we need it
This adds the ability to select from the default table output, or have
more machine readable output in yaml or json format.
Which issue(s) this PR fixes
Fixes: #626
Describe testing done for PR
Built plugins locally and tested output:
Special notes for your reviewer
We should decide if this is enough, or if we would like to emit additional data in these formats that we wouldn't necessarily display in the table output.
Does this PR introduce a user-facing change?