Refactor main flow and introduce explicit plugin and config handling

[rhuss]

This PR is a significant refactoring of the main flow of how commands and plugins are processed. I'm aware such big changes should not be introduced lightly and only for good reasons. While working to implement #579 and to give some guidance for how to improve the plugin error message flow (cc: @dsimansky), I stumbled upon the following issues (and more):

• Calling out to plugins happens as a side-effect in NewKnDefaultCommand(). This code is more or less taken over literally from kubectl, but it definitely has it has issues in understandability and maintainability. Plugin error handling is tough here (i.e. how to return the error)
• The options --plugins-dir and --lookup-plugins are parsed manually (without cobra support), and it was buggy (i.e. doesn't honour both --plugin-dir=/my/dir and --plugin-dir /my/dir).
• Mangling of args is not transparent.
• Flow of execution is not easy to follow, i.e. not easy to see what happens where.
• Handling of the configuration file is not straight-forward; also it is not clear why the config file has to be written back (which completely messes up any user formatting or comments).
• Too much functionality has ended up in the kn/commands/plugins package but should be more top-level as it is used there also to verify the plugin
• The NewDefaultKnCommandWithArgs constructor is very complex and too long.

To fix this and to generally enhance the main flow, the following significant changes have been introduced in this PR:

• 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.

This PR will also help to make the implementation of the following feature requests easier:

• Extract common kn functionality for plugins and specialized Knative clients #763 - Extract common kn functionality for plugins and specialized Knative clients
• Split eventing/serving commands in help message to increase discoverability #579 - Split eventing/serving commands in help message to increase discoverability
• Introduce --debug and --verbose as global options #333 - Introduce --debug and --verbose as global options
• Consider adding plugin cmds to kn --help #266 - Consider adding plugin cmds to kn --help
• Support -q quiet option in kn plugins once #182 is done #258 - Support -q quiet option in kn plugins
• Add a -q/quiet option #182 - Add a -q option
• .... and probably some more

Also, some minor refactoring for the tests is included to remove some nesting in the tests and make it easier to understand and maintain (of course this depends on the POV so happy on any feedback, positive or negative).

Two user-facing changes are suggested as part of this PR:

• Add some structure in the config file

  plugins:
    directory: /kn-plugins
      path-lookup: false
   eventing:
     sink-mappings:
     - prefix: service
       resource: services
       group: core
       version: v1

  Instead of

  plugins-dir: /kn-plugins
  lookup-plugins: false
  sink:
  - ....

  The old syntax is still supported but should be eventually removed.
• The global options --plugins-dir and --lookup-plugins are still supported but marked as hidden. IMO these should eventually be removed. I question their value as global options because they are much better fit for the configuration file as these are permanent decisions that don't change from CLI call to CLI call and also to reduce the global option churn. However, this is, of course, open to discussion.

I recommend starting a review in main.go and follow the flow for config parsing and plugin handling. Also, I added a more detailed explanation of the flow in a separate document developer-guide.md as part of this PR. I extract the relevant section describing the outcome of the refactoring in the next comment.

Yes, it's a big PR but I think it makes kn better. As the PR touches the core of kn, I would highly appreciate if it could be reviewed and potentially merged asap to avoid blocking other contributions.

[knative-prow-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: rhuss

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details

Needs approval from an approver in each of these files:

• OWNERS [rhuss]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[rhuss]

The main flow of execution has been changed as described in the developer guide:

Flow

The journey starts at main.go which is the entry point when you call kn.
You find here the main control flow, which can be roughly divided into three phases:

• Bootstrap is about retrieving essential configuration parameters from command-line flags, and a configuration file.
• Plugin Lookup finds out whether the user wants to execute a plugin or a built-in command
• Execution : Execute a plugin or the RootCommand for built-in commands

Only the code in the main function can use os.Exit for leaving the process.
It evaluates any error that bubbles up and prints it out as an error message before exiting.
There is no exception to this rule.

Let's talk now about the three phases separately.

Bootstrap

The bootstrap performed by config.BootstrapConfig() extracts all the options relevant for config file detection and plugin configuration.
The bootstrap process does not fully parse all arguments but only those that are relevant for starting up and for looking up any plugin.
The configuration can be either provided via a --config flag or is picked up from a default location.
The default configuration location conforms to the XDG Base Directory Specification and is different for Unix systems and Windows systems.

Viper loads the configuration file (if any) form this location, and the config package makes all the bootstrap and other configuration information available vial the config.GlobalConfig singleton (which btw can be easily mocked for unit tests by just replacing it with another implementation of config.Config interface)

Plugin Lookup

In the next step, a PluginManager checks whether the given command-line arguments are pointing to a plugin.
All non-flag arguments are extracted and then used to lookup via plugin.PluginManager.FindPlugin() in the plugin directory (and the execution $PATH if configured) calculated in the Bootstrap phase.

Execution

If kn detects a plugin, it is first validated and then executed.
If not, the flow executes the RootCommand which carries all builtin kn commands.

However, before kn executes a plugin, it is first validated whether it does not clash with a builtin command or command group.
If the validation succeeds, the PluginManager calls out to the identified plugin via the Plugin.Execute() interface method.

If kn does not detect a plugin, the RootCommand is executed, which is internally dispatched, based on the given commands, to the matching sub-command.
In the case that no sub-command matches the provided command arguments on the command line, kn throws an error.

Configuration

For any command that is executed by kn, the configuration can come from two places:

• Arguments and flags as provided on the command line
• Configuration stored in a configuration file

kn has a bootstrap configuration which consists of these variables:

• configFile is the location of the configuration file which by default is ~/.config/kn/config.yaml on Unix like systems and %APPDATA%\kn on Windows of now overridden with the environment variable XDG_CONFIG_HOME. It can be specified on the command line with --config.
• pluginsDir is the location of the directory holding kn plugins. By default, this is ${configFile}/plugins. The default can be overridden in with the field plugins.directory in the configuration file.
• lookupPluginsInPath is a boolean flag that, when set to true, specifies that plugins should also be looked up in the execution $PATH (default: false). Use plugins.lookup-path in the configuration file to override it.

The main flow calls out to config.BootstrapConfig() to set these three variables by evaluating the location of the configuration file first and then reading in the configuration.

All other configuration that is stored in the configuration is also available after the bootstrap.
To access this information, any other code can access the methods of config.GlobalConfig which holds an implementation of the config.Config interface.
For testing purposes, config.GlobalConfig can be replaced by a mock implementation while running a test. Use config.TestConfig for simple use cases.

[mattmoor]

Produced via:
gofmt -s -w $(find -path './vendor' -prune -o -path './third_party' -prune -o -type f -name '*.go' -print)

[mattmoor]

Produced via:
gofmt -s -w $(find -path './vendor' -prune -o -path './third_party' -prune -o -type f -name '*.go' -print)

[googlebot]

All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter.

We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only @googlebot I consent. in this pull request.

Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.

[rhuss]

@mattmoor Could you please @googlebot I consent? It looks like the CLA bot always fails to recognises you probably as soon as one does a rebase on master and do a force pushed. Directly applying for the suggestions works without problem, but after reading the CLA bot is not happy anymore.

[mattmoor]

Produced via:
gofmt -s -w $(find -path './vendor' -prune -o -path './third_party' -prune -o -type f -name '*.go' -print)

[mattmoor]

Produced via:
gofmt -s -w $(find -path './vendor' -prune -o -path './third_party' -prune -o -type f -name '*.go' -print)

[googlebot]

☹️ Sorry, but only Googlers may change the label cla: yes.

[rhuss]

@itsmurugappan here's an update which should bring back the fix for unknown subcommand checks.

[itsmurugappan]

@itsmurugappan here's an update which should bring back the fix for unknown subcommand checks.

Verified it. Works as before

[googlebot]

All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter.

We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only @googlebot I consent. in this pull request.

Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.

[googlebot]

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

[navidshaikh]

Nice!

Liked the improved code readability and re-grouping, the imports are clean too! Thank you!

I've suggested a few nits mostly around wording of panic error messages,
and commented at a few places where the links are missing (we can fill in the links in a subsequent PR once merged)

[knative-metrics-robot]

The following is the coverage report on the affected files.
Say /test pull-knative-client-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
cmd/kn/main.go	Do not exist	77.0%
pkg/kn/commands/plugin/list.go	Do not exist	87.2%
pkg/kn/commands/plugin/plugin.go	Do not exist	100.0%
pkg/kn/commands/testing_helper.go	54.3%	91.1%	36.8
pkg/kn/commands/types.go	56.1%	53.8%	-2.3
pkg/kn/config/config.go	Do not exist	75.0%
pkg/kn/config/testing.go	Do not exist	100.0%
pkg/kn/plugin/manager.go	Do not exist	84.1%
pkg/kn/plugin/stat.go	Do not exist	80.0%
pkg/kn/plugin/verify.go	Do not exist	71.3%
pkg/kn/root/root.go	Do not exist	87.2%

[rhuss]

Thanks a lot for the reviews so far, it looks like we are on the right track.

In order to remove some review pressure, let's go for a lazy consensus approach: This PR doesn't add any new major feature but makes maintenance easier (see also intro). Adjustments can happen later, too, so the suggestion is that if there are no objections until the end of the week, the PR gets merged.

• If there are objections or change requests, please raise them in a review.
• If there are objections against the suggested process and/or time window, please raise them here, too and suggest another process/time-line for proceeding.

If no objections stated here, this is assumed to be a consensus so that the PR gets merged on Friday in this case.

[dsimansk]

Per our chat conversation it's good to go now...
Thanks for an excellent refactor @rhuss!
/lgtm