fixes(#827): allow plugins to extend all command groups

CHANGELOG.adoc

@@ -60,6 +60,10 @@
 | Add `-a` flag as an alias for `--annotation`
 | https://github.com/knative/client/pull/782[#782]
 
+| 🎁
+| Allow plugins to extend all command groups
+| https://github.com/knative/client/pull/834[#834]
+
 |===
 
 ## v0.13.2 (2020-04-15)

pkg/kn/commands/plugin/plugin.go

@@ -53,19 +53,5 @@ func BindPluginsFlagToViper(cmd *cobra.Command) {
 	viper.SetDefault("lookup-plugins", false)
 }
 
-// AllowedExtensibleCommandGroups the list of command groups that can be
-// extended with plugins, e.g., a plugin named `kn-source-kafka` for Kafka
-// event sources is allowed. This is defined as a fixed [...]string since
-// cannot defined Golang []string constants
-var AllowedExtensibleCommandGroups = [...]string{"source"}
-
-// InAllowedExtensibleCommandGroups checks that the name is in the list of allowed
-// extensible command groups
-func InAllowedExtensibleCommandGroups(name string) bool {
-	for _, groupName := range AllowedExtensibleCommandGroups {
-		if name == groupName {
-			return true
-		}
-	}
-	return false
-}
+// CoreCommandNames names of all core `kn` commands
+var CoreCommandNames = []string{}

pkg/kn/commands/plugin/plugin_test.go

@@ -72,13 +72,3 @@ func TestNewPluginCommand(t *testing.T) {
 		assert.Assert(t, pluginCmd.Args == nil)
 	})
 }
-
-func TestInAllowedExtensibleCommandGroups(t *testing.T) {
-	isExtensibleCommand := InAllowedExtensibleCommandGroups("fake")
-	assert.Assert(t, isExtensibleCommand == false)
-
-	for _, name := range AllowedExtensibleCommandGroups {
-		isExtensibleCommand = InAllowedExtensibleCommandGroups(name)
-		assert.Assert(t, isExtensibleCommand == true)
-	}
-}

pkg/kn/commands/plugin/verifier.go

@@ -110,7 +110,7 @@ func (v *pluginVerifier) addErrorIfOverwritingExistingCommand(eaw errorsAndWarni
 	for _, c := range [][]string{cmds, convertUnderscoresToDashes(cmds)} {
 		cmd, _, err := v.root.Find(c)
 		if err == nil {
-			if !InAllowedExtensibleCommandGroups(cmd.Name()) {
+			if !inAllowedExtensibleCommandGroups(cmd.Name()) {
 				overwrittenCommands[cmd.CommandPath()] = true
 			}
 		}
@@ -293,3 +293,12 @@ func convertUnderscoresToDashes(cmds []string) []string {
 func isSymlink(mode os.FileMode) bool {
 	return mode&os.ModeSymlink != 0
 }
+
+func inAllowedExtensibleCommandGroups(name string) bool {
+	for _, groupName := range CoreCommandNames {
+		if name == groupName {
+			return true
+		}
+	}
+	return false
+}

pkg/kn/core/root.go

@@ -74,21 +74,49 @@ func NewDefaultKnCommandWithArgs(rootCmd *cobra.Command,
 	if pluginHandler == nil {
 		return rootCmd, nil
 	}
+
+	// process possible plugin call
 	if len(args) > 1 {
 		cmdPathPieces := args[1:]
 		cmdPathPieces = removeKnPluginFlags(cmdPathPieces) // Plugin does not need these flags
 
+		// Return fast if -h or --help is in path pieces
+		if helpOptionsPresent(cmdPathPieces) {
+			return rootCmd, nil
+		}
+
 		// only look for suitable extension executables if
 		// the specified command does not already exist
 		foundCmd, innerArgs, err := rootCmd.Find(cmdPathPieces)
-		if err != nil || plugin.InAllowedExtensibleCommandGroups(foundCmd.Name()) {
+		if err != nil {
 			err := plugin.HandlePluginCommand(pluginHandler, cmdPathPieces)
 			if err != nil {
 				return &cobra.Command{}, fmt.Errorf("unknown command '%s' \nRun 'kn --help' for usage", args[1])
 			}
-		} else if foundCmd.HasSubCommands() {
-			if _, _, err := rootCmd.Find(innerArgs); err != nil {
-				return &cobra.Command{}, fmt.Errorf(showSubcommands(foundCmd, cmdPathPieces, innerArgs[0]))
+		}
+
+		// when the call is on a leaf command, with sub commands
+		if foundCmd.HasSubCommands() {
+			// look for case of a plugin's command that shadows
+			// an existing command's subcommand
+			if len(innerArgs) > 0 {
+				cmdName := innerArgs[0]
+				for _, subcommand := range foundCmd.Commands() {
+					if subcommand.Name() == cmdName {
+						return &cobra.Command{}, fmt.Errorf("Error: sub-command '%s' for '%s' already exists.\nRun 'kn --help' for usage.\n", cmdName, foundCmd.Name())
+					}
+				}
+
+				// try to handle a plugin for a command extending a core comand group
+				err = plugin.HandlePluginCommand(pluginHandler, cmdPathPieces)
+				if err != nil {
+					return &cobra.Command{}, fmt.Errorf("Error: unknown sub-command '%s' for command '%s'\nRun 'kn --help' for usage.\n", cmdName, foundCmd.Name())
+				}
+			} else {
+				_, _, err := rootCmd.Find(innerArgs)
+				if err != nil {
+					return &cobra.Command{}, fmt.Errorf(showSubcommands(foundCmd, cmdPathPieces, innerArgs[0]))
+				}
 			}
 		}
 	}
@@ -173,6 +201,11 @@ func NewKnCommand(params ...commands.KnParams) *cobra.Command {
 	// For glog parse error.
 	flag.CommandLine.Parse([]string{})
 
+	// Set all current core commands to plugin.CoreCommandNames
+	for _, cmd := range rootCmd.Commands() {
+		plugin.CoreCommandNames = append(plugin.CoreCommandNames, cmd.Name())
+	}
+
 	return rootCmd
 }
 
@@ -337,7 +370,9 @@ func removeKnPluginFlags(args []string) []string {
 		if arg == "--plugins-dir" ||
 			strings.HasPrefix(arg, "--plugins-dir=") ||
 			arg == "--lookup-plugins" ||
-			strings.HasPrefix(arg, "--lookup-plugins=") {
+			strings.HasPrefix(arg, "--lookup-plugins=") ||
+			// remove -test.* args which are added when running go test
+			strings.HasPrefix(arg, "-test.") {
 			continue
 		} else {
 			remainingArgs = append(remainingArgs, arg)
@@ -370,3 +405,12 @@ func showSubcommands(cmd *cobra.Command, args []string, innerArg string) string
 	}
 	return fmt.Sprintf("Error: unknown subcommand '%s' for '%s'. Available subcommands: %s\nRun 'kn --help' for usage.\n", innerArg, getCommands(args, innerArg), strings.Join(strs, ", "))
 }
+
+func helpOptionsPresent(args []string) bool {
+	for _, arg := range args {
+		if arg == "-h" || arg == "--help" {
+			return true
+		}
+	}
+	return false
+}

pkg/kn/core/root_test.go

@@ -76,11 +76,12 @@ func TestNewDefaultKnCommandWithArgs(t *testing.T) {
 				err                                error
 			)
 
+			pluginName = "fake-plugin-name"
+
 			beforeEach := func(t *testing.T) {
 				tmpPathDir, err = ioutil.TempDir("", "plugin_list")
 				assert.Assert(t, err == nil)
 
-				pluginName = "fake-plugin-name"
 				pluginPath = plugin.CreateTestPluginInPath(t, "kn-"+pluginName, plugin.KnTestPluginScript, plugin.FileModeExecutable, tmpPathDir)
 			}
 
@@ -89,12 +90,56 @@ func TestNewDefaultKnCommandWithArgs(t *testing.T) {
 				assert.Assert(t, err == nil)
 			}
 
-			beforeEach(t)
-			args = []string{pluginPath, pluginName}
-			setup(t)
-			defer afterEach(t)
+			t.Run("when -h or --help option is present for plugin, return valid root command", func(t *testing.T) {
+				helpOptions := []string{"-h", "--help"}
+				for _, helpOption := range helpOptions {
+					beforeEach(t)
+					args = []string{pluginPath, pluginName, helpOption}
+					setup(t)
+					defer afterEach(t)
+
+					checkRootCmd(t, rootCmd)
+				}
+			})
+
+			t.Run("when --help option is present for normal command, return valid root command", func(t *testing.T) {
+				helpOptions := []string{"-h", "--help"}
+				for _, helpOption := range helpOptions {
+					beforeEach(t)
+					args = []string{"service", helpOption}
+					setup(t)
+					defer afterEach(t)
+
+					checkRootCmd(t, rootCmd)
+				}
+			})
 
 			t.Run("tries to handle args[1:] as plugin and return valid root command", func(t *testing.T) {
+				beforeEach(t)
+				args = []string{pluginPath, pluginName}
+				setup(t)
+				defer afterEach(t)
+
+				checkRootCmd(t, rootCmd)
+			})
+
+			t.Run("when plugin extends an existing command group it return valid root command", func(t *testing.T) {
+				pluginName = "service-fakecmd"
+				beforeEach(t)
+				args = []string{pluginPath, pluginName}
+				setup(t)
+				defer afterEach(t)
+
+				checkRootCmd(t, rootCmd)
+			})
+
+			t.Run("when plugin extends and shadows an existing command group it fails", func(t *testing.T) {
+				pluginName = "service-create"
+				beforeEach(t)
+				args = []string{pluginPath, pluginName, "test"}
+				setup(t)
+				defer afterEach(t)
+
 				checkRootCmd(t, rootCmd)
 			})
 		})

test/e2e/basic_workflow_test.go

@@ -78,7 +78,7 @@ func TestWrongCommand(t *testing.T) {
 	defer r.DumpIfFailed()
 
 	out := test.Kn{}.Run("source", "apiserver", "noverb", "--tag=0.13")
-	assert.Check(t, util.ContainsAll(out.Stderr, "Error", "unknown subcommand", "noverb"))
+	assert.Check(t, util.ContainsAll(out.Stderr, "Error", "unknown sub-command", "noverb"))
 	r.AssertError(out)
 
 	out = test.Kn{}.Run("rev")

test/e2e/plugins_test.go

@@ -105,8 +105,6 @@ func TestPluginWithoutLookup(t *testing.T) {
 
 	t.Log("does not list any other plugin in $PATH")
 	listPlugin(r, knFlags, []string{pc.knPluginPath}, []string{pc.knPluginPath2})
-
-	t.Log("with --lookup-plugins is true")
 }
 
 func TestPluginWithLookup(t *testing.T) {
@@ -160,6 +158,14 @@ func TestExecutePluginInPath(t *testing.T) {
 	runPlugin(r, knFlags, "hello2e2e", []string{}, []string{"Hello Knative, I'm a Kn plugin"})
 }
 
+// Private
+
+func createPluginFile(fileName, fileContent, filePath string, fileMode os.FileMode) (string, error) {
+	file := filepath.Join(filePath, fileName)
+	err := ioutil.WriteFile(file, []byte(fileContent), fileMode)
+	return file, err
+}
+
 func setupPluginTestConfigWithNewPath(t *testing.T) (pluginTestConfig, string) {
 	pc := pluginTestConfig{}
 	assert.NilError(t, pc.setup())
@@ -173,8 +179,6 @@ func tearDownWithPath(pc pluginTestConfig, oldPath string) {
 	pc.teardown()
 }
 
-// Private
-
 func listPlugin(r *test.KnRunResultCollector, knFlags []string, expectedPlugins []string, unexpectedPlugins []string) {
 	knArgs := append(knFlags, "plugin", "list")