fix: invalid subcommands will lead to a proper error message

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
+}

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 {

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
+}

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)

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