-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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 JSON and YAML to several tsh commands #11681
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.
Overall looks good and the issues are minor. Approving, as nothing critical stands out.
@@ -252,13 +271,42 @@ func onDatabaseConfig(cf *CLIConf) error { | |||
default: | |||
return trace.BadParameter("unknown database protocol: %q", database) | |||
} | |||
switch cf.Format { | |||
|
|||
format := strings.ToLower(cf.Format) |
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.
+1 for ignoring the case of the format. Do we have consistency with our other CLI tools in this regard? We may want to apply the same treatment e.g. for tctl
, although preferably with a separate PR.
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.
@atburke Implementation looks good now apart from a few nitpicks, but this PR also needs test coverage. There are a couple of ways to test this I think, you can either setup auth/teleport process in tests, populate them with respective objects (apps, dbs, etc. - we should already be doing this somewhere here) and call respective onListXXX
methods, or factor out json/yaml rendering logic into separate functions (like you already have showApps, showDatabases) and test them to make sure they return correct output based on the provided inputs.
tool/tsh/tsh.go
Outdated
if apps == nil { | ||
apps = []types.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.
Will Marshal panic is apps are nil?
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.
it won't panic, but it will serialize nil
as null
instead of []
.
tool/tsh/tsh.go
Outdated
if databases == nil { | ||
databases = []types.Database{} | ||
} |
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.
Will Marshal panic is databases are nil?
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.
Same as above.
@r0mant Can you take another look at this? |
This PR adds support for JSON and YAML output to several tsh commands via the --format flag.
This change adds support for JSON and YAML output to several tsh commands via the --format flag.
This PR adds support for JSON and YAML output to several tsh commands via the
--format
flag.Commands with JSON/YAML
version
app ls
app config
db ls
db env
db config
play
ls
clusters
status
env
request ls
request show
kube ls
kube sessions
mfa ls
Commands without JSON/YAML
help
ssh
aws
app login
app logout
proxy ssh
proxy db
proxy app
db login
db logout
db connect
join
scp
login
logout
request create
request review
kube login
kube exec
kube join
mfa add
mfa rm
Resolves #6742.