Introducing options.cloud #3348

olegbespalov · 2023-09-21T13:56:02Z

What?

This is an attempt to introduce the new option cloud, which should start the process of sunsetting the options.ext.loadimpact

Why?

The options.ext.loadimpact is the legacy option, and we should migrate from it to something more neutral like cloud

Checklist

I have performed a self-review of my code.
I have added tests for my changes.
I have run linter locally (make ci-like-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)

Closes #1155

codecov-commenter · 2023-10-09T15:53:45Z

Codecov Report

Attention: 12 lines in your changes are missing coverage. Please review.

Comparison is base (9dbf343) 73.55% compared to head (de0815c) 73.56%.
Report is 2 commits behind head on master.

❗ Current head de0815c differs from pull request most recent head bb6f6c5. Consider uploading reports for the commit bb6f6c5 to get more accurate results

Files	Patch %	Lines
cloudapi/config.go	80.00%	5 Missing and 3 partials ⚠️
cmd/cloud.go	75.00%	1 Missing and 1 partial ⚠️
cmd/login_cloud.go	0.00%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #3348      +/-   ##
==========================================
+ Coverage   73.55%   73.56%   +0.01%     
==========================================
  Files         277      277              
  Lines       20201    20228      +27     
==========================================
+ Hits        14858    14881      +23     
- Misses       4396     4399       +3     
- Partials      947      948       +1

Flag	Coverage Δ
macos	`73.48% <79.66%> (+0.01%)`	⬆️
ubuntu	`?`
windows	`73.42% <79.66%> (+0.03%)`	⬆️

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.

codebien

@olegbespalov did a first cycle, thanks for the great work 🙇

cloudapi/config.go

codebien · 2023-10-13T11:47:03Z

cloudapi/config.go

+	}
+
+	// Original comment
+	// TODO: Important! Separate configs and fix the whole 2 configs mess!


Can you explicit this comment a bit more, please? I'm not getting what is the suggestion here

The issue that I'm not getting this comment to, it's an original comment that was presented in the place around.

TBH for the case of refactoring that I did it wasn't so relevant, but on the other side I'm not comfortable removing it.

codebien · 2023-10-13T11:51:24Z

cloudapi/config.go

@@ -158,6 +162,9 @@ type Config struct {

 	// Deprecated: Remove this when migration from the cloud output v1 will be completed
 	MaxMetricSamplesPerPackage null.Int `json:"maxMetricSamplesPerPackage" envconfig:"K6_CLOUD_MAX_METRIC_SAMPLES_PER_PACKAGE"`
+
+	// WarningMessage is a string that will be shown to the user if the config has some invalid values.
+	WarningMessage string `json:"-"`


Suggested change

WarningMessage string `json:"-"`

WarningMessage []string `json:"-"`

Considering we are adding this mechanism, should we instead have some sort of form to support a list? Also, why not have them as a result of the Consolidation function?

In my opinion, we could start small, and if we see the need to extend it, but that particular place I don't mind if you think this serve some future needs I could convert to the array

codebien · 2023-10-13T11:55:19Z

cloudapi/config.go

+	if err := json.Unmarshal(source, &tmpConfig); err != nil {
+		return err
+	}
+	// Only take out the ProjectID, Name and Token from the options.cloud (or legacy loadimpact struct) map:


It seems to me reading this comment that we are taking out only part of the available fields. The question now I have is why? I guess it has a reason, can you clarify it, please? As it isn't clear to me just reading the code.

And if it has then probably it can be directly part of the comment of the function explaining what should be the expected return.

As a reference I evaluate this list still valid for the as configurable options. https://k6.io/docs/cloud/creating-and-running-a-test/cloud-scripting-extras/cloud-options/

This is again the comment from the past https://github.com/grafana/k6/pull/3348/files#diff-55ae88ea92051c07942fe7b2477d0b5ee5c7889eb78825e8dd8db7fbf0f96e69L302

cloudapi/config.go

cmd/archive_test.go

cloudapi/config_test.go

cmd/cloud.go

cmd/login_cloud.go

cloudapi/config.go

lib/options.go

cloudapi/config.go

codebien

Thanks for the effort 🙇

olegbespalov · 2023-10-19T11:18:06Z

@codebien I've extracted warnings (or referings) of the options.cloud to the #3407. The reason is that it could be so that we could be in a state where we can merge this PR, but not in the state where we should actively tell customers to use the options.cloud . Feel free to bring your opinion.

codebien

I've extracted warnings (or referings) of the options.cloud to the #3407

Yep, I like the idea. LGTM

mstoykov

LGTM! 👏

but I have not tried to break it extensively locally.

My main feedback is that I still am of the opinion that if options.cloud and options.ext.loadimpact exist - this should lead to an early error instead of one of them taking precedence.

There is IMO no reason someone will have both and at best it will be confusing to users which one they need to change.

olegbespalov · 2024-02-05T09:55:11Z

Okay, @codebien, here is an explicit request for a review of this (and #3407) PRs ☺️

Co-authored-by: Ivan <2103732+codebien@users.noreply.github.com>

olegbespalov · 2024-02-13T08:31:11Z

@codebien @mstoykov please re-approve. The PR was rebased because of the deletion of the cloud v1

codebien

Approving ~~with the assumption we have a green CI~~ we have 🎉

yorugac · 2024-02-19T15:22:09Z

Hello 👋 Just for understanding of this change: can a user specify both the loadimpact and cloud at the same time? E.g. put the name in loadimpact and distribution and project ID in cloud?

olegbespalov · 2024-02-19T15:25:30Z

hey @yorugac . Nope, mixing isn't acceptable. If the options.cloud exists, we'll use it as the source; otherwise, we fall back to the options.ext.loadimpact.

olegbespalov added this to the v0.48.0 milestone Sep 22, 2023

olegbespalov changed the title ~~WIP: Introducing options.cloud~~ Introducing options.cloud Sep 22, 2023

olegbespalov force-pushed the feat/refactor-cloud-options branch from 2145363 to 4752984 Compare October 9, 2023 15:52

olegbespalov self-assigned this Oct 10, 2023

olegbespalov force-pushed the feat/refactor-cloud-options branch 3 times, most recently from c968699 to 75f526e Compare October 12, 2023 12:00

olegbespalov marked this pull request as ready for review October 12, 2023 12:25

github-actions bot requested review from codebien and mstoykov October 12, 2023 12:25

olegbespalov added the documentation-needed A PR which will need a separate PR for documentation label Oct 13, 2023

codebien requested changes Oct 13, 2023

View reviewed changes

olegbespalov commented Oct 13, 2023

View reviewed changes

cloudapi/config.go Outdated Show resolved Hide resolved

olegbespalov force-pushed the feat/refactor-cloud-options branch from fac3508 to 7ed4a71 Compare October 17, 2023 06:50

olegbespalov requested a review from codebien October 17, 2023 06:52

codebien reviewed Oct 17, 2023

View reviewed changes

lib/options.go Show resolved Hide resolved

codebien reviewed Oct 18, 2023

View reviewed changes

cloudapi/config.go Outdated Show resolved Hide resolved

olegbespalov mentioned this pull request Oct 18, 2023

Add k6 new subcommand #3394

Merged

5 tasks

olegbespalov force-pushed the feat/refactor-cloud-options branch from 7ed4a71 to 7da005e Compare October 18, 2023 16:54

olegbespalov requested a review from codebien October 18, 2023 16:56

codebien previously approved these changes Oct 18, 2023

View reviewed changes

olegbespalov dismissed codebien’s stale review via 5da752f October 19, 2023 11:05

olegbespalov force-pushed the feat/refactor-cloud-options branch from 7da005e to 5da752f Compare October 19, 2023 11:05

olegbespalov removed the documentation-needed A PR which will need a separate PR for documentation label Oct 19, 2023

olegbespalov mentioned this pull request Oct 19, 2023

Warnings about options.cloud #3407

Merged

7 tasks

olegbespalov requested a review from codebien October 19, 2023 11:20

codebien previously approved these changes Oct 20, 2023

View reviewed changes

mstoykov previously approved these changes Nov 1, 2023

View reviewed changes

olegbespalov added ux refactor labels Nov 23, 2023

olegbespalov modified the milestones: v0.49.0, v0.50.0 Jan 10, 2024

olegbespalov force-pushed the feat/refactor-cloud-options branch from 5add9e5 to 50418f8 Compare January 17, 2024 12:44

olegbespalov requested a review from a team as a code owner January 17, 2024 12:44

codebien removed the request for review from a team January 18, 2024 11:07

olegbespalov force-pushed the feat/refactor-cloud-options branch from 50418f8 to a54097a Compare February 5, 2024 09:46

olegbespalov requested a review from codebien February 5, 2024 09:54

codebien previously approved these changes Feb 5, 2024

View reviewed changes

mstoykov previously approved these changes Feb 8, 2024

View reviewed changes

olegbespalov and others added 2 commits February 13, 2024 09:29

Introducing options.cloud

0b6be3e

Co-authored-by: Ivan <2103732+codebien@users.noreply.github.com>

Respect options.cloud during archiving

bb6f6c5

olegbespalov dismissed stale reviews from mstoykov and codebien via bb6f6c5 February 13, 2024 08:29

olegbespalov force-pushed the feat/refactor-cloud-options branch from a54097a to bb6f6c5 Compare February 13, 2024 08:29

olegbespalov requested review from mstoykov and codebien February 13, 2024 08:30

mstoykov approved these changes Feb 13, 2024

View reviewed changes

codebien approved these changes Feb 13, 2024

View reviewed changes

olegbespalov merged commit b5a6feb into master Feb 13, 2024
24 checks passed

olegbespalov deleted the feat/refactor-cloud-options branch February 13, 2024 13:30

andrewslotin mentioned this pull request Feb 15, 2024

Drop options.ext.loadimpact references #3602

Closed

4 tasks

olegbespalov mentioned this pull request Feb 19, 2024

Adds support options.cloud grafana/k6-operator#373

Merged

olegbespalov mentioned this pull request Mar 15, 2024

Introducing options.cloud grafana/k6-docs#1525

Merged

5 tasks

BrewTestBot mentioned this pull request Mar 25, 2024

k6 0.50.0 Homebrew/homebrew-core#167104

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introducing options.cloud #3348

Introducing options.cloud #3348

olegbespalov commented Sep 21, 2023 •

edited

Loading

codecov-commenter commented Oct 9, 2023 •

edited

Loading

codebien left a comment

codebien Oct 13, 2023

olegbespalov Oct 13, 2023

codebien Oct 13, 2023

olegbespalov Oct 13, 2023

codebien Oct 13, 2023

olegbespalov Oct 13, 2023

codebien left a comment

olegbespalov commented Oct 19, 2023

codebien left a comment

mstoykov left a comment

olegbespalov commented Feb 5, 2024

olegbespalov commented Feb 13, 2024

codebien left a comment •

edited

Loading

yorugac commented Feb 19, 2024

olegbespalov commented Feb 19, 2024

	WarningMessage string `json:"-"`
	WarningMessage []string `json:"-"`

Introducing options.cloud #3348

Introducing options.cloud #3348

Conversation

olegbespalov commented Sep 21, 2023 • edited Loading

What?

Why?

Checklist

Related PR(s)/Issue(s)

codecov-commenter commented Oct 9, 2023 • edited Loading

Codecov Report

codebien left a comment

Choose a reason for hiding this comment

codebien Oct 13, 2023

Choose a reason for hiding this comment

olegbespalov Oct 13, 2023

Choose a reason for hiding this comment

codebien Oct 13, 2023

Choose a reason for hiding this comment

olegbespalov Oct 13, 2023

Choose a reason for hiding this comment

codebien Oct 13, 2023

Choose a reason for hiding this comment

olegbespalov Oct 13, 2023

Choose a reason for hiding this comment

codebien left a comment

Choose a reason for hiding this comment

olegbespalov commented Oct 19, 2023

codebien left a comment

Choose a reason for hiding this comment

mstoykov left a comment

Choose a reason for hiding this comment

olegbespalov commented Feb 5, 2024

olegbespalov commented Feb 13, 2024

codebien left a comment • edited Loading

Choose a reason for hiding this comment

yorugac commented Feb 19, 2024

olegbespalov commented Feb 19, 2024

olegbespalov commented Sep 21, 2023 •

edited

Loading

codecov-commenter commented Oct 9, 2023 •

edited

Loading

codebien left a comment •

edited

Loading