From a01d2a729d232bdf641afe2a1b569605b9ce4f06 Mon Sep 17 00:00:00 2001 From: Daniel Helfand Date: Fri, 26 Jun 2020 10:47:55 -0500 Subject: [PATCH 1/2] don't display kn --help message with plugin errors --- cmd/kn/main.go | 13 +++++++-- test/e2e/plugins_test.go | 60 ++++++++++++++++++++++++++++++++++------ 2 files changed, 63 insertions(+), 10 deletions(-) diff --git a/cmd/kn/main.go b/cmd/kn/main.go index 876cd50fe1..a8b3f0ba16 100644 --- a/cmd/kn/main.go +++ b/cmd/kn/main.go @@ -30,6 +30,8 @@ import ( "knative.dev/client/pkg/kn/root" ) +var pluginErr = false + func init() { rand.Seed(time.Now().UnixNano()) } @@ -79,7 +81,12 @@ func run(args []string) error { return err } - return plugin.Execute(argsWithoutCommands(args, plugin.CommandParts())) + err = plugin.Execute(argsWithoutCommands(args, plugin.CommandParts())) + if err != nil { + // Used to not print `--kn help` message with plugin errors + pluginErr = true + } + return err } else { // Validate args for root command err = validateRootCommand(rootCmd) @@ -190,7 +197,9 @@ func validateRootCommand(cmd *cobra.Command) error { // printError prints out any given error func printError(err error) { 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 !pluginErr { + 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 diff --git a/test/e2e/plugins_test.go b/test/e2e/plugins_test.go index 2a93fd56a5..36cda11b1c 100644 --- a/test/e2e/plugins_test.go +++ b/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)) } } From 9678cf18b41a6666eb32b91c2b7dfa1679218c3c Mon Sep 17 00:00:00 2001 From: Daniel Helfand Date: Mon, 29 Jun 2020 10:19:35 -0500 Subject: [PATCH 2/2] remove global var and add bool to return signature --- cmd/kn/main.go | 45 ++++++++++++++++++++++----------------------- cmd/kn/main_test.go | 28 ++++++++++++++++++++++++++-- 2 files changed, 48 insertions(+), 25 deletions(-) diff --git a/cmd/kn/main.go b/cmd/kn/main.go index a8b3f0ba16..c806a24a4f 100644 --- a/cmd/kn/main.go +++ b/cmd/kn/main.go @@ -30,72 +30,71 @@ import ( "knative.dev/client/pkg/kn/root" ) -var pluginErr = false - func init() { rand.Seed(time.Now().UnixNano()) } 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 } err = plugin.Execute(argsWithoutCommands(args, plugin.CommandParts())) if err != nil { - // Used to not print `--kn help` message with plugin errors - pluginErr = true - } - return err - } else { - // Validate args for root command - err = validateRootCommand(rootCmd) - 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 @@ -195,9 +194,9 @@ 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())) - if !pluginErr { + if displayHelp { fmt.Fprintf(os.Stderr, "Run '%s --help' for usage\n", extractCommandPathFromErrorMessage(err.Error(), os.Args[0])) } } diff --git a/cmd/kn/main_test.go b/cmd/kn/main_test.go index 278573788a..b58dd0c2b1 100644 --- a/cmd/kn/main_test.go +++ b/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")) }