diff --git a/CHANGELOG.adoc b/CHANGELOG.adoc index 9728988b86..4cf9a994f3 100644 --- a/CHANGELOG.adoc +++ b/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) diff --git a/pkg/kn/commands/plugin/plugin.go b/pkg/kn/commands/plugin/plugin.go index 58143957cf..03b252c53d 100644 --- a/pkg/kn/commands/plugin/plugin.go +++ b/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{} diff --git a/pkg/kn/commands/plugin/plugin_test.go b/pkg/kn/commands/plugin/plugin_test.go index ac604b3918..fb34cfb59b 100644 --- a/pkg/kn/commands/plugin/plugin_test.go +++ b/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) - } -} diff --git a/pkg/kn/commands/plugin/verifier.go b/pkg/kn/commands/plugin/verifier.go index a218781425..a541851899 100644 --- a/pkg/kn/commands/plugin/verifier.go +++ b/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 +} diff --git a/pkg/kn/core/root.go b/pkg/kn/core/root.go index f3a17d9260..2469613c12 100644 --- a/pkg/kn/core/root.go +++ b/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 +} diff --git a/pkg/kn/core/root_test.go b/pkg/kn/core/root_test.go index 78d8b015e7..51eb29defc 100644 --- a/pkg/kn/core/root_test.go +++ b/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) }) }) diff --git a/test/e2e/basic_workflow_test.go b/test/e2e/basic_workflow_test.go index 479c7f9bd5..9957cf8b39 100644 --- a/test/e2e/basic_workflow_test.go +++ b/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") diff --git a/test/e2e/plugins_test.go b/test/e2e/plugins_test.go index 53b7bff4ae..396090e5e5 100644 --- a/test/e2e/plugins_test.go +++ b/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")