Skip to content
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

feat: --format=json #7

Merged
merged 5 commits into from
Jan 22, 2021
Merged

feat: --format=json #7

merged 5 commits into from
Jan 22, 2021

Conversation

jfatta
Copy link
Contributor

@jfatta jfatta commented Jan 22, 2021

Add support for --format=json
go run ./cmd/auth0 clients list --format=json | jq
image

@@ -33,6 +34,7 @@ type cli struct {

verbose bool
tenant string
format string
Copy link
Contributor Author

@jfatta jfatta Jan 22, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd like to use display.OutputFormat here directly (instead of string) but I didn't know how to make it work case insensitive

@@ -96,9 +98,17 @@ func (c *cli) init() error {
c.tenant = c.data.DefaultTenant
}

format := strings.ToLower(c.format)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

json, JSON it should work for the user

Comment on lines +109 to +110
MessageWriter: os.Stderr,
ResultWriter: os.Stdout,
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

intermediate messages use Stderr to keep the stdout clean for the final result (json or not), this way the output can be piped to another command (like jq)

}

func (r *Renderer) Table(header []string, data [][]string) {
func writeTable(w io.Writer, header []string, data [][]string) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

private now in favor of .Result(). The cli setup will know when to render as table, or json, YAML, etc

Comment on lines +31 to 35
res = append(res, &clientView{
Name: auth0.StringValue(c.Name),
Type: appTypeFor(c.AppType),
ClientID: auth0.StringValue(c.ClientID),
})
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What if for JSON we just dumped the whole object? let users do whatever filtering/processing/formatting they need with jq or similar? There's a lot of info there it could be really useful to have (https://github.com/go-auth0/auth0/blob/master/management/client.go#L3)

Copy link
Contributor Author

@jfatta jfatta Jan 22, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like the idea of having --full or --deep mode. not completely sure if the format should also imply the level of details. As a user, I would expect the same columns and transformations (like appTypeFor()) instead of having the internals exposed, makes sense?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

imagine if this command includes everything available here 😅: https://docs.github.com/en/rest/reference/pulls#list-pull-request

❯ gh pr list --state closed

Showing 17 of 17 pull requests in auth0/pkg that match your search

#17  This bumps go-instrumentation to 0.3.2 which bumps sentry to 0.0.9                  sentry-v0.0.9-bump
#16  Bumping lightstep tracer to 0.23.0                                                  bump-lightstep-tracer-to-0.23.0
#15  loggertest: ExpectLogged now takes in a proto.Message                               change-expect-log-to-use-proto
#14  Log the Addr to which the http server binds                                         log-bound-addr
#13  Bump datadag to 4.2.0                                                               bump-datadog-to-4-2-0
#12  Add error reporter via sentry                                                       add-error-reporter-via-sentry
#11  Add secretbox with aes+gcm implementation                                           add-crypto-aes-gcm-package
#10  Setup CI with Jenkins                                                               jenkins
#9   Update version of statds library, with fixed typo for With{out}AggregationInterval  ja30278:master
#8   Use auth0/go-instrumentation for logging                                            go-instrumentation
#7   Support defining a namespace for service metrics                                    support-metric-namespace
#6   Add Timing to metrics interface                                                     add-timing-to-interface
#5   Add Timing to metrics                                                               timing
#4   Adds lightstep tracer                                                               add-tracer
#3   enable client side aggregation                                                      use-client-side-aggregation
#2   service: extract standard service concept                                           extract-service-concept
#1   Add github actions                                                                  add-github-actions-ci

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think for text mode it makes sense to curate what the user sees, but for JSON mode it's likely just being piped to something else and the full object probably makes more sense IMO.

But in any case, absolutely not a blocker and can be discussed (bikeshedded 😆) later.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it is a pretty valid point, eventually we'll need to show more details. we can go back to this as we move on with the commands 🙇

Copy link
Contributor

@paddycarey paddycarey left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

internal/cli/cli.go Outdated Show resolved Hide resolved
Comment on lines +74 to +77
type View interface {
AsTableHeader() []string
AsTableRow() []string
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I partially agree with @paddycarey regarding full results eventually. For the purposes of this PR, let's keep it as it is.

The only change I think we need to do in the future is maybe:

  1. Make View have a Raw interface{} which we can dump the entire SDK's response to.
  2. For the JSON format, we just use the Raw instead of the View directly.

Either way great progress!

@cyx cyx force-pushed the feat-json-format branch from f633e09 to a0fa473 Compare January 22, 2021 19:29
@cyx
Copy link
Contributor

cyx commented Jan 22, 2021

@jfatta took the liberty to rebase and fix the breakage with the logs stuff expecting a Writer.

@cyx cyx merged commit 769c0f1 into main Jan 22, 2021
@cyx cyx deleted the feat-json-format branch January 22, 2021 19:31
@jfatta
Copy link
Contributor Author

jfatta commented Jan 22, 2021

@jfatta took the liberty to rebase and fix the breakage with the logs stuff expecting a Writer.

Awesome, thanks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants