-
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
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should revisit which of all of these still make sense for just uploading the archive.
aad9c76
to
2b0bc1f
Compare
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)