Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

kn plugins for eventing sources should be able to extend kn source group #814

Closed
maximilien opened this issue Apr 21, 2020 · 5 comments · Fixed by #818
Closed

kn plugins for eventing sources should be able to extend kn source group #814

maximilien opened this issue Apr 21, 2020 · 5 comments · Fixed by #818
Assignees
Labels
kind/feature New feature or request
Milestone

Comments

@maximilien
Copy link
Contributor

Feature request

Current kn plugin architecture prevents plugins to extend existing command groups. This makes sense since we would not want a kn-service-create plugin that redefines the service create commands. Same for almost all other kn command groups.

But there is at least one exception. This is for command groups where we want to define sub-groups but at the same time allow extensions. One such example is the kn source eventing command group.

Use case

Kn ships with some eventing sources, e.g., ping, but we also want to have plugins be created and defined that also extends this command group, e.g., source kafka and source github and others.

These plugins are being defined in the client-contrib repo for instance the kn-source-kafka is one such example.

UI Example

kn source kafka create my-kafka-source --sink my-kafka-sink ...
...
kn source github create my-hithub-source --sink my-github-sink
...

/kind proposal
/kind bug

@maximilien maximilien added the kind/feature New feature or request label Apr 21, 2020
@maximilien
Copy link
Contributor Author

/assign @maximilien

@rhuss
Copy link
Contributor

rhuss commented Apr 21, 2020

Current kn plugin architecture prevents plugins to extend existing command groups. This makes sense since we would not want a kn-service-create plugin that redefines the service create commands. Same for almost all other kn command groups.

tbh, I thought this would be true only for case where a terminal command would overwrite an built-in command (actually the command on which an "Run" is attached).

I suggest to implement this in the generic fashion to just verify that there is no collision on the terminal (sub) command.

maximilien added a commit to maximilien/client that referenced this issue Apr 21, 2020
* introduce a list of command group that can be extended (currently only source)
* check if plugin main group is in that list and execute, otherwise fail
* add e2e test for both plugin that is allowed and not allowed to extend existing group
maximilien added a commit to maximilien/client that referenced this issue Apr 21, 2020
* introduce a list of command group that can be extended (currently only source)
* check if plugin main group is in that list and execute, otherwise fail
* add e2e test for both plugin that is allowed and not allowed to extend existing group
maximilien added a commit to maximilien/client that referenced this issue Apr 21, 2020
* introduce a list of command group that can be extended (currently only source)
* check if plugin main group is in that list and execute, otherwise fail
* add e2e test for both plugin that is allowed and not allowed to extend existing group
maximilien added a commit to maximilien/client that referenced this issue Apr 21, 2020
* introduce a list of command group that can be extended (currently only source)
* check if plugin main group is in that list and execute, otherwise fail
* add e2e test for both plugin that is allowed and not allowed to extend existing group
maximilien added a commit to maximilien/client that referenced this issue Apr 21, 2020
* introduce a list of command group that can be extended (currently only source)
* check if plugin main group is in that list and execute, otherwise fail
* add e2e test for both plugin that is allowed and not allowed to extend existing group
maximilien added a commit to maximilien/client that referenced this issue Apr 21, 2020
* introduce a list of command group that can be extended (currently only source)
* check if plugin main group is in that list and execute, otherwise fail
* add e2e test for both plugin that is allowed and not allowed to extend existing group
maximilien added a commit to maximilien/client that referenced this issue Apr 21, 2020
* introduce a list of command group that can be extended (currently only source)
* check if plugin main group is in that list and execute, otherwise fail
* add e2e test for both plugin that is allowed and not allowed to extend existing group
maximilien added a commit to maximilien/client that referenced this issue Apr 21, 2020
* introduce a list of command group that can be extended (currently only source)
* check if plugin main group is in that list and execute, otherwise fail
* add e2e test for both plugin that is allowed and not allowed to extend existing group
maximilien added a commit to maximilien/client that referenced this issue Apr 22, 2020
* introduce a list of command group that can be extended (currently only source)
* check if plugin main group is in that list and execute, otherwise fail
* add e2e test for both plugin that is allowed and not allowed to extend existing group
@maximilien
Copy link
Contributor Author

tbh, I thought this would be true only for case where a terminal command would overwrite an built-in command (actually the command on which an "Run" is attached).

I think there is value to both. Some command group should not allow extensions. For instance adding any sub commands fo service would IMO a bad thing.

I suggest to implement this in the generic fashion to just verify that there is no collision on the terminal (sub) command.

Sure but only for allowed command groups.

@rhuss
Copy link
Contributor

rhuss commented Apr 28, 2020

I think there is value to both. Some command group should not allow extensions. For instance adding any sub commands fo service would IMO a bad thing.

Why ? What's about a kn service log for showing los on a kubernetes based system (like stern does) ? or kn source apiserver log (so logs on various levels).

@rhuss
Copy link
Contributor

rhuss commented Apr 28, 2020

I would allow it for every group, but forbid to override.

knative-prow-robot pushed a commit that referenced this issue May 5, 2020
* fixes(#814) allow plugins to extend the 'source' command group

* introduce a list of command group that can be extended (currently only source)
* check if plugin main group is in that list and execute, otherwise fail
* add e2e test for both plugin that is allowed and not allowed to extend existing group

* * move IsAllowedExtensibleCommandGroup to plugin/verifier.go
* added tests for the public function
* change verifier to use IsAllowedExtensibleCommandGroup to allow plugins for extensible command groups

* moved `InAllowedExtensibleCommandGroups` to plugin.go so that it is
usable in windows builds
@navidshaikh navidshaikh added this to the v0.15.0 milestone May 14, 2020
coryrc pushed a commit to coryrc/client that referenced this issue May 14, 2020
dsimansk added a commit to dsimansk/client that referenced this issue Sep 17, 2021
* [release-v0.24.0] Update spec file version

* [release-v0.24.0] Add kafka plugin tests
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/feature New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants