Cloud subcommand 'upload' to replace --upload-only #3850
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.
joanlopez
added
cloud
breaking change
joanlopez
changed the title
cmd/cloud_run.go
Outdated
| @@ -367,8 +351,6 @@ func (c *cmdCloudRun) flagSet() *pflag.FlagSet { | |||
| "exits when test reaches the running status") | |||
| flags.BoolVar(&c.showCloudLogs, "show-logs", c.showCloudLogs, | |||
| "enable showing of logs when a test is executed in the cloud") | |||
| flags.BoolVar(&c.uploadOnly, "upload-only", c.uploadOnly, | |||
There was a problem hiding this comment.
As we're introducing new (sub)commands, I'd prefer to stop supporting the (deprecated) --upload-only flag from the very beginning (or as soon as the k6 cloud upload command is available).
| test, err := loadAndConfigureLocalTest(c.gs, cmd, args, getPartialConfig) | ||
| if err != nil { | ||
| return err | ||
| } | ||
|
|
||
| // It's important to NOT set the derived options back to the runner | ||
| // here, only the consolidated ones. Otherwise, if the script used | ||
| // an execution shortcut option (e.g. `iterations` or `duration`), | ||
| // we will have multiple conflicting execution options since the | ||
| // derivation will set `scenarios` as well. | ||
| testRunState, err := test.buildTestRunState(test.consolidatedConfig.Options) | ||
| if err != nil { | ||
| return err | ||
| } | ||
|
|
||
| // TODO: validate for usage of execution segment | ||
| // TODO: validate for externally controlled executor (i.e. executors that aren't distributable) | ||
| // TODO: move those validations to a separate function and reuse validateConfig()? |
There was a problem hiding this comment.
Most of this code, as well as some of the code below, can be shared with k6 cloud run. Perhaps, we could try to extract some common functions, also to make these very long methods a bit more readable?
| return err | ||
| } | ||
|
|
||
| refID := cloudTestRun.ReferenceID |
There was a problem hiding this comment.
I guess this is out of the scope of this pull request, but do we really want the k6 cloud upload command to create a new test run? Or just a test (if not exists)? Is it technically/logically viable?
| } | ||
| testURL := cloudapi.URLForResults(refID, cloudConfig) | ||
| executionPlan := test.derivedConfig.Scenarios.GetFullExecutionRequirements(et) | ||
| printExecutionDescription( |
There was a problem hiding this comment.
I'm not super convinced how much sense this makes here; as long as we keep generating a new test run, it partly does (because it displays the URL), but some information there (like the scenario, etc) definitely does not.
| flags.AddFlagSet(optionFlagSet()) | ||
| flags.AddFlagSet(runtimeOptionFlagSet(false)) |
There was a problem hiding this comment.
We should revisit which of all of these still make sense for just uploading the archive.
oleiade
force-pushed
the
cloud_subcommands
branch
2 times, most recently
from
aad9c76 to
2b0bc1f
Compare
|
As discussed during our latest pair programming session, this should also drop the |
|
Closing in favor of #3906 |
What?
It replaces (deprecates) the use of
--upload-only(fork6 cloud) by a new subcommand:k6 cloud upload.Why?
Checklist
make lint) and all checks pass.make tests) and all tests pass.Related PR(s)/Issue(s)