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

Krew plugin criteria #152

Open
wants to merge 6 commits into
base: master
from

Conversation

Projects
None yet
4 participants
@corneliusweig
Copy link
Contributor

commented May 19, 2019

Hi @ahmetb @juanvallejo,

as you all know, krew-index currently has no criteria to apply for accepting or rejecting plugins. This creates friction for both contributors and maintainers:

  • plugin contributors, because they have no idea whether their idea might be accepted.
  • krew-index maintainers, because they need to do subjective case-by-case decisions whether a plugin actually "fits".

So I put up this draft for plugin criteria to get the discussion going. It is an opinionated crude first draft and probably misses a lot of things. But maybe it helps nevertheless. Let me know what you think.

cc @garethr because you wanted to tune in

Add draft for plugin criteria
Signed-off-by: Cornelius Weig <cornelius.weig@gmail.com>
@k8s-ci-robot

This comment has been minimized.

Copy link

commented May 19, 2019

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: corneliusweig
To fully approve this pull request, please assign additional approvers.
We suggest the following additional approver: ahmetb

If they are not already assigned, you can assign the PR to them by writing /assign @ahmetb in a comment when ready.

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:

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

@k8s-ci-robot k8s-ci-robot added the size/M label May 19, 2019

@ahmetb

This comment has been minimized.

Copy link
Contributor

commented May 20, 2019

Thanks for taking charge here. We definitely need this document, so this is gonna be a long PR.

Styling-wise: I feel like if you can put the Guidances right next to the situations (maybe markdown table, maybe just a sub-bullet point) it would be easier to read and leave review-comments.

Format rules in a table
Signed-off-by: Cornelius Weig <cornelius.weig@gmail.com>
@corneliusweig

This comment has been minimized.

Copy link
Contributor Author

commented May 27, 2019

Yeah, that looks so much nicer. Have you discussed about the plugin criteria during KubeCon?

@garethr
Copy link

left a comment

Thanks for kicking the conversation off. I had a good conversation at KubeCon about plugins with @ahmetb too.

Left some inline comments.

I think the main observation is to make it clear about what the criteria are for. The title says "Krew plugin" but I don't think this is defined anywhere explicitly. This should make a clear distinction between the Krew index or Krew plugins and the open Kubectl plugins.

I'd also suggestion merging this with the Naming guide, as really the naming guide is probably a subset of these criteria.


Krew is a `kubectl` plugin manager.

A plugin provides a technical solution to at least one _task_ concerning a problem around kubernetes.

This comment has been minimized.

Copy link
@garethr

garethr May 27, 2019

The first paragraph (above) is talking about Krew. This paragraph jumps to talking about "A plugin", by which I take it to mean "A kubectl plugin". The description is then very opinionated.

I think it's fine to have strong opinions about what goes in the Krew index. I don't think it's OK to try and generalise that to the open plugin system that kubectl has. Anyone can create any plugin for kubectl that they want, simply by giving it a specific name and putting it on the path. Because of that the Krew index will always be a subset of available Kubectl plugins, which I think is fine.

I'm not sure if you want to define a "Krew plugin", or whether this should read something like "A plugin +in the Krew index+" or similar.

This comment has been minimized.

Copy link
@corneliusweig

corneliusweig May 27, 2019

Author Contributor

Yes, I fully agree. We should make clear that this is about krew plugins only. Can't do it right now, but you're welcome to suggest better wording.

This comment has been minimized.

Copy link
@ahmetb

ahmetb May 29, 2019

Contributor

I recommend the following wording:

The centralized Krew plugin index (krew-index repository) holds plugins that are applicable to a broad set of kubectl users.

A plugin in Krew plugin index must add new functionality or enhance the existing functionality in kubectl, or capsulate a common operator/developer workflow for higher productivity.

We don't want to exclude people being creative and trying interesting things.

This comment has been minimized.

Copy link
@corneliusweig

corneliusweig May 29, 2019

Author Contributor

Yeah, I like the approach to argue from the user-basis as opposed to kubectl-centric.

|----|--------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------|-----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------|
| 1. | The task is already solved by kubectl. | Mere command wrappers are not acceptable, as those are better covered by shell aliases. However, convenience and productivity improvements are welcome (examples: `konfig`, `change-ns`, `view-secret`) |
| 2. | The task provides a different solution to a problem which is also solved by kubectl. However, the task would be impossible or very hard to reproduce with kubectl alone. Examples: `access-matrix`, `get-all`, `mtail`, `tail` | In general welcome unless it overlaps significantly with other plugins or has a bad usability. |
| 3. | The task addresses a completely different problem than any of the kubectl subcommands, but is clearly focused on k8s. | Gray area. If the task fits well into `kubectl`, it may be accepted (for example `kubectl debug`). |

This comment has been minimized.

Copy link
@garethr

garethr May 27, 2019

The example of kubectl debug would appear to be against the naming guidelines, in particular "Be Specific". It feels like the Login example provided there.

This comment has been minimized.

Copy link
@ahmetb

ahmetb May 29, 2019

Contributor

+1 we rejected several plugins named debug.

A common task they do is: kubectl cp a busybox binary, kubectl exec to install busybox symlinks, then kubectl exec -- sh to drop user to a shell inside the container. Although I think we accepted one named pod-debug.

This comment has been minimized.

Copy link
@corneliusweig

corneliusweig May 29, 2019

Author Contributor

Oops. I had #135 in mind, but forgot that the agreement was to change the name.
Changed to kubectl debug-pod.

| 2. | The task provides a different solution to a problem which is also solved by kubectl. However, the task would be impossible or very hard to reproduce with kubectl alone. Examples: `access-matrix`, `get-all`, `mtail`, `tail` | In general welcome unless it overlaps significantly with other plugins or has a bad usability. |
| 3. | The task addresses a completely different problem than any of the kubectl subcommands, but is clearly focused on k8s. | Gray area. If the task fits well into `kubectl`, it may be accepted (for example `kubectl debug`). |
| 4. | The task is very dissimilar to common kubectl tasks and is not primarily focused on k8s. However, it can be useful in the k8s ecosystem. | Not acceptable. |
| 5. | The task is very dissimilar to common kubectl tasks and is not focused on k8s. Its general usefulness in the k8s ecosystem is questionable. | Not acceptable. |

This comment has been minimized.

Copy link
@garethr

garethr May 27, 2019

Being arbiters if utility is bound to cause some friction. Usefulness is in the eye of the beholder. Homebrew hacks around this by saying packages have to be maintained by users, not by the original author. The gist being if a user is happy to maintain a package them by definition it's useful. Not saying Krew should follow the same pattern, but having guidelines which say "we decide based on what we think" might cause friction later.

This comment has been minimized.

Copy link
@corneliusweig

corneliusweig May 29, 2019

Author Contributor

I agree that "usefulness" is rather subjective. But I think the krew index does need some form of filtering. Otherwise, where should the line be drawn? If there is no line (like for brew), krew would be come a mere package manager and not a kubectl package manager.
Maybe you have better ideas of what could be reasonable criteria?


Usability
---
Furter required acceptance criteria:

This comment has been minimized.

Copy link
@garethr

garethr May 27, 2019

Typo, Furter rather than Further

---
Furter required acceptance criteria:

1. Standard look & feel: Adheres to standard kubectl best practices and in particular uses consistent (abbreviated) flag names. For example: bad `--ns`, good `--namespace` + `-n`.

This comment has been minimized.

Copy link
@garethr

garethr May 27, 2019

Would be good to link to a style guide here to avoid friction and nitpicking.

This comment has been minimized.

Copy link
@corneliusweig

corneliusweig May 27, 2019

Author Contributor

Good point. @ahmetb do you know if such a document exists?

This comment has been minimized.

Copy link
@ahmetb

ahmetb May 28, 2019

Contributor

Yeah we should probably write a separate "Style Guide for Plugins" document. We could easily fill it up.

@ahmetb

This comment has been minimized.

Copy link
Contributor

commented May 28, 2019

Let's not merge all criteria into one document for now, it's harder to litigate all points in one PR.

@ahmetb ahmetb referenced this pull request May 28, 2019

Merged

plugins: add tail (github.com/boz/kail) #150

2 of 2 tasks complete
@ahmetb

This comment has been minimized.

Copy link
Contributor

commented May 29, 2019

I am thinking we can check this into krew main repo instead, since this repo ships to every krew client machine, and we already have a docs/ in the main repo.


This task should be classified in one of the categories:

| # | Classification | Recommendation |

This comment has been minimized.

Copy link
@ahmetb

ahmetb May 29, 2019

Contributor

nit: (hope you won't hate me for saying this) I think if we actually do this part as an HTML table, we can also do 80-char word wrap and maintain this more easily:

<table>
<thead>
<tr>
<td>column</td>
<td>column</td>
</tr>
</thead>
<tbody>
<tr>
<td>column</td>
<td>column</td>
</tr>
</tbody>
</table>

This comment has been minimized.

Copy link
@corneliusweig

corneliusweig May 29, 2019

Author Contributor

No, I think this is absolutely necessary. Will do.

| # | Classification | Recommendation |
|----|--------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------|-----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------|
| 1. | The task is already solved by kubectl. | Mere command wrappers are not acceptable, as those are better covered by shell aliases. However, convenience and productivity improvements are welcome (examples: `konfig`, `change-ns`, `view-secret`) |
| 2. | The task provides a different solution to a problem which is also solved by kubectl. However, the task would be impossible or very hard to reproduce with kubectl alone. Examples: `access-matrix`, `get-all`, `mtail`, `tail` | In general welcome unless it overlaps significantly with other plugins or has a bad usability. |

This comment has been minimized.

Copy link
@ahmetb

ahmetb May 29, 2019

Contributor

I think we can give detailed examples. Take this sentence:

For example:

  • tail plugin allows tailing logs from pods from a label selector, a namespace or all namespaces at once, whereas kubectl logs command has more limited functionality.
  • get-all plugin is a replacement for kubectl get all command which actually fetches only a subset of API resource types.
@corneliusweig

This comment has been minimized.

Copy link
Contributor Author

commented May 29, 2019

Let's not merge all criteria into one document for now, it's harder to litigate all points in one PR.

Are you referring to the idea to merge this with the naming guide?

corneliusweig added some commits May 29, 2019

Improve the krew-index rationale
Argue user-centric instead of kubectl-centric.

Signed-off-by: Cornelius Weig <cornelius.weig@gmail.com>
Format tables as html tables instead of markdown
Signed-off-by: Cornelius Weig <cornelius.weig@gmail.com>
Elaborate on the examples, put them in lists
Signed-off-by: Cornelius Weig <cornelius.weig@gmail.com>

@k8s-ci-robot k8s-ci-robot added size/L and removed size/M labels May 29, 2019

@ahmetb

This comment has been minimized.

Copy link
Contributor

commented May 30, 2019

Thanks for doing the html migration, looks like we need to use full html i.e. <code> once we go html tables.

I want to capture an additional criteria about plugins being more broadly usable to most Kubernetes users, but this is hard to measure and evaluate.

  • For example, oidc-login plugin is a nice plugin because there are many clusters using OIDC for authentication. [citation needed] So it's nice to have.
  • On the other hand, if we had a plugin like rancher-dashboard that launches Rancher’s UI (and submitted by Rancher), then we're looking at a plugin applicable to a small % of the users [citation needed] –––and we can't measure whether people using OIDC for authentication VS clusters managed by Rancher are more.

One of them feels right, where the other one doesn’t, and this is hard to measure.

@corneliusweig

This comment has been minimized.

Copy link
Contributor Author

commented May 30, 2019

In the end, what you suggest always boils down to: how large is the userbase for setup XY. But even if we come up with an agreement what should be the acceptable number of users, how would we measure that in practice?

Hence, I'm more with @garethr here who brought up the brew philosophy: if somebody maintains it, it's fine. (Of course it still needs to match the other to-be-determined criteria).

What I could imagine to work is a whitelist of well-known k8s projects for which plugins may be submitted. For example: knative, istio, linkerd, tekton, jenkins-x, rook.io, and so on. Of course such a list needs to be curated and be kept up to date. By default, all incubating CNCF projects should be on that list too.

Replace backticks with <code> tags
Signed-off-by: Cornelius Weig <cornelius.weig@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.