Conversation
whoami command prints the currently logged in user fixes: #339
rkpattnaik780
force-pushed
the
whoami
branch
from
ee0029f to
832b3be
Compare
craicoverflow
left a comment
craicoverflow
left a comment
There was a problem hiding this comment.
I've left some feedback on what needs to be changed. It's almost there 😄
Most commands follow a very similar pattern, so check out similar commands when working on one.
pkg/cmd/whoami/whoami.go
Outdated
| return cmd | ||
| } | ||
|
|
||
| func getCurrentUser(opts *Options) (err error) { |
There was a problem hiding this comment.
I would suggest naming this command as runCmd. Though we are getting the current user, it is not being returned, rather it is just printed inside the function so can never be "gotten" from another function.
pkg/cmd/whoami/whoami.go
Outdated
|
|
||
| cfg, err := opts.Config.Load() | ||
| if err != nil { | ||
| logger.Error(err) |
There was a problem hiding this comment.
No need to log this error, it will be printed twice as returning it will print it from the main.go.
pkg/cmd/whoami/whoami.go
Outdated
|
|
||
| userName, _ := tknClaims["preferred_username"] | ||
|
|
||
| logger.Info(fmt.Sprintf("%v", userName)) |
There was a problem hiding this comment.
This needs to be printed to stdout. The username being printed to stdout is vital for scripting.
If, for example I want to run rhoas whoami from within a Bash script, I will want the script to be able to read the username and use it. If it is printed to stderr it will be invisible to the script, as stderr is for logging information that only a human could understand (think natural language).
Use fmt.Fprintln to print the username to stdout. Use the IOStreams abstraction as the io.Writer:
pkg/cmd/whoami/whoami.go
Outdated
|
|
||
| accessTkn, _ := token.Parse(cfg.AccessToken) | ||
|
|
||
| if accessTkn == nil { |
There was a problem hiding this comment.
If my session has expired, my access token will still be in the config, but I will be not logged in. So checking if the access token is nil is not enough.
Also, the access token may also be expired, but if the refresh token is valid the user can still be logged in once it is refreshed.
The best thing to do here is let the Connection instance handle checking if the user is logged in.
See how this is done in the logout command: https://github.com/bf2fc6cc711aee1a0c2a/cli/blob/76028b9a74ba70fc546cfdf15b68217a49606ed6/pkg/cmd/logout/logout.go#L56
locales/cmd/whoami/active.en.toml
Outdated
| [whoami.cmd.use] | ||
| description = "Use is the one-line usage message" | ||
| one = "whoami" | ||
|
|
||
| [whoami.cmd.shortDescription] | ||
| description = "Short description for command" | ||
| one = "Print current user name" | ||
|
|
||
| [whoami.cmd.longDescription] | ||
| description = "Long description for command" | ||
| one = ''' | ||
| Print the user name of currently active user. | ||
|
|
||
| This command prints the username associated with the user currently logged in. | ||
| ''' | ||
|
|
||
| [whoami.cmd.example] | ||
| description = 'Examples of how to use the command' | ||
| one = ''' | ||
| # print current user name | ||
| $ rhoas whoami | ||
| ''' |
There was a problem hiding this comment.
Hi @bhardesty can you take a look at this command help-text?
locales/cmd/whoami/active.en.toml
Outdated
| [whoami.error.notLoggedInError] | ||
| description = 'Text to display if no user is logged in' | ||
| one = 'not logged in. Run "rhoas login" to authenticate' No newline at end of file |
There was a problem hiding this comment.
This can go once you let the Connection handle the login status.
locales/cmd/whoami/active.en.toml
Outdated
|
|
||
| [whoami.cmd.shortDescription] | ||
| description = "Short description for command" | ||
| one = "Print current user name" |
There was a problem hiding this comment.
| one = "Print current user name" | |
| one = "Print current username" |
locales/cmd/whoami/active.en.toml
Outdated
| [whoami.cmd.longDescription] | ||
| description = "Long description for command" | ||
| one = ''' | ||
| Print the user name of currently active user. |
There was a problem hiding this comment.
| Print the user name of currently active user. | |
| Print the username of the currently active user. |
locales/cmd/whoami/active.en.toml
Outdated
|
|
||
| [whoami.error.notLoggedInError] | ||
| description = 'Text to display if no user is logged in' | ||
| one = 'not logged in. Run "rhoas login" to authenticate' No newline at end of file |
There was a problem hiding this comment.
| one = 'not logged in. Run "rhoas login" to authenticate' | |
| one = 'Not logged in. Run "rhoas login" to authenticate.' |
bhardesty
left a comment
bhardesty
left a comment
There was a problem hiding this comment.
Just a couple minor suggestions for the help text.
locales/cmd/whoami/active.en.toml
Outdated
| [whoami.cmd.example] | ||
| description = 'Examples of how to use the command' | ||
| one = ''' | ||
| # print current user name |
There was a problem hiding this comment.
| # print current user name | |
| # print current username |
3 participants
whoami command prints the currently logged in user.
fixes: #339