[WIP] Kn plugins implementation #177

maximilien · 2019-06-11T22:43:08Z

Implements Kn plugins re-using some code from kubectl plugins.
This is WIP to allow concrete discussions and testing and
hopefully also allow sub tasks to be created and worked on
in parallel.

This version contains the following:

wraps the main root Kn command to support plugin
plugins are any executable in PATH with name kn-*
'kn plugins list' to list found kn plugins
skips any kn plugins found with name that match core
commands, e.g., kn-service would be ignored
execute any valid kn plugins found

And is missing:

unit and integration tests
plugin install command
plugin repository command
plugin / Knative server version negotiation
anything else we agree on in plugin req doc

knative-prow-robot · 2019-06-11T22:43:15Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: maximilien
To fully approve this pull request, please assign additional approvers.
We suggest the following additional approver: sixolet

If they are not already assigned, you can assign the PR to them by writing /assign @sixolet in a comment when ready.

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

The pull request process is described here

Needs approval from an approver in each of these files:

OWNERS

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

maximilien · 2019-06-11T22:52:45Z

@sixolet and @cppforlife this is WIP so as to avoid one HUGE PR once we have complete agreement on the plugins docs. However, for those who want to try, this can be used and is rebased on current master:HEAD.

To test this PR. After building kn using ./hack/build.sh, do the following:

# assuming the 'kn' executable is built with this PR and in current dir
echo '#!/bin/bash

echo "Hello Knative, I am a Kn plugin"' > kn-hello
"
chmod +x kn-hello
mv kn-hello /usr/local/bin

# you can now list plugins
./kn plugin list
The following compatible plugins are available:

/usr/local/bin/kn-hello

# you can execute the 'kn-hello' plugin with
./kn hello
Hello Knative, I am a Kn plugin

I will be adding unit tests and integration tests next.

docs/cmd/kn_plugin_list.md

duglin · 2019-06-12T02:02:10Z

docs/cmd/kn_plugin_list.md

+
+```
+  -h, --help        help for list
+      --name-only   If true, display only the binary name of each plugin, rather than its full path


I think this should be reversed. In most cases I don't think people will care where the exe lives and when they do care it's probably because they're debugging to see why things are not acting correctly.

OK, up for debate. Especially if we decide to have a fix location.

pkg/kn/commands/plugin/plugin_handler.go

duglin · 2019-06-12T02:11:53Z

pkg/kn/commands/plugin/plugin_list.go

+			}
+
+			if isFirstFile {
+				fmt.Fprintf(o.ErrOut, "The following compatible plugins are available:\n\n")


this should go to stdout, not stderr

duglin · 2019-06-12T02:14:00Z

pkg/kn/commands/plugin/plugin_list.go

+	}
+
+	if !pluginsFound {
+		pluginErrors = append(pluginErrors, fmt.Errorf("error: unable to find any kubectl plugins in your PATH"))


"No plugins" shouldn't be considered an error - it's a valid state.

pkg/kn/commands/plugin/plugin_list.go

sixolet · 2019-06-12T02:39:08Z

To prepare for the world where we consider helping the user install plugins suggested by their server, I think I'm a lot more interested in there being a configurable plugin dir (remember when we removed --config? we might need it back.), and every executable in the plugin dir can be a plugin.

If we go with "anywhere on $PATH", then we have no place to install to, and also the user doesn't have independent control of what's on their PATH and what is a plugin, which bothers me somewhere in my aesthetics brain.

maximilien · 2019-06-12T19:28:41Z

To prepare for the world where we consider helping the user install plugins suggested by their server, I think I'm a lot more interested in there being a configurable plugin dir (remember when we removed --config? we might need it back.), and every executable in the plugin dir can be a plugin.

Yup. Thought of that too.

If we go with "anywhere on $PATH", then we have no place to install to, and also the user doesn't have independent control of what's on their PATH and what is a plugin, which bothers me somewhere in my aesthetics brain.

I personally don’t care. Product folks might have strong opinions. $PATH has two benefits: 1) compatible with kubectl and 2) many options and easy to determine any way. However, I can also see people wanting to have tight control over where plugins go. So I could see having both.

Anyhow, let’s decide next call and move forward. Best.

maximilien · 2019-06-12T19:31:41Z

@duglin, first off, thanks for feedback. However, I think it’s worth re-iterating two points globally before I address each of your comments:

This is [WIP] == [Work In Progress]. Says so in title.
First line of description: “Implements Kn plugins re-using some code from kubectl plugins.”

So you should expect this is not end game and that it looks and feels like kubectl plugins by design.

I wanted this to be a baseline so when we decide to deviate from kubectl plugins it’s as clear as possible.

duglin · 2019-06-12T19:38:32Z

yeah, but WIP or not, I think the suggested edits are valid

googlebot · 2019-06-17T20:13:56Z

So there's good news and bad news.

👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there.

😕 The bad news is that it appears that 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 here in the pull request.

Note to project maintainer: This is a terminal state, meaning the cla/google commit status will not change from this state. It's up to you to confirm consent of all the commit author(s), set the cla label to yes (if enabled on your project), and then merge this pull request when appropriate.

ℹ️ Googlers: Go here for more info.

googlebot · 2019-06-17T20:22:15Z

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

knative-metrics-robot · 2019-06-18T01:58:32Z

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

File	Old Coverage	New Coverage
pkg/kn/commands/plugin/path_verifier.go	Do not exist	80.6%
pkg/kn/commands/plugin/plugin.go	Do not exist	100.0%
pkg/kn/commands/plugin/plugin_flags.go	Do not exist	100.0%
pkg/kn/commands/plugin/plugin_handler.go	Do not exist	0.0%
pkg/kn/commands/plugin/plugin_list.go	Do not exist	5.6%

maximilien · 2019-06-20T01:59:06Z

pkg/kn/core/root.go

@@ -62,10 +95,17 @@ Eventing: Manage event subscriptions and channels. Connect up event sources.`,
 	if p.Output != nil {
 		rootCmd.SetOutput(p.Output)
 	}
+	rootCmd.PersistentFlags().StringVar(&commands.CfgFile, "config", "", "config file (default is $HOME/.kn/config.yaml)")
+	rootCmd.PersistentFlags().StringVar(&commands.PluginDir, "plugin-dir", "", "kn plugin directory (default is value in kn config or $PATH)")
 	rootCmd.PersistentFlags().StringVar(&commands.KubeCfgFile, "kubeconfig", "", "kubectl config file (default is $HOME/.kube/config)")


Hi, @sixolet, see this version of root.go

Still need to rebase and clean up, so still WIP. However, this is where I am adding the plugin-dir persistent flag and reading from kn's config (if exist) and defaulting to $PATH.

I am in middle of now using this in the plugin commands. Will finish this tomorrow. But at least let's make sure we are in sync. Let me know your thoughts if you get to read this otherwise, look for next update.

Cheers 🍻

maximilien · 2019-06-20T18:03:53Z

OK I need to rebase. Will try to find time to do so today.

Implements Kn plugins re-using some code from kubectl plugins. This is WIP to allow concrete discussions and testing and hopefully also allow sub tasks to be created and worked on in parallel. This version contains the following: 1. wraps the main root Kn command to support plugin 2. plugins are any executable in PATH with name kn-* 3. 'kn plugins list' to list found kn plugins 4. skips any kn plugins found with name that match core commands, e.g., kn-service would be ignored 5. execute any valid kn plugins found 6. UTs for plugin and plugin list commands And is missing: 1. integration tests 2. plugin install command 3. plugin repository command 4. plugin / Knative server version negotiation 5. anything else we agree on in plugin req doc

knative-prow-robot · 2019-06-20T23:22:11Z

@maximilien: The following tests failed, say /retest to rerun them all:

Test name	Commit	Details	Rerun command
pull-knative-client-unit-tests	`01384f1`	link	`/test pull-knative-client-unit-tests`
pull-knative-client-go-coverage	`01384f1`	link	`/test pull-knative-client-go-coverage`
pull-knative-client-integration-tests	`01384f1`	link	`/test pull-knative-client-integration-tests`
pull-knative-client-build-tests	`01384f1`	link	`/test pull-knative-client-build-tests`

Full PR test history. Your PR dashboard.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

maximilien · 2019-07-03T03:23:51Z

OK finally getting a chance to go back to this as I am wrapping up my trip in Beijing 🇨🇳

Will work on rebasing with latest updates and also move all tests to gotest.tools as per agreement on issue #161 on yesterday's WG.

maximilien · 2019-07-09T01:12:27Z

Closing this one since #249 is the first non-WIP version. Please review that one.

knative-prow-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jun 11, 2019

googlebot added the cla: yes Indicates the PR's author has signed the CLA. label Jun 11, 2019

knative-prow-robot requested review from evankanderson and sixolet June 11, 2019 22:43

knative-prow-robot added the size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. label Jun 11, 2019