Repository: grafana/tanka
Pull request #754 (2022-09-12 16:47:30)

Add new `--auto-approve=if-no-changes` option
Closes https://github.com/grafana/tanka/issues/743 
Closes https://github.com/grafana/tanka/issues/693 
It replaces the `--dangerous-auto-approve` option. It has three values: `always`, `never` and `if-no-changes`

In this PR, I also create a common structure for args that are shared between `prune`, `delete` and `apply` Doing so, I found two bugs:
- Removed the `--validate` option on `delete`, it wasn't used
- Wired in `--dry-run` to the `prune` command, it was exposed but didn't make it to kubectl

ghostsquad: would love to see this merged

Patch:
diff --git a/cmd/tk/flags.go b/cmd/tk/flags.go
index d9fa63ee..fbf7e4fc 100644
--- a/cmd/tk/flags.go
+++ b/cmd/tk/flags.go
@@ -24,6 +24,18 @@ func workflowFlags(fs *pflag.FlagSet) *workflowFlagVars {
 	return &v
 }
 
+func addApplyFlags(fs *pflag.FlagSet, opts *tanka.ApplyBaseOpts, autoApproveDeprecated *bool, autoApprove *string) {
+	fs.StringVar(&opts.DryRun, "dry-run", "", `--dry-run parameter to pass down to kubectl, must be "none", "server", or "client"`)
+	fs.BoolVar(&opts.Force, "force", false, "force applying (kubectl apply --force)")
+
+	// Parse the auto-approve flag (choice), still supporting the deprecated dangerous-auto-approve flag (boolean)
+	fs.BoolVar(autoApproveDeprecated, "dangerous-auto-approve", false, "skip interactive approval. Only for automation!")
+	if err := fs.MarkDeprecated("dangerous-auto-approve", "use --auto-approve instead"); err != nil {
+		log.Fatalf("failed to mark deprecated flag: %s", err)
+	}
+	fs.StringVar(autoApprove, "auto-approve", "", "skip interactive approval. Only for automation! Allowed values: 'always', 'never', 'if-no-changes'")
+}
+
 func labelSelectorFlag(fs *pflag.FlagSet) func() labels.Selector {
 	labelSelector := fs.StringP("selector", "l", "", "Label selector. Uses the same syntax as kubectl does")
 
diff --git a/cmd/tk/workflow.go b/cmd/tk/workflow.go
index 125ec02b..abc7dcbe 100644
--- a/cmd/tk/workflow.go
+++ b/cmd/tk/workflow.go
@@ -29,6 +29,28 @@ func validateDryRun(dryRunStr string) error {
 	return fmt.Errorf(`--dry-run must be either: "", "none", "server" or "client"`)
 }
 
+func validateAutoApprove(autoApproveDeprecated bool, autoApproveString string) (tanka.AutoApproveSetting, error) {
+	var result tanka.AutoApproveSetting
+
+	if autoApproveString != "" && autoApproveDeprecated {
+		return result, fmt.Errorf("--dangerous-auto-approve and --auto-approve are mutually exclusive")
+	}
+	if autoApproveString == "" {
+		if autoApproveDeprecated {
+			result = tanka.AutoApproveAlways
+		} else {
+			result = tanka.AutoApproveNever
+		}
+	} else {
+		if autoApproveString != string(tanka.AutoApproveAlways) && autoApproveString != string(tanka.AutoApproveNever) && autoApproveString != string(tanka.AutoApproveNoChanges) {
+			return result, fmt.Errorf("invalid value for --auto-approve: %s", autoApproveString)
+		}
+		result = tanka.AutoApproveSetting(autoApproveString)
+	}
+
+	return result, nil
+}
+
 func applyCmd() *cli.Command {
 	cmd := &cli.Command{
 		Use:   "apply <path>",
@@ -41,13 +63,15 @@ func applyCmd() *cli.Command {
 	}
 
 	var opts tanka.ApplyOpts
-	cmd.Flags().BoolVar(&opts.Force, "force", false, "force applying (kubectl apply --force)")
 	cmd.Flags().BoolVar(&opts.Validate, "validate", true, "validation of resources (kubectl --validate=false)")
-	cmd.Flags().BoolVar(&opts.AutoApprove, "dangerous-auto-approve", false, "skip interactive approval. Only for automation!")
-	cmd.Flags().StringVar(&opts.DryRun, "dry-run", "", `--dry-run parameter to pass down to kubectl, must be "none", "server", or "client"`)
 	cmd.Flags().StringVar(&opts.ApplyStrategy, "apply-strategy", "", "force the apply strategy to use. Automatically chosen if not set.")
 	cmd.Flags().StringVar(&opts.DiffStrategy, "diff-strategy", "", "force the diff strategy to use. Automatically chosen if not set.")
 
+	var (
+		autoApproveDeprecated bool
+		autoApproveString     string
+	)
+	addApplyFlags(cmd.Flags(), &opts.ApplyBaseOpts, &autoApproveDeprecated, &autoApproveString)
 	vars := workflowFlags(cmd.Flags())
 	getJsonnetOpts := jsonnetFlags(cmd.Flags())
 
@@ -56,6 +80,9 @@ func applyCmd() *cli.Command {
 		if err != nil {
 			return err
 		}
+		if opts.AutoApprove, err = validateAutoApprove(autoApproveDeprecated, autoApproveString); err != nil {
+			return err
+		}
 
 		filters, err := process.StrExps(vars.targets...)
 		if err != nil {
@@ -78,10 +105,12 @@ func pruneCmd() *cli.Command {
 	}
 
 	var opts tanka.PruneOpts
-	cmd.Flags().StringVar(&opts.DryRun, "dry-run", "", `--dry-run parameter to pass down to kubectl, must be "none", "server", or "client"`)
-	cmd.Flags().BoolVar(&opts.Force, "force", false, "force deleting (kubectl delete --force)")
-	cmd.Flags().BoolVar(&opts.AutoApprove, "dangerous-auto-approve", false, "skip interactive approval. Only for automation!")
 	cmd.Flags().StringVar(&opts.Name, "name", "", "string that only a single inline environment contains in its name")
+	var (
+		autoApproveDeprecated bool
+		autoApproveString     string
+	)
+	addApplyFlags(cmd.Flags(), &opts.ApplyBaseOpts, &autoApproveDeprecated, &autoApproveString)
 	getJsonnetOpts := jsonnetFlags(cmd.Flags())
 
 	cmd.Run = func(cmd *cli.Command, args []string) error {
@@ -89,6 +118,9 @@ func pruneCmd() *cli.Command {
 		if err != nil {
 			return err
 		}
+		if opts.AutoApprove, err = validateAutoApprove(autoApproveDeprecated, autoApproveString); err != nil {
+			return err
+		}
 
 		opts.JsonnetOpts = getJsonnetOpts()
 
@@ -106,11 +138,12 @@ func deleteCmd() *cli.Command {
 	}
 
 	var opts tanka.DeleteOpts
-	cmd.Flags().StringVar(&opts.DryRun, "dry-run", "", `--dry-run parameter to pass down to kubectl, must be "none", "server", or "client"`)
-	cmd.Flags().BoolVar(&opts.Force, "force", false, "force deleting (kubectl delete --force)")
-	cmd.Flags().BoolVar(&opts.Validate, "validate", true, "validation of resources (kubectl --validate=false)")
-	cmd.Flags().BoolVar(&opts.AutoApprove, "dangerous-auto-approve", false, "skip interactive approval. Only for automation!")
 
+	var (
+		autoApproveDeprecated bool
+		autoApproveString     string
+	)
+	addApplyFlags(cmd.Flags(), &opts.ApplyBaseOpts, &autoApproveDeprecated, &autoApproveString)
 	vars := workflowFlags(cmd.Flags())
 	getJsonnetOpts := jsonnetFlags(cmd.Flags())
 
@@ -119,6 +152,9 @@ func deleteCmd() *cli.Command {
 		if err != nil {
 			return err
 		}
+		if opts.AutoApprove, err = validateAutoApprove(autoApproveDeprecated, autoApproveString); err != nil {
+			return err
+		}
 
 		filters, err := process.StrExps(vars.targets...)
 		if err != nil {
diff --git a/pkg/tanka/prune.go b/pkg/tanka/prune.go
index be45b7b2..34358c70 100644
--- a/pkg/tanka/prune.go
+++ b/pkg/tanka/prune.go
@@ -12,14 +12,7 @@ import (
 
 // PruneOpts specify additional properties for the Prune action
 type PruneOpts struct {
-	Opts
-
-	// AutoApprove skips the interactive approval
-	AutoApprove bool
-	// Force ignores any warnings kubectl might have
-	Force bool
-	// DryRun string passed to kubectl as --dry-run=<DryRun>
-	DryRun string
+	ApplyBaseOpts
 }
 
 // Prune deletes all resources from the cluster, that are no longer present in
@@ -73,13 +66,15 @@ func Prune(baseDir string, opts PruneOpts) error {
 	}
 
 	// prompt for confirm
-	if opts.AutoApprove {
+	if opts.AutoApprove == AutoApproveAlways {
+		// Skip approval
 	} else if err := confirmPrompt("Pruning from", p.Env.Spec.Namespace, kube.Info()); err != nil {
 		return err
 	}
 
 	// delete resources
 	return kube.Delete(orphaned, kubernetes.DeleteOpts{
-		Force: opts.Force,
+		Force:  opts.Force,
+		DryRun: opts.DryRun,
 	})
 }
diff --git a/pkg/tanka/workflow.go b/pkg/tanka/workflow.go
index 38cb2b79..84130f57 100644
--- a/pkg/tanka/workflow.go
+++ b/pkg/tanka/workflow.go
@@ -13,22 +13,39 @@ import (
 	"github.com/grafana/tanka/pkg/term"
 )
 
-// ApplyOpts specify additional properties for the Apply action
-type ApplyOpts struct {
+type AutoApproveSetting string
+
+const (
+	// AutoApproveNever disables auto-approval
+	AutoApproveNever AutoApproveSetting = "never"
+	// AutoApproveAlways enables auto-approval
+	AutoApproveAlways AutoApproveSetting = "always"
+	// AutoApproveNoChanges enables auto-approval if there were no changes found in the diff
+	AutoApproveNoChanges AutoApproveSetting = "if-no-changes"
+)
+
+type ApplyBaseOpts struct {
 	Opts
 
 	// AutoApprove skips the interactive approval
-	AutoApprove bool
+	AutoApprove AutoApproveSetting
+	// DryRun string passed to kubectl as --dry-run=<DryRun>
+	DryRun string
+	// Force ignores any warnings kubectl might have
+	Force bool
+}
+
+// ApplyOpts specify additional properties for the Apply action
+type ApplyOpts struct {
+	ApplyBaseOpts
+
 	// DiffStrategy to use for printing the diff before approval
 	DiffStrategy string
 	// ApplyStrategy decides how kubectl will apply the manifest
 	ApplyStrategy string
-	// Force ignores any warnings kubectl might have
-	Force bool
 	// Validate set to false ignores invalid Kubernetes schemas
 	Validate bool
-	// DryRun string passed to kubectl as --dry-run=<DryRun>
-	DryRun string
+
 	// ServerSide bool passed to kubectl as --server-side
 	ServerSide bool
 }
@@ -76,6 +93,7 @@ func Apply(baseDir string, opts ApplyOpts) error {
 	}
 	defer kube.Close()
 
+	var noChanges bool
 	if opts.DiffStrategy != "none" {
 		// show diff
 		diff, err := kube.Diff(l.Resources, kubernetes.DiffOpts{Strategy: opts.DiffStrategy})
@@ -84,6 +102,7 @@ func Apply(baseDir string, opts ApplyOpts) error {
 			// This is not fatal, the diff is not strictly required
 			log.Println("Error diffing:", err)
 		case diff == nil:
+			noChanges = true
 			// If using KUBECTL_INTERACTIVE_DIFF, the stdout buffer is always empty
 			if os.Getenv("KUBECTL_INTERACTIVE_DIFF") == "" {
 				tmp := "Warning: There are no differences. Your apply may not do anything at all."
@@ -99,7 +118,8 @@ func Apply(baseDir string, opts ApplyOpts) error {
 	}
 
 	// prompt for confirmation
-	if opts.AutoApprove || opts.DryRun != "" {
+	if opts.AutoApprove == AutoApproveAlways || (noChanges && opts.AutoApprove == AutoApproveNoChanges) || opts.DryRun != "" {
+		// Skip approval
 	} else if err := confirmPrompt("Applying to", l.Env.Spec.Namespace, kube.Info()); err != nil {
 		return err
 	}
@@ -167,17 +187,7 @@ func Diff(baseDir string, opts DiffOpts) (*string, error) {
 
 // DeleteOpts specify additional properties for the Delete operation
 type DeleteOpts struct {
-	Opts
-
-	// AutoApprove skips the interactive approval
-	AutoApprove bool
-
-	// Force ignores any warnings kubectl might have
-	Force bool
-	// Validate set to false ignores invalid Kubernetes schemas
-	Validate bool
-	// DryRun string passed to kubectl as --dry-run=<DryRun>
-	DryRun string
+	ApplyBaseOpts
 }
 
 // Delete parses the environment at the given directory (a `baseDir`) and deletes
@@ -211,15 +221,15 @@ func Delete(baseDir string, opts DeleteOpts) error {
 	}
 
 	// prompt for confirmation
-	if opts.AutoApprove || opts.DryRun != "" {
+	if opts.AutoApprove == AutoApproveAlways || opts.DryRun != "" {
+		// Skip approval
 	} else if err := confirmPrompt("Deleting from", l.Env.Spec.Namespace, kube.Info()); err != nil {
 		return err
 	}
 
 	return kube.Delete(l.Resources, kubernetes.DeleteOpts{
-		Force:    opts.Force,
-		Validate: opts.Validate,
-		DryRun:   opts.DryRun,
+		Force:  opts.Force,
+		DryRun: opts.DryRun,
 	})
 }
 


Test patch:
diff --git a/cmd/tk/workflow_test.go b/cmd/tk/workflow_test.go
new file mode 100644
index 00000000..139df3f1
--- /dev/null
+++ b/cmd/tk/workflow_test.go
@@ -0,0 +1,65 @@
+package main
+
+import (
+	"errors"
+	"testing"
+
+	"github.com/grafana/tanka/pkg/tanka"
+	"github.com/stretchr/testify/assert"
+)
+
+func TestValidateAutoApprove(t *testing.T) {
+	for _, tc := range []struct {
+		name                  string
+		autoApproveDeprecated bool
+		autoApproveString     string
+		expected              tanka.AutoApproveSetting
+		expectErr             error
+	}{
+		{
+			name:     "default",
+			expected: tanka.AutoApproveNever,
+		},
+		{
+			name:                  "deprecated bool set",
+			autoApproveDeprecated: true,
+			expected:              tanka.AutoApproveAlways,
+		},
+		{
+			name:                  "both values set",
+			autoApproveDeprecated: true,
+			autoApproveString:     "never",
+			expectErr:             errors.New("--dangerous-auto-approve and --auto-approve are mutually exclusive"),
+		},
+		{
+			name:              "always",
+			autoApproveString: "always",
+			expected:          tanka.AutoApproveAlways,
+		},
+		{
+			name:              "never",
+			autoApproveString: "never",
+			expected:          tanka.AutoApproveNever,
+		},
+		{
+			name:              "if-no-changes",
+			autoApproveString: "if-no-changes",
+			expected:          tanka.AutoApproveNoChanges,
+		},
+		{
+			name:              "bad value",
+			autoApproveString: "blabla",
+			expectErr:         errors.New("invalid value for --auto-approve: blabla"),
+		},
+	} {
+		t.Run(tc.name, func(t *testing.T) {
+			result, err := validateAutoApprove(tc.autoApproveDeprecated, tc.autoApproveString)
+			if tc.expectErr != nil {
+				assert.EqualError(t, err, tc.expectErr.Error())
+			} else {
+				assert.NoError(t, err)
+				assert.Equal(t, tc.expected, result)
+			}
+		})
+	}
+}