Don't Display kn --help message with Plugin Errors

cmd/kn/main.go

@@ -35,60 +35,66 @@ func init() {
 }
 
 func main() {
-	err := run(os.Args[1:])
+	displayHelp, err := run(os.Args[1:])
 	if err != nil && len(os.Args) > 1 {
-		printError(err)
+		printError(err, displayHelp)
 		// This is the only point from where to exit when an error occurs
 		os.Exit(1)
 	}
 }
 
 // Run the main program. Args are the args as given on the command line (excluding the program name itself)
-func run(args []string) error {
+func run(args []string) (bool, error) {
 	// Parse config & plugin flags early to read in configuration file
 	// and bind to viper. After that you can access all configuration and
 	// global options via methods on config.GlobalConfig
 	err := config.BootstrapConfig()
 	if err != nil {
-		return err
+		return true, err
 	}
 
 	// Strip of all flags to get the non-flag commands only
 	commands, err := stripFlags(args)
 	if err != nil {
-		return err
+		return true, err
 	}
 
 	// Find plugin with the commands arguments
 	pluginManager := plugin.NewManager(config.GlobalConfig.PluginsDir(), config.GlobalConfig.LookupPluginsInPath())
 	plugin, err := pluginManager.FindPlugin(commands)
 	if err != nil {
-		return err
+		return true, err
 	}
 
 	// Create kn root command and all sub-commands
 	rootCmd, err := root.NewRootCommand()
 	if err != nil {
-		return err
+		return true, err
 	}
 
 	if plugin != nil {
 		// Validate & Execute plugin
 		err = validatePlugin(rootCmd, plugin)
 		if err != nil {
-			return err
+			return true, err
 		}
 
-		return plugin.Execute(argsWithoutCommands(args, plugin.CommandParts()))
-	} else {
-		// Validate args for root command
-		err = validateRootCommand(rootCmd)
+		err = plugin.Execute(argsWithoutCommands(args, plugin.CommandParts()))
 		if err != nil {
-			return err
+			// Return false to not print `--kn help` message with plugin errors
+			return false, err
 		}
-		// Execute kn root command, args are taken from os.Args directly
-		return rootCmd.Execute()
+
+		return true, err
 	}
+
+	// Validate args for root command
+	err = validateRootCommand(rootCmd)
+	if err != nil {
+		return true, err
+	}
+	// Execute kn root command, args are taken from os.Args directly
+	return true, rootCmd.Execute()
 }
 
 // Get only the args provided but no options. The extraction
@@ -188,9 +194,11 @@ func validateRootCommand(cmd *cobra.Command) error {
 }
 
 // printError prints out any given error
-func printError(err error) {
+func printError(err error, displayHelp bool) {
 	fmt.Fprintf(os.Stderr, "Error: %s\n", cleanupErrorMessage(err.Error()))
-	fmt.Fprintf(os.Stderr, "Run '%s --help' for usage\n", extractCommandPathFromErrorMessage(err.Error(), os.Args[0]))
+	if displayHelp {
+		fmt.Fprintf(os.Stderr, "Run '%s --help' for usage\n", extractCommandPathFromErrorMessage(err.Error(), os.Args[0]))
+	}
 }
 
 // extractCommandPathFromErrorMessage tries to extract the command name from an error message

cmd/kn/main_test.go

@@ -244,7 +244,7 @@ func TestRunWithError(t *testing.T) {
 	}
 	for _, d := range data {
 		capture := test.CaptureOutput(t)
-		printError(errors.New(d.given))
+		printError(errors.New(d.given), true)
 		stdOut, errOut := capture.Close()
 
 		assert.Equal(t, stdOut, "")
@@ -253,6 +253,29 @@ func TestRunWithError(t *testing.T) {
 	}
 }
 
+func TestRunWithPluginError(t *testing.T) {
+	data := []struct {
+		given    string
+		expected string
+	}{
+		{
+			"exit status 1",
+			"Error: exit status 1",
+		},
+	}
+	for _, d := range data {
+		capture := test.CaptureOutput(t)
+		// displayHelp argument is false for plugin error
+		printError(errors.New(d.given), false)
+		stdOut, errOut := capture.Close()
+
+		assert.Equal(t, stdOut, "")
+		assert.Assert(t, strings.Contains(errOut, d.expected))
+		// check that --help message isn't displayed
+		assert.Assert(t, util.ContainsNone(errOut, "Run", "--help", "usage"))
+	}
+}
+
 // Smoke test
 func TestRun(t *testing.T) {
 	oldArgs := os.Args
@@ -262,9 +285,10 @@ func TestRun(t *testing.T) {
 	})()
 
 	capture := test.CaptureOutput(t)
-	err := run(os.Args[1:])
+	displayHelp, err := run(os.Args[1:])
 	out, _ := capture.Close()
 
 	assert.NilError(t, err)
+	assert.Assert(t, displayHelp == true, "plugin error occurred (but should not have)")
 	assert.Assert(t, util.ContainsAllIgnoreCase(out, "version", "build", "git"))
 }

test/e2e/plugins_test.go

@@ -38,11 +38,15 @@ const (
 echo "Hello Knative, I'm a Kn plugin"
 echo "  My plugin file is $0"
 echo "  I received arguments: $1 $2 $3 $4"`
+
+	TestPluginCodeErr string = `#!/bin/bash
+
+exit 1`
 )
 
 type pluginTestConfig struct {
-	knConfigDir, knPluginsDir, knPluginsDir2  string
-	knConfigPath, knPluginPath, knPluginPath2 string
+	knConfigDir, knPluginsDir, knPluginsDir2, knPluginsDir3  string
+	knConfigPath, knPluginPath, knPluginPath2, knPluginPath3 string
 }
 
 func (pc *pluginTestConfig) setup() error {
@@ -64,6 +68,12 @@ func (pc *pluginTestConfig) setup() error {
 		return err
 	}
 
+	pc.knPluginsDir3 = filepath.Join(pc.knConfigDir, "plugins3")
+	err = os.MkdirAll(pc.knPluginsDir3, test.FileModeExecutable)
+	if err != nil {
+		return err
+	}
+
 	pc.knConfigPath, err = test.CreateFile("config.yaml", "", pc.knConfigDir, test.FileModeReadWrite)
 	if err != nil {
 		return err
@@ -77,6 +87,10 @@ func (pc *pluginTestConfig) setup() error {
 	if err != nil {
 		return err
 	}
+	pc.knPluginPath3, err = test.CreateFile("kn-hello3e2e", TestPluginCodeErr, pc.knPluginsDir3, test.FileModeExecutable)
+	if err != nil {
+		return err
+	}
 	return nil
 }
 
@@ -102,7 +116,7 @@ func TestPluginWithoutLookup(t *testing.T) {
 	listPlugin(r, knFlags, []string{pc.knPluginPath}, []string{})
 
 	t.Log("execute plugin in --plugins-dir")
-	runPlugin(r, knFlags, "helloe2e", []string{"e2e", "test"}, []string{"Hello Knative, I'm a Kn plugin", "I received arguments", "e2e"})
+	runPlugin(r, knFlags, "helloe2e", []string{"e2e", "test"}, []string{"Hello Knative, I'm a Kn plugin", "I received arguments", "e2e"}, []string{}, false)
 
 	t.Log("does not list any other plugin in $PATH")
 	listPlugin(r, knFlags, []string{pc.knPluginPath}, []string{pc.knPluginPath2})
@@ -138,7 +152,7 @@ func TestPluginWithLookup(t *testing.T) {
 	listPlugin(r, knFlags, []string{pc.knPluginPath}, []string{pc.knPluginPath2})
 
 	t.Log("execute plugin in --plugins-dir")
-	runPlugin(r, knFlags, "helloe2e", []string{}, []string{"Hello Knative, I'm a Kn plugin"})
+	runPlugin(r, knFlags, "helloe2e", []string{}, []string{"Hello Knative, I'm a Kn plugin"}, []string{}, false)
 }
 
 func TestListPluginInPath(t *testing.T) {
@@ -169,7 +183,26 @@ func TestExecutePluginInPath(t *testing.T) {
 
 	t.Log("execute plugin in $PATH")
 	knFlags := []string{fmt.Sprintf("--plugins-dir=%s", pc.knPluginsDir), "--lookup-plugins=true"}
-	runPlugin(r, knFlags, "hello2e2e", []string{}, []string{"Hello Knative, I'm a Kn plugin"})
+	runPlugin(r, knFlags, "hello2e2e", []string{}, []string{"Hello Knative, I'm a Kn plugin"}, []string{}, false)
+}
+
+func TestExecutePluginInPathWithError(t *testing.T) {
+	it, err := test.NewKnTest()
+	assert.NilError(t, err)
+
+	r := test.NewKnRunResultCollector(t, it)
+	defer r.DumpIfFailed()
+
+	pc := pluginTestConfig{}
+	assert.NilError(t, pc.setup())
+	oldPath := os.Getenv("PATH")
+	assert.NilError(t, os.Setenv("PATH", fmt.Sprintf("%s:%s", oldPath, pc.knPluginsDir3)))
+	defer tearDownWithPath(pc, oldPath)
+
+	t.Log("execute plugin in $PATH that returns error")
+	knFlags := []string{fmt.Sprintf("--plugins-dir=%s", pc.knPluginsDir), "--lookup-plugins=true"}
+	// Unexpected output []string{"Run", "kn --help", "usage"} to verify no kn --help message for error with plugin
+	runPlugin(r, knFlags, "hello3e2e", []string{}, []string{"Error: exit status 1"}, []string{"Run", "kn --help", "usage"}, true)
 }
 
 // Private
@@ -206,14 +239,25 @@ func listPlugin(r *test.KnRunResultCollector, knFlags []string, expectedPlugins
 	}
 }
 
-func runPlugin(r *test.KnRunResultCollector, knFlags []string, pluginName string, args []string, expectedOutput []string) {
+func runPlugin(r *test.KnRunResultCollector, knFlags []string, pluginName string, args []string, expectedOutput []string, unexpectedOutput []string, expectErr bool) {
 	knArgs := append([]string{}, knFlags...)
 	knArgs = append(knArgs, pluginName)
 	knArgs = append(knArgs, args...)
 
 	out := test.Kn{}.Run(knArgs...)
-	r.AssertNoError(out)
+	var stdOutOrErr string
+	if !expectErr {
+		r.AssertNoError(out)
+		stdOutOrErr = out.Stdout
+	} else {
+		r.AssertError(out)
+		stdOutOrErr = out.Stderr
+	}
+
 	for _, output := range expectedOutput {
-		assert.Check(r.T(), util.ContainsAll(out.Stdout, output))
+		assert.Check(r.T(), util.ContainsAll(stdOutOrErr, output))
+	}
+	for _, output := range unexpectedOutput {
+		assert.Check(r.T(), util.ContainsNone(stdOutOrErr, output))
 	}
 }