-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Drop support for user & pass login #3835
Conversation
Important notice: this commit declare a cobra sub-command holding the logic for the `k6 cloud run` sub-command, but does not register it. In this commit, we duplicate the logic from the existing `k6 cloud` logic, with very little adjustments, to support the later registry of the `k6 cloud run` command. To simplify the collaboration on this and further reviews, we delegate any refactoring of the original cloud command's logic, to a further commit or Pull Request.
Important notice: this commit declare a cobra sub-command holding the logic for the `k6 cloud login` sub-command, but does not register it. In this commit, we duplicate the logic from the existing `k6 login` logic, with very little adjustments, to support the later registry of the `k6 cloud login` command. To simplify the collaboration on this and further reviews, we delegate any refactoring of the original cloud command's logic, to a further commit or Pull Request. This new `k6 cloud login` command is notably focusing solely on authenticating with the Grafana Cloud k6, and by design does not aim to support InfluxDB authentication.
@@ -30,7 +28,7 @@ func getCmdCloudLogin(gs *state.GlobalState) *cobra.Command { | |||
|
|||
// loginCloudCommand represents the 'cloud login' command | |||
exampleText := getExampleText(gs, ` | |||
# Log in with an email/password. | |||
# Prompt for a k6 Cloud token. |
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.
Should we keep both: prompting for a token when running k6 cloud login
and -t [TOKEN]
?
It sounds a bit redundant, and as we're planning to ship it in v1.0, I think it deserves some care in terms of carefully thinking of its implications.
Some thoughts:
- In favor of
k6 cloud login
:- Default behavior, user friendly walkthrough (although it could be replaced with a decent help)
- Doesn't fully (only partially) break the currently existing, interactive, behavior.
- In favor of
-t [TOKEN]
:- Friendlier for CLI environments (less interactivity is required)
- More flexible for future auth mechanisms (extensibility after v1.0)
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 would like to invest time in improving the interactive flow in the future (e.g., picking the default stack, using a magic link to the browser to authenticate). But yeah, I have no strong opinions on what we should do with it as of today... But it is true that if it is only to pass the token, it feels a bit redundant.
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.
Then, I'd probably vote to keep both but generally recommend the use of -t
, I guess 🤔
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.
Sorry for the delay, I agree, we should keep both: prompt for the token, and support -t
👍🏻
cmd/cloud_login.go
Outdated
@@ -79,18 +77,6 @@ func (c *cmdCloudLogin) run(cmd *cobra.Command, _ []string) error { | |||
} | |||
} | |||
|
|||
// We want to use this fully consolidated config for things like | |||
// host addresses, so users can overwrite them with env vars. | |||
consolidatedCurrentConfig, warn, err := cloudapi.GetConsolidatedConfig( |
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.
The consolidated config was used to initialize the cloudapi.Client
, no longer needed.
cmd/cloud_login.go
Outdated
consolidatedCurrentConfig.Timeout.TimeDuration()) | ||
|
||
var res *cloudapi.LoginResponse | ||
res, err = client.Login(email, password) |
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.
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 agree! It would be nice to validate the token.
Can we do this by just trying to call one of the authenticated APIs? But yeah, a dedicated endpoint maybe would be better. I think you should involve someone from backend 👀
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 discussed internally it with the Backend team, and they'll add support for the /login
endpoint to also accept a token
for auth (as an alternative for now, to keep backwards compatible; and as the unique method supported in the future).
However, as that will take some time, I guess we can move forward with this PR and add that in the next iteration (I'll create an issue to do not forget).
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.
@joanlopez Can you expand a bit on the reason for having it, please? What is the user journey that we expect here, and what are the benefits?
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.
Sure @codebien! The purpose of such check would be to make the resulting user experience better.
Now, when the user logs in with email & password, they get immediate feedback whether those credentials were correct or not, and the token saved is always valid, as it is returned by the API.
However, after these changes, the user won't log in, but would just set a token. So, if we don't add an extra check to verify whether the token is valid or not, the user won't receive such feedback until they try to run another command. While, in my opinion, would be much better to give them the feedback of whether the token was valid or not at time of running k6 cloud login
.
Does that sound good to you? Do you have any other suggestion?
Thanks! 🙇🏻
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.
Gotcha, that sounds good and valuable 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.
I would argue that this was the behaviour before for -t
on the cli.
I am not against both that and this to do some checks, but I would not block this PR on endpoint to check this.
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 would argue that this was the behaviour before for
-t
on the cli.
Yeah, that was my initial thought, but having being like this doesn't mean we cannot improve the user experience, imo. Plus, the use of token was arguably an alternative for the main login flow with standard credentials (although I know that in practice that's no longer the case), but now it will become the main (and unique?), and so I think there's more reason to keep the main login flow with a good user experience, as it used to be the case for email and password.
I am not against both that and this to do some checks, but I would not block this PR on endpoint to check this.
Yeah, I agree! That's why said above:
However, as that will take some time, I guess we can move forward with this PR and add that in the next iteration (I'll create an issue to do not forget).
aad9c76
to
2b0bc1f
Compare
Banner: "Enter your token to authenticate with k6 Cloud.\n" + | ||
"Please, consult the k6 Cloud documentation for instructions on how to generate one:\n" + | ||
"https://grafana.com/docs/grafana-cloud/testing/k6/author-run/tokens-and-cli-authentication", |
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.
Should we link directly to the Grafana app page where the token is?
Closing in favor of #3886 |
What?
Drop k6 Cloud login support for email and password login: this includes the interactive cli triggered when running k6 cloud login, it should instead request your API token.
Why?
Checklist
make lint
) and all checks pass.make tests
) and all tests pass.Related PR(s)/Issue(s)