Add support for internal plugins by rhuss · Pull Request #902 · knative/client

rhuss · 2020-06-23T06:39:53Z

This PR adds the possibility for internal plugins. This works by allowing code to add to a global plugin slice plugins.InternalPlugins. By default this slice is empty, but for a custom assembly process (outside of this project), this could be leverage to add plugins directly in the code.

The changes are minimal and well tested, but the PR was created a bit in a hurry so apologies for not having an associated issue yet (but I will create on tomorrow).

mattmoor

Produced via:
gofmt -s -w $(find -path './vendor' -prune -o -path './third_party' -prune -o -type f -name '*.go' -print)
goimports -w $(find -name '*.go' | grep -v vendor | grep -v third_party | grep -v wire_gen.go)

maximilien

Any chance to update the description or link to an issue?
This is listed as chore. Missing the context I guess.

rhuss · 2020-06-24T20:23:20Z

Any chance to update the description or link to an issue?
This is listed as chore. Missing the context I guess.

Ah, sorry. It's kind of an habit to use the conventional commit spec for doing my commit message, and while creating the PR with hub it picked up this message as title by default.

Going to adapt the title, but there is no issue yet. Do you want me to create one or could we discuss the change on this PR ?

rhuss · 2020-06-24T20:30:15Z

/hold

Do not merge until we have discussed the topic on an issue (which I will create tomorrow).

maximilien

Is this ready for review now that #929 is submitted?

rhuss · 2020-07-14T09:31:29Z

I'm going to rebase this PR as soon as #929 is merged, but its already ready to review.

Please see also the proposal at #928 that I have opened.

/unhold

daisy-ycguo

Generally, the changes look good to me. Just a small suggestion to improve the test cases. Also I think we need to add a section to docs/plugins/README.md to describe this feature.

maximilien · 2020-07-27T21:20:35Z

/test pull-knative-client-integration-tests

maximilien · 2020-07-27T21:25:10Z

@rhuss I am not able to checkout your PR. I get the following:

➜  client git:(master) gh pr checkout 902
From https://github.com/knative/client
 ! [rejected]          refs/pull/902/head -> pr/internal-plugins  (non-fast-forward)
exit status 1

Could it be a need to rebase? OR maybe I need to do something on my side... not sure. Let me digg some more

maximilien

Not able to checkout...

rhuss · 2020-07-28T06:32:49Z

@maximilien looks strange. Ok, let me try (but I'm typically not using gh but standard git)

rhuss · 2020-07-28T06:37:50Z

Hmm, I tried it with standard git commands, no issue. Maybe its a problem of gh ?

git clone https://github.com/knative/client.git
cd client
git remote add rhuss https://github.com/rhuss/knative-client.git
git fetch rhuss
git checkout -b internal-plugins rhuss/pr/internal-plugins

rhuss · 2020-08-24T10:20:25Z

/retest

rhuss · 2020-08-24T14:19:53Z

/retest

rhuss · 2020-08-24T18:03:01Z

/retest

rhuss · 2020-08-24T18:05:52Z

The integration test error is not a flake but because of a change in eventing 0.17. We need to fix this before releasing kn 0.17 regardless of whether we get this PR in or not.

maximilien

Getting consistent error trying to run tests locally. Some of the output below. Also reviewed the changes.

➜  client git:(master) ✗ ./hack/build.sh
🚒 Update
⚖️  License
📖 Docs
🚧 Compile
🧪 Test
🔥 Failure
# knative.dev/pkg/test/logging
/Users/maximilien/go/pkg/mod/knative.dev/pkg@v0.0.0-20200812224206-44c860147a87/test/logging/tlogger.go:186:5: o.t.Cleanup undefined (type *testing.T has no field or method Cleanup)
note: module requires Go 1.14
FAIL	knative.dev/client/cmd/kn [build failed]
?   	knative.dev/client/pkg/apis/client	[no test files]
?   	knative.dev/client/pkg/apis/client/v1alpha1	[no test files]
...
FAIL	knative.dev/client/pkg/kn/commands [build failed]
...
FAIL	knative.dev/client/pkg/kn/commands/completion [build failed]
...
FAIL	knative.dev/client/pkg/templates [build failed]
...

rhuss · 2020-08-25T06:20:11Z

@maximilien knative now depends on go 1.14, so you need to upgrade your go installation

…ion test

knative-metrics-robot · 2020-08-25T08:44:36Z

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	87.1%	89.8%	2.8

rhuss · 2020-08-25T09:16:41Z

@dsimansk @navidshaikh @maximilien I think we are good to merge. I adressed all PR comments, and added some extra testing. Also the CI is fixed (will also fix CI issues of other PRs).

Can I have a lgtm please ? (if there are no objections)

navidshaikh

/lgtm

navidshaikh · 2020-08-25T11:31:01Z


+| 🎁
+| Add support for internal plugins
+| https://github.com/knative/client/pull/880[#880]


902, can be fixed in update to changelog PR for release as well.

knative-prow-robot · 2020-08-25T11:34:38Z

[APPROVALNOTIFIER] This PR is APPROVED

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

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~OWNERS~~ [navidshaikh,rhuss]

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

* chore: Add support for internal plugins * added test for internal plugin lookup * add changelog entry * fix formatting * update to latest changes on main * Added more tests + some docs * formatting fix * moved to proper injection label for setting up the broker in integration test

* Add support for internal plugins (#902) * chore: Add support for internal plugins * added test for internal plugin lookup * add changelog entry * fix formatting * update to latest changes on main * Added more tests + some docs * formatting fix * moved to proper injection label for setting up the broker in integration test * update changelog * fix changelog

* chore: Add support for internal plugins * added test for internal plugin lookup * add changelog entry * fix formatting * update to latest changes on main * Added more tests + some docs * formatting fix * moved to proper injection label for setting up the broker in integration test

knative-prow-robot requested review from evankanderson and maximilien June 23, 2020 06:39

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

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

knative-prow-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Jun 23, 2020

mattmoor reviewed Jun 23, 2020

View reviewed changes

dsimansk reviewed Jun 24, 2020

View reviewed changes

Comment thread pkg/kn/plugin/manager.go

maximilien reviewed Jun 24, 2020

View reviewed changes

rhuss changed the title ~~chore: Add support for internal plugins~~ Add support for internal plugins Jun 24, 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 Jun 24, 2020

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

rhuss force-pushed the pr/internal-plugins branch from 853461c to 6ce417a Compare July 13, 2020 06:26

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

This was referenced Jul 13, 2020

Proposal: Add support for internal plugins #927

Closed

Proposal: Allow inlining of golang plugins #928

Closed

maximilien reviewed Jul 13, 2020

View reviewed changes

knative-prow-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 14, 2020

rhuss force-pushed the pr/internal-plugins branch from 6ce417a to 21322ab Compare July 14, 2020 12:58

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

rhuss force-pushed the pr/internal-plugins branch from 21322ab to b28cf16 Compare July 16, 2020 09:29

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

daisy-ycguo suggested changes Jul 21, 2020

View reviewed changes

Comment thread pkg/kn/plugin/manager_test.go

maximilien suggested changes Jul 27, 2020

View reviewed changes

formatting fix

157968f

rhuss force-pushed the pr/internal-plugins branch from b28cf16 to 157968f Compare August 24, 2020 07:45

maximilien suggested changes Aug 24, 2020

View reviewed changes

moved to proper injection label for setting up the broker in integrat…

71c242b

…ion test

rhuss mentioned this pull request Aug 25, 2020

[WIP] Added range options to --scale #986

Closed

navidshaikh approved these changes Aug 25, 2020

View reviewed changes

knative-prow-robot assigned navidshaikh Aug 25, 2020

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

knative-prow-robot merged commit a01861b into knative:master Aug 25, 2020

rhuss mentioned this pull request Aug 25, 2020

Plugin inlining (backport) #991

Merged

navidshaikh added the backport/candidate Consider this PR to be backported to the release branch label Aug 25, 2020

navidshaikh mentioned this pull request Aug 26, 2020

update changelog for 0.17.0 #992

Merged

navidshaikh added backported-to/0.16 and removed backport/candidate Consider this PR to be backported to the release branch labels Oct 12, 2020

navidshaikh modified the milestone: v0.17.0 Nov 23, 2020

navidshaikh linked an issue Nov 23, 2020 that may be closed by this pull request

Proposal: Allow inlining of golang plugins #928

Closed

mgencur referenced this pull request in openshift-knative/client Nov 21, 2022

[release-v0.26.0] Add Func plugin to v0.20.0 (#902)

44a91d7

dsimansk added a commit to dsimansk/client that referenced this pull request Mar 2, 2026

[release-v1.21] Align image dockerfiles (knative#902)

6e9cf0c

Conversation

rhuss commented Jun 23, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mattmoor left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

maximilien left a comment

Choose a reason for hiding this comment

Uh oh!

rhuss commented Jun 24, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rhuss commented Jun 24, 2020

Uh oh!

maximilien left a comment

Choose a reason for hiding this comment

Uh oh!

rhuss commented Jul 14, 2020

Uh oh!

daisy-ycguo left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

maximilien commented Jul 27, 2020

Uh oh!

maximilien commented Jul 27, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

maximilien left a comment

Choose a reason for hiding this comment

Uh oh!

rhuss commented Jul 28, 2020

Uh oh!

rhuss commented Jul 28, 2020

Uh oh!

rhuss commented Aug 24, 2020

Uh oh!

rhuss commented Aug 24, 2020

Uh oh!

rhuss commented Aug 24, 2020

Uh oh!

rhuss commented Aug 24, 2020

Uh oh!

maximilien left a comment

Choose a reason for hiding this comment

Uh oh!

rhuss commented Aug 25, 2020

Uh oh!

knative-metrics-robot commented Aug 25, 2020

Uh oh!

rhuss commented Aug 25, 2020

Uh oh!

navidshaikh left a comment

Choose a reason for hiding this comment

Uh oh!

navidshaikh Aug 25, 2020

Choose a reason for hiding this comment

Uh oh!

knative-prow-robot commented Aug 25, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

rhuss commented Jun 23, 2020 •

edited

Loading

rhuss commented Jun 24, 2020 •

edited

Loading

maximilien commented Jul 27, 2020 •

edited

Loading