Split options from flags for annotate command (#112817)

* split flags from options

Signed-off-by: Noah Ispas (iamNoah1) <noahispas@gmail.com>

* CR from Arda

Signed-off-by: Noah Ispas (iamNoah1) <noahispas@gmail.com>

Signed-off-by: Noah Ispas (iamNoah1) <noahispas@gmail.com>

staging/src/k8s.io/kubectl/pkg/cmd/annotate/annotate.go

@@ -43,42 +43,73 @@ import (
 	"k8s.io/kubectl/pkg/util/templates"
 )
 
-// AnnotateOptions have the data required to perform the annotate operation
+// AnnotateFlags directly reflect the information that CLI is gathering via flags.  They will be converted to Options, which
-type AnnotateOptions struct {
+// reflect the runtime requirements for the command.  This structure reduces the transformation to wiring and makes
-	PrintFlags *genericclioptions.PrintFlags
+// the logic itself easy to unit test
-	PrintObj   printers.ResourcePrinterFunc
+type AnnotateFlags struct {
-
-	// Filename options
-	resource.FilenameOptions
-	RecordFlags *genericclioptions.RecordFlags
-
 	// Common user flags
-	overwrite       bool
+	All           bool
-	list            bool
+	AllNamespaces bool
-	local           bool
-	dryRunStrategy  cmdutil.DryRunStrategy
-	dryRunVerifier  *resource.QueryParamVerifier
-	fieldManager    string
-	all             bool
-	allNamespaces   bool
-	resourceVersion string
-	selector        string
-	fieldSelector   string
-	outputFormat    string
 
-	// results of arg parsing
+	DryRunStrategy cmdutil.DryRunStrategy
-	resources                    []string
+	DryRunVerifier *resource.QueryParamVerifier
-	newAnnotations               map[string]string
+	FieldManager   string
-	removeAnnotations            []string
+	FieldSelector  string
-	Recorder                     genericclioptions.Recorder
+	resource.FilenameOptions
-	namespace                    string
+	List            bool
-	enforceNamespace             bool
+	Local           bool
-	builder                      *resource.Builder
+	OutputFormat    string
-	unstructuredClientForMapping func(mapping *meta.RESTMapping) (resource.RESTClient, error)
+	overwrite       bool
+	PrintFlags      *genericclioptions.PrintFlags
+	RecordFlags     *genericclioptions.RecordFlags
+	resourceVersion string
+	Selector        string
 
 	genericclioptions.IOStreams
 }
 
+// NewAnnotateFlags returns a default AnnotateFlags
+func NewAnnotateFlags(streams genericclioptions.IOStreams) *AnnotateFlags {
+	return &AnnotateFlags{
+		PrintFlags:  genericclioptions.NewPrintFlags("annotate").WithTypeSetter(scheme.Scheme),
+		RecordFlags: genericclioptions.NewRecordFlags(),
+		IOStreams:   streams,
+	}
+}
+
+// AnnotateOptions have the data required to perform the annotate operation
+type AnnotateOptions struct {
+	all           bool
+	allNamespaces bool
+
+	builder        *resource.Builder
+	dryRunStrategy cmdutil.DryRunStrategy
+	dryRunVerifier *resource.QueryParamVerifier
+
+	enforceNamespace bool
+	fieldSelector    string
+	fieldManager     string
+	resource.FilenameOptions
+
+	genericclioptions.IOStreams
+
+	list           bool
+	local          bool
+	namespace      string
+	newAnnotations map[string]string
+	overwrite      bool
+
+	PrintObj printers.ResourcePrinterFunc
+
+	Recorder          genericclioptions.Recorder
+	resources         []string
+	resourceVersion   string
+	removeAnnotations []string
+	selector          string
+
+	unstructuredClientForMapping func(mapping *meta.RESTMapping) (resource.RESTClient, error)
+}
+
 var (
 	annotateLong = templates.LongDesc(i18n.T(`
 		Update the annotations on one or more resources.
@@ -114,20 +145,9 @@ var (
     kubectl annotate pods foo description-`))
 )
 
-// NewAnnotateOptions creates the options for annotate
-func NewAnnotateOptions(ioStreams genericclioptions.IOStreams) *AnnotateOptions {
-	return &AnnotateOptions{
-		PrintFlags: genericclioptions.NewPrintFlags("annotated").WithTypeSetter(scheme.Scheme),
-
-		RecordFlags: genericclioptions.NewRecordFlags(),
-		Recorder:    genericclioptions.NoopRecorder{},
-		IOStreams:   ioStreams,
-	}
-}
-
 // NewCmdAnnotate creates the `annotate` command
-func NewCmdAnnotate(parent string, f cmdutil.Factory, ioStreams genericclioptions.IOStreams) *cobra.Command {
+func NewCmdAnnotate(parent string, f cmdutil.Factory, streams genericclioptions.IOStreams) *cobra.Command {
-	o := NewAnnotateOptions(ioStreams)
+	flags := NewAnnotateFlags(streams)
 
 	cmd := &cobra.Command{
 		Use:                   "annotate [--overwrite] (-f FILENAME | TYPE NAME) KEY_1=VAL_1 ... KEY_N=VAL_N [--resource-version=version]",
@@ -137,115 +157,138 @@ func NewCmdAnnotate(parent string, f cmdutil.Factory, ioStreams genericclioption
 		Example:               annotateExample,
 		ValidArgsFunction:     completion.ResourceTypeAndNameCompletionFunc(f),
 		Run: func(cmd *cobra.Command, args []string) {
-			cmdutil.CheckErr(o.Complete(f, cmd, args))
+			o, err := flags.ToOptions(f, cmd, args)
-			cmdutil.CheckErr(o.Validate())
+			cmdutil.CheckErr(err)
 			cmdutil.CheckErr(o.RunAnnotate())
 		},
 	}
 
-	// bind flag structs
+	flags.AddFlags(cmd, streams)
-	o.RecordFlags.AddFlags(cmd)
-	o.PrintFlags.AddFlags(cmd)
-
-	cmd.Flags().BoolVar(&o.overwrite, "overwrite", o.overwrite, "If true, allow annotations to be overwritten, otherwise reject annotation updates that overwrite existing annotations.")
-	cmd.Flags().BoolVar(&o.list, "list", o.list, "If true, display the annotations for a given resource.")
-	cmd.Flags().BoolVar(&o.local, "local", o.local, "If true, annotation will NOT contact api-server but run locally.")
-	cmd.Flags().StringVar(&o.fieldSelector, "field-selector", o.fieldSelector, "Selector (field query) to filter on, supports '=', '==', and '!='.(e.g. --field-selector key1=value1,key2=value2). The server only supports a limited number of field queries per type.")
-	cmd.Flags().BoolVar(&o.all, "all", o.all, "Select all resources, in the namespace of the specified resource types.")
-	cmd.Flags().BoolVarP(&o.allNamespaces, "all-namespaces", "A", o.allNamespaces, "If true, check the specified action in all namespaces.")
-	cmd.Flags().StringVar(&o.resourceVersion, "resource-version", o.resourceVersion, i18n.T("If non-empty, the annotation update will only succeed if this is the current resource-version for the object. Only valid when specifying a single resource."))
-	usage := "identifying the resource to update the annotation"
-	cmdutil.AddFilenameOptionFlags(cmd, &o.FilenameOptions, usage)
-	cmdutil.AddDryRunFlag(cmd)
-	cmdutil.AddFieldManagerFlagVar(cmd, &o.fieldManager, "kubectl-annotate")
-	cmdutil.AddLabelSelectorFlagVar(cmd, &o.selector)
 
 	return cmd
 }
 
-// Complete adapts from the command line args and factory to the data required.
+// AddFlags registers flags for a cli.
-func (o *AnnotateOptions) Complete(f cmdutil.Factory, cmd *cobra.Command, args []string) error {
+func (flags *AnnotateFlags) AddFlags(cmd *cobra.Command, ioStreams genericclioptions.IOStreams) {
+	flags.PrintFlags.AddFlags(cmd)
+	flags.RecordFlags.AddFlags(cmd)
+
+	cmdutil.AddDryRunFlag(cmd)
+
+	usage := "identifying the resource to update the annotation"
+	cmdutil.AddFilenameOptionFlags(cmd, &flags.FilenameOptions, usage)
+	cmdutil.AddFieldManagerFlagVar(cmd, &flags.FieldManager, "kubectl-annotate")
+	cmdutil.AddLabelSelectorFlagVar(cmd, &flags.Selector)
+
+	cmd.Flags().BoolVar(&flags.overwrite, "overwrite", flags.overwrite, "If true, allow annotations to be overwritten, otherwise reject annotation updates that overwrite existing annotations.")
+	cmd.Flags().BoolVar(&flags.List, "list", flags.List, "If true, display the annotations for a given resource.")
+	cmd.Flags().BoolVar(&flags.Local, "local", flags.Local, "If true, annotation will NOT contact api-server but run locally.")
+	cmd.Flags().StringVar(&flags.FieldSelector, "field-selector", flags.FieldSelector, "Selector (field query) to filter on, supports '=', '==', and '!='.(e.g. --field-selector key1=value1,key2=value2). The server only supports a limited number of field queries per type.")
+	cmd.Flags().BoolVar(&flags.All, "all", flags.All, "Select all resources, in the namespace of the specified resource types.")
+	cmd.Flags().BoolVarP(&flags.AllNamespaces, "all-namespaces", "A", flags.AllNamespaces, "If true, check the specified action in all namespaces.")
+	cmd.Flags().StringVar(&flags.resourceVersion, "resource-version", flags.resourceVersion, i18n.T("If non-empty, the annotation update will only succeed if this is the current resource-version for the object. Only valid when specifying a single resource."))
+}
+
+// ToOptions converts from CLI inputs to runtime inputs.
+func (flags *AnnotateFlags) ToOptions(f cmdutil.Factory, cmd *cobra.Command, args []string) (*AnnotateOptions, error) {
+	options := &AnnotateOptions{
+		all:             flags.All,
+		allNamespaces:   flags.AllNamespaces,
+		FilenameOptions: flags.FilenameOptions,
+		fieldSelector:   flags.FieldSelector,
+		fieldManager:    flags.FieldManager,
+		IOStreams:       flags.IOStreams,
+		local:           flags.Local,
+		list:            flags.List,
+		overwrite:       flags.overwrite,
+		resourceVersion: flags.resourceVersion,
+		Recorder:        genericclioptions.NoopRecorder{},
+		selector:        flags.Selector,
+	}
+
 	var err error
 
-	o.RecordFlags.Complete(cmd)
+	flags.RecordFlags.Complete(cmd)
-	o.Recorder, err = o.RecordFlags.ToRecorder()
+	options.Recorder, err = flags.RecordFlags.ToRecorder()
 	if err != nil {
-		return err
+		return nil, err
 	}
 
-	o.outputFormat = cmdutil.GetFlagString(cmd, "output")
+	options.dryRunStrategy, err = cmdutil.GetDryRunStrategy(cmd)
-	o.dryRunStrategy, err = cmdutil.GetDryRunStrategy(cmd)
 	if err != nil {
-		return err
+		return nil, err
 	}
+
 	dynamicClient, err := f.DynamicClient()
 	if err != nil {
-		return err
+		return nil, err
 	}
-	o.dryRunVerifier = resource.NewQueryParamVerifier(dynamicClient, f.OpenAPIGetter(), resource.QueryParamDryRun)
 
-	cmdutil.PrintFlagsWithDryRunStrategy(o.PrintFlags, o.dryRunStrategy)
+	options.dryRunVerifier = resource.NewQueryParamVerifier(dynamicClient, f.OpenAPIGetter(), resource.QueryParamDryRun)
-	printer, err := o.PrintFlags.ToPrinter()
+
+	cmdutil.PrintFlagsWithDryRunStrategy(flags.PrintFlags, options.dryRunStrategy)
+	printer, err := flags.PrintFlags.ToPrinter()
 	if err != nil {
-		return err
+		return nil, err
 	}
-	o.PrintObj = func(obj runtime.Object, out io.Writer) error {
+	options.PrintObj = func(obj runtime.Object, out io.Writer) error {
 		return printer.PrintObj(obj, out)
 	}
 
-	if o.list && len(o.outputFormat) > 0 {
+	options.namespace, options.enforceNamespace, err = f.ToRawKubeConfigLoader().Namespace()
-		return fmt.Errorf("--list and --output may not be specified together")
-	}
-
-	o.namespace, o.enforceNamespace, err = f.ToRawKubeConfigLoader().Namespace()
 	if err != nil {
-		return err
+		return nil, err
 	}
-	o.builder = f.NewBuilder()
+	options.builder = f.NewBuilder()
-	o.unstructuredClientForMapping = f.UnstructuredClientForMapping
+	options.unstructuredClientForMapping = f.UnstructuredClientForMapping
 
 	// retrieves resource and annotation args from args
 	// also checks args to verify that all resources are specified before annotations
 	resources, annotationArgs, err := cmdutil.GetResourcesAndPairs(args, "annotation")
 	if err != nil {
-		return err
+		return nil, err
 	}
-	o.resources = resources
+	options.resources = resources
-	o.newAnnotations, o.removeAnnotations, err = parseAnnotations(annotationArgs)
+	options.newAnnotations, options.removeAnnotations, err = parseAnnotations(annotationArgs)
 	if err != nil {
-		return err
+		return nil, err
 	}
 
-	return nil
+	// Checks the options and flags to see if there is sufficient information run the command.
-}
+	if flags.List && len(flags.OutputFormat) > 0 {
+		return nil, fmt.Errorf("--list and --output may not be specified together")
+	}
+	if flags.All && len(flags.Selector) > 0 {
+		return nil, fmt.Errorf("cannot set --all and --selector at the same time")
+	}
+	if flags.All && len(flags.FieldSelector) > 0 {
+		return nil, fmt.Errorf("cannot set --all and --field-selector at the same time")
+	}
 
-// Validate checks to the AnnotateOptions to see if there is sufficient information run the command.
+	if !flags.Local {
-func (o AnnotateOptions) Validate() error {
+		if len(options.resources) < 1 && cmdutil.IsFilenameSliceEmpty(flags.Filenames, flags.Kustomize) {
-	if o.all && len(o.selector) > 0 {
+			return nil, fmt.Errorf("one or more resources must be specified as <resource> <name> or <resource>/<name>")
-		return fmt.Errorf("cannot set --all and --selector at the same time")
-	}
-	if o.all && len(o.fieldSelector) > 0 {
-		return fmt.Errorf("cannot set --all and --field-selector at the same time")
-	}
-	if !o.local {
-		if len(o.resources) < 1 && cmdutil.IsFilenameSliceEmpty(o.Filenames, o.Kustomize) {
-			return fmt.Errorf("one or more resources must be specified as <resource> <name> or <resource>/<name>")
 		}
 	} else {
-		if o.dryRunStrategy == cmdutil.DryRunServer {
+		if options.dryRunStrategy == cmdutil.DryRunServer {
-			return fmt.Errorf("cannot specify --local and --dry-run=server - did you mean --dry-run=client?")
+			return nil, fmt.Errorf("cannot specify --local and --dry-run=server - did you mean --dry-run=client?")
 		}
-		if len(o.resources) > 0 {
+		if len(options.resources) > 0 {
-			return fmt.Errorf("can only use local files by -f rsrc.yaml or --filename=rsrc.json when --local=true is set")
+			return nil, fmt.Errorf("can only use local files by -f rsrc.yaml or --filename=rsrc.json when --local=true is set")
 		}
-		if cmdutil.IsFilenameSliceEmpty(o.Filenames, o.Kustomize) {
+		if cmdutil.IsFilenameSliceEmpty(flags.Filenames, flags.Kustomize) {
-			return fmt.Errorf("one or more files must be specified as -f rsrc.yaml or --filename=rsrc.json")
+			return nil, fmt.Errorf("one or more files must be specified as -f rsrc.yaml or --filename=rsrc.json")
 		}
 	}
-	if len(o.newAnnotations) < 1 && len(o.removeAnnotations) < 1 && !o.list {
+	if len(options.newAnnotations) < 1 && len(options.removeAnnotations) < 1 && !flags.List {
-		return fmt.Errorf("at least one annotation update is required")
+		return nil, fmt.Errorf("at least one annotation update is required")
 	}
-	return validateAnnotations(o.removeAnnotations, o.newAnnotations)
+	err = validateAnnotations(options.removeAnnotations, options.newAnnotations)
+	if err != nil {
+		return nil, err
+	}
+
+	return options, nil
 }
 
 // RunAnnotate does the work

staging/src/k8s.io/kubectl/pkg/cmd/annotate/annotate_test.go

@@ -445,11 +445,8 @@ func TestAnnotateErrors(t *testing.T) {
 			cmd.SetOut(bufOut)
 			cmd.SetErr(bufOut)
 
-			options := NewAnnotateOptions(iostreams)
+			flags := NewAnnotateFlags(iostreams)
-			err := options.Complete(tf, cmd, testCase.args)
+			_, err := flags.ToOptions(tf, cmd, testCase.args)
-			if err == nil {
-				err = options.Validate()
-			}
 			if !testCase.errFn(err) {
 				t.Errorf("%s: unexpected error: %v", k, err)
 				return
@@ -505,12 +502,11 @@ func TestAnnotateObject(t *testing.T) {
 	cmd := NewCmdAnnotate("kubectl", tf, iostreams)
 	cmd.SetOut(bufOut)
 	cmd.SetErr(bufOut)
-	options := NewAnnotateOptions(iostreams)
+	flags := NewAnnotateFlags(iostreams)
 	args := []string{"pods/foo", "a=b", "c-"}
-	if err := options.Complete(tf, cmd, args); err != nil {
+
-		t.Fatalf("unexpected error: %v", err)
+	options, err := flags.ToOptions(tf, cmd, args)
-	}
+	if err != nil {
-	if err := options.Validate(); err != nil {
 		t.Fatalf("unexpected error: %v", err)
 	}
 	if err := options.RunAnnotate(); err != nil {
@@ -572,13 +568,13 @@ func TestAnnotateResourceVersion(t *testing.T) {
 	cmd := NewCmdAnnotate("kubectl", tf, iostreams)
 	cmd.SetOut(bufOut)
 	cmd.SetErr(bufOut)
-	options := NewAnnotateOptions(iostreams)
+	//options := NewAnnotateOptions(iostreams)
-	options.resourceVersion = "10"
+	flags := NewAnnotateFlags(iostreams)
+	flags.resourceVersion = "10"
 	args := []string{"pods/foo", "a=b"}
-	if err := options.Complete(tf, cmd, args); err != nil {
+
-		t.Fatalf("unexpected error: %v", err)
+	options, err := flags.ToOptions(tf, cmd, args)
-	}
+	if err != nil {
-	if err := options.Validate(); err != nil {
 		t.Fatalf("unexpected error: %v", err)
 	}
 	if err := options.RunAnnotate(); err != nil {
@@ -627,15 +623,15 @@ func TestAnnotateObjectFromFile(t *testing.T) {
 	cmd := NewCmdAnnotate("kubectl", tf, iostreams)
 	cmd.SetOut(bufOut)
 	cmd.SetErr(bufOut)
-	options := NewAnnotateOptions(iostreams)
+	flags := NewAnnotateFlags(iostreams)
-	options.Filenames = []string{"../../../testdata/controller.yaml"}
+	flags.Filenames = []string{"../../../testdata/controller.yaml"}
 	args := []string{"a=b", "c-"}
-	if err := options.Complete(tf, cmd, args); err != nil {
+
-		t.Fatalf("unexpected error: %v", err)
+	options, err := flags.ToOptions(tf, cmd, args)
-	}
+	if err != nil {
-	if err := options.Validate(); err != nil {
 		t.Fatalf("unexpected error: %v", err)
 	}
+
 	if err := options.RunAnnotate(); err != nil {
 		t.Fatalf("unexpected error: %v", err)
 	}
@@ -657,16 +653,17 @@ func TestAnnotateLocal(t *testing.T) {
 
 	iostreams, _, _, _ := genericclioptions.NewTestIOStreams()
 	cmd := NewCmdAnnotate("kubectl", tf, iostreams)
-	options := NewAnnotateOptions(iostreams)
+	flags := NewAnnotateFlags(iostreams)
-	options.local = true
+	flags.Local = true
-	options.Filenames = []string{"../../../testdata/controller.yaml"}
+	flags.Filenames = []string{"../../../testdata/controller.yaml"}
 	args := []string{"a=b"}
-	if err := options.Complete(tf, cmd, args); err != nil {
+
-		t.Fatalf("unexpected error: %v", err)
+	options, err := flags.ToOptions(tf, cmd, args)
-	}
+
-	if err := options.Validate(); err != nil {
+	if err != nil {
 		t.Fatalf("unexpected error: %v", err)
 	}
+
 	if err := options.RunAnnotate(); err != nil {
 		t.Fatalf("unexpected error: %v", err)
 	}
@@ -714,13 +711,12 @@ func TestAnnotateMultipleObjects(t *testing.T) {
 	cmd := NewCmdAnnotate("kubectl", tf, iostreams)
 	cmd.SetOut(iostreams.Out)
 	cmd.SetErr(iostreams.Out)
-	options := NewAnnotateOptions(iostreams)
+	flags := NewAnnotateFlags(iostreams)
-	options.all = true
+	flags.All = true
 	args := []string{"pods", "a=b", "c-"}
-	if err := options.Complete(tf, cmd, args); err != nil {
+
-		t.Fatalf("unexpected error: %v", err)
+	options, err := flags.ToOptions(tf, cmd, args)
-	}
+	if err != nil {
-	if err := options.Validate(); err != nil {
 		t.Fatalf("unexpected error: %v", err)
 	}
 	if err := options.RunAnnotate(); err != nil {