fixes(#827): allow plugins to extend all command groups #834

maximilien · 2020-05-12T00:17:58Z

Description

Allow kn plugins to extend existing command groups. So a plugin named: kn-service-blah is now allowed. It extends the service command group with command blah. These extensions are for any existing command groups. This is per issue #827

Changes

allow plugins to extends all command groups, e.g., kn-service-blah is OK
prevent plugins that shadow commands of existing groups, e.g., kn-service-create is NOT OK
list plugins that extend existing groups
warn plugins that shadow command of existing groups
add UTs for above

Reference

Fixes #827

/lint

knative-prow-robot

@maximilien: 1 warning.

In response to this:

Description

Allow kn plugins to extend existing command groups. So a plugin named: kn-service-blah is now allowed. It extends the service command group with command blah. These extensions are for any existing command groups. This is per issue #827

Changes

allow plugins to extends all command groups, e.g., kn-service-blah is OK

prevent plugins that shadow commands of existing groups, e.g., kn-service-create is NOT OK

list plugins that extend existing groups

warn plugins that shadow command of existing groups

add UTs for above

Reference

Fixes #827

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

pkg/kn/commands/plugin/plugin.go

maximilien · 2020-05-12T00:18:14Z

/ok-to-test

maximilien · 2020-05-12T00:24:49Z

To reviewers. Some local tests I have done that might be useful to verify this yourself.

➜  client git:(issue827) ./kn plugin list
/Users/maximilien/.config/kn/plugins/kn-curl
/Users/maximilien/.config/kn/plugins/kn-service-blah
/Users/maximilien/.config/kn/plugins/kn-service-create
/Users/maximilien/.config/kn/plugins/kn-source-create
/Users/maximilien/.config/kn/plugins/kn-source_github
ERROR:
  - kn-service-create overwrites existing built-in command: kn service create
kn-service-create overwrites existing built-in command: kn service create
➜  client git:(issue827) cat /Users/maximilien/.config/kn/plugins/kn-service-blah
#!/bin/bash
echo "Hello kn: $1 $2"
➜  client git:(issue827) ./kn service blah max
Hello kn: max
➜  client git:(issue827) ./kn service create
required flag(s) "image" not set
➜  client git:(issue827) ./kn service list
NAME          URL                                                                                                            LATEST                AGE   CONDITIONS   READY   REASON
hello-world   http://hello-world-default.kn-plugins-a4511d9cf0901749cf183d1384ce2bca-0000.sjc04.containers.appdomain.cloud   hello-world-kzyhx-1   55d   3 OK / 3     True

maximilien · 2020-05-12T04:39:10Z

Ok looks like I need to add more tests to root.go so that the coverage gets above 50%.

Will work on that tomorrow.

/hold

maximilien · 2020-05-12T19:39:14Z

/hold cancel

…is OK * prevent plugins that shadow commands of exsiting groups, e.g., kn-service-create is NOT OK * list plugins that extend existing groups * warn plugins that shadow command of existing groups * add UTs for above

maximilien · 2020-05-13T00:53:44Z

Looks like I am still not over 50% coverage for root.go :(

* Wait on istio before installing knative serving * retry knative serving install if it fails * retry the serving install once after 60 second if it fails the first time * Added logs for the retry serving install and wait on istio components * Update comment for waiting on istio installation if necessary Co-Authored-By: Adriano Cunha <[email protected]>

rhuss

Thanks, looks good ! I have adde some minor comments/requests inline .

pkg/kn/commands/plugin/plugin.go

pkg/kn/commands/plugin/plugin_test.go

pkg/kn/core/root.go

pkg/kn/core/root_test.go

maximilien · 2020-05-22T22:02:48Z

/retest

go: github.com/google/licenseclassifier upgrade => v0.0.0-20200402202327-879cb1424de0
go: downloading github.com/sergi/go-diff v1.0.0
go: github.com/sergi/go-diff upgrade => v1.1.0
go: downloading github.com/sergi/go-diff v1.1.0
2020/05/22 21:54:37 Error creating license classifier: cannot register licenses: EOF
Step failed: default_build_test_runner
============================
==== BUILD TESTS FAILED ====
============================
==== Fri May 22 14:54:37 PDT 2020
============================
Step failed: run_build_tests
+ EXIT_VALUE=1
+ set +o xtrace

maximilien · 2020-05-22T22:08:39Z

Getting a weird error....

go: github.com/google/licenseclassifier upgrade => v0.0.0-20200402202327-879cb1424de0
go: downloading github.com/sergi/go-diff v1.0.0
go: github.com/sergi/go-diff upgrade => v1.1.0
go: downloading github.com/sergi/go-diff v1.1.0
2020/05/22 21:54:37 Error creating license classifier: cannot register licenses: EOF
Step failed: default_build_test_runner
============================
==== BUILD TESTS FAILED ====
============================
==== Fri May 22 14:54:37 PDT 2020
============================
Step failed: run_build_tests
+ EXIT_VALUE=1
+ set +o xtrace

maximilien · 2020-05-22T22:08:49Z

/retest

maximilien · 2020-05-22T22:17:24Z

Same consistent CI issue:

2020/05/22 21:54:37 Error creating license classifier: cannot register licenses: EOF
Step failed: default_build_test_runner

I'll have to take a look at this next Tuesday --- long weekend here in US due to Memorial Day.

rhuss · 2020-05-25T13:51:33Z

Looks like an incompatible change in the test-infra scripts. @coryrc do you have an idea what has changed ? I think we have the same kind of error over there for client-contrib tests, too.

rhuss · 2020-05-25T15:26:44Z

/retest

rhuss · 2020-05-27T05:36:26Z

@maximilien the IT tests should be running again, but there's a conflict now which needs to be resolved.

CHANGELOG.adoc

* addressed comments on PR * fix bug when dealing with sub sub-commands for known commands

maximilien · 2020-05-28T00:33:20Z

OK thanks @rhuss and @navidshaikh. Just merged. Let's see what other tests I need to add or not.

maximilien · 2020-05-28T21:53:06Z

@rhuss and @navidshaikh this passes now and is ready. However, I want to make some small refactoring to simplify the code in root.go a bit and add more comments. Mostly to make it more readable. Will push an update in next hour and we should be good to go.

* change root.go to match merge conflict * cleaned up NewDefaultKnCommandWithArgs call and added more comments

knative-metrics-robot · 2020-05-28T22:40:31Z

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%	51.6%	-0.1

rhuss · 2020-06-01T09:43:10Z

Cool, let's get that merged now.

As I'm starting to work now to include grouping for help messages (#579) and also to weave in plugin help message is needed. This will probably lead to some refactoring also in this part, so please don't touch those in the next week for now.

/lgtm
/approve

knative-prow-robot · 2020-06-01T09:43:19Z

[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

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

knative-prow-robot requested review from cppforlife and mattmoor May 12, 2020 00:18

knative-prow-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 12, 2020

knative-prow-robot reviewed May 12, 2020

View reviewed changes

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

knative-prow-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label May 12, 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 May 12, 2020

maximilien force-pushed the issue827 branch from 047c199 to 8cae259 Compare May 12, 2020 00:20

maximilien changed the title ~~fixes(#827): allow plugins to extends all command groups~~ fixes(#827): allow plugins to extend all command groups May 12, 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 May 12, 2020

maximilien force-pushed the issue827 branch from 8cae259 to d7b20b8 Compare May 12, 2020 19:38

knative-prow-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels May 12, 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 May 12, 2020

maximilien force-pushed the issue827 branch from d7b20b8 to a286aa0 Compare May 12, 2020 19:41

maximilien force-pushed the issue827 branch from a286aa0 to 953f36e Compare May 13, 2020 00:08

rhuss requested changes May 19, 2020

View reviewed changes

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

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

pkg/kn/core/root.go Outdated Show resolved Hide resolved

pkg/kn/core/root.go Show resolved Hide resolved

pkg/kn/core/root_test.go Show resolved Hide resolved

rhuss mentioned this pull request May 19, 2020

Fix kn source -h #846

Merged

maximilien mentioned this pull request May 21, 2020

Avoid os.Exit(...) calls other than in top level command handling in main.go #850

Closed

danielhelfand mentioned this pull request May 22, 2020

Remove Use of os.Exit() and Return Error Instead #851

Merged

maximilien force-pushed the issue827 branch from cbffda4 to d2556de Compare May 22, 2020 21:50

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

navidshaikh reviewed May 27, 2020

View reviewed changes

CHANGELOG.adoc Outdated Show resolved Hide resolved

* merge branch 'master' into issue827

c1cd120

* addressed comments on PR * fix bug when dealing with sub sub-commands for known commands

maximilien force-pushed the issue827 branch from d2556de to c1cd120 Compare May 28, 2020 00:14

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

* fixed CHANGELOG.md

8008473

* change root.go to match merge conflict * cleaned up NewDefaultKnCommandWithArgs call and added more comments

maximilien force-pushed the issue827 branch from 2a62403 to 8008473 Compare May 28, 2020 22:37

knative-prow-robot assigned rhuss Jun 1, 2020

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

knative-prow-robot merged commit e1ad66a into knative:master Jun 1, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fixes(#827): allow plugins to extend all command groups #834

fixes(#827): allow plugins to extend all command groups #834

maximilien commented May 12, 2020

knative-prow-robot left a comment

maximilien commented May 12, 2020

maximilien commented May 12, 2020

maximilien commented May 12, 2020

maximilien commented May 12, 2020

maximilien commented May 13, 2020

rhuss left a comment

maximilien commented May 22, 2020

maximilien commented May 22, 2020

maximilien commented May 22, 2020

maximilien commented May 22, 2020

rhuss commented May 25, 2020

rhuss commented May 25, 2020

rhuss commented May 27, 2020

maximilien commented May 28, 2020

maximilien commented May 28, 2020 •

edited

Loading

knative-metrics-robot commented May 28, 2020

rhuss commented Jun 1, 2020

knative-prow-robot commented Jun 1, 2020

fixes(#827): allow plugins to extend all command groups #834

fixes(#827): allow plugins to extend all command groups #834

Conversation

maximilien commented May 12, 2020

Description

Changes

Reference

knative-prow-robot left a comment

Choose a reason for hiding this comment

Description

Changes

Reference

maximilien commented May 12, 2020

maximilien commented May 12, 2020

maximilien commented May 12, 2020

maximilien commented May 12, 2020

maximilien commented May 13, 2020

rhuss left a comment

Choose a reason for hiding this comment

maximilien commented May 22, 2020

maximilien commented May 22, 2020

maximilien commented May 22, 2020

maximilien commented May 22, 2020

rhuss commented May 25, 2020

rhuss commented May 25, 2020

rhuss commented May 27, 2020

maximilien commented May 28, 2020

maximilien commented May 28, 2020 • edited Loading

knative-metrics-robot commented May 28, 2020

rhuss commented Jun 1, 2020

knative-prow-robot commented Jun 1, 2020

maximilien commented May 28, 2020 •

edited

Loading