Merge pull request #39401 from AdoHe/delete_refactor

Automatic merge from submit-queue (batch tested with PRs 39806, 39887, 39401)

refactor delete to remove cobra dependency

FYI. As part of CLI Q1 roadmap, we would like to reduce the dependency of Cobra from actual commands implementations. In this PR, I tried to refactor delete command to achieve this. @kubernetes/sig-cli-misc a quick review is quite welcome, and I am just working on more PRs.

pkg/kubectl/cmd/delete.go

@@ -89,8 +89,33 @@ var (
 		kubectl delete pods --all`)
 )
 
+type DeleteOptions struct {
+	resource.FilenameOptions
+
+	Selector        string
+	DeleteAll       bool
+	IgnoreNotFound  bool
+	Cascade         bool
+	DeleteNow       bool
+	ForceDeletion   bool
+	WaitForDeletion bool
+
+	GracePeriod int
+	Timeout     time.Duration
+
+	Include3rdParty bool
+	Output          string
+
+	Mapper meta.RESTMapper
+	Result *resource.Result
+
+	f      cmdutil.Factory
+	Out    io.Writer
+	ErrOut io.Writer
+}
+
 func NewCmdDelete(f cmdutil.Factory, out, errOut io.Writer) *cobra.Command {
-	options := &resource.FilenameOptions{}
+	options := &DeleteOptions{}
 
 	// retrieve a list of handled resources from printer as valid args
 	validArgs, argAliases := []string{}, []string{}
@@ -110,44 +135,53 @@ func NewCmdDelete(f cmdutil.Factory, out, errOut io.Writer) *cobra.Command {
 		Example: delete_example,
 		Run: func(cmd *cobra.Command, args []string) {
 			cmdutil.CheckErr(cmdutil.ValidateOutputArgs(cmd))
-			err := RunDelete(f, out, errOut, cmd, args, options)
-			cmdutil.CheckErr(err)
+			if err := options.Complete(f, out, errOut, args); err != nil {
+				cmdutil.CheckErr(err)
+			}
+			if err := options.Validate(f, cmd); err != nil {
+				cmdutil.CheckErr(cmdutil.UsageError(cmd, err.Error()))
+			}
+			if err := options.RunDelete(); err != nil {
+				cmdutil.CheckErr(err)
+			}
 		},
 		SuggestFor: []string{"rm"},
 		ValidArgs:  validArgs,
 		ArgAliases: argAliases,
 	}
 	usage := "containing the resource to delete."
-	cmdutil.AddFilenameOptionFlags(cmd, options, usage)
-	cmd.Flags().StringP("selector", "l", "", "Selector (label query) to filter on.")
-	cmd.Flags().Bool("all", false, "[-all] to select all the specified resources.")
-	cmd.Flags().Bool("ignore-not-found", false, "Treat \"resource not found\" as a successful delete. Defaults to \"true\" when --all is specified.")
-	cmd.Flags().Bool("cascade", true, "If true, cascade the deletion of the resources managed by this resource (e.g. Pods created by a ReplicationController).  Default true.")
-	cmd.Flags().Int("grace-period", -1, "Period of time in seconds given to the resource to terminate gracefully. Ignored if negative.")
-	cmd.Flags().Bool("now", false, "If true, resources are signaled for immediate shutdown (same as --grace-period=1).")
-	cmd.Flags().Bool("force", false, "Immediate deletion of some resources may result in inconsistency or data loss and requires confirmation.")
-	cmd.Flags().Duration("timeout", 0, "The length of time to wait before giving up on a delete, zero means determine a timeout from the size of the object")
-	cmdutil.AddOutputFlagsForMutation(cmd)
-	cmdutil.AddInclude3rdPartyFlags(cmd)
+	cmdutil.AddFilenameOptionFlags(cmd, &options.FilenameOptions, usage)
+	cmd.Flags().StringVarP(&options.Selector, "selector", "l", "", "Selector (label query) to filter on.")
+	cmd.Flags().BoolVar(&options.DeleteAll, "all", false, "[-all] to select all the specified resources.")
+	cmd.Flags().BoolVar(&options.IgnoreNotFound, "ignore-not-found", false, "Treat \"resource not found\" as a successful delete. Defaults to \"true\" when --all is specified.")
+	cmd.Flags().BoolVar(&options.Cascade, "cascade", true, "If true, cascade the deletion of the resources managed by this resource (e.g. Pods created by a ReplicationController).  Default true.")
+	cmd.Flags().IntVar(&options.GracePeriod, "grace-period", -1, "Period of time in seconds given to the resource to terminate gracefully. Ignored if negative.")
+	cmd.Flags().BoolVar(&options.DeleteNow, "now", false, "If true, resources are signaled for immediate shutdown (same as --grace-period=1).")
+	cmd.Flags().BoolVar(&options.ForceDeletion, "force", false, "Immediate deletion of some resources may result in inconsistency or data loss and requires confirmation.")
+	cmd.Flags().DurationVar(&options.Timeout, "timeout", 0, "The length of time to wait before giving up on a delete, zero means determine a timeout from the size of the object")
+	cmdutil.AddOutputVarFlagsForMutation(cmd, &options.Output)
+	cmdutil.AddInclude3rdPartyVarFlags(cmd, &options.Include3rdParty)
 	return cmd
 }
 
-func RunDelete(f cmdutil.Factory, out, errOut io.Writer, cmd *cobra.Command, args []string, options *resource.FilenameOptions) error {
+func (o *DeleteOptions) Complete(f cmdutil.Factory, out, errOut io.Writer, args []string) error {
 	cmdNamespace, enforceNamespace, err := f.DefaultNamespace()
 	if err != nil {
 		return err
 	}
-	deleteAll := cmdutil.GetFlagBool(cmd, "all")
+
+	// Set up client based support.
 	mapper, typer, err := f.UnstructuredObject()
 	if err != nil {
 		return err
 	}
+	o.Mapper = mapper
 	r := resource.NewBuilder(mapper, typer, resource.ClientMapperFunc(f.UnstructuredClientForMapping), unstructured.UnstructuredJSONScheme).
 		ContinueOnError().
 		NamespaceParam(cmdNamespace).DefaultNamespace().
-		FilenameParam(enforceNamespace, options).
-		SelectorParam(cmdutil.GetFlagString(cmd, "selector")).
-		SelectAllParam(deleteAll).
+		FilenameParam(enforceNamespace, &o.FilenameOptions).
+		SelectorParam(o.Selector).
+		SelectAllParam(o.DeleteAll).
 		ResourceTypeOrNameArgs(false, args...).RequireObject(false).
 		Flatten().
 		Do()
@@ -155,9 +189,18 @@ func RunDelete(f cmdutil.Factory, out, errOut io.Writer, cmd *cobra.Command, arg
 	if err != nil {
 		return err
 	}
+	o.Result = r
 
-	ignoreNotFound := cmdutil.GetFlagBool(cmd, "ignore-not-found")
-	if deleteAll {
+	o.f = f
+	// Set up writer
+	o.Out = out
+	o.ErrOut = errOut
+
+	return nil
+}
+
+func (o *DeleteOptions) Validate(f cmdutil.Factory, cmd *cobra.Command) error {
+	if o.DeleteAll {
 		f := cmd.Flags().Lookup("ignore-not-found")
 		// The flag should never be missing
 		if f == nil {
@@ -165,37 +208,36 @@ func RunDelete(f cmdutil.Factory, out, errOut io.Writer, cmd *cobra.Command, arg
 		}
 		// If the user didn't explicitly set the option, default to ignoring NotFound errors when used with --all
 		if !f.Changed {
-			ignoreNotFound = true
+			o.IgnoreNotFound = true
 		}
 	}
-
-	gracePeriod := cmdutil.GetFlagInt(cmd, "grace-period")
-	force := cmdutil.GetFlagBool(cmd, "force")
-	if cmdutil.GetFlagBool(cmd, "now") {
-		if gracePeriod != -1 {
+	if o.DeleteNow {
+		if o.GracePeriod != -1 {
 			return fmt.Errorf("--now and --grace-period cannot be specified together")
 		}
-		gracePeriod = 1
+		o.GracePeriod = 1
 	}
-	wait := false
-	if gracePeriod == 0 {
-		if force {
-			fmt.Fprintf(errOut, "warning: Immediate deletion does not wait for confirmation that the running resource has been terminated. The resource may continue to run on the cluster indefinitely.\n")
+	if o.GracePeriod == 0 {
+		if o.ForceDeletion {
+			fmt.Fprintf(o.ErrOut, "warning: Immediate deletion does not wait for confirmation that the running resource has been terminated. The resource may continue to run on the cluster indefinitely.\n")
 		} else {
 			// To preserve backwards compatibility, but prevent accidental data loss, we convert --grace-period=0
 			// into --grace-period=1 and wait until the object is successfully deleted. Users may provide --force
 			// to bypass this wait.
-			wait = true
-			gracePeriod = 1
+			o.WaitForDeletion = true
+			o.GracePeriod = 1
 		}
 	}
+	return nil
+}
 
-	shortOutput := cmdutil.GetFlagString(cmd, "output") == "name"
+func (o *DeleteOptions) RunDelete() error {
+	shortOutput := o.Output == "name"
 	// By default use a reaper to delete all related resources.
-	if cmdutil.GetFlagBool(cmd, "cascade") {
-		return ReapResult(r, f, out, cmdutil.GetFlagBool(cmd, "cascade"), ignoreNotFound, cmdutil.GetFlagDuration(cmd, "timeout"), gracePeriod, wait, shortOutput, mapper, false)
+	if o.Cascade {
+		return ReapResult(o.Result, o.f, o.Out, true, o.IgnoreNotFound, o.Timeout, o.GracePeriod, o.WaitForDeletion, shortOutput, o.Mapper, false)
 	}
-	return DeleteResult(r, out, ignoreNotFound, shortOutput, mapper)
+	return DeleteResult(o.Result, o.Out, o.IgnoreNotFound, shortOutput, o.Mapper)
 }
 
 func ReapResult(r *resource.Result, f cmdutil.Factory, out io.Writer, isDefaultDelete, ignoreNotFound bool, timeout time.Duration, gracePeriod int, waitForDeletion, shortOutput bool, mapper meta.RESTMapper, quiet bool) error {

pkg/kubectl/cmd/delete_test.go

@@ -227,12 +227,19 @@ func TestDeleteObjectNotFound(t *testing.T) {
 	tf.Namespace = "test"
 	buf, errBuf := bytes.NewBuffer([]byte{}), bytes.NewBuffer([]byte{})
 
-	cmd := NewCmdDelete(f, buf, errBuf)
-	options := &resource.FilenameOptions{}
-	options.Filenames = []string{"../../../examples/guestbook/legacy/redis-master-controller.yaml"}
-	cmd.Flags().Set("cascade", "false")
-	cmd.Flags().Set("output", "name")
-	err := RunDelete(f, buf, errBuf, cmd, []string{}, options)
+	options := &DeleteOptions{
+		FilenameOptions: resource.FilenameOptions{
+			Filenames: []string{"../../../examples/guestbook/legacy/redis-master-controller.yaml"},
+		},
+		GracePeriod: -1,
+		Cascade:     false,
+		Output:      "name",
+	}
+	err := options.Complete(f, buf, errBuf, []string{})
+	if err != nil {
+		t.Errorf("unexpected error: %v", err)
+	}
+	err = options.RunDelete()
 	if err == nil || !errors.IsNotFound(err) {
 		t.Errorf("unexpected error: expected NotFound, got %v", err)
 	}
@@ -296,14 +303,20 @@ func TestDeleteAllNotFound(t *testing.T) {
 	tf.Namespace = "test"
 	buf, errBuf := bytes.NewBuffer([]byte{}), bytes.NewBuffer([]byte{})
 
-	cmd := NewCmdDelete(f, buf, errBuf)
-	cmd.Flags().Set("all", "true")
-	cmd.Flags().Set("cascade", "false")
 	// Make sure we can explicitly choose to fail on NotFound errors, even with --all
-	cmd.Flags().Set("ignore-not-found", "false")
-	cmd.Flags().Set("output", "name")
-
-	err := RunDelete(f, buf, errBuf, cmd, []string{"services"}, &resource.FilenameOptions{})
+	options := &DeleteOptions{
+		FilenameOptions: resource.FilenameOptions{},
+		GracePeriod:     -1,
+		Cascade:         false,
+		DeleteAll:       true,
+		IgnoreNotFound:  false,
+		Output:          "name",
+	}
+	err := options.Complete(f, buf, errBuf, []string{"services"})
+	if err != nil {
+		t.Errorf("unexpected error: %v", err)
+	}
+	err = options.RunDelete()
 	if err == nil || !errors.IsNotFound(err) {
 		t.Errorf("unexpected error: expected NotFound, got %v", err)
 	}
@@ -405,12 +418,19 @@ func TestDeleteMultipleObjectContinueOnMissing(t *testing.T) {
 	tf.Namespace = "test"
 	buf, errBuf := bytes.NewBuffer([]byte{}), bytes.NewBuffer([]byte{})
 
-	cmd := NewCmdDelete(f, buf, errBuf)
-	options := &resource.FilenameOptions{}
-	options.Filenames = []string{"../../../examples/guestbook/legacy/redis-master-controller.yaml", "../../../examples/guestbook/frontend-service.yaml"}
-	cmd.Flags().Set("cascade", "false")
-	cmd.Flags().Set("output", "name")
-	err := RunDelete(f, buf, errBuf, cmd, []string{}, options)
+	options := &DeleteOptions{
+		FilenameOptions: resource.FilenameOptions{
+			Filenames: []string{"../../../examples/guestbook/legacy/redis-master-controller.yaml", "../../../examples/guestbook/frontend-service.yaml"},
+		},
+		GracePeriod: -1,
+		Cascade:     false,
+		Output:      "name",
+	}
+	err := options.Complete(f, buf, errBuf, []string{})
+	if err != nil {
+		t.Errorf("unexpected error: %v", err)
+	}
+	err = options.RunDelete()
 	if err == nil || !errors.IsNotFound(err) {
 		t.Errorf("unexpected error: expected NotFound, got %v", err)
 	}
@@ -533,7 +553,6 @@ func TestDeleteMultipleSelector(t *testing.T) {
 func TestResourceErrors(t *testing.T) {
 	testCases := map[string]struct {
 		args  []string
-		flags map[string]string
 		errFn func(error) bool
 	}{
 		"no args": {
@@ -562,17 +581,18 @@ func TestResourceErrors(t *testing.T) {
 
 		buf, errBuf := bytes.NewBuffer([]byte{}), bytes.NewBuffer([]byte{})
 
-		cmd := NewCmdDelete(f, buf, errBuf)
-		cmd.SetOutput(buf)
-
-		for k, v := range testCase.flags {
-			cmd.Flags().Set(k, v)
+		options := &DeleteOptions{
+			FilenameOptions: resource.FilenameOptions{},
+			GracePeriod:     -1,
+			Cascade:         false,
+			Output:          "name",
 		}
-		err := RunDelete(f, buf, errBuf, cmd, testCase.args, &resource.FilenameOptions{})
+		err := options.Complete(f, buf, errBuf, testCase.args)
 		if !testCase.errFn(err) {
 			t.Errorf("%s: unexpected error: %v", k, err)
 			continue
 		}
+
 		if tf.Printer.(*testPrinter).Objects != nil {
 			t.Errorf("unexpected print to default printer")
 		}

pkg/kubectl/cmd/util/helpers.go

@@ -557,6 +557,11 @@ func ShouldRecord(cmd *cobra.Command, info *resource.Info) bool {
 	return GetRecordFlag(cmd) || (ContainsChangeCause(info) && !cmd.Flags().Changed("record"))
 }
 
+func AddInclude3rdPartyVarFlags(cmd *cobra.Command, include3rdParty *bool) {
+	cmd.Flags().BoolVar(include3rdParty, "include-extended-apis", true, "If true, include definitions of new APIs via calls to the API server. [default true]")
+	cmd.Flags().MarkDeprecated("include-extended-apis", "No longer required.")
+}
+
 func AddInclude3rdPartyFlags(cmd *cobra.Command) {
 	cmd.Flags().Bool("include-extended-apis", true, "If true, include definitions of new APIs via calls to the API server. [default true]")
 	cmd.Flags().MarkDeprecated("include-extended-apis", "No longer required.")

pkg/kubectl/cmd/util/printing.go

@@ -46,6 +46,11 @@ func AddOutputFlagsForMutation(cmd *cobra.Command) {
 	cmd.Flags().StringP("output", "o", "", "Output mode. Use \"-o name\" for shorter output (resource/name).")
 }
 
+// AddOutputVarFlagsForMutation adds output related flags to a command. Used by mutations only.
+func AddOutputVarFlagsForMutation(cmd *cobra.Command, output *string) {
+	cmd.Flags().StringVarP(output, "output", "o", "", "Output mode. Use \"-o name\" for shorter output (resource/name).")
+}
+
 // AddOutputFlags adds output related flags to a command.
 func AddOutputFlags(cmd *cobra.Command) {
 	cmd.Flags().StringP("output", "o", "", "Output format. One of: json|yaml|wide|name|custom-columns=...|custom-columns-file=...|go-template=...|go-template-file=...|jsonpath=...|jsonpath-file=... See custom columns [http://kubernetes.io/docs/user-guide/kubectl-overview/#custom-columns], golang template [http://golang.org/pkg/text/template/#pkg-overview] and jsonpath template [http://kubernetes.io/docs/user-guide/jsonpath].")