refactor(plugins): Improved and simplified verifier and plugin list #313

rhuss · 2019-07-27T14:15:19Z

First sweep:

Added proper executable check for the current user (+ extensive tests for all combinations)
Refactor plugin_list to make it more straightforward
Separate errors and warnings
Don't return an error when no plugin is installed and plugin list is called
Simplified tests
Check assumption that a prefix is given in verify()
Fixed broken homedir expansion ( ~/ (not ~) must be expanded only when at the beginning of a path)

After this:

Remove plugin options from the command line
Remove multiple prefix support

This PR is a result of the discussion on #249 where we agreed to merge now (as the PR has been aged and grown considerably) but work on improvements right after.

rhuss · 2019-07-27T14:52:39Z

Note to reviewers: As this a refactoring PR the diff is not easy to read. All changes are confined to the plugin package.

I suggest to start at those files with these entry points:

For the refactored verifier look here --> https://github.com/knative/client/pull/313/files#diff-cf339cceb78d7b2e50576e72699ffce3R60
For the simplified plugin list command here -->

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

Line 61 in c2bd8d2

func listPlugins(cmd *cobra.Command, flags pluginListFlags) error {
For the changed tests
- plugin_list_test:
  
  client/pkg/kn/commands/plugin/plugin_list_test.go
  
  Line 93 in c2bd8d2
  
  t.Run("creates a new cobra.Command", func(t *testing.T) {
- plugin_verifier:
  
  client/pkg/kn/commands/plugin/plugin_verifier_test.go
  
  Line 46 in c2bd8d2
  
  t.Run("with nil root command", func(t *testing.T) {

rhuss · 2019-07-28T09:33:48Z

/retest

rhuss · 2019-07-28T15:15:57Z

/retest

maximilien · 2019-08-01T20:27:11Z

@rhuss I will get to this next week. Lots of things to tie up this week and also taking a few days off for a quick trip to London.

I will move this up my TODO list once I am back or if I have WiFi in plane on Tuesday. Promise :)

duglin · 2019-08-06T14:51:44Z

Can you also address this:

$ kn --lookup-plugins-in-path=true plugin list
Unable read directory '/root/.kn/plugins' from your plugins path: open /root/.kn/plugins: no such file or directory. Skipping...The following compatible plugins are available, using options:
  - plugins dir: '~/.kn/plugins'
  - lookup plugins in path: 'true'

/root/bin/kn-abc
Unable read directory '/root/src/k8s.io/kubernetes/_output/dockerized/bin/linux/amd64' from your plugins path: open /root/src/k8s.io/kubernetes/_output/dockerized/bin/linux/amd64: no such file or directory. Skipping...

That's just weird output ;-)

and

$ kn --lookup-plugins-in-path=true plugin list
Unable read directory '/root/.kn/plugins' from your plugins path: open /root/.kn/plugins: no such file or directory. Skipping...Unable read directory '/root/src/k8s.io/kubernetes/_output/dockerized/bin/linux/amd64' from your plugins path: open /root/src/k8s.io/kubernetes/_output/dockerized/bin/linux/amd64: no such file or directory. Skipping...
warning: unable to find any kn plugins in your plugin path: '[/root/.kn/plugins /root/bin /usr/local/sbin /usr/local/bin /usr/sbin /usr/bin /sbin /bin /usr/games /usr/local/games /usr/local/go/bin /root/src/bin /root/src/k8s.io/kubernetes/_output/local/go/bin /root/src/k8s.io/kubernetes/_output/dockerized/bin/linux/amd64]'

Should be empty.

$ ./kn plugin list
No plugins found in path /root/.kn/plugins

It should show nothing IMO

rhuss · 2019-08-06T15:03:13Z

@duglin

Non-existing path directories are ignored (like for real path handling), so no output here anymore
Configuration of where to lookup configuration is only shown with --verbose
For no plugin installed kn plugin list shows

No plugins found in ~/.kn/plugins

The later error message could be improved to say "No plugins are found in ~/.kn/plugins nor in your path" if path lookup is enable

rhuss · 2019-08-06T15:04:19Z

It should show nothing IMO

Yes, that could be done, too. I suggest to print nothing, but with --verbose we print the line above.

duglin · 2019-08-06T15:10:50Z

For no plugin installed kn plugin list shows

No plugins found in ~/.kn/plugins

I don't see the value in showing this - let it be controlled via --verbose as you said. But if we MUST show something w/o --verbose then I would suggest you remove the in ~/.kn/plugins since for consistency you should then show the $PATH value when we search $PATH - and that's just annoying. Also, we don't need to remind people where the dir is.

duglin · 2019-08-06T15:52:14Z

It should show nothing IMO

Yes, that could be done, too. I suggest to print nothing, but with --verbose we print the line above.

Yep. I tend to look at things like this from two perspectives:
1 - a non-day-zero user. Meaning, I've been using the tool for a long time and want "just the facts" - and any noise I have to "skip over" is just annoying. So --verbose makes a lot more sense to me. As an extreme case imagine if ls did that:

$ ls
no files found

yuck!! ;-)
2 - scripting. I tend to bash script a lot of these things and being able to know if there's output via [[ -z ${output} ]] (or wc) is really really handy. Otherwise I have to do ugly grep -v stuff - which is just annoying for something everyone will just ignore.

rhuss · 2019-08-06T17:05:27Z

/retest

sixolet

Actually not bad to review at all :)

pkg/kn/commands/plugin/plugin_verifier.go

* Added proper executable check for current user * Refactor plugin_list to make it more straightforward * Seperate errors and warnings * Don't return an error when no plugin is installed and `plugin list` is called * Simplified tests * Check assumption that a prefix is given in verify()

E.g. if u-x, g+x and the user has the group of the file, but is also owner of the file then he is not allowed to execute the file.

Also, allow symlinks without warnings.

rhuss · 2019-08-07T08:06:43Z

/retest

maximilien · 2019-08-07T23:03:37Z

pkg/kn/commands/plugin/plugin_list_test.go

-		pluginsDir = filepath.Join(tmpPathDir, "plugins")
-		pluginsDirFlag = fmt.Sprintf("--plugins-dir=%s", pluginsDir)
+func (ctx *testContext) createTestPluginWithPath(pluginName string, fileMode os.FileMode, path string) error {
+	if runtime.GOOS == "windows" {


Nice. I completely forgot and ignored Windows users. But good on this.

Though I have no way to test actual Windows plugin

same for me ;-( I think we need at least a CI test suite somehow. I wonder how other CLI's like kubectl does it ?

maximilien · 2019-08-07T23:10:01Z

pkg/kn/commands/plugin/plugin_verifier.go

+	}
+}
+
+func printLabelWithConditionalPluralS(out io.Writer, label string, nr int) {


...PluralS... why not ...Plurals?

Because I refer to the plural "S" to be added at the end, and I don't refer to the plural of plural ;-) (which would be "plurals", supposed that something like this exists)

OK cool... weird, but cool :)

pkg/kn/commands/plugin/plugin_verifier.go

maximilien

Overall looks good. Left some comments, mostly minor. I still think we should make plugin list verbose by default since the point of the command in my view is to allow the user to see the various plugins available. It’s important that she knows which plugins are valid, executable, and “ready” versus those that are causing problems since otherwise how would she know? So overall for me errors and warnings are important to give as much info as possible to the user for that command.

knative-prow-robot · 2019-08-07T23:16:24Z

[APPROVALNOTIFIER] This PR is APPROVED

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

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

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

rhuss · 2019-08-08T06:52:58Z

So overall for me errors and warnings are important to give as much info as possible to the user for that command.

warnings and errors are printed even without --verbose. It's only that the static configuration information where plugins are looked up is hidden without --verbose.

…hem shorter

… exists

knative-metrics-robot · 2019-08-08T07:14:47Z

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/plugin_list.go	78.0%	81.8%	3.8
pkg/kn/commands/plugin/plugin_list_flags.go	Do not exist	100.0%
pkg/kn/commands/plugin/plugin_test_helper.go	91.7%	86.7%	-5.0
pkg/kn/commands/plugin/plugin_verifier.go	Do not exist	78.2%

rhuss · 2019-08-08T09:16:08Z

/retest

maximilien · 2019-08-08T21:05:31Z

/lgtm

* Add RPM SPEC file Minimize the file diff between midstream and downstream. * Update RELEASE.md with instructions to update SPEC file and Dockerfile.cliartifacts.rhel * Remove gcc from BuildRequires

* Add image to back up artifacts This copies images from gcr.io/knative-releases into the gcr.io/knative-backups registry using gcrane. This also copies knative release yamls (kube manifests) from gs://knative-releases to gs://knative-backups using gsutil. * Add job to back up artifacts This just wires up the prow config to use the backups image. * Properly escape $ in shell command in Makefile Co-Authored-By: jonjohnsonjr <[email protected]> * Remove resource requests

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

knative-prow-robot requested review from cppforlife and mattmoor July 27, 2019 14:15

knative-prow-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Jul 27, 2019

rhuss force-pushed the pr/plugin-improvements branch from 5efbc97 to c2bd8d2 Compare July 27, 2019 14:34

rhuss requested review from sixolet and maximilien and removed request for mattmoor July 28, 2019 06:17

knative-prow-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Jul 28, 2019

rhuss force-pushed the pr/plugin-improvements branch from a07bdb1 to a0ce710 Compare July 28, 2019 08:57

rhuss force-pushed the pr/plugin-improvements branch 2 times, most recently from 52beec1 to cc7667d Compare July 28, 2019 17:50

navidshaikh mentioned this pull request Jul 29, 2019

plugin list command error #314

Closed

This was referenced Aug 6, 2019

Introduce --debug and --verbose as global options #333

Open

Don't show warnings that are not bad conditions #339

Closed

sixolet approved these changes Aug 6, 2019

View reviewed changes

pkg/kn/commands/plugin/plugin_verifier.go Outdated Show resolved Hide resolved

rhuss added 2 commits August 7, 2019 08:33

fix(plugins): Fixed broken homedir expansion

1b1ffae

rhuss added 4 commits August 7, 2019 08:33

fix(plugin): Adapted to proper permission handling

40722b2

E.g. if u-x, g+x and the user has the group of the file, but is also owner of the file then he is not allowed to execute the file.

test(plugins): Added checks for the execution check for plugins

882560d

chore(plugins): optimized tests to reflect the real unix behaviour

7c3be1f

chore(plugins): Moved plugin seenMap to top-level verify method.

4cafffd

Also, allow symlinks without warnings.

rhuss force-pushed the pr/plugin-improvements branch from cc7667d to 4cafffd Compare August 7, 2019 07:28

maximilien reviewed Aug 7, 2019

View reviewed changes

pkg/kn/commands/plugin/plugin_verifier.go Show resolved Hide resolved

maximilien approved these changes Aug 7, 2019

View reviewed changes

maximilien mentioned this pull request Aug 7, 2019

Kn plugins integration test #308

Closed

rhuss added 2 commits August 8, 2019 08:53

refactor(plugins): Split up some methods in plugin_verifier to make t…

d64e381

…hem shorter

fix: Tuned plugin list for better handling when no plugin directory…

0bc456c

… exists

knative-prow-robot assigned maximilien Aug 8, 2019

knative-prow-robot added the lgtm Indicates that a PR is ready to be merged. label Aug 8, 2019

knative-prow-robot merged commit 3594b39 into knative:master Aug 8, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor(plugins): Improved and simplified verifier and plugin list #313

refactor(plugins): Improved and simplified verifier and plugin list #313

rhuss commented Jul 27, 2019 •

edited

Loading

rhuss commented Jul 27, 2019 •

edited

Loading

rhuss commented Jul 28, 2019

rhuss commented Jul 28, 2019

maximilien commented Aug 1, 2019

duglin commented Aug 6, 2019

rhuss commented Aug 6, 2019

rhuss commented Aug 6, 2019

duglin commented Aug 6, 2019

duglin commented Aug 6, 2019 •

edited

Loading

rhuss commented Aug 6, 2019

sixolet left a comment

rhuss commented Aug 7, 2019

maximilien Aug 7, 2019

maximilien Aug 7, 2019

rhuss Aug 8, 2019

maximilien Aug 7, 2019

rhuss Aug 8, 2019

maximilien Aug 8, 2019

maximilien left a comment

knative-prow-robot commented Aug 7, 2019

rhuss commented Aug 8, 2019

knative-metrics-robot commented Aug 8, 2019

rhuss commented Aug 8, 2019

maximilien commented Aug 8, 2019

refactor(plugins): Improved and simplified verifier and plugin list #313

refactor(plugins): Improved and simplified verifier and plugin list #313

Conversation

rhuss commented Jul 27, 2019 • edited Loading

rhuss commented Jul 27, 2019 • edited Loading

rhuss commented Jul 28, 2019

rhuss commented Jul 28, 2019

maximilien commented Aug 1, 2019

duglin commented Aug 6, 2019

rhuss commented Aug 6, 2019

rhuss commented Aug 6, 2019

duglin commented Aug 6, 2019

duglin commented Aug 6, 2019 • edited Loading

rhuss commented Aug 6, 2019

sixolet left a comment

Choose a reason for hiding this comment

rhuss commented Aug 7, 2019

maximilien Aug 7, 2019

Choose a reason for hiding this comment

maximilien Aug 7, 2019

Choose a reason for hiding this comment

rhuss Aug 8, 2019

Choose a reason for hiding this comment

maximilien Aug 7, 2019

Choose a reason for hiding this comment

rhuss Aug 8, 2019

Choose a reason for hiding this comment

maximilien Aug 8, 2019

Choose a reason for hiding this comment

maximilien left a comment

Choose a reason for hiding this comment

knative-prow-robot commented Aug 7, 2019

rhuss commented Aug 8, 2019

knative-metrics-robot commented Aug 8, 2019

rhuss commented Aug 8, 2019

maximilien commented Aug 8, 2019

rhuss commented Jul 27, 2019 •

edited

Loading

rhuss commented Jul 27, 2019 •

edited

Loading

duglin commented Aug 6, 2019 •

edited

Loading