Better handling of per-command default values

cmd/skaffold/app/cmd/cmd.go

@@ -45,31 +45,6 @@ var (
 	overwrite    bool
 )
 
-func setSpecialDefaults(cmd *cobra.Command) {
-	switch cmd.Use {
-	case "dev":
-		// In dev mode, the default is to enable the rpc server
-		if !cmd.Flag("enable-rpc").Changed {
-			opts.EnableRPC = true
-		}
-
-		// In dev mode, the default is to force deployments
-		if !cmd.Flag("force").Changed {
-			opts.Force = true
-		}
-
-		// In dev mode, the default is to tail the logs
-		if !cmd.Flag("tail").Changed {
-			opts.Tail = true
-		}
-	case "debug":
-		// In debug mode, the default is to tail the logs
-		if !cmd.Flag("tail").Changed {
-			opts.Tail = true
-		}
-	}
-}
-
 func NewSkaffoldCommand(out, err io.Writer) *cobra.Command {
 	updateMsg := make(chan string)
 	var shutdownAPIServer func() error
@@ -88,9 +63,6 @@ func NewSkaffoldCommand(out, err io.Writer) *cobra.Command {
 
 			opts.Command = cmd.Use
 
-			// Some flags have special default values depending on the command.
-			setSpecialDefaults(cmd)
-
 			color.SetupColors(out, defaultColor, forceColors)
 			cmd.Root().SetOutput(out)
 

cmd/skaffold/app/cmd/debug_test.go

@@ -0,0 +1,39 @@
+/*
+Copyright 2020 The Skaffold Authors
+
+Licensed under the Apache License, Version 2.0 (the "License");
+you may not use this file except in compliance with the License.
+You may obtain a copy of the License at
+
+    http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+*/
+
+package cmd
+
+import (
+	"testing"
+
+	"github.com/GoogleContainerTools/skaffold/pkg/skaffold/config"
+	"github.com/GoogleContainerTools/skaffold/testutil"
+)
+
+func TestNewCmdDebug(t *testing.T) {
+	testutil.Run(t, "", func(t *testutil.T) {
+		t.NewTempDir().Chdir()
+		t.Override(&opts, config.SkaffoldOptions{})
+
+		cmd := NewCmdDebug()
+		cmd.SilenceUsage = true
+		cmd.Execute()
+
+		t.CheckDeepEqual(true, opts.Tail)
+		t.CheckDeepEqual(false, opts.Force)
+		t.CheckDeepEqual(false, opts.Force)
+	})
+}

cmd/skaffold/app/cmd/dev_test.go

@@ -162,3 +162,18 @@ func TestDevConfigChange(t *testing.T) {
 		t.CheckDeepEqual(mockRunner.cycles, 2)
 	})
 }
+
+func TestNewCmdDev(t *testing.T) {
+	testutil.Run(t, "", func(t *testutil.T) {
+		t.NewTempDir().Chdir()
+		t.Override(&opts, config.SkaffoldOptions{})
+
+		cmd := NewCmdDev()
+		cmd.SilenceUsage = true
+		cmd.Execute()
+
+		t.CheckDeepEqual(true, opts.Tail)
+		t.CheckDeepEqual(true, opts.Force)
+		t.CheckDeepEqual(true, opts.Force)
+	})
+}

cmd/skaffold/app/cmd/flags.go

@@ -17,6 +17,7 @@ limitations under the License.
 package cmd
 
 import (
+	"fmt"
 	"reflect"
 
 	"github.com/spf13/cobra"
@@ -28,14 +29,15 @@ import (
 // Flag defines a Skaffold CLI flag which contains a list of
 // subcommands the flag belongs to in `DefinedOn` field.
 type Flag struct {
-	Name          string
-	Shorthand     string
-	Usage         string
-	Value         interface{}
-	DefValue      interface{}
-	FlagAddMethod string
-	DefinedOn     []string
-	Hidden        bool
+	Name               string
+	Shorthand          string
+	Usage              string
+	Value              interface{}
+	DefValue           interface{}
+	DefValuePerCommand map[string]interface{}
+	FlagAddMethod      string
+	DefinedOn          []string
+	Hidden             bool
 
 	pflag *pflag.Flag
 }
@@ -109,10 +111,13 @@ var FlagRegistry = []Flag{
 		DefinedOn:     []string{"dev", "build", "run", "debug"},
 	},
 	{
-		Name:          "enable-rpc",
-		Usage:         "Enable gRPC for exposing Skaffold events (true by default for `skaffold dev`)",
-		Value:         &opts.EnableRPC,
-		DefValue:      false,
+		Name:     "enable-rpc",
+		Usage:    "Enable gRPC for exposing Skaffold events (true by default for `skaffold dev`)",
+		Value:    &opts.EnableRPC,
+		DefValue: false,
+		DefValuePerCommand: map[string]interface{}{
+			"dev": true,
+		},
 		FlagAddMethod: "BoolVar",
 		DefinedOn:     []string{"dev", "build", "run", "debug", "deploy"},
 	},
@@ -150,18 +155,25 @@ var FlagRegistry = []Flag{
 		DefinedOn:     []string{"dev", "build", "run", "debug", "deploy"},
 	},
 	{
-		Name:          "tail",
-		Usage:         "Stream logs from deployed objects (true by default for `skaffold dev` and `skaffold debug`)",
-		Value:         &opts.Tail,
-		DefValue:      false, // true for "dev" and "debug"
+		Name:     "tail",
+		Usage:    "Stream logs from deployed objects (true by default for `skaffold dev` and `skaffold debug`)",
+		Value:    &opts.Tail,
+		DefValue: false,
+		DefValuePerCommand: map[string]interface{}{
+			"dev":   true,
+			"debug": true,
+		},
 		FlagAddMethod: "BoolVar",
 		DefinedOn:     []string{"dev", "run", "debug", "deploy"},
 	},
 	{
-		Name:          "force",
-		Usage:         "Recreate Kubernetes resources if necessary for deployment, warning: might cause downtime! (true by default for `skaffold dev`)",
-		Value:         &opts.Force,
-		DefValue:      false, // true for "dev"
+		Name:     "force",
+		Usage:    "Recreate Kubernetes resources if necessary for deployment, warning: might cause downtime! (true by default for `skaffold dev`)",
+		Value:    &opts.Force,
+		DefValue: false,
+		DefValuePerCommand: map[string]interface{}{
+			"dev": true,
+		},
 		FlagAddMethod: "BoolVar",
 		DefinedOn:     []string{"deploy", "dev", "run", "debug"},
 	},
@@ -282,29 +294,69 @@ func (fl *Flag) flag() *pflag.Flag {
 		return fl.pflag
 	}
 
-	inputs := []reflect.Value{reflect.ValueOf(fl.Value), reflect.ValueOf(fl.Name)}
+	inputs := []interface{}{fl.Value, fl.Name}
 	if fl.FlagAddMethod != "Var" {
-		inputs = append(inputs, reflect.ValueOf(fl.DefValue))
+		inputs = append(inputs, fl.DefValue)
 	}
-	inputs = append(inputs, reflect.ValueOf(fl.Usage))
+	inputs = append(inputs, fl.Usage)
 
 	fs := pflag.NewFlagSet(fl.Name, pflag.ContinueOnError)
-	reflect.ValueOf(fs).MethodByName(fl.FlagAddMethod).Call(inputs)
+	reflect.ValueOf(fs).MethodByName(fl.FlagAddMethod).Call(reflectValueOf(inputs))
 	f := fs.Lookup(fl.Name)
 	f.Shorthand = fl.Shorthand
 	f.Hidden = fl.Hidden
 
 	fl.pflag = f
-	return fl.pflag
+	return f
+}
+
+func reflectValueOf(values []interface{}) []reflect.Value {
+	var results []reflect.Value
+	for _, v := range values {
+		results = append(results, reflect.ValueOf(v))
+	}
+	return results
 }
 
+// AddFlags adds to the command the common flags that are annotated with the command name.
 func AddFlags(cmd *cobra.Command) {
+	var flagsForCommand []*Flag
+
 	for i := range FlagRegistry {
 		fl := &FlagRegistry[i]
+		if !hasCmdAnnotation(cmd.Use, fl.DefinedOn) {
+			continue
+		}
 
-		if hasCmdAnnotation(cmd.Use, fl.DefinedOn) {
-			cmd.Flags().AddFlag(fl.flag())
+		cmd.Flags().AddFlag(fl.flag())
+
+		flagsForCommand = append(flagsForCommand, fl)
+	}
+
+	// Apply command-specific default values to flags.
+	cmd.PersistentPreRunE = func(cmd *cobra.Command, args []string) error {
+		// Update default values.
+		for _, fl := range flagsForCommand {
+			if defValue, present := fl.DefValuePerCommand[cmd.Use]; present {
+				if flag := cmd.Flag(fl.Name); !flag.Changed {
+					flag.Value.Set(fmt.Sprintf("%v", defValue))
+				}
+			}
 		}
+
+		// Since PersistentPreRunE replaces the parent's PersistentPreRunE,
+		// make sure we call it, if it is set.
+		if parent := cmd.Parent(); parent != nil {
+			if preRun := parent.PersistentPreRunE; preRun != nil {
+				if err := preRun(cmd, args); err != nil {
+					return err
+				}
+			} else if preRun := parent.PersistentPreRun; preRun != nil {
+				preRun(cmd, args)
+			}
+		}
+
+		return nil
 	}
 }
 

cmd/skaffold/app/cmd/run_test.go

@@ -0,0 +1,39 @@
+/*
+Copyright 2020 The Skaffold Authors
+
+Licensed under the Apache License, Version 2.0 (the "License");
+you may not use this file except in compliance with the License.
+You may obtain a copy of the License at
+
+    http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+*/
+
+package cmd
+
+import (
+	"testing"
+
+	"github.com/GoogleContainerTools/skaffold/pkg/skaffold/config"
+	"github.com/GoogleContainerTools/skaffold/testutil"
+)
+
+func TestNewCmdRun(t *testing.T) {
+	testutil.Run(t, "", func(t *testutil.T) {
+		t.NewTempDir().Chdir()
+		t.Override(&opts, config.SkaffoldOptions{})
+
+		cmd := NewCmdRun()
+		cmd.SilenceUsage = true
+		cmd.Execute()
+
+		t.CheckDeepEqual(false, opts.Tail)
+		t.CheckDeepEqual(false, opts.Force)
+		t.CheckDeepEqual(false, opts.Force)
+	})
+}