-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Move cloud run and login functionalities under cloud subcommands #3813
Conversation
b7efb31
to
1468a97
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #3813 +/- ##
==========================================
- Coverage 71.87% 71.69% -0.18%
==========================================
Files 291 293 +2
Lines 21274 21606 +332
==========================================
+ Hits 15290 15490 +200
- Misses 4920 5029 +109
- Partials 1064 1087 +23
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
cmd/cloud.go
Outdated
} | ||
|
||
if len(args) == 2 && !isRunSubcommand && !isLoginSubcommand { | ||
return fmt.Errorf("unexpected argument: %s", args[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.
Cleaning up the PR history in preparation for leaving draft state 👀 |
ddce8cb
to
cf7e0b9
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.
Great job! 🚀
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.
Co-authored-by: Joan López de la Franca Beltran <[email protected]>
aad9c76
to
2b0bc1f
Compare
I have squashed the PR suggestion commits. I've also applied a pretty significant refactor to avoid code duplication: the cloud run command relies on the cloud command's code directly, and I've refactored the tests to share the same test code between both the If we're happy with this approach, I'll also apply it the |
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.
Logs good to me, left a few minor non-blocking suggestions 👍
Co-authored-by: Oleg Bespalov <[email protected]>
What?
This PR modifies the k6 command-line interface and introduces the cloud run and login functionalities under specific subcommands:
k6 cloud run
andk6 cloud login
.These two new subcommands supersede the existing
k6 cloud
andk6 login cloud
functionalities, but as planned, they will remain in place until we reach k6 version 1.0.0. However, they now display a deprecation warning inviting users to move to the new commands.Future plans include to eventually, on k6 v1.0.0 release, we expect to modify the
k6 cloud
command behavior so that it doesn't run scripts in the cloud anymore but only displays the help text documenting its subcommands.Technicalities
From a technical perspective, this PR "only" duplicates the existing logic under new subcommands and registers those. Some outstanding refactors are mentioned throughout their code, but we adopted the approach of delegating those refactors and improvements to a future PR.
Why?
This PR is part of the effort to improve the UX of our command-line interface towards k6 v1.0.0. It aligns with our goals to unify and expand the k6 CLI's capabilities to interact with the cloud. In my absence for UX-specific concerns and discussions, please get in touch with @dgzlopes, who have as much context as we do.
TaskList
k6 cloud login
support for email and password login: this includes the interactive cli triggered when runningk6 cloud login
, it should instead request your API token.k6 cloud run --upload-only
flag, and replace it withk6 cloud upload
. If this is a bigger task that I anticipate, we should do it in a separate PR.k6 cloud run
command (asserting the arguments management especially)k6 cloud login
command: I wonder what we can improve on that front, but it would be worth looking into it.Checklist
make lint
) and all checks pass.make tests
) and all tests pass.Related PR(s)/Issue(s)