feature(kn plugins): Implements Kn plugins re-using some code from kubectl plugins. #249

maximilien · 2019-07-09T01:11:54Z

This version contains the following:

wraps the main root Kn command to support plugin
plugins are any executable in kn's config new pluginDir
variable which defaults to $PATH
plugins must have name kn-*
'kn plugin list' sub-command to list found kn plugins
skips any kn plugins found with name that match core
commands, e.g., kn-service would be ignored
can execute any valid kn plugins found, e.g.,
kn valid where the plugin file kn-valid is in path
specified in 2.
unit tests (using gotest.tools)

And is missing:

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

I plan to create issues for the things missing so we don't
end up with an even bigger PR. It's already big as is but is a
good MVP as per plugins requirement doc.

Fixes #92

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

/assign @duglin @sixolet

maximilien · 2019-07-09T01:59:55Z

OK so pull-knative-client-go-coverage is failing since I don't have any unit tests for:

pkg/kn/commands/plugin/plugin_handler.go
pkg/kn/commands/plugin/plugin_list.go

Fair and good catch. I will add these tomorrow and update this PR. However, feel free to start your review and/or testing this. Thx.

This version contains the following: 1. wraps the main root Kn command to support plugin 2. plugins are any executable in kn's config new pluginDir variable which defaults to $PATH 3. plugins must have name kn-* 4. 'kn plugin list' sub-command to list found kn plugins 5. skips any kn plugins found with name that match core commands, e.g., kn-service would be ignored 6. can execute any valid kn plugins found, e.g., `kn valid` where the plugin file `kn-valid` is in path specified in 2. 7. unit tests (using gotest.tools) 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 I plan to create issues for the things missing so we don't end up with an even bigger PR. It's already big as is but is a good MVP as per plugins requirement doc.

maximilien · 2019-07-09T18:47:43Z

OK added tests (lots) for pkg/kn/commands/plugin/plugin_list.go.

I'll work on adding for pkg/kn/commands/plugin/plugin_handler.go next.

rhuss · 2019-07-09T20:01:46Z

Maybe now it's a good time to move help_testing.go to helper_test.go ? ;-)

maximilien · 2019-07-09T20:19:37Z

Maybe now it's a good time to move help_testing.go to helper_test.go ? ;-)

OK, we are ping ponging this file, but helper_test.go does not work since it will not compile. I get these failures when I rename:

➜  client git:(kn-plugins4) ✗ ./hack/build.sh
🧽  Format
⚖️  License
📖 Docs
🚧 Compile
🧪  Test
🔥 Failure
=== RUN   TestDummy
--- PASS: TestDummy (0.00s)
PASS
ok  	github.com/knative/client/pkg	(cached)
# github.com/knative/client/pkg/kn/commands/plugin [github.com/knative/client/pkg/kn/commands/plugin.test]
pkg/kn/commands/plugin/path_verifier_test.go:35:18: undefined: commands.CreateTestKnCommand
pkg/kn/commands/plugin/plugin_list_test.go:40:19: undefined: commands.CreateTestKnCommand
[...]

Unless I am missing something, I think the only thing I can do is to have it as test_helper.go if you prefer. Cheers 🍻

rhuss · 2019-07-09T20:42:38Z

Ok, I see.

I really talked with some folks who know testing better than me, and they assured me its possible to have _test.go files without any tests which are used only during tests. That's why is so dogged ;)

Let's get the fixed because I really think that these helpers should be reachable only in test scope.

maximilien · 2019-07-10T16:27:33Z

Let's get the fixed because I really think that these helpers should be reachable only in test scope.

Let me push the pkg/kn/commands/plugin/plugin_handler_test.go today. Which completes this PR and we can see what makes sense. Thx.

maximilien · 2019-07-10T23:51:30Z

OK @rhuss and others, this version has all the test but the coverage fails since I have a common testing_helper.go that the coverage tool needs to ignore. I cannot name it helper_test.go since doing so fails compilation.

I think we need to ask the test-infra team if they have a way to flag or tag a file to be skipped in the coverage test. Similar to tagging files for ignoring during golang builds, e.g.:

// +build !linux
...

Perhaps @adrcunha has some magic up his sleeves already... I know he's been super helpful in the past. Hoping.

cmd/kn/main.go

docs/cmd/kn.md

docs/cmd/kn_completion.md

docs/cmd/kn_plugin.md

adrcunha · 2019-07-11T14:13:29Z

OK @rhuss and others, this version has all the test but the coverage fails since I have a common testing_helper.go that the coverage tool needs to ignore. I cannot name it helper_test.go since doing so fails compilation.

Its current code coverage is at 47%. 3pp more and the code coverage job will pass. If you click the file name in the coverage report above you'll see the methods that weren't tested.

I think I mentioned it in some other PR, but if the requirement of having at least 50% code coverage in PRs is currently too hard, an admin can mark the job optional so it won't block submissions.

docs/cmd/kn_plugin.md

docs/cmd/kn_plugin_list.md

pkg/kn/commands/plugin/path_verifier.go

maximilien · 2019-07-22T22:13:08Z

OK @rhuss and @sixolet. Pushed latest version. It's close but not quite ready as I can see three remaining issues:

using lookupPluginsInPath and pluginsDir options will list correct plugins (for the 4 combinations). However, executing the plugins is trickier. Currently only plugins in pluginsDir is executing. I think I need to remove --lookup-plugins-in-path flag if present to allow current code to work. But I need to test this before committing the change.
Viper loading and saving of config seems broken. Likely how I am using Viper. Essentially, when I change the config and run ./kn ... they are not loaded, so the default is used.
I have some integration tests that I need to also include. However, I need Improves(e2e): improves integration tests and to better run locally #274 to be merged first and simply had not had a chance to rebase 274 so it could be merged. Will do so tonight or tomorrow AM.

That said, one approach if the current code looks good to you is to merge so I can address these issues separately. Otherwise, I'll keep churning tomorrow and get this PR with fixes for above.

Let me know if you also see any additional issues. Thanks.

rhuss · 2019-07-23T17:31:33Z

I will have a look tomorrow morning our time (but from my understanding I'm really not sure that (a) we need both options and (b) whether it should be not only configuration variables.

maximilien · 2019-07-24T00:10:45Z

OK @rhuss and @sixolet this is ready to merge. Assuming I pass CI.

Please note:

I have not added my integration tests since not done, however, manual tests worked. I have lots of plugins on my system in different directories and have a small script (converting to IT) to check.
While your requested changes appear easy, they are not :) but it's done. Pay attention to what I had to do in root.go for instance. Needed since the cobra.Command is actually not executed for a plugin, and thus the flags are not parsed to bound variables. Was fun to discover!
I am sure we can improve things. But I suggest we get this in (assuming no major comments) and we can refine after.

Ping me on Slack if you need my urgent attention on something. Would be good to have this in soon. Thx.

maximilien · 2019-07-24T00:28:04Z

@sixolet see issue #288 for the modifiable prefix for plugins

maximilien · 2019-07-24T00:38:21Z

:) CI green @rhuss @sixolet

rhuss · 2019-07-24T06:31:36Z

@sixolet see issue #288 for the modifiable prefix for plugins

I don't think it was about a configurable prefix what @sixolet meant (but to avoid a prefix in ~/.kn/plugins as its redundant). I added my opinion on this in #288 (comment) and why I think this is not a good idea.

rhuss · 2019-07-25T13:26:30Z

@maximilien I'm going to make my review later today. sorry for the delay, just tons on my plate right now :(

maximilien · 2019-07-25T16:31:46Z

@maximilien I'm going to make my review later today. sorry for the delay, just tons on my plate right now :(

Fully understood. You are great at reviews so not worried. I’ll submit the kn plugins IT as separate PR that depends on this one.

Thanks!

rhuss

Thanks for revisiting this PR ! I'm somewhat fine with this PR (although most of my comments were not addressed) except two things which I consider as blocking issues:

The lookup order should be always pluginDir before PATH and PATH lookup should just be skipped when lookupInPath is false. See feature(kn plugins): Implements Kn plugins re-using some code from kubectl plugins. #249 (comment)
The extracting of plugin names with options interspersed is broken (as already mentioned in the first review). See feature(kn plugins): Implements Kn plugins re-using some code from kubectl plugins. #249 (comment)

When those two are fixed, IMO we can merge that and I would address then the following concerns:

Using os.Separator
No global commandline options, just config vars
Executable check
Only one prefix
Dangerous assumption about the first parameter in Verify()

I think it's better now to elaborate on my points in code than in comments.

docs/cmd/kn.md

pkg/kn/commands/plugin/path_verifier.go

pkg/kn/commands/plugin/plugin_handler.go

knative-metrics-robot · 2019-07-26T18:32:09Z

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	Delta
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	89.2%
pkg/kn/commands/plugin/plugin_list.go	Do not exist	78.0%
pkg/kn/commands/plugin/plugin_test_helper.go	Do not exist	91.7%
pkg/kn/commands/testing_helper.go	Do not exist	97.4%
pkg/kn/core/root.go	52.5%	56.5%	4.0

knative-metrics-robot · 2019-07-26T18:36:10Z

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	Delta
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	89.2%
pkg/kn/commands/plugin/plugin_list.go	Do not exist	78.0%
pkg/kn/commands/plugin/plugin_test_helper.go	Do not exist	91.7%
pkg/kn/commands/testing_helper.go	Do not exist	97.4%
pkg/kn/core/root.go	52.5%	56.5%	4.0

maximilien · 2019-07-26T19:07:19Z

OK @rhuss see latest update. Passes CI and my local ITs---I will submit different issue/PR for that soon today or Monday worst case. Ping me if any other issues. I'll monitor slack. Thx.

rhuss

Thanks a lot for the quick fix ! lgtm and I will follow up with my suggestions which we then can discuss in the next month or so ;-P

/lgtm
/approve

knative-prow-robot · 2019-07-26T19:39:32Z

[APPROVALNOTIFIER] This PR is APPROVED

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

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,rhuss]

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

maximilien · 2019-07-26T19:44:50Z

@sixolet I think you need to also add your lgtm. Or let me know if something urgent is missing. Planning to open issues on items that require more discussions and agreement, e.g., prefix.

rhuss · 2019-07-26T19:53:36Z

@sixolet I think you need to also add your lgtm

I think we are good for now, but with some homework left. 'happy to take that over. Thanks a ton to @maximilien for doing the hard work !

…ve#249) This version contains the following: 1. wraps the main root Kn command to support plugin 2. plugins are any executable in kn's config new pluginDir variable which defaults to $PATH 3. plugins must have name kn-* 4. 'kn plugin list' sub-command to list found kn plugins 5. skips any kn plugins found with name that match core commands, e.g., kn-service would be ignored 6. can execute any valid kn plugins found, e.g., `kn valid` where the plugin file `kn-valid` is in path specified in 2. 7. unit tests (using gotest.tools) 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 I plan to create issues for the things missing so we don't end up with an even bigger PR. It's already big as is but is a good MVP as per plugins requirement doc.

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

knative-prow-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 9, 2019

knative-prow-robot requested review from cppforlife and rhuss July 9, 2019 01:12

knative-prow-robot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Jul 9, 2019

maximilien mentioned this pull request Jul 9, 2019

[WIP] Kn plugins implementation #177

Closed

knative-prow-robot assigned duglin Jul 9, 2019

maximilien force-pushed the kn-plugins4 branch from 179a9d3 to a4c94ae Compare July 9, 2019 18:46

maximilien force-pushed the kn-plugins4 branch from b7941e4 to 9e7b65c Compare July 9, 2019 19:29

maximilien force-pushed the kn-plugins4 branch 2 times, most recently from d44b95b to 0c6ebac Compare July 10, 2019 23:43