Look up plugins on $PATH by default

[psschwei]

Description

Look for knative plugins on the user's $PATH by default

Changes

• Set default value of lookupPluginsInPath to true

This results in the following behavior (copying from Roland's comment):

* Lookup plugins from both, the path and `~/.config/kn/plugins`.
* Same named plugin in `~/.config/kn/plugins` take precedence over plugins found on the PATH.

This would also allow for system-wide plugins as installed by brew in /usr/local/bin, but allow individual users to override plugins in their ~/.config/kn/plugins directory. Also, for people that currently collect their plugins in ~/.config/kn/plugins nothing would change. It's just as if you would add this directory to the front of your $PATH.

Reference

Fixes #1399

/lint

[knative-prow-robot]

@psschwei: 0 warnings.

In response to this:

Description

Look for knative plugins on the user's $PATH by default

Changes

• Removed the lookupInPath option and enabled the logic it gated by default
• Removed all references to disabling the path lookup in the code and tests

This results in the following behavior (copying from Roland's comment):

• Lookup plugins from both, the path and ~/.config/kn/plugins.
• Same named plugin in ~/.config/kn/plugins take precedence over plugins found on the PATH.

This would also allow for system-wide plugins as installed by brew in /usr/local/bin, but allow individual users to override plugins in their ~/.config/kn/plugins directory. Also, for people that currently collect their plugins in ~/.config/kn/plugins nothing would change. It's just as if you would add this directory to the front of your $PATH.

Reference

Fixes #1399

(adoc to be added after PR creation)

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

[codecov]

Codecov Report

Merging #1412 (53e74db) into main (60c740f) will not change coverage.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##             main    #1412   +/-   ##
=======================================
  Coverage   78.51%   78.51%           
=======================================
  Files         160      160           
  Lines        8285     8285           
=======================================
  Hits         6505     6505           
  Misses       1097     1097           
  Partials      683      683           

Impacted Files	Coverage Δ
pkg/kn/config/config.go	61.38% <100.00%> (ø)

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 60c740f...53e74db. Read the comment docs.

[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
pkg/kn/commands/plugin/list.go	87.2%	87.0%	-0.3
pkg/kn/plugin/manager.go	89.8%	90.9%	1.0

[psschwei]

/hold
Removing the lookupPluginsInPath variable was causing trouble with some of the tests, so as a temporary step this PR just sets the value to true.

@rhuss @maximilien any preference as to whether we do this all in one shot (remove the config variable now) or if it's done piecemeal (change default value in this PR, remove variable in follow up PR)? (my thought was the latter to be sure the change would land in the next release)

[psschwei]

test/e2e: TestServiceImport
kubetest2: Test

🤔 Don't think changing the default value of lookupPluginsInPath would cause these to fail...

[maximilien]

You may need to change some tests @psschwei

[maximilien]

/ok-to-test

[psschwei]

You may need to change some tests @psschwei

Interestingly, when I was testing locally, the TestServiceImport e2e test fails when run against the main branch (i.e. with no changes) using either the latest kn release and the binary built directly from source, with the same error message:

Error: cannot lookup plugin kn-service-import-/tmp/kn_file315602552/foo_with_revisions
in directory /home/paulschw/.config/kn/plugins (lookup in path: true): error for looking 
up kn-service-import-/tmp/kn_file315602552/foo_with_revisions in path: exec: 
"kn-service-import-/tmp/kn_file315602552/foo_with_revisions": stat kn-service-
import-/tmpkn_file315602552/foo_with_revisions: no such file or directory

Let me look at the test itself...

(I wasn't able to figure out what exactly the kubetest2 test was, so haven't been able to check that one locally)

[psschwei]

Ok, looks like it's not a test issue, but rather something with how kn is processing the command...

I ran the following in the terminal:

$ kn service import ./hello-world-ksvc.yaml 
Error: cannot lookup plugin kn-service-import-./hello_world_ksvc.yaml in directory
 /home/paulschw/.config/kn/plugins (lookup in path: true): error for looking up
 kn-service-import-./hello_world_ksvc.yaml in path: exec: 
"kn-service-import-./hello_world_ksvc.yaml": stat kn-service-import-./hello_world_ksvc.yaml: 
no such file or directory
Run 'kn --help' for usage

... and got the same error I saw in the test. It appears it's translating kn service import file to kn-service-import-file, which seems to be what's causing the error. And if I don't use ./ before the file name...

$ kn service import hello-world-ksvc.yaml 
Error: provided import file doesn't contain service name, please note that 
only kn's custom export format is supported
Run 'kn --help' for usage

So I'm somewhat confident that the test failure I'm seeing isn't related to my PR. Before I go too far down the rabbit hole looking at kn service import, @rhuss or @maximilien does my explanation here sound reasonable? (and if it is kn service import, should that fix be a separate PR?)

[dsimansk]

Indeed we fail to look up in path when there's slash in arguments. I'm looking into why exec.LookPath would potentially do that.

client/pkg/kn/plugin/manager.go

Lines 468 to 477 in c18a55f

	if lookupInPath {
	path, err = exec.LookPath(name)
	if err == nil {
	// Found in path
	return path, nil
	}
	if !errors.Is(err, exec.ErrNotFound) {
	return "", fmt.Errorf("error for looking up %s in path: %w", name, err)
	}
	}

A simple skip of such argument seems to be enough, before it's added to suspected name.

index f9b9e284..ca7a8c89 100644
--- a/pkg/kn/plugin/manager.go
+++ b/pkg/kn/plugin/manager.go
@@ -403,6 +403,9 @@ func findMostSpecificPluginInPath(dir string, parts []string, lookupInPath bool)
                for _, p := range parts[0:i] {
                        // Subcommands with "-" are translated to "_"
                        // (e.g. a command  "kn log-all" is translated to a plugin "kn-log_all")
+                       if strings.Contains(p, "/") || strings.Contains(p, "\\") {
+                               continue
+                       }
                        nameParts = append(nameParts, convertDashToUnderscore(p))
                        commandParts = append(commandParts, p)
                }

Update:

// LookPath searches for an executable named file in the
// directories named by the PATH environment variable.
// If file contains a slash, it is tried directly and the PATH is not consulted.
// The result may be an absolute path or a path relative to the current directory.

So that's an explanation.

https://cs.opensource.google/go/go/+/refs/tags/go1.16.6:src/os/exec/lp_unix.go;l=33

[dsimansk]

@psschwei that's a nice bug catch! :)

[psschwei]

Thanks @dsimansk !

Can confirm that your fix does solve the test problem:

$ test/local-e2e-tests.sh -run TestServiceImport -v
🧪  Testing
=== RUN   TestServiceImport
=== PAUSE TestServiceImport
=== CONT  TestServiceImport
    service_import_test.go:46: import service foo with revision
    service_import_test.go:56: import existing service foo error
    service_import_test.go:59: import service from missing file error
--- PASS: TestServiceImport (196.99s)
PASS
ok      knative.dev/client/test/e2e     197.003s

(run with kn built from source using fix above)

I opened #1415 as a fix

[psschwei]

/hold cancel
(#1415 merged)

[rhuss]

Looks good, but maybe let's already add a deprecation for this configuration variable ? (so that we can remove it earlier in 0.29 ?)

Let's get this merged and I'll send a followup PR.

/approve
/lgtm

[knative-prow-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: psschwei, 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 [rhuss]

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

[psschwei]

Looks good, but maybe let's already add a deprecation for this configuration variable ? (so that we can remove it earlier in 0.29 ?)

Sorry @rhuss , I totally spaced on the docs ☹️ ... thanks for catching that!