Introducing options.cloud

Co-authored-by: Ivan <[email protected]>

cloudapi/config.go

@@ -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
@@ -215,7 +219,7 @@ func NewConfig() Config {
 }
 
 // Apply saves config non-zero config values from the passed config in the receiver.
-func (c Config) Apply(cfg Config) Config {
+func (c Config) Apply(cfg Config) Config { //nolint:funlen,gocognit,cyclop
 	if cfg.Token.Valid {
 		c.Token = cfg.Token
 	}
@@ -306,44 +310,40 @@ func (c Config) Apply(cfg Config) Config {
 	return c
 }
 
-// MergeFromExternal merges three fields from the JSON in a loadimpact key of
-// the provided external map. Used for options.ext.loadimpact settings.
-func MergeFromExternal(external map[string]json.RawMessage, conf *Config) error {
-	if val, ok := external["loadimpact"]; ok {
-		// TODO: Important! Separate configs and fix the whole 2 configs mess!
-		tmpConfig := Config{}
-		if err := json.Unmarshal(val, &tmpConfig); err != nil {
-			return err
-		}
-		// Only take out the ProjectID, Name and Token from the options.ext.loadimpact map:
-		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
-}
-
 // 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, external map[string]json.RawMessage,
-) (Config, error) {
+	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, err
+			return result, warn, err
 		}
 		result = result.Apply(jsonConf)
 	}
-	if err := MergeFromExternal(external, &result); err != nil {
-		return result, err
+
+	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{}
@@ -352,13 +352,93 @@ func GetConsolidatedConfig(
 		return v, ok
 	}); err != nil {
 		// TODO: get rid of envconfig and actually use the env parameter...
-		return result, err
+		return result, warn, err
 	}
 	result = result.Apply(envConfig)
 
 	if configArg != "" {
 		result.Name = null.StringFrom(configArg)
 	}
 
-	return result, nil
+	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:
+	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
+// 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
 }

cloudapi/config_test.go

@@ -63,17 +63,78 @@ func TestConfigApply(t *testing.T) {
 
 func TestGetConsolidatedConfig(t *testing.T) {
 	t.Parallel()
-	config, err := GetConsolidatedConfig(json.RawMessage(`{"token":"jsonraw"}`), nil, "", nil)
+	config, warn, err := GetConsolidatedConfig(json.RawMessage(`{"token":"jsonraw"}`), nil, "", nil, nil)
 	require.NoError(t, err)
 	require.Equal(t, config.Token.String, "jsonraw")
+	require.Empty(t, warn)
 
-	config, err = GetConsolidatedConfig(json.RawMessage(`{"token":"jsonraw"}`), nil, "",
+	config, warn, err = GetConsolidatedConfig(
+		json.RawMessage(`{"token":"jsonraw"}`),
+		nil,
+		"",
+		json.RawMessage(`{"token":"ext"}`),
+		nil,
+	)
+	require.NoError(t, err)
+	require.Equal(t, config.Token.String, "ext")
+	require.Empty(t, warn)
+
+	config, warn, err = GetConsolidatedConfig(
+		json.RawMessage(`{"token":"jsonraw"}`),
+		map[string]string{"K6_CLOUD_TOKEN": "envvalue"},
+		"",
+		json.RawMessage(`{"token":"ext"}`),
+		nil,
+	)
+	require.NoError(t, err)
+	require.Equal(t, config.Token.String, "envvalue")
+	require.Empty(t, warn)
+}
+
+func TestGetConsolidatedConfig_WithLegacyOnly(t *testing.T) {
+	t.Parallel()
+
+	config, warn, err := GetConsolidatedConfig(json.RawMessage(`{"token":"jsonraw"}`), nil, "", nil,
 		map[string]json.RawMessage{"loadimpact": json.RawMessage(`{"token":"ext"}`)})
 	require.NoError(t, err)
 	require.Equal(t, config.Token.String, "ext")
+	require.NotEmpty(t, warn)
 
-	config, err = GetConsolidatedConfig(json.RawMessage(`{"token":"jsonraw"}`), map[string]string{"K6_CLOUD_TOKEN": "envvalue"}, "",
+	config, warn, err = GetConsolidatedConfig(json.RawMessage(`{"token":"jsonraw"}`), map[string]string{"K6_CLOUD_TOKEN": "envvalue"}, "", nil,
 		map[string]json.RawMessage{"loadimpact": json.RawMessage(`{"token":"ext"}`)})
 	require.NoError(t, err)
 	require.Equal(t, config.Token.String, "envvalue")
+	require.NotEmpty(t, warn)
+}
+
+func TestGetConsolidatedConfig_LegacyHasLowerPriority(t *testing.T) {
+	t.Parallel()
+
+	config, warn, err := GetConsolidatedConfig(
+		json.RawMessage(`{"token":"jsonraw"}`),
+		nil,
+		"",
+		json.RawMessage(`{"token":"cloud"}`),
+		map[string]json.RawMessage{"loadimpact": json.RawMessage(`{"token":"ext"}`)},
+	)
+
+	require.NoError(t, err)
+	require.Equal(t, config.Token.String, "cloud")
+	require.Empty(t, warn)
+}
+
+func TestGetConsolidatedConfig_EnvHasHigherPriority(t *testing.T) {
+	t.Parallel()
+
+	config, warn, err := GetConsolidatedConfig(
+		json.RawMessage(`{"token":"jsonraw"}`),
+		map[string]string{"K6_CLOUD_TOKEN": "envvalue"},
+		"",
+		json.RawMessage(`{"token":"cloud"}`),
+		map[string]json.RawMessage{"loadimpact": json.RawMessage(`{"token":"ext"}`)},
+	)
+	require.NoError(t, err)
+
+	require.Equal(t, config.Token.String, "envvalue")
+	require.Empty(t, warn)
 }

cmd/cloud.go

@@ -1,7 +1,6 @@
 package cmd
 
 import (
-	"bytes"
 	"context"
 	"encoding/json"
 	"errors"
@@ -106,34 +105,20 @@ func (c *cmdCloud) run(cmd *cobra.Command, args []string) error {
 	modifyAndPrintBar(c.gs, progressBar, pb.WithConstProgress(0, "Building the archive..."))
 	arc := testRunState.Runner.MakeArchive()
 
-	// TODO: Fix this
-	// We reuse cloud.Config for parsing options.ext.loadimpact, 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 marshall 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 loadimpact's api we unmarshal in
-	// map[string]interface{} and copy what we need if it isn't set already
-	var tmpCloudConfig map[string]interface{}
-	if val, ok := arc.Options.External["loadimpact"]; ok {
-		dec := json.NewDecoder(bytes.NewReader(val))
-		dec.UseNumber() // otherwise float64 are used
-		if err = dec.Decode(&tmpCloudConfig); err != nil {
-			return err
-		}
+	tmpCloudConfig, err := cloudapi.GetTemporaryCloudConfig(arc.Options.Cloud, arc.Options.External)
+	if err != nil {
+		return err
 	}
 
 	// Cloud config
-	cloudConfig, err := cloudapi.GetConsolidatedConfig(
-		test.derivedConfig.Collectors["cloud"], c.gs.Env, "", arc.Options.External)
+	cloudConfig, _, err := cloudapi.GetConsolidatedConfig(
+		test.derivedConfig.Collectors["cloud"], c.gs.Env, "", arc.Options.Cloud, arc.Options.External)
 	if err != nil {
 		return err
 	}
 	if !cloudConfig.Token.Valid {
 		return errors.New("Not logged in, please use `k6 login cloud`.") //nolint:golint,revive,stylecheck
 	}
-	if tmpCloudConfig == nil {
-		tmpCloudConfig = make(map[string]interface{}, 3)
-	}
 
 	if cloudConfig.Token.Valid {
 		tmpCloudConfig["token"] = cloudConfig.Token
@@ -148,11 +133,15 @@ func (c *cmdCloud) run(cmd *cobra.Command, args []string) error {
 	if arc.Options.External == nil {
 		arc.Options.External = make(map[string]json.RawMessage)
 	}
-	arc.Options.External["loadimpact"], err = json.Marshal(tmpCloudConfig)
+
+	b, err := json.Marshal(tmpCloudConfig)
 	if err != nil {
 		return err
 	}
 
+	arc.Options.Cloud = b
+	arc.Options.External[cloudapi.LegacyCloudConfigKey] = b
+
 	name := cloudConfig.Name.String
 	if !cloudConfig.Name.Valid || cloudConfig.Name.String == "" {
 		name = filepath.Base(test.sourceRootPath)

cmd/login_cloud.go

@@ -23,10 +23,10 @@ func getCmdLoginCloud(gs *state.GlobalState) *cobra.Command {
 	exampleText := getExampleText(gs, `
   # Show the stored token.
   {{.}} login cloud -s
-  
+
   # Store a token.
   {{.}} login cloud -t YOUR_TOKEN
-  
+
   # Log in with an email/password.
   {{.}} login cloud`[1:])
 
@@ -55,11 +55,12 @@ This will set the default token used when just "k6 run -o cloud" is passed.`,
 
 			// We want to use this fully consolidated config for things like
 			// host addresses, so users can overwrite them with env vars.
-			consolidatedCurrentConfig, err := cloudapi.GetConsolidatedConfig(
-				currentJSONConfigRaw, gs.Env, "", nil)
+			consolidatedCurrentConfig, _, err := cloudapi.GetConsolidatedConfig(
+				currentJSONConfigRaw, gs.Env, "", nil, nil)
 			if err != nil {
 				return err
 			}
+
 			// But we don't want to save them back to the JSON file, we only
 			// want to save what already existed there and the login details.
 			newCloudConf := currentJSONConfig