From fe087c61263959efbb82fd96c02cb2f70f3c5005 Mon Sep 17 00:00:00 2001 From: Matt Rickard Date: Mon, 22 Aug 2016 09:50:13 -0700 Subject: [PATCH 1/4] Use config when flags are not set for logging Use viper in addition to flags for logging options. Since these flags are used in glog, we can't directly use viper. Instead, we use viper's built in precedence logic (https://github.com/spf13/viper#why-viper) to set the flags directly. --- cmd/minikube/cmd/root.go | 29 ++++++++++++++++++++----- cmd/minikube/cmd/root_test.go | 41 +++++++++++++++++++++++++++++++++++ 2 files changed, 65 insertions(+), 5 deletions(-) diff --git a/cmd/minikube/cmd/root.go b/cmd/minikube/cmd/root.go index 8e5dcb4cac77..025c03080691 100644 --- a/cmd/minikube/cmd/root.go +++ b/cmd/minikube/cmd/root.go @@ -40,8 +40,8 @@ var dirs = [...]string{ constants.MakeMiniPath("cache", "localkube"), constants.MakeMiniPath("config")} -var ( - showLibmachineLogs bool +const ( + showLibmachineLogs = "show-libmachine-logs" ) // RootCmd represents the base command when called without any subcommands @@ -50,14 +50,15 @@ var RootCmd = &cobra.Command{ Short: "Minikube is a tool for managing local Kubernetes clusters.", Long: `Minikube is a CLI tool that provisions and manages single-node Kubernetes clusters optimized for development workflows.`, PersistentPreRun: func(cmd *cobra.Command, args []string) { + setFlagsUsingViper() for _, path := range dirs { if err := os.MkdirAll(path, 0777); err != nil { glog.Exitf("Error creating minikube directory: %s", err) } } - log.SetDebug(showLibmachineLogs) - if !showLibmachineLogs { + log.SetDebug(viper.Get(showLibmachineLogs)) + if !viper.GetBool(showLibmachineLogs) { log.SetOutWriter(ioutil.Discard) log.SetErrWriter(ioutil.Discard) } @@ -73,9 +74,27 @@ func Execute() { } } +// Handle config values for flags used in external packages (e.g. glog) +// by setting them directly, using values from viper when not passed in as args +func setFlagsUsingViper() { + setFlagValues := func(a *pflag.Flag) { + viper.SetDefault(a.Name, a.DefValue) + // If the flag is set, override viper value + if a.Changed { + viper.Set(a.Name, a.Value.String()) + } + // Viper will give precedence first to calls to the Set command, + // then to values from the config.yml + a.Value.Set(viper.GetString(a.Name)) + } + + pflag.VisitAll(setFlagValues) +} + func init() { - RootCmd.PersistentFlags().BoolVarP(&showLibmachineLogs, "show-libmachine-logs", "", false, "Whether or not to show logs from libmachine.") + RootCmd.PersistentFlags().Bool(showLibmachineLogs, false, "Whether or not to show logs from libmachine.") pflag.CommandLine.AddGoFlagSet(goflag.CommandLine) + viper.BindPFlags(RootCmd.PersistentFlags()) cobra.OnInitialize(initConfig) } diff --git a/cmd/minikube/cmd/root_test.go b/cmd/minikube/cmd/root_test.go index cc0d44b6e4df..ab254632d6fb 100644 --- a/cmd/minikube/cmd/root_test.go +++ b/cmd/minikube/cmd/root_test.go @@ -21,6 +21,7 @@ import ( "testing" "github.com/spf13/cobra" + "github.com/spf13/pflag" "github.com/spf13/viper" "k8s.io/minikube/pkg/minikube/constants" "k8s.io/minikube/pkg/minikube/tests" @@ -59,3 +60,43 @@ func TestEnvVariable(t *testing.T) { t.Fatalf("Viper did not respect environment variable") } } + +func cleanup() { + pflag.Set("v", "0") + pflag.Lookup("v").Changed = false +} + +func TestFlagShouldOverrideConfig(t *testing.T) { + defer cleanup() + viper.Set("v", "1337") + pflag.Set("v", "100") + setFlagsUsingViper() + if viper.GetInt("v") != 100 { + viper.Debug() + t.Fatal("Value from viper config overrode explicit flag value") + } +} + +func TestConfigShouldOverrideDefault(t *testing.T) { + defer cleanup() + viper.Set("v", "1337") + setFlagsUsingViper() + if viper.GetInt("v") != 1337 { + viper.Debug() + t.Fatalf("Value from viper config did not override default flag value") + } +} + +func TestFallbackToDefaultFlag(t *testing.T) { + setFlagsUsingViper() + + if viper.GetInt("stderrthreshold") != 2 { + t.Logf("stderrthreshold %s", viper.GetInt("stderrthreshold")) + t.Fatalf("The default flag value was overwritten") + } + + if viper.GetString("log-flush-frequency") != "5s" { + t.Logf("log flush frequency: %s", viper.GetString("log-flush-frequency")) + t.Fatalf("The default flag value was overwritten") + } +} From d330686a2788033e3b232dfe90421f5ccccd645a Mon Sep 17 00:00:00 2001 From: Matt Rickard Date: Fri, 12 Aug 2016 14:33:58 -0700 Subject: [PATCH 2/4] Refactoring VersionCmd to run RootPersistentPreRun While keeping the behavior of not checking for new versions while running `minikube version` --- cmd/minikube/cmd/version.go | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/cmd/minikube/cmd/version.go b/cmd/minikube/cmd/version.go index cd171534fa0e..55db30b7a4c6 100644 --- a/cmd/minikube/cmd/version.go +++ b/cmd/minikube/cmd/version.go @@ -20,7 +20,9 @@ import ( "fmt" "github.com/spf13/cobra" + "github.com/spf13/viper" + "k8s.io/minikube/pkg/minikube/config" "k8s.io/minikube/pkg/version" ) @@ -29,8 +31,10 @@ var versionCmd = &cobra.Command{ Short: "Print the version of minikube.", Long: `Print the version of minikube.`, PersistentPreRun: func(cmd *cobra.Command, args []string) { - // Empty func to override parent pre-run - don't want to perform - // check for new version. + // Explicitly disable update checking for the version command + viper.Set(config.WantUpdateNotification, "false") + + RootCmd.PersistentPreRun(cmd, args) }, Run: func(command *cobra.Command, args []string) { From 6fc94598abbce6ec160d4d0cf4e2fa71241c4ea7 Mon Sep 17 00:00:00 2001 From: Matt Rickard Date: Tue, 16 Aug 2016 17:15:05 -0700 Subject: [PATCH 3/4] Refactor viper flag tests Make the tests for the config table driven Add a whitelist for the flags that viper can configure. Date: Tue Aug 18 16:41:05 2016 -0700 --- cmd/minikube/cmd/root.go | 25 +++--- cmd/minikube/cmd/root_test.go | 145 +++++++++++++++++++++++++--------- 2 files changed, 124 insertions(+), 46 deletions(-) diff --git a/cmd/minikube/cmd/root.go b/cmd/minikube/cmd/root.go index 025c03080691..c7009d1d5ff8 100644 --- a/cmd/minikube/cmd/root.go +++ b/cmd/minikube/cmd/root.go @@ -44,13 +44,18 @@ const ( showLibmachineLogs = "show-libmachine-logs" ) +var viperWhiteList = []string{ + "v", + "alsologtostderr", + "log_dir", +} + // RootCmd represents the base command when called without any subcommands var RootCmd = &cobra.Command{ Use: "minikube", Short: "Minikube is a tool for managing local Kubernetes clusters.", Long: `Minikube is a CLI tool that provisions and manages single-node Kubernetes clusters optimized for development workflows.`, PersistentPreRun: func(cmd *cobra.Command, args []string) { - setFlagsUsingViper() for _, path := range dirs { if err := os.MkdirAll(path, 0777); err != nil { glog.Exitf("Error creating minikube directory: %s", err) @@ -77,7 +82,8 @@ func Execute() { // Handle config values for flags used in external packages (e.g. glog) // by setting them directly, using values from viper when not passed in as args func setFlagsUsingViper() { - setFlagValues := func(a *pflag.Flag) { + for _, config := range viperWhiteList { + var a = pflag.Lookup(config) viper.SetDefault(a.Name, a.DefValue) // If the flag is set, override viper value if a.Changed { @@ -87,8 +93,6 @@ func setFlagsUsingViper() { // then to values from the config.yml a.Value.Set(viper.GetString(a.Name)) } - - pflag.VisitAll(setFlagValues) } func init() { @@ -101,17 +105,20 @@ func init() { // initConfig reads in config file and ENV variables if set. func initConfig() { configPath := constants.MakeMiniPath("config") - - // Bind all viper values to env variables - viper.SetEnvPrefix(constants.MinikubeEnvPrefix) - viper.AutomaticEnv() - viper.SetConfigName("config") viper.AddConfigPath(configPath) err := viper.ReadInConfig() if err != nil { glog.Warningf("Error reading config file at %s: %s", configPath, err) } + setupViper() +} + +func setupViper() { + viper.SetEnvPrefix(constants.MinikubeEnvPrefix) + viper.AutomaticEnv() + viper.SetDefault(config.WantUpdateNotification, true) viper.SetDefault(config.ReminderWaitPeriodInHours, 24) + setFlagsUsingViper() } diff --git a/cmd/minikube/cmd/root_test.go b/cmd/minikube/cmd/root_test.go index ab254632d6fb..80df35404775 100644 --- a/cmd/minikube/cmd/root_test.go +++ b/cmd/minikube/cmd/root_test.go @@ -17,7 +17,9 @@ limitations under the License. package cmd import ( + "bytes" "os" + "strings" "testing" "github.com/spf13/cobra" @@ -27,6 +29,76 @@ import ( "k8s.io/minikube/pkg/minikube/tests" ) +var yamlExampleConfig = []byte(`v: 999 +alsologtostderr: true +log_dir: "/etc/hosts" +log-flush-frequency: "3s" +`) + +const configName = ".test_minikube_config.yml" + +type configTest struct { + Name string + EnvValue string + ConfigValue string + FlagValue string + ExpectedValue string +} + +var configTests = []configTest{ + { + Name: "v", + ExpectedValue: "0", + }, + { + Name: "v", + ConfigValue: "999", + ExpectedValue: "999", + }, + { + Name: "v", + FlagValue: "0", + ExpectedValue: "0", + }, + { + Name: "v", + EnvValue: "123", + ExpectedValue: "123", + }, + { + Name: "v", + FlagValue: "3", + ExpectedValue: "3", + }, + // Flag should override config and env + { + Name: "v", + FlagValue: "3", + ConfigValue: "222", + EnvValue: "888", + ExpectedValue: "3", + }, + // Env should override config + { + Name: "v", + EnvValue: "2", + ConfigValue: "999", + ExpectedValue: "2", + }, + // Config should not override flags not on whitelist + { + Name: "log-flush-frequency", + ConfigValue: "6s", + ExpectedValue: "5s", + }, + // Env should not override flags not on whitelist + { + Name: "log_backtrace_at", + EnvValue: ":2", + ExpectedValue: ":0", + }, +} + func runCommand(f func(*cobra.Command, []string)) { cmd := cobra.Command{} var args []string @@ -48,55 +120,54 @@ func TestPreRunDirectories(t *testing.T) { } } -func getEnvVarName(name string) string { - return constants.MinikubeEnvPrefix + name +func initTestConfig(config string) { + viper.SetConfigType("yml") + r := bytes.NewReader([]byte(config)) + viper.ReadConfig(r) } -func TestEnvVariable(t *testing.T) { - defer os.Unsetenv("WANTUPDATENOTIFICATION") - initConfig() - os.Setenv(getEnvVarName("WANTUPDATENOTIFICATION"), "true") - if !viper.GetBool("WantUpdateNotification") { - t.Fatalf("Viper did not respect environment variable") +func TestViperConfig(t *testing.T) { + defer viper.Reset() + initTestConfig("v: 999") + if viper.GetString("v") != "999" { + t.Fatalf("Viper did not read test config file") } } -func cleanup() { - pflag.Set("v", "0") - pflag.Lookup("v").Changed = false +func getEnvVarName(name string) string { + return constants.MinikubeEnvPrefix + "_" + strings.ToUpper(name) } -func TestFlagShouldOverrideConfig(t *testing.T) { - defer cleanup() - viper.Set("v", "1337") - pflag.Set("v", "100") - setFlagsUsingViper() - if viper.GetInt("v") != 100 { - viper.Debug() - t.Fatal("Value from viper config overrode explicit flag value") +func setValues(tt configTest) { + if tt.FlagValue != "" { + pflag.Set(tt.Name, tt.FlagValue) } -} - -func TestConfigShouldOverrideDefault(t *testing.T) { - defer cleanup() - viper.Set("v", "1337") - setFlagsUsingViper() - if viper.GetInt("v") != 1337 { - viper.Debug() - t.Fatalf("Value from viper config did not override default flag value") + if tt.EnvValue != "" { + os.Setenv(getEnvVarName(tt.Name), tt.EnvValue) + } + if tt.ConfigValue != "" { + initTestConfig(tt.Name + ": " + tt.ConfigValue) } } -func TestFallbackToDefaultFlag(t *testing.T) { - setFlagsUsingViper() +func unsetValues(tt configTest) { + var f = pflag.Lookup(tt.Name) + f.Value.Set(f.DefValue) + f.Changed = false - if viper.GetInt("stderrthreshold") != 2 { - t.Logf("stderrthreshold %s", viper.GetInt("stderrthreshold")) - t.Fatalf("The default flag value was overwritten") - } + os.Unsetenv(getEnvVarName(tt.Name)) - if viper.GetString("log-flush-frequency") != "5s" { - t.Logf("log flush frequency: %s", viper.GetString("log-flush-frequency")) - t.Fatalf("The default flag value was overwritten") + viper.Reset() +} + +func TestViperAndFlags(t *testing.T) { + for _, tt := range configTests { + setValues(tt) + setupViper() + var actual = pflag.Lookup(tt.Name).Value.String() + if actual != tt.ExpectedValue { + t.Errorf("pflag.Value(%s) => %s, wanted %s [%+v]", tt.Name, actual, tt.ExpectedValue, tt) + } + unsetValues(tt) } } From 02956e8870d652a4b1a75c4d1c4af6352de8b8df Mon Sep 17 00:00:00 2001 From: Matt Rickard Date: Thu, 18 Aug 2016 16:57:41 -0700 Subject: [PATCH 4/4] Make enableUpdateNotification check a pkg level var --- cmd/minikube/cmd/root.go | 15 ++++++++++++--- cmd/minikube/cmd/root_test.go | 2 -- cmd/minikube/cmd/version.go | 4 +--- 3 files changed, 13 insertions(+), 8 deletions(-) diff --git a/cmd/minikube/cmd/root.go b/cmd/minikube/cmd/root.go index c7009d1d5ff8..727f69e4fc5a 100644 --- a/cmd/minikube/cmd/root.go +++ b/cmd/minikube/cmd/root.go @@ -44,6 +44,10 @@ const ( showLibmachineLogs = "show-libmachine-logs" ) +var ( + enableUpdateNotification = true +) + var viperWhiteList = []string{ "v", "alsologtostderr", @@ -62,12 +66,17 @@ var RootCmd = &cobra.Command{ } } - log.SetDebug(viper.Get(showLibmachineLogs)) - if !viper.GetBool(showLibmachineLogs) { + shouldShowLibmachineLogs := viper.GetBool(showLibmachineLogs) + + log.SetDebug(shouldShowLibmachineLogs) + if !shouldShowLibmachineLogs { log.SetOutWriter(ioutil.Discard) log.SetErrWriter(ioutil.Discard) } - notify.MaybePrintUpdateTextFromGithub(os.Stdout) + + if enableUpdateNotification { + notify.MaybePrintUpdateTextFromGithub(os.Stdout) + } }, } diff --git a/cmd/minikube/cmd/root_test.go b/cmd/minikube/cmd/root_test.go index 80df35404775..ea9d612775d5 100644 --- a/cmd/minikube/cmd/root_test.go +++ b/cmd/minikube/cmd/root_test.go @@ -35,8 +35,6 @@ log_dir: "/etc/hosts" log-flush-frequency: "3s" `) -const configName = ".test_minikube_config.yml" - type configTest struct { Name string EnvValue string diff --git a/cmd/minikube/cmd/version.go b/cmd/minikube/cmd/version.go index 55db30b7a4c6..88894eda8b85 100644 --- a/cmd/minikube/cmd/version.go +++ b/cmd/minikube/cmd/version.go @@ -20,9 +20,7 @@ import ( "fmt" "github.com/spf13/cobra" - "github.com/spf13/viper" - "k8s.io/minikube/pkg/minikube/config" "k8s.io/minikube/pkg/version" ) @@ -32,7 +30,7 @@ var versionCmd = &cobra.Command{ Long: `Print the version of minikube.`, PersistentPreRun: func(cmd *cobra.Command, args []string) { // Explicitly disable update checking for the version command - viper.Set(config.WantUpdateNotification, "false") + enableUpdateNotification = false RootCmd.PersistentPreRun(cmd, args) },