From 17919b184153b2d123a7c75f5b0dc7e486a724db Mon Sep 17 00:00:00 2001 From: Sai Harsha Kottapalli Date: Mon, 8 Nov 2021 20:32:00 +0530 Subject: [PATCH] Refactor Apply cmd to split flags from options (#102240) * Refactor Apply cmd to split flags from options * refactor code * fix subcommands --- .../src/k8s.io/kubectl/pkg/cmd/apply/apply.go | 283 +++++++++++------- 1 file changed, 170 insertions(+), 113 deletions(-) diff --git a/staging/src/k8s.io/kubectl/pkg/cmd/apply/apply.go b/staging/src/k8s.io/kubectl/pkg/cmd/apply/apply.go index e44a342f896..fe39617852a 100644 --- a/staging/src/k8s.io/kubectl/pkg/cmd/apply/apply.go +++ b/staging/src/k8s.io/kubectl/pkg/cmd/apply/apply.go @@ -46,15 +46,36 @@ import ( "k8s.io/kubectl/pkg/validation" ) +// ApplyFlags directly reflect the information that CLI is gathering via flags. They will be converted to Options, which +// reflect the runtime requirements for the command. This structure reduces the transformation to wiring and makes +// the logic itself easy to unit test +type ApplyFlags struct { + Factory cmdutil.Factory + + RecordFlags *genericclioptions.RecordFlags + PrintFlags *genericclioptions.PrintFlags + + DeleteFlags *delete.DeleteFlags + + FieldManager string + Selector string + Prune bool + PruneResources []pruneResource + All bool + Overwrite bool + OpenAPIPatch bool + PruneWhitelist []string + + genericclioptions.IOStreams +} + // ApplyOptions defines flags and other configuration parameters for the `apply` command type ApplyOptions struct { - RecordFlags *genericclioptions.RecordFlags - Recorder genericclioptions.Recorder + Recorder genericclioptions.Recorder PrintFlags *genericclioptions.PrintFlags ToPrinter func(string) (printers.ResourcePrinter, error) - DeleteFlags *delete.DeleteFlags DeleteOptions *delete.DeleteOptions ServerSideApply bool @@ -137,9 +158,10 @@ var ( warningChangesOnDeletingResource = "Warning: Detected changes to resource %[1]s which is currently being deleted.\n" ) -// NewApplyOptions creates new ApplyOptions for the `apply` command -func NewApplyOptions(ioStreams genericclioptions.IOStreams) *ApplyOptions { - return &ApplyOptions{ +// NewApplyFlags returns a default ApplyFlags +func NewApplyFlags(f cmdutil.Factory, streams genericclioptions.IOStreams) *ApplyFlags { + return &ApplyFlags{ + Factory: f, RecordFlags: genericclioptions.NewRecordFlags(), DeleteFlags: delete.NewDeleteFlags("that contains the configuration to apply"), PrintFlags: genericclioptions.NewPrintFlags("created").WithTypeSetter(scheme.Scheme), @@ -147,25 +169,13 @@ func NewApplyOptions(ioStreams genericclioptions.IOStreams) *ApplyOptions { Overwrite: true, OpenAPIPatch: true, - Recorder: genericclioptions.NoopRecorder{}, - - IOStreams: ioStreams, - - objects: []*resource.Info{}, - objectsCached: false, - - VisitedUids: sets.NewString(), - VisitedNamespaces: sets.NewString(), + IOStreams: streams, } } // NewCmdApply creates the `apply` command func NewCmdApply(baseName string, f cmdutil.Factory, ioStreams genericclioptions.IOStreams) *cobra.Command { - o := NewApplyOptions(ioStreams) - - // Store baseName for use in printing warnings / messages involving the base command name. - // This is useful for downstream command that wrap this one. - o.cmdBaseName = baseName + flags := NewApplyFlags(f, ioStreams) cmd := &cobra.Command{ Use: "apply (-f FILENAME | -k DIRECTORY)", @@ -174,52 +184,156 @@ func NewCmdApply(baseName string, f cmdutil.Factory, ioStreams genericclioptions Long: applyLong, Example: applyExample, Run: func(cmd *cobra.Command, args []string) { - cmdutil.CheckErr(o.Complete(f, cmd)) - cmdutil.CheckErr(validateArgs(cmd, args)) - cmdutil.CheckErr(validatePruneAll(o.Prune, o.All, o.Selector)) + o, err := flags.ToOptions(cmd, baseName, args) + cmdutil.CheckErr(err) + cmdutil.CheckErr(o.Validate(cmd, args)) cmdutil.CheckErr(o.Run()) }, } - // bind flag structs - o.DeleteFlags.AddFlags(cmd) - o.RecordFlags.AddFlags(cmd) - o.PrintFlags.AddFlags(cmd) - - cmd.Flags().BoolVar(&o.Overwrite, "overwrite", o.Overwrite, "Automatically resolve conflicts between the modified and live configuration by using values from the modified configuration") - cmd.Flags().BoolVar(&o.Prune, "prune", o.Prune, "Automatically delete resource objects, that do not appear in the configs and are created by either apply or create --save-config. Should be used with either -l or --all.") - cmdutil.AddValidateFlags(cmd) - cmd.Flags().StringVarP(&o.Selector, "selector", "l", o.Selector, "Selector (label query) to filter on, supports '=', '==', and '!='.(e.g. -l key1=value1,key2=value2)") - cmd.Flags().BoolVar(&o.All, "all", o.All, "Select all resources in the namespace of the specified resource types.") - cmd.Flags().StringArrayVar(&o.PruneWhitelist, "prune-whitelist", o.PruneWhitelist, "Overwrite the default whitelist with for --prune") - cmd.Flags().BoolVar(&o.OpenAPIPatch, "openapi-patch", o.OpenAPIPatch, "If true, use openapi to calculate diff when the openapi presents and the resource can be found in the openapi spec. Otherwise, fall back to use baked-in types.") - cmdutil.AddDryRunFlag(cmd) - cmdutil.AddServerSideApplyFlags(cmd) - cmdutil.AddFieldManagerFlagVar(cmd, &o.FieldManager, FieldManagerClientSideApply) + flags.AddFlags(cmd) // apply subcommands - cmd.AddCommand(NewCmdApplyViewLastApplied(f, ioStreams)) - cmd.AddCommand(NewCmdApplySetLastApplied(f, ioStreams)) - cmd.AddCommand(NewCmdApplyEditLastApplied(f, ioStreams)) + cmd.AddCommand(NewCmdApplyViewLastApplied(flags.Factory, flags.IOStreams)) + cmd.AddCommand(NewCmdApplySetLastApplied(flags.Factory, flags.IOStreams)) + cmd.AddCommand(NewCmdApplyEditLastApplied(flags.Factory, flags.IOStreams)) return cmd } -// Complete verifies if ApplyOptions are valid and without conflicts. -func (o *ApplyOptions) Complete(f cmdutil.Factory, cmd *cobra.Command) error { - var err error - o.ServerSideApply = cmdutil.GetServerSideApplyFlag(cmd) - o.ForceConflicts = cmdutil.GetForceConflictsFlag(cmd) - o.DryRunStrategy, err = cmdutil.GetDryRunStrategy(cmd) +// AddFlags registers flags for a cli +func (flags *ApplyFlags) AddFlags(cmd *cobra.Command) { + // bind flag structs + flags.DeleteFlags.AddFlags(cmd) + flags.RecordFlags.AddFlags(cmd) + flags.PrintFlags.AddFlags(cmd) + + cmdutil.AddValidateFlags(cmd) + cmdutil.AddDryRunFlag(cmd) + cmdutil.AddServerSideApplyFlags(cmd) + cmdutil.AddFieldManagerFlagVar(cmd, &flags.FieldManager, FieldManagerClientSideApply) + + cmd.Flags().BoolVar(&flags.Overwrite, "overwrite", flags.Overwrite, "Automatically resolve conflicts between the modified and live configuration by using values from the modified configuration") + cmd.Flags().BoolVar(&flags.Prune, "prune", flags.Prune, "Automatically delete resource objects, that do not appear in the configs and are created by either apply or create --save-config. Should be used with either -l or --all.") + cmd.Flags().StringVarP(&flags.Selector, "selector", "l", flags.Selector, "Selector (label query) to filter on, supports '=', '==', and '!='.(e.g. -l key1=value1,key2=value2)") + cmd.Flags().BoolVar(&flags.All, "all", flags.All, "Select all resources in the namespace of the specified resource types.") + cmd.Flags().StringArrayVar(&flags.PruneWhitelist, "prune-whitelist", flags.PruneWhitelist, "Overwrite the default whitelist with for --prune") + cmd.Flags().BoolVar(&flags.OpenAPIPatch, "openapi-patch", flags.OpenAPIPatch, "If true, use openapi to calculate diff when the openapi presents and the resource can be found in the openapi spec. Otherwise, fall back to use baked-in types.") +} + +// ToOptions converts from CLI inputs to runtime inputs +func (flags *ApplyFlags) ToOptions(cmd *cobra.Command, baseName string, args []string) (*ApplyOptions, error) { + serverSideApply := cmdutil.GetServerSideApplyFlag(cmd) + forceConflicts := cmdutil.GetForceConflictsFlag(cmd) + dryRunStrategy, err := cmdutil.GetDryRunStrategy(cmd) if err != nil { - return err + return nil, err } - o.DynamicClient, err = f.DynamicClient() + + dynamicClient, err := flags.Factory.DynamicClient() if err != nil { - return err + return nil, err + } + + dryRunVerifier := resource.NewDryRunVerifier(dynamicClient, flags.Factory.OpenAPIGetter()) + fieldManager := GetApplyFieldManagerFlag(cmd, serverSideApply) + + // allow for a success message operation to be specified at print time + toPrinter := func(operation string) (printers.ResourcePrinter, error) { + flags.PrintFlags.NamePrintFlags.Operation = operation + cmdutil.PrintFlagsWithDryRunStrategy(flags.PrintFlags, dryRunStrategy) + return flags.PrintFlags.ToPrinter() + } + + flags.RecordFlags.Complete(cmd) + recorder, err := flags.RecordFlags.ToRecorder() + if err != nil { + return nil, err + } + + deleteOptions, err := flags.DeleteFlags.ToOptions(dynamicClient, flags.IOStreams) + if err != nil { + return nil, err + } + + err = deleteOptions.FilenameOptions.RequireFilenameOrKustomize() + if err != nil { + return nil, err + } + + openAPISchema, _ := flags.Factory.OpenAPISchema() + validator, err := flags.Factory.Validator(cmdutil.GetFlagBool(cmd, "validate")) + if err != nil { + return nil, err + } + builder := flags.Factory.NewBuilder() + mapper, err := flags.Factory.ToRESTMapper() + if err != nil { + return nil, err + } + + namespace, enforceNamespace, err := flags.Factory.ToRawKubeConfigLoader().Namespace() + if err != nil { + return nil, err + } + + if flags.Prune { + flags.PruneResources, err = parsePruneResources(mapper, flags.PruneWhitelist) + if err != nil { + return nil, err + } + } + + o := &ApplyOptions{ + // Store baseName for use in printing warnings / messages involving the base command name. + // This is useful for downstream command that wrap this one. + cmdBaseName: baseName, + + PrintFlags: flags.PrintFlags, + + DeleteOptions: deleteOptions, + ToPrinter: toPrinter, + ServerSideApply: serverSideApply, + ForceConflicts: forceConflicts, + FieldManager: fieldManager, + Selector: flags.Selector, + DryRunStrategy: dryRunStrategy, + DryRunVerifier: dryRunVerifier, + Prune: flags.Prune, + PruneResources: flags.PruneResources, + All: flags.All, + Overwrite: flags.Overwrite, + OpenAPIPatch: flags.OpenAPIPatch, + PruneWhitelist: flags.PruneWhitelist, + + Recorder: recorder, + Namespace: namespace, + EnforceNamespace: enforceNamespace, + Validator: validator, + Builder: builder, + Mapper: mapper, + DynamicClient: dynamicClient, + OpenAPISchema: openAPISchema, + + IOStreams: flags.IOStreams, + + objects: []*resource.Info{}, + objectsCached: false, + + VisitedUids: sets.NewString(), + VisitedNamespaces: sets.NewString(), + } + + o.PostProcessorFn = o.PrintAndPrunePostProcessor() + + return o, nil +} + +// Validate verifies if ApplyOptions are valid and without conflicts. +func (o *ApplyOptions) Validate(cmd *cobra.Command, args []string) error { + if len(args) != 0 { + return cmdutil.UsageErrorf(cmd, "Unexpected args: %v", args) } - o.DryRunVerifier = resource.NewDryRunVerifier(o.DynamicClient, f.OpenAPIGetter()) - o.FieldManager = GetApplyFieldManagerFlag(cmd, o.ServerSideApply) if o.ForceConflicts && !o.ServerSideApply { return fmt.Errorf("--force-conflicts only works with --server-side") @@ -229,29 +343,6 @@ func (o *ApplyOptions) Complete(f cmdutil.Factory, cmd *cobra.Command) error { return fmt.Errorf("--dry-run=client doesn't work with --server-side (did you mean --dry-run=server instead?)") } - // allow for a success message operation to be specified at print time - o.ToPrinter = func(operation string) (printers.ResourcePrinter, error) { - o.PrintFlags.NamePrintFlags.Operation = operation - cmdutil.PrintFlagsWithDryRunStrategy(o.PrintFlags, o.DryRunStrategy) - return o.PrintFlags.ToPrinter() - } - - o.RecordFlags.Complete(cmd) - o.Recorder, err = o.RecordFlags.ToRecorder() - if err != nil { - return err - } - - o.DeleteOptions, err = o.DeleteFlags.ToOptions(o.DynamicClient, o.IOStreams) - if err != nil { - return err - } - - err = o.DeleteOptions.FilenameOptions.RequireFilenameOrKustomize() - if err != nil { - return err - } - if o.ServerSideApply && o.DeleteOptions.ForceDeletion { return fmt.Errorf("--force cannot be used with --server-side") } @@ -260,48 +351,14 @@ func (o *ApplyOptions) Complete(f cmdutil.Factory, cmd *cobra.Command) error { return fmt.Errorf("--dry-run=server cannot be used with --force") } - o.OpenAPISchema, _ = f.OpenAPISchema() - o.Validator, err = f.Validator(cmdutil.GetFlagBool(cmd, "validate")) - if err != nil { - return err - } - o.Builder = f.NewBuilder() - o.Mapper, err = f.ToRESTMapper() - if err != nil { - return err - } - - o.Namespace, o.EnforceNamespace, err = f.ToRawKubeConfigLoader().Namespace() - if err != nil { - return err - } - - if o.Prune { - o.PruneResources, err = parsePruneResources(o.Mapper, o.PruneWhitelist) - if err != nil { - return err - } - } - - o.PostProcessorFn = o.PrintAndPrunePostProcessor() - - return nil -} - -func validateArgs(cmd *cobra.Command, args []string) error { - if len(args) != 0 { - return cmdutil.UsageErrorf(cmd, "Unexpected args: %v", args) - } - return nil -} - -func validatePruneAll(prune, all bool, selector string) error { - if all && len(selector) > 0 { + if o.All && len(o.Selector) > 0 { return fmt.Errorf("cannot set --all and --selector at the same time") } - if prune && !all && selector == "" { + + if o.Prune && !o.All && o.Selector == "" { return fmt.Errorf("all resources selected for prune without explicitly passing --all. To prune all resources, pass the --all flag. If you did not mean to prune all resources, specify a label selector") } + return nil }