From 02de66142d79e79ada42dfdc65ab8a3e1bbf399a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Roland=20Hu=C3=9F?= Date: Wed, 10 Jun 2020 16:59:46 +0200 Subject: [PATCH] fix: invalid subcommands will lead to a proper error message --- cmd/kn/main.go | 23 ++++++++++++++++++- cmd/kn/main_test.go | 49 ++++++++++++++++++++++++++++++++++++++++ pkg/kn/root/root.go | 31 +++++++++++++++++-------- pkg/kn/root/root_test.go | 6 ++--- vendor/modules.txt | 1 + 5 files changed, 96 insertions(+), 14 deletions(-) diff --git a/cmd/kn/main.go b/cmd/kn/main.go index 4377742cd6..36883674d8 100644 --- a/cmd/kn/main.go +++ b/cmd/kn/main.go @@ -80,7 +80,12 @@ func run(args []string) error { return plugin.Execute(argsWithoutCommands(args, plugin.CommandParts())) } else { - // Execute kn root command + // Validate args for root command + err = validateRootCommand(rootCmd) + if err != nil { + return err + } + // Execute kn root command, args are taken from os.Args directly return rootCmd.Execute() } } @@ -158,3 +163,19 @@ func validatePlugin(root *cobra.Command, plugin plugin.Plugin) error { } return nil } + +// Check whether an unknown sub-command is addressed and return an error if this is the case +// Needs to be called after the plugin has been extracted (as a plugin name can also lead to +// an unknown sub command error otherwise) +func validateRootCommand(cmd *cobra.Command) error { + foundCmd, innerArgs, err := cmd.Find(os.Args[1:]) + if err == nil && foundCmd.HasSubCommands() && len(innerArgs) > 0 { + argsWithoutFlags, err := stripFlags(innerArgs) + if len(argsWithoutFlags) > 0 || err != nil { + return errors.Errorf("unknown sub-command '%s' for '%s'. Available sub-commands: %s", innerArgs[0], foundCmd.Name(), strings.Join(root.ExtractSubCommandNames(foundCmd.Commands()), ", ")) + } + // If no args where given (only flags), then fall through to execute the command itself, which leads to + // a more appropriate error message + } + return nil +} diff --git a/cmd/kn/main_test.go b/cmd/kn/main_test.go index 6051cc6b41..d7db1e6340 100644 --- a/cmd/kn/main_test.go +++ b/cmd/kn/main_test.go @@ -23,6 +23,7 @@ import ( "gotest.tools/assert" "knative.dev/client/pkg/kn/commands" + "knative.dev/client/pkg/kn/root" "knative.dev/client/pkg/util" ) @@ -126,6 +127,54 @@ func TestArgsWithoutCommands(t *testing.T) { } } +func TestUnknownCommands(t *testing.T) { + oldArgs := os.Args + defer (func() { + os.Args = oldArgs + })() + + data := []struct { + givenCmdArgs []string + commandPath []string + expectedError []string + }{ + { + []string{"service", "udpate", "test", "--min-scale=0"}, + []string{"service"}, + []string{"unknown sub-command", "udpate"}, + }, + { + []string{"service", "--foo=bar"}, + []string{"service"}, + []string{}, + }, + { + []string{"source", "ping", "blub", "--foo=bar"}, + []string{"source", "ping"}, + []string{"unknown sub-command", "blub"}, + }, + } + for _, d := range data { + args := append([]string{"kn"}, d.givenCmdArgs...) + rootCmd, err := root.NewRootCommand() + os.Args = args + assert.NilError(t, err) + err = validateRootCommand(rootCmd) + if len(d.expectedError) == 0 { + assert.NilError(t, err) + continue + } + assert.Assert(t, err != nil) + assert.Assert(t, util.ContainsAll(err.Error(), d.expectedError...)) + cmd, _, e := rootCmd.Find(d.commandPath) + assert.NilError(t, e) + for _, sub := range cmd.Commands() { + assert.ErrorContains(t, err, sub.Name()) + } + } + +} + func TestStripFlags(t *testing.T) { data := []struct { diff --git a/pkg/kn/root/root.go b/pkg/kn/root/root.go index d402571372..b434ffb418 100644 --- a/pkg/kn/root/root.go +++ b/pkg/kn/root/root.go @@ -90,39 +90,41 @@ func NewRootCommand() (*cobra.Command, error) { // Initialize default `help` cmd early to prevent unknown command errors rootCmd.InitDefaultHelpCmd() - // Deal with empty and unknown sub command groups - err := addEmptyAndUnknownSubCommandsValidation(rootCmd) + // Check that command groups can't execute and that leaf commands don't h + err := validateCommandStructure(rootCmd) if err != nil { return nil, err } + // Wrap usage. fitUsageMessageToTerminalWidth(rootCmd) - // For glog parse error. + // For glog parse error. TOO: Check why this is needed flag.CommandLine.Parse([]string{}) return rootCmd, nil } -// addEmptyAndUnknownSubCommandsValidation adds a RunE to all commands that are groups to -// deal with errors when called with empty or unknown sub command -func addEmptyAndUnknownSubCommandsValidation(cmd *cobra.Command) error { +// Verify that command groups are not executable and that leaf commands have a run function +func validateCommandStructure(cmd *cobra.Command) error { for _, childCmd := range cmd.Commands() { if childCmd.HasSubCommands() { if childCmd.RunE != nil || childCmd.Run != nil { return errors.Errorf("internal: command group '%s' must not enable any direct logic, only leaf commands are allowed to take actions", childCmd.Name()) } + subCommands := childCmd.Commands() + name := childCmd.Name() childCmd.RunE = func(aCmd *cobra.Command, args []string) error { - aCmd.Help() + subText := fmt.Sprintf("Available sub-commands: %s", strings.Join(ExtractSubCommandNames(subCommands), ", ")) if len(args) == 0 { - return fmt.Errorf("please provide a valid sub-command for \"kn %s\"", aCmd.Name()) + return fmt.Errorf("no sub-command given for 'kn %s'. %s", name, subText) } - return fmt.Errorf("unknown sub-command \"%s\" for \"kn %s\"", args[0], aCmd.Name()) + return fmt.Errorf("unknown sub-command '%s' for 'kn %s'. %s", args[0], aCmd.Name(), subText) } } // recurse to deal with child commands that are themselves command groups - err := addEmptyAndUnknownSubCommandsValidation(childCmd) + err := validateCommandStructure(childCmd) if err != nil { return err } @@ -138,3 +140,12 @@ func fitUsageMessageToTerminalWidth(rootCmd *cobra.Command) { rootCmd.SetUsageTemplate(newUsage) } } + +// ExtractSubCommandNames extracts the names of all sub commands of a given command +func ExtractSubCommandNames(cmds []*cobra.Command) []string { + var ret []string + for _, subCmd := range cmds { + ret = append(ret, subCmd.Name()) + } + return ret +} diff --git a/pkg/kn/root/root_test.go b/pkg/kn/root/root_test.go index c739f9a810..ed99dc5120 100644 --- a/pkg/kn/root/root_test.go +++ b/pkg/kn/root/root_test.go @@ -77,7 +77,7 @@ func TestEmptyAndUnknownSubCommands(t *testing.T) { fakeGroupCmd.AddCommand(fakeSubCmd) rootCmd.AddCommand(fakeGroupCmd) - err := addEmptyAndUnknownSubCommandsValidation(rootCmd) + err := validateCommandStructure(rootCmd) assert.NilError(t, err) checkLeafCommand(t, "fake-subcommand", fakeGroupCmd) checkCommandGroup(t, []string{"fake-group"}, rootCmd) @@ -99,7 +99,7 @@ func TestCommandGroupWithRunMethod(t *testing.T) { fakeGroupCmd.AddCommand(fakeSubCmd) rootCmd.AddCommand(fakeGroupCmd) - err := addEmptyAndUnknownSubCommandsValidation(rootCmd) + err := validateCommandStructure(rootCmd) assert.Assert(t, err != nil) assert.Assert(t, util.ContainsAll(err.Error(), fakeGroupCmd.Name(), "internal", "not enable")) } @@ -124,7 +124,7 @@ func checkCommandGroup(t *testing.T, commands []string, rootCmd *cobra.Command) err = cmd.RunE(cmd, []string{}) assert.Assert(t, err != nil) - assert.Assert(t, util.ContainsAll(err.Error(), "provide", "valid", "sub-command", cmd.Name())) + assert.Assert(t, util.ContainsAll(err.Error(), "no", "sub-command", cmd.Name())) err = cmd.RunE(cmd, []string{"deeper"}) assert.Assert(t, err != nil) diff --git a/vendor/modules.txt b/vendor/modules.txt index 24300b3155..e12cce8a7e 100644 --- a/vendor/modules.txt +++ b/vendor/modules.txt @@ -98,6 +98,7 @@ github.com/pelletier/go-toml # github.com/peterbourgon/diskv v2.0.1+incompatible github.com/peterbourgon/diskv # github.com/pkg/errors v0.9.1 +## explicit github.com/pkg/errors # github.com/robfig/cron/v3 v3.0.1 github.com/robfig/cron/v3