-
Notifications
You must be signed in to change notification settings - Fork 22
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 to generate API token with tanzu api-token create
#820
Conversation
3a0faa6
to
60a3bf0
Compare
tz api-token create
tanzu api-token create
a4d8c6d
to
5bde1ca
Compare
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.
Some comments about the output as I continue the review
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.
looking good in general, thanks the for changes.
I have a few questions and suggestions.
And some UX output needs to be updated
7d63052
to
328a84c
Compare
pkg/command/apitoken.go
Outdated
|
||
fmt.Fprint(cmd.OutOrStdout(), bold.Sprint("==\n\n")) | ||
fmt.Fprintf(cmd.OutOrStdout(), "%s Your generated API token is: %s\n\n", bold.Sprint("API Token Generation Successful!"), cyanBold.Sprint(token.RefreshToken)) | ||
fmt.Fprintf(cmd.OutOrStdout(), "For non-interactive login use the API token as follow: %s\n\n", cyanBold.Sprint("TANZU_API_TOKEN=<token> tanzu login --endpoint <tanzu-platform-endpoint>")) |
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 token can only be used for UAA, correct?
If so, instead of <tanzu-platform-endpoint>
, should we put something with uaa
in it?
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.
That UAA is used is implementation details that may be lost on many users. In a comment earlier, I suggested using terminology that is more comprehensible. Still thinking of options...
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.
You are right. Maybe "TPSM" is more appropriate, or the long form of it?
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.
Maybe the way it is implemented is reasonable enough. Users who have to use the form "tanzu login --endpoint ..." and who has successfully use "api-token create" to get to this point should know what tanzu-platform-endpoint refers to.
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 have updated the log message to mention actual Tanzu Platform endpoint that needs to be used instead of <tanzu-platform-endpoint>
. Let me know if any other changes are needed here.
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.
Nice.
Minor suggestions to take or leave.
f718d5d
to
5ab4d76
Compare
@marckhouzam regarding this, I discussed with @vuil and the conclusion is we do not have a way to know the expiration time for the Refresh Token at the moment. So, if needed we need to document the expiration time separately in the docs. |
5ab4d76
to
f008087
Compare
So we will document that the token has a lifetime of X time? I assume there is no advanced notification for the user to know the token is about to expire? |
Yes. We do not know the lifetime of this refresh token at the moment. And the time can be updated without CLI's knowledge from the backend so it is safe to put that information in the docs.
Correct, there isn't any advanced notification for the user to know the token is about to expire. Please note that this API token generation from the CLI is a temporary workaround for supporting the non- interactive login with UAA because UAA does not have any UI to generate this token. There will be updates and enhancement in this area in future that would likely make CLI more user friendly. |
f008087
to
ed7ae50
Compare
ed7ae50
to
e1aa16b
Compare
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 think we may still want to tweak the messages but the rest lgtm, thanks!
bold := color.New(color.Bold) | ||
|
||
fmt.Fprint(cmd.OutOrStdout(), bold.Sprint("==\n\n")) | ||
fmt.Fprintf(cmd.OutOrStdout(), "%s Your generated API token is: %s\n\n", bold.Sprint("API Token Generation Successful!"), cyanBold.Sprint(token.RefreshToken)) |
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.
We may have to workshop the err/output messages in a followup. Some thoughts
- Is it better to output the token in stdout, and show the rest in stderr?
- do we want to advocate TANZU_API_TOKEN=%s tanzu login, or mention the config file way to persist the env var?
The messages in these function
- may confuse users who are interacting with the public endpoint, who ends up creating a valid
tanzu
endpoint and still get a surprise when this fails for him - the instructions for regenerate and re-login might be confusing to the user of the current CLI instance (who has obviously logged in)
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.
Is it better to output the token in stdout, and show the rest in stderr?
I would prefer using stdout
for both the token and the rest of the output. Since this is an output message intended for the user, it's more conventional to keep all output in stdout
.
e1aa16b
to
25baf0d
Compare
Signed-off-by: Anuj Chaudhari <[email protected]>
25baf0d
to
b3f007f
Compare
* Add support to generate API token with `tanzu api-token create` Signed-off-by: Anuj Chaudhari <[email protected]>
What this PR does / why we need it
tanzu api-token create
commandSample output:
Which issue(s) this PR fixes
Fixes #
Describe testing done for PR
Release note
Additional information
Special notes for your reviewer