Build the command and use it to report user errors so that the comman…

…d help message is printed

Signed-off-by: Adrian Orive <adrian.orive.oneca@gmail.com>

pkg/cli/cli.go

@@ -18,7 +18,6 @@ package cli
 
 import (
 	"fmt"
-	"log"
 	"os"
 	"strings"
 
@@ -95,51 +94,33 @@ type cli struct { //nolint:maligned
 	// A filtered set of plugins that should be used by command constructors.
 	resolvedPlugins []plugin.Plugin
 
-	// Whether some generic help should be printed, i.e. if the binary
-	// was invoked outside of a project with incorrect flags or -h|--help.
-	doHelp bool
-
 	// Root command.
 	cmd *cobra.Command
 }
 
 // New creates a new cli instance.
+// Developer errors (e.g. not registering any plugins, extra commands with conflicting names) return an error
+// while user errors (e.g. errors while parsing flags, unresolvable plugins) create a command which return the error.
 func New(opts ...Option) (CLI, error) {
 	// Create the CLI.
 	c, err := newCLI(opts...)
 	if err != nil {
 		return nil, err
 	}
 
-	// Get project version and plugin keys.
-	if err := c.getInfo(); err != nil {
-		return nil, err
+	// Build the cmd tree.
+	if err := c.buildCmd(); err != nil {
+		c.cmd.RunE = errCmdFunc(err)
+		return c, nil
 	}
 
-	// Resolve plugins for project version and plugin keys.
-	if err := c.resolve(); err != nil {
-		return nil, err
-	}
-
-	// Build the root command.
-	c.cmd = c.buildRootCmd()
-
 	// Add extra commands injected by options.
-	for _, cmd := range c.extraCommands {
-		for _, subCmd := range c.cmd.Commands() {
-			if cmd.Name() == subCmd.Name() {
-				return nil, fmt.Errorf("command %q already exists", cmd.Name())
-			}
-		}
-		c.cmd.AddCommand(cmd)
+	if err := c.addExtraCommands(); err != nil {
+		return nil, err
 	}
 
 	// Write deprecation notices after all commands have been constructed.
-	for _, p := range c.resolvedPlugins {
-		if d, isDeprecated := p.(plugin.Deprecated); isDeprecated {
-			fmt.Printf(noticeColor, fmt.Sprintf(deprecationFmt, d.DeprecationWarning()))
-		}
-	}
+	c.printDeprecationWarnings()
 
 	return c, nil
 }
@@ -166,27 +147,25 @@ func newCLI(opts ...Option) (*cli, error) {
 }
 
 // getInfoFromFlags obtains the project version and plugin keys from flags.
-func (c *cli) getInfoFromFlags() (string, []string) {
+func (c *cli) getInfoFromFlags() (string, []string, error) {
 	// Partially parse the command line arguments
-	fs := pflag.NewFlagSet("base", pflag.ExitOnError)
+	fs := pflag.NewFlagSet("base", pflag.ContinueOnError)
 
 	// Omit unknown flags to avoid parsing errors
 	fs.ParseErrorsWhitelist = pflag.ParseErrorsWhitelist{UnknownFlags: true}
 
 	// Define the flags needed for plugin resolution
 	var projectVersion, plugins string
-	var help bool
 	fs.StringVar(&projectVersion, projectVersionFlag, "", "project version")
 	fs.StringVar(&plugins, pluginsFlag, "", "plugins to run")
-	fs.BoolVarP(&help, "help", "h", false, "help flag")
+	// FlagSet special cases --help and -h, so we need to create a dummy flag with these 2 values to prevent the default
+	// behavior (printing the usage of this FlagSet) as we want to print the usage message of the underlying command.
+	fs.BoolP("help", "h", false, fmt.Sprintf("help for %s", c.commandName))
 
 	// Parse the arguments
-	err := fs.Parse(os.Args[1:])
-
-	// User needs *generic* help if args are incorrect or --help is set and
-	// --project-version is not set. Plugin-specific help is given if a
-	// plugin.Context is updated, which does not require this field.
-	c.doHelp = err != nil || help && !fs.Lookup(projectVersionFlag).Changed
+	if err := fs.Parse(os.Args[1:]); err != nil {
+		return "", []string{}, err
+	}
 
 	// Split the comma-separated plugins
 	var pluginSet []string
@@ -196,7 +175,7 @@ func (c *cli) getInfoFromFlags() (string, []string) {
 		}
 	}
 
-	return projectVersion, pluginSet
+	return projectVersion, pluginSet, nil
 }
 
 // getInfoFromConfigFile obtains the project version and plugin keys from the project config file.
@@ -293,14 +272,16 @@ func (c cli) resolveFlagsAndConfigFileConflicts(
 // getInfo obtains the project version and plugin keys resolving conflicts among flags and the project config file.
 func (c *cli) getInfo() error {
 	// Get project version and plugin info from flags
-	flagProjectVersion, flagPlugins := c.getInfoFromFlags()
+	flagProjectVersion, flagPlugins, err := c.getInfoFromFlags()
+	if err != nil {
+		return err
+	}
 	// Get project version and plugin info from project configuration file
 	cfgProjectVersion, cfgPlugins, _ := getInfoFromConfigFile()
 	// We discard the error because not being able to read a project configuration file
 	// is not fatal for some commands. The ones that require it need to check its existence.
 
 	// Resolve project version and plugin keys
-	var err error
 	c.projectVersion, c.pluginKeys, err = c.resolveFlagsAndConfigFileConflicts(
 		flagProjectVersion, cfgProjectVersion, flagPlugins, cfgPlugins,
 	)
@@ -399,15 +380,13 @@ func (c *cli) resolve() error {
 	return nil
 }
 
-// buildRootCmd returns a root command with a subcommand tree reflecting the
+// addSubcommands returns a root command with a subcommand tree reflecting the
 // current project's state.
-func (c cli) buildRootCmd() *cobra.Command {
-	rootCmd := c.defaultCommand()
-
+func (c *cli) addSubcommands() {
 	// kubebuilder completion
 	// Only add completion if requested
 	if c.completionCommand {
-		rootCmd.AddCommand(c.newCompletionCmd())
+		c.cmd.AddCommand(c.newCompletionCmd())
 	}
 
 	// kubebuilder create
@@ -416,76 +395,61 @@ func (c cli) buildRootCmd() *cobra.Command {
 	createCmd.AddCommand(c.newCreateAPICmd())
 	createCmd.AddCommand(c.newCreateWebhookCmd())
 	if createCmd.HasSubCommands() {
-		rootCmd.AddCommand(createCmd)
+		c.cmd.AddCommand(createCmd)
 	}
 
 	// kubebuilder edit
-	rootCmd.AddCommand(c.newEditCmd())
+	c.cmd.AddCommand(c.newEditCmd())
 
 	// kubebuilder init
-	rootCmd.AddCommand(c.newInitCmd())
+	c.cmd.AddCommand(c.newInitCmd())
 
 	// kubebuilder version
 	// Only add version if a version string was provided
 	if c.version != "" {
-		rootCmd.AddCommand(c.newVersionCmd())
+		c.cmd.AddCommand(c.newVersionCmd())
 	}
-
-	return rootCmd
 }
 
-// defaultCommand returns the root command without its subcommands.
-func (c cli) defaultCommand() *cobra.Command {
-	return &cobra.Command{
-		Use:   c.commandName,
-		Short: "Development kit for building Kubernetes extensions and tools.",
-		Long: fmt.Sprintf(`Development kit for building Kubernetes extensions and tools.
-
-Provides libraries and tools to create new projects, APIs and controllers.
-Includes tools for packaging artifacts into an installer container.
+// buildCmd creates the underlying cobra command and stores it internally.
+func (c *cli) buildCmd() error {
+	c.cmd = c.newRootCmd()
 
-Typical project lifecycle:
-
-- initialize a project:
-
-  %[1]s init --domain example.com --license apache2 --owner "The Kubernetes authors"
-
-- create one or more a new resource APIs and add your code to them:
-
-  %[1]s create api --group <group> --version <version> --kind <Kind>
-
-Create resource will prompt the user for if it should scaffold the Resource and / or Controller. To only
-scaffold a Controller for an existing Resource, select "n" for Resource. To only define
-the schema for a Resource without writing a Controller, select "n" for Controller.
-
-After the scaffold is written, api will run make on the project.
-`,
-			c.commandName),
-		Example: fmt.Sprintf(`
-  # Initialize your project
-  %[1]s init --domain example.com --license apache2 --owner "The Kubernetes authors"
-
-  # Create a frigates API with Group: ship, Version: v1beta1 and Kind: Frigate
-  %[1]s create api --group ship --version v1beta1 --kind Frigate
+	// Get project version and plugin keys.
+	if err := c.getInfo(); err != nil {
+		return err
+	}
 
-  # Edit the API Scheme
-  nano api/v1beta1/frigate_types.go
+	// Resolve plugins for project version and plugin keys.
+	if err := c.resolve(); err != nil {
+		return err
+	}
 
-  # Edit the Controller
-  nano controllers/frigate_controller.go
+	// Add the subcommands
+	c.addSubcommands()
 
-  # Install CRDs into the Kubernetes cluster using kubectl apply
-  make install
+	return nil
+}
 
-  # Regenerate code and run against the Kubernetes cluster configured by ~/.kube/config
-  make run
-`,
-			c.commandName),
-		Run: func(cmd *cobra.Command, args []string) {
-			if err := cmd.Help(); err != nil {
-				log.Fatal(err)
+// addExtraCommands adds the additional commands.
+func (c *cli) addExtraCommands() error {
+	for _, cmd := range c.extraCommands {
+		for _, subCmd := range c.cmd.Commands() {
+			if cmd.Name() == subCmd.Name() {
+				return fmt.Errorf("command %q already exists", cmd.Name())
 			}
-		},
+		}
+		c.cmd.AddCommand(cmd)
+	}
+	return nil
+}
+
+// printDeprecationWarnings prints the deprecation warnings of the resolved plugins.
+func (c cli) printDeprecationWarnings() {
+	for _, p := range c.resolvedPlugins {
+		if d, isDeprecated := p.(plugin.Deprecated); isDeprecated {
+			fmt.Printf(noticeColor, fmt.Sprintf(deprecationFmt, d.DeprecationWarning()))
+		}
 	}
 }
 

pkg/cli/cli_test.go

@@ -50,6 +50,10 @@ func setFlag(flag, value string) {
 	os.Args = append(os.Args, "subcommand", "--"+flag, value)
 }
 
+func setBoolFlag(flag string) {
+	os.Args = append(os.Args, "subcommand", "--"+flag)
+}
+
 // nolint:unparam
 func setProjectVersionFlag(value string) {
 	setFlag(projectVersionFlag, value)
@@ -74,6 +78,7 @@ var _ = Describe("CLI", func() {
 		var (
 			projectVersion string
 			plugins        []string
+			err            error
 			c              *cli
 		)
 
@@ -87,7 +92,8 @@ var _ = Describe("CLI", func() {
 
 		When("no flag is set", func() {
 			It("should success", func() {
-				projectVersion, plugins = c.getInfoFromFlags()
+				projectVersion, plugins, err = c.getInfoFromFlags()
+				Expect(err).NotTo(HaveOccurred())
 				Expect(projectVersion).To(Equal(""))
 				Expect(len(plugins)).To(Equal(0))
 			})
@@ -96,7 +102,8 @@ var _ = Describe("CLI", func() {
 		When(fmt.Sprintf("--%s flag is set", projectVersionFlag), func() {
 			It("should success", func() {
 				setProjectVersionFlag("2")
-				projectVersion, plugins = c.getInfoFromFlags()
+				projectVersion, plugins, err = c.getInfoFromFlags()
+				Expect(err).NotTo(HaveOccurred())
 				Expect(projectVersion).To(Equal("2"))
 				Expect(len(plugins)).To(Equal(0))
 			})
@@ -105,21 +112,24 @@ var _ = Describe("CLI", func() {
 		When(fmt.Sprintf("--%s flag is set", pluginsFlag), func() {
 			It("should success using one plugin key", func() {
 				setPluginsFlag("go/v1")
-				projectVersion, plugins = c.getInfoFromFlags()
+				projectVersion, plugins, err = c.getInfoFromFlags()
+				Expect(err).NotTo(HaveOccurred())
 				Expect(projectVersion).To(Equal(""))
 				Expect(plugins).To(Equal([]string{"go/v1"}))
 			})
 
 			It("should success using more than one plugin key", func() {
 				setPluginsFlag("go/v1,example/v2,test/v1")
-				projectVersion, plugins = c.getInfoFromFlags()
+				projectVersion, plugins, err = c.getInfoFromFlags()
+				Expect(err).NotTo(HaveOccurred())
 				Expect(projectVersion).To(Equal(""))
 				Expect(plugins).To(Equal([]string{"go/v1", "example/v2", "test/v1"}))
 			})
 
 			It("should success using more than one plugin key with spaces", func() {
 				setPluginsFlag("go/v1 , example/v2 , test/v1")
-				projectVersion, plugins = c.getInfoFromFlags()
+				projectVersion, plugins, err = c.getInfoFromFlags()
+				Expect(err).NotTo(HaveOccurred())
 				Expect(projectVersion).To(Equal(""))
 				Expect(plugins).To(Equal([]string{"go/v1", "example/v2", "test/v1"}))
 			})
@@ -129,23 +139,26 @@ var _ = Describe("CLI", func() {
 			It("should success using one plugin key", func() {
 				setProjectVersionFlag("2")
 				setPluginsFlag("go/v1")
-				projectVersion, plugins = c.getInfoFromFlags()
+				projectVersion, plugins, err = c.getInfoFromFlags()
+				Expect(err).NotTo(HaveOccurred())
 				Expect(projectVersion).To(Equal("2"))
 				Expect(plugins).To(Equal([]string{"go/v1"}))
 			})
 
 			It("should success using more than one plugin keys", func() {
 				setProjectVersionFlag("2")
 				setPluginsFlag("go/v1,example/v2,test/v1")
-				projectVersion, plugins = c.getInfoFromFlags()
+				projectVersion, plugins, err = c.getInfoFromFlags()
+				Expect(err).NotTo(HaveOccurred())
 				Expect(projectVersion).To(Equal("2"))
 				Expect(plugins).To(Equal([]string{"go/v1", "example/v2", "test/v1"}))
 			})
 
 			It("should success using more than one plugin keys with spaces", func() {
 				setProjectVersionFlag("2")
 				setPluginsFlag("go/v1 , example/v2 , test/v1")
-				projectVersion, plugins = c.getInfoFromFlags()
+				projectVersion, plugins, err = c.getInfoFromFlags()
+				Expect(err).NotTo(HaveOccurred())
 				Expect(projectVersion).To(Equal("2"))
 				Expect(plugins).To(Equal([]string{"go/v1", "example/v2", "test/v1"}))
 			})
@@ -154,7 +167,15 @@ var _ = Describe("CLI", func() {
 		When("additional flags are set", func() {
 			It("should not fail", func() {
 				setFlag("extra-flag", "extra-value")
-				c.getInfoFromFlags()
+				_, _, err = c.getInfoFromFlags()
+				Expect(err).NotTo(HaveOccurred())
+			})
+
+			// `--help` is not captured by the whitelist, so we need to special case it
+			It("should not fail for `--help`", func() {
+				setBoolFlag("help")
+				_, _, err = c.getInfoFromFlags()
+				Expect(err).NotTo(HaveOccurred())
 			})
 		})
 	})
@@ -707,10 +728,11 @@ var _ = Describe("CLI", func() {
 			BeforeEach(func() { args = os.Args })
 			AfterEach(func() { os.Args = args })
 
-			It("should return an error", func() {
+			It("should return a CLI that returns an error", func() {
 				setPluginsFlag("foo")
-				_, err = New()
-				Expect(err).To(HaveOccurred())
+				c, err = New()
+				Expect(err).NotTo(HaveOccurred())
+				Expect(c.Run()).NotTo(Succeed())
 			})
 		})