Introducing options.cloud #3348
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,6 +1,7 @@ | ||
| package cloudapi | ||
|
|
||
| import ( | ||
| "bytes" | ||
| "encoding/json" | ||
| "time" | ||
|
|
||
|
|
@@ -9,6 +10,9 @@ import ( | |
| "gopkg.in/guregu/null.v3" | ||
| ) | ||
|
|
||
| // LegacyCloudConfigKey is the key used in the JSON config for the cloud output. | ||
| const LegacyCloudConfigKey = "loadimpact" | ||
|
|
||
| // Config holds all the necessary data and options for sending metrics to the k6 Cloud. | ||
| // | ||
| //nolint:lll | ||
|
|
@@ -165,44 +169,40 @@ func (c Config) Apply(cfg Config) Config { | |
| return c | ||
| } | ||
|
|
||
| // GetConsolidatedConfig combines the default config values with the JSON config | ||
| // values and environment variables and returns the final result. | ||
| // it also returns a warning message that could be shown to the user. | ||
| // to bring some attention to the fact that the user. | ||
| func GetConsolidatedConfig( | ||
| jsonRawConf json.RawMessage, | ||
| env map[string]string, | ||
| configArg string, | ||
| cloudConfig json.RawMessage, | ||
| external map[string]json.RawMessage, | ||
| ) (Config, string, error) { | ||
| warn := "" | ||
|
|
||
| result := NewConfig() | ||
| if jsonRawConf != nil { | ||
| jsonConf := Config{} | ||
| if err := json.Unmarshal(jsonRawConf, &jsonConf); err != nil { | ||
| return result, warn, err | ||
| } | ||
| result = result.Apply(jsonConf) | ||
| } | ||
|
|
||
| if err := mergeFromCloudOptionAndExternal(cloudConfig, external, &result); err != nil { | ||
| return result, warn, err | ||
| } | ||
|
|
||
| // We want to show a warning if the user is using the only old way of defining the config. | ||
| // Note: Since the migration to the options.cloud is a long process, this warning is planned | ||
| // to be emitted for a long time (1-2 years), after some point, and depending on the state | ||
| // of migration we could re-evaluate this warning. | ||
| if cloudConfig == nil && external != nil { | ||
| if _, ok := external[LegacyCloudConfigKey]; ok { | ||
| warn = "The options.ext.loadimpact option is deprecated, please use options.cloud instead" | ||
| } | ||
| } | ||
|
|
||
| envConfig := Config{} | ||
|
|
@@ -211,13 +211,93 @@ func GetConsolidatedConfig( | |
| return v, ok | ||
| }); err != nil { | ||
| // TODO: get rid of envconfig and actually use the env parameter... | ||
| return result, warn, err | ||
| } | ||
| result = result.Apply(envConfig) | ||
|
|
||
| if configArg != "" { | ||
| result.Name = null.StringFrom(configArg) | ||
| } | ||
|
|
||
| return result, warn, nil | ||
| } | ||
|
|
||
| // mergeFromCloudOptionAndExternal merges three fields from the JSON in a cloud key of | ||
| // the provided external map. Used for options.cloud settings. | ||
| func mergeFromCloudOptionAndExternal( | ||
| cloudConfig json.RawMessage, | ||
| external map[string]json.RawMessage, | ||
| conf *Config, | ||
| ) error { | ||
| source := pickSource(cloudConfig, external) | ||
| if source == nil { | ||
| return nil | ||
| } | ||
|
|
||
| // Original comment | ||
| // TODO: Important! Separate configs and fix the whole 2 configs mess! | ||
| tmpConfig := Config{} | ||
| 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: | ||
|
There was a problem hiding this comment. 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/ There was a problem hiding this comment. This is again the comment from the past https://github.com/grafana/k6/pull/3348/files#diff-55ae88ea92051c07942fe7b2477d0b5ee5c7889eb78825e8dd8db7fbf0f96e69L302 |
||
| if tmpConfig.ProjectID.Valid { | ||
| conf.ProjectID = tmpConfig.ProjectID | ||
| } | ||
| if tmpConfig.Name.Valid { | ||
| conf.Name = tmpConfig.Name | ||
| } | ||
| if tmpConfig.Token.Valid { | ||
| conf.Token = tmpConfig.Token | ||
| } | ||
|
|
||
| return nil | ||
| } | ||
|
|
||
| // GetTemporaryCloudConfig returns a temporary cloud config. | ||
| // Original comment | ||
| // TODO: Fix this | ||
|
There was a problem hiding this comment. If we keep this 2-way forever, how could we really fix it in the future without the temp map? If you see a way then I would like to include the suggestion in the comment. There was a problem hiding this comment. I don't see a way of improving it, and to be honest, I prefer to keep the scope smaller to have a chance of delivering it. |
||
| // We reuse cloud.Config for parsing options.cloud (or legacy loadimpact struct), but this probably shouldn't be | ||
| // done, as the idea of options.ext is that they are extensible without touching k6. But in | ||
| // order for this to happen, we shouldn't actually marshal cloud.Config on top of it, because | ||
| // it will be missing some fields that aren't actually mentioned in the struct. | ||
| // So in order for use to copy the fields that we need for k6 cloud's api we unmarshal in | ||
| // map[string]interface{} and copy what we need if it isn't set already | ||
| func GetTemporaryCloudConfig( | ||
| cloudConfig json.RawMessage, | ||
| external map[string]json.RawMessage, | ||
| ) (map[string]interface{}, error) { | ||
| tmpCloudConfig := make(map[string]interface{}, 3) | ||
|
|
||
| source := pickSource(cloudConfig, external) | ||
| if source == nil { | ||
| return tmpCloudConfig, nil | ||
| } | ||
|
|
||
| dec := json.NewDecoder(bytes.NewReader(source)) | ||
| dec.UseNumber() // otherwise float64 are used | ||
| if err := dec.Decode(&tmpCloudConfig); err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| return tmpCloudConfig, nil | ||
| } | ||
|
|
||
| // pickSource returns the config source to use. | ||
| func pickSource( | ||
| cloudConfig json.RawMessage, | ||
| external map[string]json.RawMessage, | ||
| ) json.RawMessage { | ||
| // priority is the new way of defining the config | ||
| // via options.cloud | ||
| if cloudConfig != nil { | ||
| return cloudConfig | ||
| } | ||
|
|
||
| // fallback to the old way of defining the config | ||
| if val, ok := external[LegacyCloudConfigKey]; ok { | ||
| return val | ||
| } | ||
|
|
||
| return nil | ||
| } | ||
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
Can you explicit this comment a bit more, please? I'm not getting what is the suggestion here
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.
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.