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

CHANGELOG.adoc

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

pkg/kn/commands/plugin/plugin.go

@@ -53,16 +53,13 @@ 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"}
+// CoreCommandNames names of all core `kn` commands
+var CoreCommandNames = []string{}
 
 // InAllowedExtensibleCommandGroups checks that the name is in the list of allowed
 // extensible command groups
 func InAllowedExtensibleCommandGroups(name string) bool {
-	for _, groupName := range AllowedExtensibleCommandGroups {
+	for _, groupName := range CoreCommandNames {
 		if name == groupName {
 			return true
 		}

pkg/kn/commands/plugin/plugin_test.go

@@ -74,11 +74,15 @@ func TestNewPluginCommand(t *testing.T) {
 }
 
 func TestInAllowedExtensibleCommandGroups(t *testing.T) {
-	isExtensibleCommand := InAllowedExtensibleCommandGroups("fake")
-	assert.Assert(t, isExtensibleCommand == false)
+	t.Run("returns true for any string in CoreCommandNames", func(t *testing.T) {
+		assert.Assert(t, len(CoreCommandNames) > 0)
+		for _, cmdName := range CoreCommandNames {
+			assert.Assert(t, InAllowedExtensibleCommandGroups(cmdName) == true)
+		}
+	})
 
-	for _, name := range AllowedExtensibleCommandGroups {
-		isExtensibleCommand = InAllowedExtensibleCommandGroups(name)
-		assert.Assert(t, isExtensibleCommand == true)
-	}
+	t.Run("returns false for random string", func(t *testing.T) {
+		assert.Assert(t, len(CoreCommandNames) > 0)
+		assert.Assert(t, InAllowedExtensibleCommandGroups("fake-cmd") == false)
+	})
 }

pkg/kn/core/root.go

@@ -78,18 +78,36 @@ func NewDefaultKnCommandWithArgs(rootCmd *cobra.Command,
 		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
+		}
+
 		// 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 {
 				fmt.Fprintf(rootCmd.OutOrStderr(), "Error: unknown command '%s' \nRun 'kn --help' for usage.\n", args[1])
 				os.Exit(1)
 			}
-		} else if foundCmd.HasSubCommands() {
-			if _, _, err := rootCmd.Find(innerArgs); err != nil {
-				fmt.Fprintf(rootCmd.OutOrStderr(), showSubcommands(foundCmd, cmdPathPieces, innerArgs[0]))
+		}
+
+		// look for case of plugins with command that shadows existing command
+		if foundCmd.HasSubCommands() && len(innerArgs) > 0 {
+			cmdName := innerArgs[0]
+			for _, subcommand := range foundCmd.Commands() {
+				if subcommand.Name() == cmdName {
+					fmt.Fprintf(rootCmd.OutOrStderr(), fmt.Sprintf("Error: subcommand '%s' for '%s' already exists.\nRun 'kn --help' for usage.\n", cmdName, foundCmd.Name()))
+					os.Exit(1)
+				}
+			}
+
+			// try to handle a plugin for a command extending a core comand group
+			err = plugin.HandlePluginCommand(pluginHandler, cmdPathPieces)
+			if err != nil {
+				fmt.Fprintf(rootCmd.OutOrStderr(), "Error: unknown command '%s' \nRun 'kn --help' for usage.\n", args[1])
 				os.Exit(1)
 			}
 		}
@@ -175,6 +193,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
 }
 
@@ -340,7 +363,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)
@@ -373,3 +398,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/plugins_test.go

@@ -84,12 +84,6 @@ func (pc *pluginTestConfig) teardown() {
 	os.RemoveAll(pc.knConfigDir)
 }
 
-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 TestPluginWithoutLookup(t *testing.T) {
 	t.Parallel()
 
@@ -112,8 +106,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) {
@@ -167,6 +159,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())
@@ -180,8 +180,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")