Skip to content
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

WIP Add a k6 cloud run --local-execution mode of execution #3818

Closed
wants to merge 7 commits into from

Conversation

oleiade
Copy link
Member

@oleiade oleiade commented Jun 27, 2024

What?

This PR adds support for a new --local-execution flag to the k6 cloud run command. Using it, users can run their test script or archive on their local machine, while streaming the results to the cloud.

It is built upon the latter, and ⚠️ cannot be merged before #3813 ⚠️.

In the context of this PR, the goal is to reproduce the behavior of the k6 run -o cloud behavior, and depending on the advancement of internal work, to also add support for adding archive upload. In a future PR, at a later point in time, we will also add support for logs and traces streaming to the cloud but are dependent on internal work to be able to do so.

⚠️ This is currently a work-in-progress, and non-functional (yet) PR ⚠️

Why?

We aim to integrate the experience of running tests locally, whilst streaming results to the cloud more intuitive, and integrated in the k6 experience. We also aim to take this opportunity to align some of the logic and behavior involved in running tests in the cloud and running tests locally better.

Task list

  • We're gonna introduce a couple of options specific to --local-execution. One of the ideas we've been having in the k6 v1 UX work stream revolved around the idea of scoping options with a dot separator like in Prometheus: --local.execution, --local.send-archive, --local.send-logs, etc. cc @dgzlopes
  • Once we're at that point we'd like sending the archive to be opt-in (via an option that users need to explicitly pass). Same goes for logs. The reason being that they might contain sensitive info. cc @dgzlopes
  • When using k6 cloud run --local-execution, we should make sure the outputs field of the terminal output of the command displays a sensible value (like the URL of the job in the grafana cloud app?). cc @dgzlopes
  • Should the k6 cloud run --local-execution show the same terminal output as k6 run or k6 cloud run?

Checklist

  • I have performed a self-review of my code.
  • I have added tests for my changes.
  • I have run linter locally (make lint) and all checks pass.
  • I have run tests locally (make tests) and all tests pass.
  • I have commented on my code, particularly in hard-to-understand areas.

Related PR(s)/Issue(s)

@oleiade oleiade changed the base branch from master to cloud_subcommands June 27, 2024 15:56
@codecov-commenter
Copy link

codecov-commenter commented Jun 27, 2024

Codecov Report

Attention: Patch coverage is 15.29412% with 216 lines in your changes missing coverage. Please review.

Please upload report for BASE (cloud_subcommands@1468a97). Learn more about missing BASE report.

Current head 0226b54 differs from pull request most recent head 812efc5

Please upload reports for the commit 812efc5 to get more accurate results.

Files Patch % Lines
cmd/cloud_run.go 9.32% 212 Missing and 2 partials ⚠️
js/modules/k6/html/html.go 90.90% 0 Missing and 1 partial ⚠️
js/runner.go 50.00% 1 Missing ⚠️
Additional details and impacted files
@@                 Coverage Diff                  @@
##             cloud_subcommands    #3818   +/-   ##
====================================================
  Coverage                     ?   70.98%           
====================================================
  Files                        ?      293           
  Lines                        ?    21827           
  Branches                     ?        0           
====================================================
  Hits                         ?    15494           
  Misses                       ?     5244           
  Partials                     ?     1089           
Flag Coverage Δ
ubuntu 70.92% <15.29%> (?)
windows 70.85% <15.29%> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@joanlopez
Copy link
Contributor

joanlopez commented Jul 15, 2024

One of the ideas we've been having in the k6 v1 UX work stream revolved around the idea of scoping options with a dot separator like in Prometheus: --local.execution, --local.send-archive, --local.send-logs, etc

I don't know how sensible that would be, but as far as I know, using the dot as separator is not very Windows friendly (it requires quoting, see PowerShell/PowerShell#19640), just to be considered when evaluating it.

Comment on lines +108 to +117
// TODO: Figure out a better way to handle the CLI flags
flags.BoolVar(&c.exitOnRunning, "exit-on-running", c.exitOnRunning,
"exits when test reaches the running status")
Copy link
Contributor

@joanlopez joanlopez Jul 15, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we add some kind of validation for incompatible flags? For instance, --exit-on-running & --local-execution, which I think don't have sense together.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great idea, let's look into it 👍🏻

Comment on lines +113 to +121
flags.BoolVar(&c.uploadOnly, "upload-only", c.uploadOnly,
"only upload the test to the cloud without actually starting a test run")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See #3850

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Dropping this 👍

oleiade and others added 6 commits July 18, 2024 16:22
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 <5459617+joanlopez@users.noreply.github.com>
Base automatically changed from cloud_subcommands to master July 24, 2024 10:53
@joanlopez joanlopez mentioned this pull request Jul 30, 2024
5 tasks
@codebien
Copy link
Collaborator

codebien commented Jul 30, 2024

One of the ideas we've been having in the k6 v1 UX work stream revolved around the idea of scoping options with a dot separator like in Prometheus

If we already envision the need for it, isn't better to have a dedicated subcommand? I mean something like k6 cloud local-run --send-logs script.js

@oleiade
Copy link
Member Author

oleiade commented Aug 21, 2024

Closing in favor of #3904

@oleiade oleiade closed this Aug 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants