fixes(#814) allow plugins to extend the 'source' command group #818

maximilien · 2020-04-21T19:07:17Z

Description

In rare cases (e.g., source) an existing command group should be extensible via plugins. This PR addresses that. Source should be extensible since many of the eventing sources will be supported via plugins.

Changes

introduce a list of command group that can be extended (currently only source)
check if plugin main group is in that list and execute, otherwise fail
add e2e test for both plugin that is allowed and not allowed to extend existing group

Reference

Fixes #814

/lint

knative-prow-robot

@maximilien: 0 warnings.

In response to this:

Description

In rare cases (e.g., source) an existing command group should be extensible via plugins. This PR addresses that. Source should be extensible since many of the eventing sources will be supported via plugins.

Changes

introduce a list of command group that can be extended (currently only source)

check if plugin main group is in that list and execute, otherwise fail

add e2e test for both plugin that is allowed and not allowed to extend existing group

Reference

Fixes #814

/lint

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.

maximilien · 2020-04-21T19:07:45Z

/ok-to-test

* introduce a list of command group that can be extended (currently only source) * check if plugin main group is in that list and execute, otherwise fail * add e2e test for both plugin that is allowed and not allowed to extend existing group

knative-metrics-robot · 2020-04-22T01:02:51Z

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
pkg/kn/core/root.go	51.7%	52.3%	0.6

navidshaikh

tested the PR and seeing unknown command 'source'

➜  client git:(7b488dfe) ./hack/build.sh -f
🚧 Compile

➜  client git:(7b488dfe) ./kn plugin list
/home/nshaikh/.config/kn/plugins/kn-source-test.sh
ERROR:
  - kn-source-test.sh overwrites existing built-in command: kn source
kn-source-test.sh overwrites existing built-in command: kn source

➜  client git:(7b488dfe) ./kn source test        
Error: unknown command 'source' 
Run 'kn --help' for usage.

maximilien · 2020-04-22T15:58:49Z

tested the PR and seeing unknown command 'source'

Hmm, OK, thx, let me double check. Not seeing this on my side.

maximilien · 2020-04-24T00:57:49Z

/hold

maximilien · 2020-04-28T22:28:28Z

OK @navidshaikh back on this. I think I see what's going on, I need to add more code to the kn plugin list which runs the verifier which looks for plugins extending all existing commands. However, the current code will actually allow you to create a plugin extending source command group. See following, which you can try as well.

➜  client git:(issue814) ls -la ~/.config/kn/plugins
total 87240
drwxr-xr-x  5 maximilien  staff       160 Apr 28 15:24 .
drwxr-xr-x  3 maximilien  staff        96 Apr  6 19:03 ..
-rwxr-xr-x  1 maximilien  staff       804 Mar 17 16:57 kn-curl
-rwxr-xr-x  1 maximilien  staff        35 Apr 28 15:24 kn-source-create
-rwx------  1 maximilien  staff  44657756 Apr 23 18:23 kn-source-github
➜  client git:(issue814) cat ~/.config/kn/plugins/kn-source-create
#!/bin/bash
echo "Hello kn: $1 $2"
➜  client git:(issue814) ./kn source create max
Hello kn: max

Let me try and add a fix for kn plugin list and perhaps more tests.

* added tests for the public function * change verifier to use IsAllowedExtensibleCommandGroup to allow plugins for extensible command groups

maximilien · 2020-04-28T23:01:36Z

@navidshaikh see latest updates. Also did a series of local tests that worked well for me:

➜  client git:(issue814) ✗ ls -la ~/.config/kn/plugins
total 87256
drwxr-xr-x  7 maximilien  staff       224 Apr 28 15:49 .
drwxr-xr-x  3 maximilien  staff        96 Apr  6 19:03 ..
-rwxr-xr-x  1 maximilien  staff       804 Mar 17 16:57 kn-curl
-rwxr-xr-x  1 maximilien  staff        35 Apr 28 15:49 kn-service-create
-rwxr-xr-x  1 maximilien  staff        35 Apr 28 15:49 kn-service-max
-rwxr-xr-x  1 maximilien  staff        35 Apr 28 15:24 kn-source-create
-rwx------  1 maximilien  staff  44657756 Apr 23 18:23 kn-source_github

➜  client git:(issue814) ✗ ./kn plugin list
/Users/maximilien/.config/kn/plugins/kn-curl
/Users/maximilien/.config/kn/plugins/kn-service-create
/Users/maximilien/.config/kn/plugins/kn-service-max
/Users/maximilien/.config/kn/plugins/kn-source-create
/Users/maximilien/.config/kn/plugins/kn-source_github
ERRORs:
  - kn-service-create overwrites existing built-in command: kn service create
  - kn-service-max overwrites existing built-in command: kn service
kn-service-create overwrites existing built-in command: kn service create,kn-service-max overwrites existing built-in command: kn service

➜  client git:(issue814) ✗ ./kn service max
Error: unknown subcommand 'max' for 'kn service'. Available subcommands: create, delete, describe, export, list, update
Run 'kn --help' for usage.

➜  client git:(issue814) ✗ ./kn source create max
Hello kn: max

➜  client git:(issue814) rm ~/.config/kn/plugins/kn-service-*

➜  client git:(issue814) ls -la ~/.config/kn/plugins
total 87240
drwxr-xr-x  5 maximilien  staff       160 Apr 28 15:59 .
drwxr-xr-x  3 maximilien  staff        96 Apr  6 19:03 ..
-rwxr-xr-x  1 maximilien  staff       804 Mar 17 16:57 kn-curl
-rwxr-xr-x  1 maximilien  staff        35 Apr 28 15:24 kn-source-create
-rwx------  1 maximilien  staff  44657756 Apr 23 18:23 kn-source_github

➜  client git:(issue814) ./kn plugin list
/Users/maximilien/.config/kn/plugins/kn-curl
/Users/maximilien/.config/kn/plugins/kn-source-create
/Users/maximilien/.config/kn/plugins/kn-source_github

➜  client git:(issue814) ./kn source create max
Hello kn: max

Hopefully we can merge this as @daisy-ycguo and I need this feature for our kn-source-kafka and kn-source-github plugins and the many other source plugins that will follow.

maximilien · 2020-04-28T23:17:54Z

/test pull-knative-client-build-tests

maximilien · 2020-04-28T23:30:12Z

/retest

maximilien · 2020-04-29T00:21:33Z

/hold cancel
/retest

maximilien · 2020-04-29T00:22:17Z

/retest

usable in windows builds

maximilien · 2020-04-29T17:07:02Z

Hi, @navidshaikh, we should be good to go now. Please merge when greeen.

cc @daisy-ycguo

rhuss · 2020-04-29T17:59:51Z

/retest

navidshaikh

Should we consider to allow plugins to extend (all) command groups and report only when there is clash ?

navidshaikh · 2020-05-04T11:11:00Z

pkg/kn/commands/plugin/list_test.go

+						err = ctx.execute("plugin", "list", "--lookup-plugins=true")
+						assert.NilError(t, err)


should this also verify the plugin name from the returned list ?

navidshaikh · 2020-05-04T11:49:33Z

pkg/kn/commands/plugin/plugin.go

+// extended with plugins, e.g., a plugin named `kn-source-kafka` for Kafka
+// event sources is allowed. This is defined as a fixed [...]string since
+// cannot defined Golang []string constants
+var AllowedExtensibleCommandGroups = [...]string{"source"}


~~I think the command group to extend should be configurable outside the kn, i.e config.~~
OR
IMO, it shouldn't mind extending command groups if the plugin commands dont clash with kn's own commands.

Not sure about that since we would not want extensions of core commands

as per today’s discussion, please merge this PR and I will open an issue with your suggestion with more details and open a 24 hours vote. Thanks @navidshaikh and @rhuss

navidshaikh

/lgtm

Lets merge this to unblock contrib source plugins. We'd like to allow extending the command groups via plugins as long as it doesnt clash with kn's own commands.

knative-prow-robot · 2020-05-05T17:46:04Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: maximilien, navidshaikh

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~~ [maximilien,navidshaikh]

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

…ive#818)

googlebot added the cla: yes Indicates the PR's author has signed the CLA. label Apr 21, 2020

knative-prow-robot reviewed Apr 21, 2020

View reviewed changes

knative-prow-robot requested review from cppforlife and navidshaikh April 21, 2020 19:07

knative-prow-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Apr 21, 2020

knative-prow-robot added the ok-to-test Indicates a non-member PR verified by an org member that is safe to test. label Apr 21, 2020

maximilien force-pushed the issue814 branch 7 times, most recently from 628ef79 to 458ad7c Compare April 21, 2020 23:27

maximilien force-pushed the issue814 branch from 458ad7c to 7b488df Compare April 22, 2020 01:00

navidshaikh requested changes Apr 22, 2020

View reviewed changes

knative-prow-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 22, 2020

knative-prow-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Apr 24, 2020

merge latest

fdcd206

* move IsAllowedExtensibleCommandGroup to plugin/verifier.go

6ed686f

* added tests for the public function * change verifier to use IsAllowedExtensibleCommandGroup to allow plugins for extensible command groups

knative-prow-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 28, 2020

knative-prow-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Apr 29, 2020

moved InAllowedExtensibleCommandGroups to plugin.go so that it is

3b0fe24

usable in windows builds

navidshaikh reviewed May 4, 2020

View reviewed changes

navidshaikh approved these changes May 5, 2020

View reviewed changes

knative-prow-robot assigned navidshaikh May 5, 2020

knative-prow-robot added the lgtm Indicates that a PR is ready to be merged. label May 5, 2020

knative-prow-robot merged commit 9774d07 into knative:master May 5, 2020

maximilien mentioned this pull request May 5, 2020

Supporting extending all command groups? #827

Closed

coryrc pushed a commit to coryrc/client that referenced this pull request May 14, 2020

Unify alias name for productivity approvers/reviewers in OWNERS (knat…

601cb70

…ive#818)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fixes(#814) allow plugins to extend the 'source' command group #818

fixes(#814) allow plugins to extend the 'source' command group #818

maximilien commented Apr 21, 2020

knative-prow-robot left a comment

maximilien commented Apr 21, 2020

knative-metrics-robot commented Apr 22, 2020

navidshaikh left a comment •

edited

Loading

maximilien commented Apr 22, 2020 •

edited

Loading

maximilien commented Apr 24, 2020

maximilien commented Apr 28, 2020 •

edited

Loading

maximilien commented Apr 28, 2020 •

edited

Loading

maximilien commented Apr 28, 2020

maximilien commented Apr 28, 2020

maximilien commented Apr 29, 2020

maximilien commented Apr 29, 2020

maximilien commented Apr 29, 2020

rhuss commented Apr 29, 2020

navidshaikh left a comment

navidshaikh May 4, 2020

navidshaikh May 4, 2020 •

edited

Loading

maximilien May 5, 2020

maximilien May 5, 2020

navidshaikh left a comment

knative-prow-robot commented May 5, 2020

		err = ctx.execute("plugin", "list", "--lookup-plugins=true")
		assert.NilError(t, err)

fixes(#814) allow plugins to extend the 'source' command group #818

fixes(#814) allow plugins to extend the 'source' command group #818

Conversation

maximilien commented Apr 21, 2020

Description

Changes

Reference

knative-prow-robot left a comment

Choose a reason for hiding this comment

Description

Changes

Reference

maximilien commented Apr 21, 2020

knative-metrics-robot commented Apr 22, 2020

navidshaikh left a comment • edited Loading

Choose a reason for hiding this comment

maximilien commented Apr 22, 2020 • edited Loading

maximilien commented Apr 24, 2020

maximilien commented Apr 28, 2020 • edited Loading

maximilien commented Apr 28, 2020 • edited Loading

maximilien commented Apr 28, 2020

maximilien commented Apr 28, 2020

maximilien commented Apr 29, 2020

maximilien commented Apr 29, 2020

maximilien commented Apr 29, 2020

rhuss commented Apr 29, 2020

navidshaikh left a comment

Choose a reason for hiding this comment

navidshaikh May 4, 2020

Choose a reason for hiding this comment

navidshaikh May 4, 2020 • edited Loading

Choose a reason for hiding this comment

maximilien May 5, 2020

Choose a reason for hiding this comment

maximilien May 5, 2020

Choose a reason for hiding this comment

navidshaikh left a comment

Choose a reason for hiding this comment

knative-prow-robot commented May 5, 2020

navidshaikh left a comment •

edited

Loading

maximilien commented Apr 22, 2020 •

edited

Loading

maximilien commented Apr 28, 2020 •

edited

Loading

maximilien commented Apr 28, 2020 •

edited

Loading

navidshaikh May 4, 2020 •

edited

Loading