Refactor main flow, plugin and configuration handling

* The plugin handling has been moved out of the `KnDefaultCommand` constructor where it was executed as a side-effect. The original code from `kubectl` suffers from the same issue that plugin handling is not a top-level concern but was very likely introduced as an after-thought. Instead, the plugin handling is done now by a `PluginManager` which is explicitly called in `main()`.
* Configuration and bootstrap option handling is centralized in the package `option`. After the bootstrap happened, the content of the configuration file, as well as any other global configuration, can be obtained from methods on  `config.GlobalConfig`. Also, all flag handling is delegated to cobra so that no own parsing is needed.
* Many of the logic in `pkg/kn/commands/plugin` for plugin management has been moved up to `pkg/kn/plugin` as this code is not only relevant for `plugin list` but also for the bootstrap process.

cmd/kn/main.go

@@ -18,37 +18,143 @@ import (
 	"fmt"
 	"math/rand"
 	"os"
+	"strings"
 	"time"
 
-	"github.com/spf13/viper"
-	"knative.dev/client/pkg/kn/core"
+	"github.com/pkg/errors"
+	"github.com/spf13/cobra"
+
+	"knative.dev/client/pkg/kn/config"
+	"knative.dev/client/pkg/kn/plugin"
+	"knative.dev/client/pkg/kn/root"
 )
 
 func init() {
-	core.InitializeConfig()
+	rand.Seed(time.Now().UnixNano())
 }
 
-var err error
-
 func main() {
-	defer cleanup()
-	rand.Seed(time.Now().UnixNano())
-	kn, err := core.NewDefaultKnCommand()
+	err := run(os.Args[1:])
 	if err != nil {
-		fmt.Fprintln(os.Stderr, err)
+		fmt.Fprintf(os.Stderr, "Error: %v\n", err)
+		// 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 {
+	// 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
+	}
+
+	// Strip of all flags to get the non-flag commands only
+	commands, err := stripFlags(args)
+	if err != nil {
+		return 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
+	}
+
+	// Create kn root command and all sub-commands
+	rootCmd, err := root.NewRootCommand()
+	if err != nil {
+		return err
+	}
 
-	if err := kn.Execute(); err != nil {
-		if err.Error() != "subcommand is required" {
-			fmt.Fprintln(os.Stderr, err)
+	if plugin != nil {
+		// Validate & Execute plugin
+		err = validatePlugin(rootCmd, plugin)
+		if err != nil {
+			return err
 		}
-		os.Exit(1)
+
+		return plugin.Execute(argsWithoutCommands(args, plugin.CommandParts()))
+	} else {
+		// Execute kn root command
+		return rootCmd.Execute()
 	}
 }
 
-func cleanup() {
+// Get only the args provided but no options. The extraction
+// process is a bit tricky as Cobra doesn't provide such
+// functionality out of the box
+func stripFlags(args []string) ([]string, error) {
+	// Store all command
+	commandsFound := &[]string{}
+
+	// Use a canary command that allows all options and only extracts
+	// commands. Doesn't work with arbitrary boolean flags but is good enough
+	// for us here
+	extractCommand := cobra.Command{
+		Run: func(cmd *cobra.Command, args []string) {
+			for _, arg := range args {
+				*commandsFound = append(*commandsFound, arg)
+			}
+		},
+	}
+
+	// Filter out --help and -h options to avoid special treatment which we don't
+	// need here
+	extractCommand.SetArgs(filterHelpOptions(args))
+
+	// Adding all global flags here
+	config.AddBootstrapFlags(extractCommand.Flags())
+
+	// Allow all options
+	extractCommand.FParseErrWhitelist = cobra.FParseErrWhitelist{UnknownFlags: true}
+
+	// Execute to get to the command args
+	err := extractCommand.Execute()
+	if err != nil {
+		return nil, err
+	}
+	return *commandsFound, nil
+}
+
+// Strip all plugin commands before calling out to the plugin
+func argsWithoutCommands(cmdArgs []string, pluginCommandsParts []string) []string {
+	var ret []string
+	for _, arg := range cmdArgs {
+		if len(pluginCommandsParts) > 0 && pluginCommandsParts[0] == arg {
+			pluginCommandsParts = pluginCommandsParts[1:]
+			continue
+		}
+		ret = append(ret, arg)
+	}
+	return ret
+}
+
+// Remove all help options
+func filterHelpOptions(args []string) []string {
+	var ret []string
+	for _, arg := range args {
+		if arg != "-h" && arg != "--help" {
+			ret = append(ret, arg)
+		}
+	}
+	return ret
+}
+
+// Check if the plugin collides with any command specified in the root command
+func validatePlugin(root *cobra.Command, plugin plugin.Plugin) error {
+	// Check if a given plugin can be identified as a command
+	cmd, args, err := root.Find(plugin.CommandParts())
+
 	if err == nil {
-		viper.WriteConfig()
+		if !cmd.HasSubCommands() || // a leaf command can't be overridden
+			cmd.HasSubCommands() && len(args) == 0 { // a group can't be overridden either
+			return errors.Errorf("plugin %s is overriding built-in command '%s' which is not allowed", plugin.Path(), strings.Join(plugin.CommandParts(), " "))
+		}
 	}
+	return nil
 }

cmd/kn/main_test.go

@@ -0,0 +1,182 @@
+// Copyright © 2018 The Knative Authors
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//     http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package main
+
+import (
+	"fmt"
+	"testing"
+
+	"github.com/spf13/cobra"
+	"gotest.tools/assert"
+
+	"knative.dev/client/pkg/util"
+)
+
+func TestValidatePlugin(t *testing.T) {
+
+	// Build up simple command hierarchy
+	root := cobra.Command{}
+	one := &cobra.Command{Use: "one"}
+	one.AddCommand(&cobra.Command{Use: "eins"}, &cobra.Command{Use: "zwei"})
+	two := &cobra.Command{Use: "two"}
+	root.AddCommand(one, two)
+
+	data := []struct {
+		givenPluginCommandParts []string
+		expectedErrors          []string
+	}{
+		{
+			// Allowed because it add a new top-level plugin
+			[]string{"test"},
+			nil,
+		},
+		{
+			// Allowed because it adds to an existing command-group
+			[]string{"one", "drei"},
+			nil,
+		},
+		{
+			// Forbidden because it overrides an command-group
+			[]string{"one"},
+			[]string{"pluginPath", "one"},
+		},
+		{
+			// Forbidden because it overrides a leaf-command
+			[]string{"one", "zwei"},
+			[]string{"pluginPath", "one", "zwei"},
+		},
+		{
+			// Forbidden because it would mis-use a leaf-comman to a command-group
+			[]string{"one", "zwei", "trois"},
+			[]string{"pluginPath", "one", "zwei"},
+		},
+		{
+			// Forbidden because it overrides a (top-level) leaf-command
+			[]string{"two"},
+			[]string{"pluginPath", "two"},
+		},
+		{
+			// Forbidden because it would add to a leaf command
+			[]string{"two", "deux", "and", "more"},
+			[]string{"pluginPath", "two", "deux"},
+		},
+	}
+
+	for i, d := range data {
+		step := fmt.Sprintf("Check %d", i)
+		err := validatePlugin(&root, commandPartsOnlyPlugin(d.givenPluginCommandParts))
+		if len(d.expectedErrors) == 0 {
+			assert.NilError(t, err, step)
+		} else {
+			assert.Assert(t, err != nil, step)
+			assert.Assert(t, util.ContainsAll(err.Error(), d.expectedErrors...), step)
+		}
+	}
+
+}
+
+// Used above for wrapping the command part to check
+type commandPartsOnlyPlugin []string
+
+func (f commandPartsOnlyPlugin) CommandParts() []string       { return f }
+func (f commandPartsOnlyPlugin) Name() string                 { return "" }
+func (f commandPartsOnlyPlugin) Execute(args []string) error  { return nil }
+func (f commandPartsOnlyPlugin) Description() (string, error) { return "", nil }
+func (f commandPartsOnlyPlugin) Path() string                 { return "pluginPath" }
+
+func TestArgsWithoutCommands(t *testing.T) {
+	data := []struct {
+		givenCmdArgs            []string
+		givenPluginCommandParts []string
+		expectedResult          []string
+	}{
+		{
+			[]string{"--option", "val", "one", "second", "rest"},
+			[]string{"one", "second"},
+			[]string{"--option", "val", "rest"},
+		},
+		{
+			[]string{"--option", "val", "one", "second", "rest"},
+			[]string{"second", "one"},
+			[]string{"--option", "val", "one", "rest"},
+		},
+		{
+			[]string{"--option", "val", "one", "second", "third", "one", "rest"},
+			[]string{"second", "one"},
+			[]string{"--option", "val", "one", "third", "rest"},
+		},
+	}
+	for _, d := range data {
+		result := argsWithoutCommands(d.givenCmdArgs, d.givenPluginCommandParts)
+		assert.DeepEqual(t, result, d.expectedResult)
+	}
+}
+
+func TestStripFlags(t *testing.T) {
+
+	data := []struct {
+		givenArgs        []string
+		expectedCommands []string
+		expectedError    string
+	}{
+		{
+			[]string{"test", "-h", "second", "--bla"},
+			[]string{"test", "second"},
+			"",
+		},
+		{
+			[]string{"--help", "test"},
+			[]string{"test"},
+			"",
+		},
+		{
+			[]string{"--unknown-option", "bla", "test", "second"},
+			[]string{"test", "second"},
+			"",
+		},
+		{
+			[]string{"--lookup-plugins", "bla", "test", "second"},
+			[]string{"bla", "test", "second"},
+			"",
+		},
+		{
+			[]string{"--config-file", "bla", "test", "second"},
+			[]string{"test", "second"},
+			"",
+		},
+		{
+			[]string{"test"},
+			[]string{"test"},
+			"",
+		},
+	}
+
+	for i, f := range data {
+		step := fmt.Sprintf("Check %d", i)
+		commands, err := stripFlags(f.givenArgs)
+		assert.DeepEqual(t, commands, f.expectedCommands)
+		if f.expectedError != "" {
+			assert.ErrorContains(t, err, f.expectedError, step)
+		} else {
+			assert.NilError(t, err, step)
+		}
+	}
+}
+
+// Smoke test
+func TestRun(t *testing.T) {
+	err := run([]string{"version"})
+	assert.NilError(t, err)
+}

docs/cmd/kn.md

@@ -12,12 +12,10 @@ Manage your Knative building blocks:
 ### Options
 
 ```
-      --config string        kn config file (default is ~/.config/kn/config.yaml)
-  -h, --help                 help for kn
-      --kubeconfig string    kubectl config file (default is ~/.kube/config)
-      --log-http             log http traffic
-      --lookup-plugins       look for kn plugins in $PATH
-      --plugins-dir string   kn plugins directory (default "~/.config/kn/plugins")
+      --config string       kn configuration file (default: ~/.config/kn/config.yaml)
+  -h, --help                help for kn
+      --kubeconfig string   kubectl configuration file (default: ~/.kube/config)
+      --log-http            log http traffic
 ```
 
 ### SEE ALSO

docs/cmd/kn_completion.md

@@ -37,8 +37,8 @@ kn completion [SHELL] [flags]
 ### Options inherited from parent commands
 
 ```
-      --config string       kn config file (default is ~/.config/kn/config.yaml)
-      --kubeconfig string   kubectl config file (default is ~/.kube/config)
+      --config string       kn configuration file (default: ~/.config/kn/config.yaml)
+      --kubeconfig string   kubectl configuration file (default: ~/.kube/config)
       --log-http            log http traffic
 ```
 

docs/cmd/kn_plugin.md

@@ -16,16 +16,14 @@ kn plugin [flags]
 ### Options
 
 ```
-  -h, --help                 help for plugin
-      --lookup-plugins       look for kn plugins in $PATH
-      --plugins-dir string   kn plugins directory (default "~/.config/kn/plugins")
+  -h, --help   help for plugin
 ```
 
 ### Options inherited from parent commands
 
 ```
-      --config string       kn config file (default is ~/.config/kn/config.yaml)
-      --kubeconfig string   kubectl config file (default is ~/.kube/config)
+      --config string       kn configuration file (default: ~/.config/kn/config.yaml)
+      --kubeconfig string   kubectl configuration file (default: ~/.kube/config)
       --log-http            log http traffic
 ```