Fix binding configuration file settings to command flags

When we were binding viper values to the cobra flags, it was occuring before the config file was loaded, so if you configured a flag in the config file, it wasn't persisting the correct value to the variable bound to the flag. When the flag was not bound to the global porter Config.Data, such as porter build --no-lint, the configuration file values were not applied. This is the regression describe in getporter#2735 which was caused by apply cobra values to viper too soon (before the config file was loaded). I have split our viper configuration into a three step process: 1. Configure the viper instance, for example binding custom open telemetry environment variables. 2. Bind viper to the cobra flags, AFTER the config file has been loaded. 3. Apply the consolidated configuration in viper (which now contains both cobra flags, env vars and the config file) back to the global config in Config.Data This ensures that viper has all relevant configuration loaded before binding its value back to the original command flags (i.e. our variables in code). Fixes getporter#2735 Signed-off-by: Carolyn Van Slyck <me@carolynvanslyck.com>
bdegeeter · May 11, 2023 · 5d6eba7 · 5d6eba7
1 parent 3159036
commit 5d6eba7
Show file tree

Hide file tree

Showing 6 changed files with 390 additions and 29 deletions.
diff --git a/cmd/porter/main_test.go b/cmd/porter/main_test.go
@@ -6,6 +6,8 @@ import (
 	"strings"
 	"testing"
 
+	"get.porter.sh/porter/pkg"
+	"get.porter.sh/porter/pkg/config"
 	"get.porter.sh/porter/pkg/experimental"
 	"get.porter.sh/porter/pkg/porter"
 	"github.com/stretchr/testify/assert"
@@ -80,12 +82,16 @@ func TestHelp(t *testing.T) {
 	})
 }
 
+// Validate that porter is correctly binding experimental which is a flag on some commands AND is persisted on config.Data
+// This is a regression test to ensure that we are applying our configuration from viper and cobra in the proper order
+// such that flags defined on config.Data are persisted.
+// I'm testing both experimental and verbosity because honestly, I've seen both break enough that I'd rather have excessive test than see it break again.
 func TestExperimentalFlags(t *testing.T) {
 	// do not run in parallel
 	expEnvVar := "PORTER_EXPERIMENTAL"
 	os.Unsetenv(expEnvVar)
 
-	t.Run("flag unset, env unset", func(t *testing.T) {
+	t.Run("default", func(t *testing.T) {
 		p := porter.NewTestPorter(t)
 		defer p.Close()
 
@@ -106,7 +112,7 @@ func TestExperimentalFlags(t *testing.T) {
 		assert.True(t, p.Config.IsFeatureEnabled(experimental.FlagNoopFeature))
 	})
 
-	t.Run("flag unset, env set", func(t *testing.T) {
+	t.Run("env set", func(t *testing.T) {
 		os.Setenv(expEnvVar, experimental.NoopFeature)
 		defer os.Unsetenv(expEnvVar)
 
@@ -119,4 +125,279 @@ func TestExperimentalFlags(t *testing.T) {
 
 		assert.True(t, p.Config.IsFeatureEnabled(experimental.FlagNoopFeature))
 	})
+
+	t.Run("cfg set", func(t *testing.T) {
+		p := porter.NewTestPorter(t)
+		defer p.Close()
+
+		cfg := []byte(`experimental: [no-op]`)
+		require.NoError(t, p.FileSystem.WriteFile("/home/myuser/.porter/config.yaml", cfg, pkg.FileModeWritable))
+		cmd := buildRootCommandFrom(p.Porter)
+		cmd.SetArgs([]string{"install"})
+		cmd.Execute()
+
+		assert.True(t, p.Config.IsFeatureEnabled(experimental.FlagNoopFeature))
+	})
+
+	t.Run("flag set, cfg set", func(t *testing.T) {
+		p := porter.NewTestPorter(t)
+		defer p.Close()
+
+		cfg := []byte(`experimental: []`)
+		require.NoError(t, p.FileSystem.WriteFile("/home/myuser/.porter/config.yaml", cfg, pkg.FileModeWritable))
+		cmd := buildRootCommandFrom(p.Porter)
+		cmd.SetArgs([]string{"install", "--experimental", "no-op"})
+		cmd.Execute()
+
+		assert.True(t, p.Config.IsFeatureEnabled(experimental.FlagNoopFeature))
+	})
+
+	t.Run("flag set, env set", func(t *testing.T) {
+		os.Setenv(expEnvVar, "")
+		defer os.Unsetenv(expEnvVar)
+
+		p := porter.NewTestPorter(t)
+		defer p.Close()
+
+		cmd := buildRootCommandFrom(p.Porter)
+		cmd.SetArgs([]string{"install", "--experimental", "no-op"})
+		cmd.Execute()
+
+		assert.True(t, p.Config.IsFeatureEnabled(experimental.FlagNoopFeature))
+	})
+
+	t.Run("env set, cfg set", func(t *testing.T) {
+		os.Setenv(expEnvVar, "")
+		defer os.Unsetenv(expEnvVar)
+
+		p := porter.NewTestPorter(t)
+		defer p.Close()
+
+		cfg := []byte(`experimental: [no-op]`)
+		require.NoError(t, p.FileSystem.WriteFile("/home/myuser/.porter/config.yaml", cfg, pkg.FileModeWritable))
+		cmd := buildRootCommandFrom(p.Porter)
+		cmd.SetArgs([]string{"install"})
+		cmd.Execute()
+
+		assert.False(t, p.Config.IsFeatureEnabled(experimental.FlagNoopFeature))
+	})
+}
+
+// Validate that porter is correctly binding verbosity which is a flag on all commands AND is persisted on config.Data
+// This is a regression test to ensure that we are applying our configuration from viper and cobra in the proper order
+// such that flags defined on config.Data are persisted.
+// I'm testing both experimental and verbosity because honestly, I've seen both break enough that I'd rather have excessive test than see it break again.
+func TestVerbosity(t *testing.T) {
+	// do not run in parallel
+	envVar := "PORTER_VERBOSITY"
+	os.Unsetenv(envVar)
+
+	t.Run("default", func(t *testing.T) {
+		p := porter.NewTestPorter(t)
+		defer p.Close()
+
+		cmd := buildRootCommandFrom(p.Porter)
+		cmd.SetArgs([]string{"install"})
+		cmd.Execute()
+		assert.Equal(t, config.LogLevelInfo, p.Config.GetVerbosity())
+	})
+
+	t.Run("flag set", func(t *testing.T) {
+		p := porter.NewTestPorter(t)
+		defer p.Close()
+
+		cmd := buildRootCommandFrom(p.Porter)
+		cmd.SetArgs([]string{"install", "--verbosity=debug"})
+		cmd.Execute()
+
+		assert.Equal(t, config.LogLevelDebug, p.Config.GetVerbosity())
+	})
+
+	t.Run("env set", func(t *testing.T) {
+		os.Setenv(envVar, "error")
+		defer os.Unsetenv(envVar)
+
+		p := porter.NewTestPorter(t)
+		defer p.Close()
+
+		cmd := buildRootCommandFrom(p.Porter)
+		cmd.SetArgs([]string{"install"})
+		cmd.Execute()
+
+		assert.Equal(t, config.LogLevelError, p.Config.GetVerbosity())
+	})
+
+	t.Run("cfg set", func(t *testing.T) {
+		p := porter.NewTestPorter(t)
+		defer p.Close()
+
+		cfg := []byte(`verbosity: warning`)
+		require.NoError(t, p.FileSystem.WriteFile("/home/myuser/.porter/config.yaml", cfg, pkg.FileModeWritable))
+		cmd := buildRootCommandFrom(p.Porter)
+		cmd.SetArgs([]string{"install"})
+		cmd.Execute()
+
+		assert.Equal(t, config.LogLevelWarn, p.Config.GetVerbosity())
+	})
+
+	t.Run("flag set, cfg set", func(t *testing.T) {
+		p := porter.NewTestPorter(t)
+		defer p.Close()
+
+		cfg := []byte(`verbosity: debug`)
+		require.NoError(t, p.FileSystem.WriteFile("/home/myuser/.porter/config.yaml", cfg, pkg.FileModeWritable))
+		cmd := buildRootCommandFrom(p.Porter)
+		cmd.SetArgs([]string{"install", "--verbosity", "warn"})
+		cmd.Execute()
+
+		assert.Equal(t, config.LogLevelWarn, p.Config.GetVerbosity())
+	})
+
+	t.Run("flag set, env set", func(t *testing.T) {
+		os.Setenv(envVar, "warn")
+		defer os.Unsetenv(envVar)
+
+		p := porter.NewTestPorter(t)
+		defer p.Close()
+
+		cmd := buildRootCommandFrom(p.Porter)
+		cmd.SetArgs([]string{"install", "--verbosity=debug"})
+		cmd.Execute()
+
+		assert.Equal(t, config.LogLevelDebug, p.Config.GetVerbosity())
+	})
+
+	t.Run("env set, cfg set", func(t *testing.T) {
+		os.Setenv(envVar, "warn")
+		defer os.Unsetenv(envVar)
+
+		p := porter.NewTestPorter(t)
+		defer p.Close()
+
+		cfg := []byte(`verbosity: debug`)
+		require.NoError(t, p.FileSystem.WriteFile("/home/myuser/.porter/config.yaml", cfg, pkg.FileModeWritable))
+		cmd := buildRootCommandFrom(p.Porter)
+		cmd.SetArgs([]string{"install"})
+		cmd.Execute()
+
+		assert.Equal(t, config.LogLevelWarn, p.Config.GetVerbosity())
+	})
+}
+
+// Validate that porter is correctly binding porter explain --output which is a flag that is NOT bound to config.Data
+// This is a regression test to ensure that we are applying our configuration from viper and cobra in the proper order
+// such that flags defined on a separate data structure from config.Data are persisted.
+func TestExplainOutput(t *testing.T) {
+	// do not run in parallel
+	envVar := "PORTER_OUTPUT"
+	os.Unsetenv(envVar)
+
+	const ref = "ghcr.io/getporter/examples/porter-hello:v0.2.0"
+
+	assertPlainOutput := func(t *testing.T, output string) {
+		t.Helper()
+		assert.Contains(t, "Name: examples/porter-hello", output, "explain should have output plain text")
+	}
+
+	assertJsonOutput := func(t *testing.T, output string) {
+		t.Helper()
+		assert.Contains(t, `"name": "examples/porter-hello",`, output, "explain should have output JSON")
+	}
+
+	assertYamlOutput := func(t *testing.T, output string) {
+		t.Helper()
+		assert.Contains(t, `- name: name`, output, "explain should have output YAML")
+	}
+
+	t.Run("default", func(t *testing.T) {
+		p := porter.NewTestPorter(t)
+		defer p.Close()
+
+		cmd := buildRootCommandFrom(p.Porter)
+		cmd.SetArgs([]string{"explain", ref})
+		require.NoError(t, cmd.Execute(), "explain failed")
+
+		assertPlainOutput(t, p.TestConfig.TestContext.GetOutput())
+	})
+
+	t.Run("flag set", func(t *testing.T) {
+		p := porter.NewTestPorter(t)
+		defer p.Close()
+
+		cmd := buildRootCommandFrom(p.Porter)
+		cmd.SetArgs([]string{"explain", ref, "--output=json"})
+		require.NoError(t, cmd.Execute(), "explain failed")
+
+		assertJsonOutput(t, p.TestConfig.TestContext.GetOutput())
+	})
+
+	t.Run("env set", func(t *testing.T) {
+		os.Setenv(envVar, "json")
+		defer os.Unsetenv(envVar)
+
+		p := porter.NewTestPorter(t)
+		defer p.Close()
+
+		cmd := buildRootCommandFrom(p.Porter)
+		cmd.SetArgs([]string{"explain", ref})
+		require.NoError(t, cmd.Execute(), "explain failed")
+
+		assertJsonOutput(t, p.TestConfig.TestContext.GetOutput())
+	})
+
+	t.Run("cfg set", func(t *testing.T) {
+		p := porter.NewTestPorter(t)
+		defer p.Close()
+
+		cfg := []byte(`output: json`)
+		require.NoError(t, p.FileSystem.WriteFile("/home/myuser/.porter/config.yaml", cfg, pkg.FileModeWritable))
+		cmd := buildRootCommandFrom(p.Porter)
+		cmd.SetArgs([]string{"explain", ref})
+		require.NoError(t, cmd.Execute(), "explain failed")
+
+		assertJsonOutput(t, p.TestConfig.TestContext.GetOutput())
+	})
+
+	t.Run("flag set, cfg set", func(t *testing.T) {
+		p := porter.NewTestPorter(t)
+		defer p.Close()
+
+		cfg := []byte(`output: json`)
+		require.NoError(t, p.FileSystem.WriteFile("/home/myuser/.porter/config.yaml", cfg, pkg.FileModeWritable))
+		cmd := buildRootCommandFrom(p.Porter)
+		cmd.SetArgs([]string{"explain", ref, "--output=yaml"})
+		require.NoError(t, cmd.Execute(), "explain failed")
+
+		assertYamlOutput(t, p.TestConfig.TestContext.GetOutput())
+	})
+
+	t.Run("flag set, env set", func(t *testing.T) {
+		os.Setenv(envVar, "json")
+		defer os.Unsetenv(envVar)
+
+		p := porter.NewTestPorter(t)
+		defer p.Close()
+
+		cmd := buildRootCommandFrom(p.Porter)
+		cmd.SetArgs([]string{"explain", ref, "--output=yaml"})
+		require.NoError(t, cmd.Execute(), "explain failed")
+
+		assertYamlOutput(t, p.TestConfig.TestContext.GetOutput())
+	})
+
+	t.Run("env set, cfg set", func(t *testing.T) {
+		os.Setenv(envVar, "yaml")
+		defer os.Unsetenv(envVar)
+
+		p := porter.NewTestPorter(t)
+		defer p.Close()
+
+		cfg := []byte(`output: json`)
+		require.NoError(t, p.FileSystem.WriteFile("/home/myuser/.porter/config.yaml", cfg, pkg.FileModeWritable))
+		cmd := buildRootCommandFrom(p.Porter)
+		cmd.SetArgs([]string{"explain", ref})
+		require.NoError(t, cmd.Execute(), "explain failed")
+
+		assertYamlOutput(t, p.TestConfig.TestContext.GetOutput())
+	})
 }
diff --git a/pkg/cli/config.go b/pkg/cli/config.go
@@ -16,11 +16,7 @@ import (
 // * Config file
 // * Flag default (lowest)
 func LoadHierarchicalConfig(cmd *cobra.Command) config.DataStoreLoaderFunc {
-	return config.LoadFromViper(func(v *viper.Viper) {
-		v.AutomaticEnv()
-		v.SetEnvPrefix("PORTER")
-		v.SetEnvKeyReplacer(strings.NewReplacer("-", "_"))
-
+	bindCobraFlagsToViper := func(v *viper.Viper) {
 		// Apply the configuration file value to the flag when the flag is not set
 		flags := cmd.Flags()
 		flags.VisitAll(func(f *pflag.Flag) {
@@ -42,7 +38,8 @@ func LoadHierarchicalConfig(cmd *cobra.Command) config.DataStoreLoaderFunc {
 				flags.Set(f.Name, val)
 			}
 		})
-	})
+	}
+	return config.LoadFromViper(config.BindViperToEnvironmentVariables, bindCobraFlagsToViper)
 }
 
 func getFlagValue(v *viper.Viper, key string) string {

diff --git a/pkg/config/config_test.go b/pkg/config/config_test.go
@@ -60,7 +60,9 @@ func TestConfigExperimentalFlags(t *testing.T) {
 	// Do not run in parallel since we are using os.Setenv
 
 	t.Run("default off", func(t *testing.T) {
-		c := NewTestConfig(t)
+		c := New()
+		defer c.Close()
+
 		assert.False(t, c.IsFeatureEnabled(experimental.FlagNoopFeature))
 	})
 
@@ -69,13 +71,18 @@ func TestConfigExperimentalFlags(t *testing.T) {
 		defer os.Unsetenv("PORTER_EXPERIMENTAL")
 
 		c := New()
+		defer c.Close()
+		c.DataLoader = LoadFromEnvironment()
+
 		_, err := c.Load(context.Background(), nil)
 		require.NoError(t, err, "Load failed")
 		assert.True(t, c.IsFeatureEnabled(experimental.FlagNoopFeature))
 	})
 
 	t.Run("programmatically enabled", func(t *testing.T) {
-		c := NewTestConfig(t)
+		c := New()
+		defer c.Close()
+
 		c.Data.ExperimentalFlags = nil
 		c.SetExperimentalFlags(experimental.FlagNoopFeature)
 		assert.True(t, c.IsFeatureEnabled(experimental.FlagNoopFeature))