Repository: knative/client
Pull request #1415 (2021-08-04 18:49:55)

Fixing bug when looking up plugins on path with slash in argument
## Description

Per discussion in [PR 1412](https://github.com/knative/client/pull/1412#issuecomment-892857273), found a bug where we fail to look up in path when there's slash in arguments. (This bug was also causing the service import test to fail). 

## Changes

* fixing bug when looking up plugins on path with slash in argument

(changelog entry to follow)

<!--
Please add an entry to CHANGELOG.adoc file, too, as part of your Pull Request.

In the following cases, add a short description of PR to the unreleased section in CHANGELOG.adoc:

- 🎁 New feature
- 🐛 Bug fix
- ✨ Feature Update
- 🐣 Refactoring
- 🗑️ Remove feature or internal logic

See other entries in CHANGELOG.adoc as an example for how to add the entry, including a reference to the corresponding issue/PR

PLEASE DON'T ADD THAT LINE HERE IN THE PULL-REQUEST DESCRIPTION BUT DIRECTLY IN CHANGELOG.ADOC AND ADD CHANGELOG.ADOC AS PART OF YOUR PULL-REQUEST.
-->

<!--
To automatically lint go code in this pull request uncomment the line below. You get feedback as comments on your pull request then -->

/lint


knative-prow-robot: [APPROVALNOTIFIER] This PR is **NOT APPROVED**

This pull-request has been approved by: *<a href="https://github.com/knative/client/pull/1415#" title="Author self-approved">psschwei</a>*
To complete the [pull request process](https://git.k8s.io/community/contributors/guide/owners.md#the-code-review-process), please assign **maximilien** after the PR has been reviewed.
You can assign the PR to them by writing `/assign @maximilien` in a comment when ready.

The full list of commands accepted by this bot can be found [here](https://go.k8s.io/bot-commands?repo=knative%2Fclient).

<details open>
Needs approval from an approver in each of these files:

- **[OWNERS](https://github.com/knative/client/blob/main/OWNERS)**

Approvers can indicate their approval by writing `/approve` in a comment
Approvers can cancel approval by writing `/approve cancel` in a comment
</details>
<!-- META={"approvers":["maximilien"]} -->
codecov[bot]: # [Codecov](https://codecov.io/gh/knative/client/pull/1415?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=knative) Report
> Merging [#1415](https://codecov.io/gh/knative/client/pull/1415?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=knative) (180ecd5) into [main](https://codecov.io/gh/knative/client/commit/c18a55f92610fbc5a24bd7b800ee16c1b9e15734?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=knative) (c18a55f) will **decrease** coverage by `0.01%`.
> The diff coverage is `0.00%`.

[![Impacted file tree graph](https://codecov.io/gh/knative/client/pull/1415/graphs/tree.svg?width=650&height=150&src=pr&token=bkTp6BzJiV&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=knative)](https://codecov.io/gh/knative/client/pull/1415?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=knative)

```diff
@@            Coverage Diff             @@
##             main    #1415      +/-   ##
==========================================
- Coverage   78.33%   78.31%   -0.02%     
==========================================
  Files         160      160              
  Lines        8270     8272       +2     
==========================================
  Hits         6478     6478              
- Misses       1103     1104       +1     
- Partials      689      690       +1     
```


| [Impacted Files](https://codecov.io/gh/knative/client/pull/1415?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=knative) | Coverage Δ | |
|---|---|---|
| [pkg/kn/plugin/manager.go](https://codecov.io/gh/knative/client/pull/1415/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=knative#diff-cGtnL2tuL3BsdWdpbi9tYW5hZ2VyLmdv) | `82.92% <0.00%> (-1.03%)` | :arrow_down: |

------

[Continue to review full report at Codecov](https://codecov.io/gh/knative/client/pull/1415?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=knative).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=knative)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/knative/client/pull/1415?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=knative). Last update [c18a55f...180ecd5](https://codecov.io/gh/knative/client/pull/1415?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=knative). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=knative).

dsimansk: @psschwei thansk!
Could you please try to add unit test that would verify plugin with slash in arguments won't have that slash part in name maybe? If it's too cumbersome to reach that meaningful coverage, I'd happy to go with it.


@rhuss as a lesson here, maybe it isn't a good idea to have file to path in `args`. I recall some review feedback when working on `kn service import`. :)
psschwei: As I'm writing a test, I have a question about what the assumptions are for plugin arguments.

Take this example:

```
kn plugin test /with/slash argument
```

According to the logic in this PR, that would "find" a plugin called `kn-plugin-test-argument` (i.e. it would simply drop the argument containing a slash). Is that the behavior we'd expect? Or do we expect that `kn` would drop ALL terms after and including one with a slash (i.e. our plugin would be `kn-plugin-test` and `/with/slash` and `argument` are parameters for the plugin)?

If it's the latter, then I think we'd need to update the logic some to parse the [initial `parts` slice](https://github.com/knative/client/blob/180ecd5057f5d3da0dd8f812b6dc68dcd89849e3/pkg/kn/plugin/manager.go#L398) before processing. 


dsimansk: >  Or do we expect that kn would drop ALL terms after and including one with a slash (i.e. our plugin would be kn-plugin-test and /with/slash and argument are parameters for the plugin)?

Yes, I believe this is already true with current implementation.

I hope we may safely assume that there won't plugin sub-command with slashes in name.

Handling of plugin name and arguments is done by the [outer for-loop ](https://github.com/knative/client/blob/main/pkg/kn/plugin/manager.go#L398) that's moving through params array and reducing it.
Until the plugin can recognized/found, the plugin object is returned. In addition, original args are preserved for the [execution time](https://github.com/knative/client/blob/main/cmd/kn/main.go#L100). Therefore dropping arg with slashes in look-up function shouldn't result in missing arg in execution.

I believe it should go like this.
```
kn plugin test arg0 /arg1/slash arg2
```

```
/arg1/slash => always dropped due to slashes breaking on path lookup
kn plugin test arg0 arg2 => plugin not found
kn plugin test arg0  => plugin not found
kn plugin test  => found 

//the rest of original array [arg0, /arg1/slash/, arg2] are arguments to the plugin cmd "kn plugin test"
```
psschwei: Got it -- I think I was confusing the plugin lookup and actually executing the plugin :) 
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/plugin/manager.go](https://storage.cloud.google.com/knative-prow/pr-logs/pull/knative_client/1415/pull-knative-client-go-coverage/1423044343236661248/artifacts/line-cov.html#file0) | 89.8% | 89.9% | 0.1

dsimansk: Thanks for the fix & the test coverage @psschwei!

@rhuss @maximilien can you please take a look?

/lgtm
knative-prow-robot: New changes are detected. LGTM label has been removed.
knative-prow-robot: [APPROVALNOTIFIER] This PR is **NOT APPROVED**

This pull-request has been approved by: *<a href="https://github.com/knative/client/pull/1415#" title="Author self-approved">psschwei</a>*
To complete the [pull request process](https://git.k8s.io/community/contributors/guide/owners.md#the-code-review-process), please ask for approval from **dsimansk** after the PR has been reviewed.

The full list of commands accepted by this bot can be found [here](https://go.k8s.io/bot-commands?repo=knative%2Fclient).

<details open>
Needs approval from an approver in each of these files:

- **[OWNERS](https://github.com/knative/client/blob/main/OWNERS)**

Approvers can indicate their approval by writing `/approve` in a comment
Approvers can cancel approval by writing `/approve cancel` in a comment
</details>
<!-- META={"approvers":["dsimansk"]} -->
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/plugin/manager.go](https://storage.cloud.google.com/knative-prow/pr-logs/pull/knative_client/1415/pull-knative-client-go-coverage/1423301566516957184/artifacts/line-cov.html#file0) | 89.8% | 89.9% | 0.1

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/plugin/manager.go](https://storage.cloud.google.com/knative-prow/pr-logs/pull/knative_client/1415/pull-knative-client-go-coverage/1423420348640006144/artifacts/line-cov.html#file0) | 89.8% | 89.9% | 0.1

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/plugin/manager.go](https://storage.cloud.google.com/knative-prow/pr-logs/pull/knative_client/1415/pull-knative-client-go-coverage/1423659433711374336/artifacts/line-cov.html#file0) | 89.8% | 89.9% | 0.1

knative-prow-robot: @psschwei: The following test **failed**, say `/retest` to rerun all failed tests or `/retest-required` to rerun all mandatory failed tests:

Test name | Commit | Details | Rerun command
--- | --- | --- | ---
pull-knative-client-integration-tests | bb380a85bba7312d4ca8dbb24d34d8ba22caed51 | [link](https://prow.knative.dev/view/gs/knative-prow/pr-logs/pull/knative_client/1415/pull-knative-client-integration-tests/1423659433682014208) | `/test pull-knative-client-integration-tests`

[Full PR test history](https://gubernator.knative.dev/pr/knative_client/1415). [Your PR dashboard](https://gubernator.knative.dev/pr/psschwei).

<details>

Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md).  If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository. I understand the commands that are listed [here](https://go.k8s.io/bot-commands).
</details>
<!-- test report -->
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/plugin/manager.go](https://storage.cloud.google.com/knative-prow/pr-logs/pull/knative_client/1415/pull-knative-client-go-coverage/1423667803252068352/artifacts/line-cov.html#file0) | 89.8% | 89.9% | 0.1

knative-prow-robot: [APPROVALNOTIFIER] This PR is **APPROVED**

This pull-request has been approved by: *<a href="https://github.com/knative/client/pull/1415#" title="Author self-approved">psschwei</a>*, *<a href="https://github.com/knative/client/pull/1415#pullrequestreview-724488253" title="LGTM">rhuss</a>*

The full list of commands accepted by this bot can be found [here](https://go.k8s.io/bot-commands?repo=knative%2Fclient).

The pull request process is described [here](https://git.k8s.io/community/contributors/guide/owners.md#the-code-review-process)

<details >
Needs approval from an approver in each of these files:

- ~~[OWNERS](https://github.com/knative/client/blob/main/OWNERS)~~ [rhuss]

Approvers can indicate their approval by writing `/approve` in a comment
Approvers can cancel approval by writing `/approve cancel` in a comment
</details>
<!-- META={"approvers":[]} -->

Patch:
diff --git a/pkg/kn/plugin/manager.go b/pkg/kn/plugin/manager.go
index f9b9e284..c5f6c3d0 100644
--- a/pkg/kn/plugin/manager.go
+++ b/pkg/kn/plugin/manager.go
@@ -401,6 +401,11 @@ func findMostSpecificPluginInPath(dir string, parts []string, lookupInPath bool)
 		var nameParts []string
 		var commandParts []string
 		for _, p := range parts[0:i] {
+			// for arguments that contain the path separator,
+			// stop the loop once the separator appears
+			if strings.Contains(p, string(os.PathSeparator)) {
+				break
+			}
 			// Subcommands with "-" are translated to "_"
 			// (e.g. a command  "kn log-all" is translated to a plugin "kn-log_all")
 			nameParts = append(nameParts, convertDashToUnderscore(p))


Test patch:
diff --git a/pkg/kn/plugin/manager_test.go b/pkg/kn/plugin/manager_test.go
index 57614631..55bf1edc 100644
--- a/pkg/kn/plugin/manager_test.go
+++ b/pkg/kn/plugin/manager_test.go
@@ -301,6 +301,54 @@ func TestPluginList(t *testing.T) {
 	}
 }
 
+func TestNoSlashInPlugin(t *testing.T) {
+	ctx := setup(t)
+	defer cleanup(t, ctx)
+
+	// Prepare PATH
+	tmpPathDir, cleanupFunc := preparePathDirectory(t)
+	defer cleanupFunc()
+
+	var middleSlashPlugin string
+	var middleSlashArg string
+	ctx.pluginManager.lookupInPath = true
+
+	// Windows does not allow slashes in filenames, so testing for a slash
+	// in the middle of an argument will have different results depending on OS
+	if os.PathSeparator == '/' {
+		middleSlashPlugin = "kn-slash-test-with\\slash"
+		middleSlashArg = "with\\slash"
+	} else {
+		middleSlashPlugin = "kn-slash-test"
+		middleSlashArg = "with/slash"
+	}
+
+	createTestPluginInDirectory(t, "kn-slash-test", tmpPathDir)
+	createTestPluginInDirectory(t, middleSlashPlugin, tmpPathDir)
+
+	data := []struct {
+		parts []string
+		name  string
+	}{
+		{[]string{"slash", "test", string(os.PathSeparator) + "withslash"}, "kn-slash-test"},
+		{[]string{"slash", "test", string(os.PathSeparator) + "withslash", "extraarg"}, "kn-slash-test"},
+		{[]string{"slash", "test", "with" + string(os.PathSeparator) + "slash", "extraarg"}, "kn-slash-test"},
+		{[]string{"slash", "test", middleSlashArg}, middleSlashPlugin},
+	}
+
+	for _, d := range data {
+		plugin, err := ctx.pluginManager.FindPlugin(d.parts)
+		assert.NilError(t, err)
+		assert.Assert(t, plugin != nil)
+		desc, err := plugin.Description()
+		name := plugin.Name()
+		assert.NilError(t, err)
+		assert.Assert(t, desc != "")
+		assert.Equal(t, plugin.Path(), filepath.Join(tmpPathDir, d.name))
+		assert.Assert(t, !strings.Contains(name, string(os.PathSeparator)))
+	}
+}
+
 // ====================================================================
 // Private