Supporting extending all command groups?

[maximilien]

Feature request

PR #818 allows extensions to the source command group. This is needed since kn has core commands under source to support some eventing sources, e.g., ping. However, since the number of variety of eventing sources is large and growing, we envisioned additional eventing source support to be added as plugins. See for instance the source kafka and source github and other plugins being developed in client-contrib repository.

So the question then comes up should we generalize support for extending and overriding of core commands. This was raised by @navidshaikh and @rhuss. And proposed by Navid in comments to PR #818.

While both sides of the issues are useful. Before implementing we will use this issue to discuss and eventually vote a direction. So watch for the comment with pole for voting below once we have agreement on the feature request.

Use case

There are two principle use cases:

1. As a kn user and developer, I would like to add a new command to an existing command group via a plugin. For instance, a debug command to the core service command group.

2. As a kn user and developer, I would like to override an existing command with my own implementation via a plugin. So imagine you have a fancy-create command for service that allows some extra decorations to services and other features when user calls create. You would like to have user call kn service create vs kn service fancy-create.

Please note that use case 2 extends on use case 1. So use case 2 can only exist if we allow use case 1.

UI Example

# assume a plugin called kn-service-debug in ~/.config/kn/plugins directory
# also assume user has a new value in ~/.config/kn/config.yml set, 
# e.g., allow-core-extensions: true
kn service debug my-service # executes the kn-service-debug plugin

# similarly for use case 2 above, if a plugin named ~/.config/kn/plugins/kn-service-create is 
# present it will be executed for all `service create` calls but other commands in `service` 
# will be left as is 
kn service create my-service ... # executes ~/.config/kn/plugins/kn-service-create
kn service update my-service ... # executes core `update` command

/kind proposal

[maximilien]

@navidshaikh and @rhuss and others, please chime in here with any opinions and changes on what this feature should look like.

My goal is to update description with any sensible suggestion(s) and open a vote in about 24 hours. We will keep vote open for about the same (24 hours) and if we are a go, I commit to implement this feature thereafter. It’s tricky but doable and know how to approach it based on #818. Thanks for your attention and opinion on this.

——
And to be clear, my opinion is that while I can see some value and convenience to these features, I personally believe, as I have noted multiple times, that we SHOULD NOT do any of this. Rationale is simple. Both of these features are syntactic sugars; it’s better to protect a common user experience than adding this flexibility. “I have spoken” :)

[maximilien]

@sixolet @cppforlife @duglin @jchesterpivotal @daisy-ycguo and others who might not have seen this and have been part of this project early on, please chime in your opinion on this or wait for vote. Thanks 🙏🏽

[maximilien]

/assign @maximilien

[navidshaikh]

IMO, we should allow a plugin to extend command groups and bring its own leaf commands as long as they don't collide with kn's own commands.

No for use case 2 to override kn's (leaf or group) commands with plugins, this could bring uncertainty.

[maximilien]

OK thanks for chiming in @navidshaikh. I certainly like this better.

Let's see what others say and I will update. Best.

[rhuss]

For me, it is clear that we must not allow use case 2, i.e. no leaf command can be overridden. If some plugin author feels the need to do things differently than the builtin kn commands, then he should name his plugin individually (like 'kn-service-mycreate').

For use case 1, I'm all in for not applying any restriction to add new commands to existing command groups at any level. My arguments are:

• It is useful to add plugins for commands like kn service log, kn service debug to extend service related functionality. Same for other groups.
• If we would allow only some command-groups for extension (like for source), who would decide which command group at which level is permitted and not for what reason?
• If we then do change our minds later, we do need to make a new release.
• kubectl doesn't have any restrictions for deep integration, too.
• As we stick to the scheme kn <noun> <verb> for kn, each plugin's name is supposed to start with a noun after the kn-prefix. This scheme will lead to an inflation of the top-level namespace and doesn't allow easily to group functionality together, which is not helpful for documentation and help-messages
• What's about plugin themselves that introduces command groups? Consider two plugins, kn-admin-serving and kn-admin-eventing, would this be allowed although both share the same command group admin?
• It's just simpler to have no restrictions and results in much better integration into the UI so that we can provide a seamless user experience where the plugins and core commands are both first-class citizens, and the user doesn't need to know the difference.

[maximilien]

OK so based on the current feedback. Seems like the consensus is to only allow use case #1. I repeat and refine it here in the least amount of text as I could.

---

Polll: allow any plugin to extend any command group. Please vote with 👍 if you are in favour of this suggestion, and 👎 if not.

This extension implies core groups (where "core" needs to be defined), e.g., service as well as groups introduced by an existing plugin, e.g., kn-max-foo which introduces command group max and command foo.

There are some details that we also need to agree on, I am listing here but these can be refined as needed. They are:

1. The config flag allow-core-extensions: true will be required for core group extensions. Without it core groups will not be allowed to be extended.
2. No config flag is needed for plugin group extensions, however, since plugins can be in PATH (by using --lookup-plugins option or config variant) then the order of plugins in the PATH will decide which plugins gets executed when two or more plugins clash completely, i.e., both command group and command name.

---

Please vote 👍 or 👎 here until about 24 hours. Simple majority wins. Only vote on this comment.

[rhuss]

[Updated the description above to clarify what the poll is about, so the comments here are about the options given above]

Sorry, I can't vote on this ballot. What are the options ? What happens if the majority is against it ? Is 👍 option 1, and 👎 option 2 ?

For option 1 to decide, what is the definition of "core" extension ? When two conflicting plugins introduce the same command group, which one 'wins' ?

What should be voted for when neither 1. or 2. is appropriate ? (2 I guess)

... then the order of plugins in the PATH will decide which plugins gets executed when two or more plugins clash completely, i.e., both command group and command name.

You mean when you have two plugins with the exact same name? Well, what happens if you have two executables called 'kn' ? Then one first in path wins. I suggest the same is true for plugins. Feels very natural for me.

The other question is when you have two plugins which hook in into different levels of the command hierarchy. (e.g. kn-service-log-loki and kn-service-log). Then the more specific command should win, in the example kn-service-log-loki) (but I think we already specified this)

[rhuss]

Regardless what, my vote is for no restriction at all except for an exact clash to a core command (like kn-service-create) (with "core" defining the command coming with "kn")

[rhuss]

@maximilien I added some additional explanation to your poll description. Please check whether this appropriate and reflects your thoughts.

[rhuss]

it’s better to protect a common user experience than adding this flexibility. “I have spoken” :)

@maximilien ok, but please let me just add this one thought/question (also posted to slack): Why is the kn user-experience protected when using a kn service-log compared to a kn service log kind of plugin ? IMO it's exactly vice versa.

I'm with you that we definitely should strive for a coherent UX, kn combined with plugins. But for me this is more about how to name and model options so that for a casual user it is indistingushable whether she is using a plugin or a core command. IMO that is the perfect user experience, without any UI break. Forcing plugins into the top-level leads to strange names and groupings. Maybe we should craft some kind of Turing test to let user guess whether a feature is a plugin or built-in :-) ?

Therefore I'm currently rewriting our CLI conventions guide with detailed instructions on how to model flags for different types (scalar, list, map, binary, ...). This guide should then be applied for plugin UI as well. You can expect a PR today.

[rhuss]

If we allow extension of any group, we should not allow to convert a leaf-command to a command group because of a plugin. E.g. a kn-service-create-mystuff has to be disallowed as it would convert a builtin terminal command (create) to a command group as the longest path normally wins in the plugin resolution.

[maximilien]

OK looks like people want to have use case 1. That is allow plugins to extend a core command group so long as the user sets allow-core-group-extensions: true in their ~/.config/kn/config.yml file. It will be set to false by default.

So a user that has this feature set in their configuration can add a plugin named kn-service-blah and will be able to call: $kn service blah as part of their workflows when using kn.

By core command group I mean all of the command groups defined by kn Itself. So for instance, service, revision etc. Plugins command group have always been extensible so that’s not an issue.

—-
Last chance to comment here if you see any issues or have more to say. Otherwise, will implement as specified above.

[maximilien]

From Slack discussion, to document my view for posterity, especially since I am in the minority and I lost this vote:

——
I can see both sides of the story. However, while I tend to be liberal in philosophy for many things. For this issue, I am conservative. Reasoning is simple. This use case (extending core command group) is a minority one.

Meaning If kn becomes super successful, I venture to guess that few users would want to extend core command group. And even in the event they do, we, as a core WG, would invite them to participate and do that extension as part of the core. Especially if its a great idea. Plus we would always want to expand the WG with contributors so inviting them would make sense.

Therefore, discouraging users to add commands to the core namespace is a reasonable restriction — except in some rare cases, e.g., source group, and for different reasons. And the net effect is some minor annoyance to a minority use case, which has a clear way to dealing with their needs if they really want to see this, via participating in the WG.
——

And to be extra clear my views are that extending any core group namespace should not be allowed. So NO to both kn service-blah blah and to kn service blah. Because service is a core command group. Same for all other command groups defined by kn.

[duglin]

Just FYI - I don't see the need for the config flag - seems like an unnecessary burden on people. IMO it would have only been needed if we allowed people to override core kn cmds

[maximilien]

I guess I added the flag because of how much I hate this feature overall :)

However, since I lost the vote, sure. I can remove the unnecessary burden. Comment here @rhuss, @navidshaikh, @dsimansk (the ones who voted) if you are not OK for no config flag. No comment => @duglin wins and no config flag.