Move Annotations to flags and group flags into Phases

Annotations are now attached to the flags. Flags are grouped as Build, Dev, Run, Deploy
and Cleanup Phases.

cmd/skaffold/app/cmd/build.go

@@ -43,17 +43,12 @@ var (
 // NewCmdBuild describes the CLI command to build artifacts.
 func NewCmdBuild(out io.Writer) *cobra.Command {
 	cmd := &cobra.Command{
-		Use:   "build",
+		Use:   Build,
 		Short: "Builds the artifacts",
 		Args:  cobra.NoArgs,
 		RunE: func(cmd *cobra.Command, args []string) error {
 			return runBuild(out)
 		},
-		Annotations: map[string]string{
-			BuildAnnotation:  "true",
-			TestAnnotation:   "true",
-			EventsAnnotation: "true",
-		},
 	}
 	AddFlags(cmd)
 	cmd.Flags().StringArrayVarP(&opts.TargetImages, "build-image", "b", nil, "Choose which artifacts to build. Artifacts with image names that contain the expression will be built only. Default is to build sources for all artifacts")

cmd/skaffold/app/cmd/debug.go

@@ -27,20 +27,13 @@ import (
 // NewCmdDebug describes the CLI command to run a pipeline in debug mode.
 func NewCmdDebug(out io.Writer) *cobra.Command {
 	cmd := &cobra.Command{
-		Use:   "debug",
+		Use:   Debug,
 		Short: "Runs a pipeline file in debug mode",
 		Long:  "Similar to `dev`, but configures the pipeline for debugging.",
 		Args:  cobra.NoArgs,
 		RunE: func(cmd *cobra.Command, args []string) error {
 			return debug(out)
 		},
-		Annotations: map[string]string{
-			BuildAnnotation:   "true",
-			DeployAnnotation:  "true",
-			TestAnnotation:    "true",
-			CleanupAnnotation: "true",
-			EventsAnnotation:  "true",
-		},
 	}
 	AddFlags(cmd)
 	cmd.Flags().BoolVar(&opts.PortForward, "port-forward", true, "Port-forward exposed container ports within pods")

cmd/skaffold/app/cmd/deploy.go

@@ -25,17 +25,13 @@ import (
 // NewCmdDeploy describes the CLI command to deploy artifacts.
 func NewCmdDeploy(out io.Writer) *cobra.Command {
 	cmd := &cobra.Command{
-		Use:   "deploy",
+		Use:   Deploy,
 		Short: "Deploys the artifacts",
 		Args:  cobra.NoArgs,
 		RunE: func(cmd *cobra.Command, args []string) error {
 			// Same actions as `skaffold run`, but with pre-built images.
 			return run(out)
 		},
-		Annotations: map[string]string{
-			DeployAnnotation: "true",
-			EventsAnnotation: "true",
-		},
 	}
 	AddFlags(cmd)
 	cmd.Flags().StringSliceVar(&opts.PreBuiltImages, "images", nil, "A list of pre-built images to deploy")

cmd/skaffold/app/cmd/dev.go

@@ -29,21 +29,13 @@ import (
 // NewCmdDev describes the CLI command to run a pipeline in development mode.
 func NewCmdDev(out io.Writer) *cobra.Command {
 	cmd := &cobra.Command{
-		Use:   "dev",
+		Use:   Dev,
 		Short: "Runs a pipeline file in development mode",
 		Args:  cobra.NoArgs,
 		RunE: func(cmd *cobra.Command, args []string) error {
 			return dev(out)
 		},
-		Annotations: map[string]string{
-			BuildAnnotation:   "true",
-			DeployAnnotation:  "true",
-			TestAnnotation:    "true",
-			EventsAnnotation:  "true",
-			CleanupAnnotation: "true",
-		},
 	}
-	addTailDevFlag(cmd)
 	AddFlags(cmd)
 	cmd.Flags().StringVar(&opts.Trigger, "trigger", "polling", "How are changes detected? (polling, manual or notify)")
 	cmd.Flags().StringSliceVarP(&opts.TargetImages, "watch-image", "w", nil, "Choose which artifacts to watch. Artifacts with image names that contain the expression will be watched only. Default is to watch sources for all artifacts")

cmd/skaffold/app/cmd/flags.go

@@ -18,28 +18,31 @@ package cmd
 
 import (
 	"github.com/GoogleContainerTools/skaffold/pkg/skaffold/constants"
+	"github.com/sirupsen/logrus"
 	"github.com/spf13/cobra"
 	flag "github.com/spf13/pflag"
 )
 
 const (
-	BuildAnnotation   = "build"
-	DeployAnnotation  = "deploy"
-	TestAnnotation    = "test"
-	DeleteAnnotation  = "delete"
-	CleanupAnnotation = "cleanup"
-	EventsAnnotation  = "events"
+	CmdKey = "cmds"
+	Dev    = "dev"
+	Build  = "build"
+	Run    = "run"
+	Debug  = "debug"
+	Deploy = "deploy"
+	Common = "common"
 )
 
 var (
-	CommonFlagSet = commonFlagSet("common")
-
-	AnnotationToFlag = map[string]*flag.FlagSet{
-		BuildAnnotation:   buildFlagSet(BuildAnnotation),
-		EventsAnnotation:  eventsAPIFlagSet(EventsAnnotation),
-		DeployAnnotation:  deployFlagSet(DeployAnnotation),
-		TestAnnotation:    testFlagSet(TestAnnotation),
-		CleanupAnnotation: cleanupFlagSet(CleanupAnnotation),
+	AllFlags = []*flag.FlagSet{
+		commonFlagSet("common"),
+		buildFlagSet("build-phase"),
+		eventsAPIFlagSet("events-feature"),
+		deployCommonFlagSet("deploy-common"),
+		deployNonDevFlagSet("deploy-non-dev"),
+		testFlagSet("test-phase"),
+		cleanupFlagSet("cleanup-phase"),
+		deployDevFlagSet("deploy-dev"),
 	}
 )
 
@@ -49,6 +52,10 @@ func commonFlagSet(name string) *flag.FlagSet {
 	commonFlags.StringArrayVarP(&opts.Profiles, "profile", "p", nil, "Activate profiles by name")
 	commonFlags.StringVarP(&opts.Namespace, "namespace", "n", "", "Run deployments in the specified namespace")
 	commonFlags.StringVarP(&opts.DefaultRepo, "default-repo", "d", "", "Default repository value (overrides global config)")
+
+	commonFlags.VisitAll(func(flag *flag.Flag) {
+		commonFlags.SetAnnotation(flag.Name, CmdKey, []string{Common})
+	})
 	return commonFlags
 }
 
@@ -57,6 +64,10 @@ func buildFlagSet(name string) *flag.FlagSet {
 	buildFlags.BoolVar(&opts.CacheArtifacts, "cache-artifacts", false, "Set to true to enable caching of artifacts.")
 	buildFlags.StringVarP(&opts.CacheFile, "cache-file", "", "", "Specify the location of the cache file (default $HOME/.skaffold/cache)")
 	buildFlags.StringArrayVar(&opts.InsecureRegistries, "insecure-registry", nil, "Target registries for built images which are not secure")
+
+	buildFlags.VisitAll(func(flag *flag.Flag) {
+		buildFlags.SetAnnotation(flag.Name, CmdKey, []string{Dev, Run, Build, Debug})
+	})
 	return buildFlags
 }
 
@@ -65,55 +76,86 @@ func eventsAPIFlagSet(name string) *flag.FlagSet {
 	eventFlags.BoolVar(&opts.EnableRPC, "enable-rpc", false, "Enable gRPC for exposing Skaffold events (true by default for `skaffold dev`)")
 	eventFlags.IntVar(&opts.RPCPort, "rpc-port", constants.DefaultRPCPort, "tcp port to expose event API")
 	eventFlags.IntVar(&opts.RPCHTTPPort, "rpc-http-port", constants.DefaultRPCHTTPPort, "tcp port to expose event REST API over HTTP")
+
+	eventFlags.VisitAll(func(flag *flag.Flag) {
+		eventFlags.SetAnnotation(flag.Name, CmdKey, []string{Dev, Run, Build, Deploy, Debug})
+	})
 	return eventFlags
 }
 
-func deployFlagSet(name string) *flag.FlagSet {
-	deployFlags := flag.NewFlagSet(name, flag.ContinueOnError)
-	deployFlags.BoolVar(&opts.Tail, "tail", false, "Stream logs from deployed objects")
-	deployFlags.BoolVar(&opts.Force, "force", false, "Recreate kubernetes resources if necessary for deployment (default: false, warning: might cause downtime!)")
-	deployFlags.StringArrayVarP(&opts.CustomLabels, "label", "l", nil, "Add custom labels to deployed objects. Set multiple times for multiple labels.")
-	deployFlags.BoolVar(&opts.Notification, "toot", false, "Emit a terminal beep after the deploy is complete")
-	return deployFlags
+func deployCommonFlagSet(name string) *flag.FlagSet {
+	deployPhaseFlags := flag.NewFlagSet(name, flag.ContinueOnError)
+	deployPhaseFlags.StringArrayVarP(&opts.CustomLabels, "label", "l", nil, "Add custom labels to deployed objects. Set multiple times for multiple labels.")
+	deployPhaseFlags.BoolVar(&opts.Notification, "toot", false, "Emit a terminal beep after the deploy is complete")
+
+	deployPhaseFlags.VisitAll(func(flag *flag.Flag) {
+		deployPhaseFlags.SetAnnotation(flag.Name, "", []string{Dev, Run, Deploy, Debug})
+	})
+	return deployPhaseFlags
+}
+
+func deployNonDevFlagSet(name string) *flag.FlagSet {
+	deployCommandFlags := flag.NewFlagSet(name, flag.ContinueOnError)
+	deployCommandFlags.BoolVar(&opts.Tail, "tail", false, "Stream logs from deployed objects")
+	deployCommandFlags.BoolVar(&opts.Force, "force", false, "Recreate kubernetes resources if necessary for deployment (default: false, warning: might cause downtime!)")
+
+	deployCommandFlags.VisitAll(func(flag *flag.Flag) {
+		deployCommandFlags.SetAnnotation(flag.Name, CmdKey, []string{Deploy, Run})
+	})
+	return deployCommandFlags
+}
+
+func deployDevFlagSet(name string) *flag.FlagSet {
+	devFlags := flag.NewFlagSet(name, flag.ContinueOnError)
+	devFlags.BoolVar(&opts.TailDev, "tail", true, "Stream logs from deployed objects")
+	devFlags.BoolVar(&opts.ForceDev, "force", true, "Recreate kubernetes resources if necessary for deployment.")
+
+	devFlags.VisitAll(func(flag *flag.Flag) {
+		devFlags.SetAnnotation(flag.Name, CmdKey, []string{Dev, Debug})
+	})
+	return devFlags
 }
 
 func testFlagSet(name string) *flag.FlagSet {
 	testFlags := flag.NewFlagSet(name, flag.ContinueOnError)
 	testFlags.BoolVar(&opts.SkipTests, "skip-tests", false, "Whether to skip the tests after building")
+
+	testFlags.VisitAll(func(flag *flag.Flag) {
+		testFlags.SetAnnotation(flag.Name, CmdKey, []string{Dev, Run, Debug, Build})
+	})
 	return testFlags
 }
 
 func cleanupFlagSet(name string) *flag.FlagSet {
 	cleanupFlags := flag.NewFlagSet(name, flag.ContinueOnError)
 	cleanupFlags.BoolVar(&opts.Cleanup, "cleanup", true, "Delete deployments after dev or debug mode is interrupted")
 	cleanupFlags.BoolVar(&opts.NoPrune, "no-prune", false, "Skip removing images and containers built by Skaffold")
+
+	cleanupFlags.VisitAll(func(flag *flag.Flag) {
+		cleanupFlags.SetAnnotation(flag.Name, CmdKey, []string{Dev, Debug})
+	})
 	return cleanupFlags
 }
 
+// AddFlags goes through all flags defined and adds them to command if they have an annotation.
 func AddFlags(cmd *cobra.Command) {
-	cmd.Flags().AddFlagSet(CommonFlagSet)
-	cmd.Flags().AddFlagSet(getAnnotatedFlags(cmd.Use, cmd.Annotations))
+	for _, flagSet := range AllFlags {
+		flagSet.VisitAll(func(flag *flag.Flag) {
+			if len(flag.Annotations) == 0 {
+				logrus.Errorf("registered a flag %s without any annotations for command", flag.Name)
+			}
+			if hasCmdAnnotation(cmd.Use, flag.Annotations[CmdKey]) {
+				cmd.Flags().AddFlag(flag)
+			}
+		})
+	}
 }
 
-func getAnnotatedFlags(name string, annotations map[string]string) *flag.FlagSet {
-	allFlags := flag.NewFlagSet(name, flag.ContinueOnError)
-	for a := range annotations {
-		flags, ok := AnnotationToFlag[a]
-		if ok {
-			allFlags.AddFlagSet(flags)
+func hasCmdAnnotation(cmdName string, annotations []string) bool {
+	for _, a := range annotations {
+		if cmdName == a || a == Common {
+			return true
 		}
 	}
-	return allFlags
-}
-
-// This is hack to get around an existing issue.
-// Flag "--tail" is used for setting "opts.TailDev" and "opts.Tail".
-// In Deploy flow, "--tail" is set to opts.Tail with default set to false.
-// However, in Run and Dev flow, "--tail" is set to opts.TailDev with default set to true.
-// Dev and Run, both run deploy, and hence default to --tail is set to false based on order
-// of the keys in *cobra.Command.Annotations (determined at runtime)
-// Dev and Run commands should first call addTailDevFlag() and register "--tail" flag with default set to true.
-// This will ignore the "--tail" flag added in `AddFlags` function
-func addTailDevFlag(cmd *cobra.Command) {
-	cmd.Flags().BoolVar(&opts.TailDev, "tail", true, "Stream logs from deployed objects")
+	return false
 }

cmd/skaffold/app/cmd/flags_test.go

@@ -18,39 +18,95 @@ package cmd
 
 import (
 	"testing"
+
+	"github.com/GoogleContainerTools/skaffold/testutil"
+	"github.com/spf13/cobra"
+	flag "github.com/spf13/pflag"
 )
 
-func TestAddFlags(t *testing.T) {
+func TestHasCmdAnnotation(t *testing.T) {
 	tests := []struct {
-		description   string
-		annotations   map[string]string
-		expectedFlags []string
+		description     string
+		cmd             string
+		flagAnnotations []string
+		expected        bool
 	}{
 		{
-			description:   "not register annotations + correct annotation",
-			annotations:   map[string]string{"some": "true", "test": "true"},
-			expectedFlags: []string{"skip-tests"},
+			description:     "flag has command annotations",
+			cmd:             "build",
+			flagAnnotations: []string{"build", "events"},
+			expected:        true,
 		},
 		{
-			description:   "not register annotations should return no flags",
-			annotations:   map[string]string{"some": "true"},
-			expectedFlags: nil,
+			description:     "flag does not have command annotations",
+			cmd:             "build",
+			flagAnnotations: []string{"some"},
 		},
 		{
-			description:   "union of annotations",
-			annotations:   map[string]string{"cleanup": "true", "test": "true"},
-			expectedFlags: []string{"skip-tests", "cleanup", "no-prune"},
+			description:     "flag has all annotations",
+			cmd:             "build",
+			flagAnnotations: []string{"common"},
+			expected:        true,
 		},
 	}
 
 	for _, test := range tests {
 		t.Run(test.description, func(t *testing.T) {
-			flags := getAnnotatedFlags("test", test.annotations)
-			for _, f := range test.expectedFlags {
-				if flags.Lookup(f) == nil {
-					t.Errorf("expected flag %s to be found.", f)
-				}
+			if test.expected != hasCmdAnnotation(test.cmd, test.flagAnnotations) {
+				t.Errorf("expected %t but found %t", test.expected, !test.expected)
 			}
 		})
 	}
 }
+
+func TestAddFlags(t *testing.T) {
+	originalFlags := AllFlags
+	defer func() { AllFlags = originalFlags }()
+	tests := []struct {
+		description        string
+		cmd                string
+		flagAnnotationsMap map[string][]string
+		flagAnnotations    []string
+		expectedFlags      map[string]bool
+	}{
+		{
+			description: "flag with command annotation should add flag to the command",
+			cmd:         "cmd1",
+			flagAnnotationsMap: map[string][]string{
+				"flag1": []string{"cmd1", "cmd2"},
+				"flag2": []string{"cmd1"},
+				"flag3": []string{"cmd2"},
+			},
+			expectedFlags: map[string]bool{"flag1": true, "flag2": true},
+		},
+		{
+			description: "flag with command annotation should add no flags",
+			cmd:         "cmd1",
+			flagAnnotationsMap: map[string][]string{
+				"flag3": []string{"cmd2"},
+			},
+			expectedFlags: map[string]bool{},
+		},
+	}
+	for _, test := range tests {
+		t.Run(test.description, func(t *testing.T) {
+			cmd := &cobra.Command{
+				Use: test.cmd,
+			}
+			flagSet := flag.NewFlagSet("flagset", flag.ContinueOnError)
+			for name, annotations := range test.flagAnnotationsMap {
+				flagSet.String(name, "value", "usage")
+				flagSet.SetAnnotation(name, CmdKey, annotations)
+			}
+			AllFlags = []*flag.FlagSet{flagSet}
+			AddFlags(cmd)
+			// Check if no other flags then those in the expected map
+			actualFlags := map[string]bool{}
+			cmd.Flags().VisitAll(func(flag *flag.Flag) {
+				actualFlags[flag.Name] = true
+			})
+			testutil.CheckDeepEqual(t, test.expectedFlags, actualFlags)
+		})
+	}
+
+}