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

import of plugins from danisla #11

Merged
merged 4 commits into from
Aug 7, 2018

Conversation

danisla
Copy link
Contributor

@danisla danisla commented Aug 4, 2018

Converting several of my kubefunc bash functions that I use frequently to plugins:

matchExpressions:
- {key: os, operator: In, values: [darwin, linux]}
version: "v1.0.1"
shortDescription: Get logs from a list of pods
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the description doesn't make it clear how's this different than kubectl-logs.
have you checked out mtail? does it differentiate enough?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is very different than mtail. It presents a list to choose from.

You should try these plugins first to gauge how they work and differ.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense, we should get this in. Do you foresee a more original name for this or are you happy to use this name (hint: this will impede discoverability for this plugin via search engines)

matchExpressions:
- {key: os, operator: In, values: [darwin, linux]}
version: "v1.0.1"
shortDescription: Run shell command in pod
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

how's the use case for this look like? (trying to see what it adds over kubectl exec). do you mind providing a long description field with some examples for when the shortDescription isn't explanatory enough like this?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is more convenient because it shows you a list of pods that you choose from interactively and tells you if there are multiple pods. Again, super convenient for daily stuff I find my self repeating.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That makes sense, we should get this in.

matchExpressions:
- {key: os, operator: In, values: [darwin, linux]}
version: "v1.0.1"
shortDescription: Install helm package manager
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Similarly, plugins ideally should not be one-off use like installing Helm.

If there's something like curl https://helm.sh/install.sh | bash, users should use that.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This one is really useful for me because it bundles all of the commands for installing helm with RBAC.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a rubric or documentation with guidelines on the usefulness of a plugin so that it can be admitted to the Krew-index?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is the RBAC problem because GKE does not automatically create cluster-admin binding to your account (I believe this will be fixed soon) OR is it because helm installation is not creating the tiller service account for you?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it's both the cluster-admin binding and the tiller service-account and rbac role.

matchExpressions:
- {key: os, operator: In, values: [darwin, linux]}
version: "v1.0.1"
shortDescription: Install cert-manager helm chart
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think just installing a helm chart doesn't warrant a plugin, especially because it's mostly an one-off operation and not repetitive. Not to mention, it won't scale to have a plugin per helm chart :)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I use this one a lot because it also installs the cluster issuer which I always have to look up. Ya if it was just installing the chart there isn’t much value added.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is still a helm chart problem.

I've brought this up in cert-manager/cert-manager#259 so please participate in the discussion and support resolution of this issue there.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It sounds like it is a helm problem, no clear simple resolution in sight other than having 2 helm charts for the issuers... I would still use my one-liner plugin to do it all.

@lbb
Copy link
Contributor

lbb commented Aug 5, 2018

I agree with @ahmetb; some plugins are too specific.

  • I like kube-node-admin. I think this is very useful for debugging.
  • One-time install plugins don't give enough value to be installed. This might be suitable for multiple clusters. For the majority, this doesn't scale.
  • I like the idea of the *-shell plugins. If we would use them they need a
    different name. I as a user can't figure out the difference between kube,pod-
    shell. Maybe name it debug-shell and pod-shell. But this can also be solved with a
    simple bash alias. The question is if it brings enough value to have a custom
    command. @ahmetb ?

We should write a rubic for plugin contributions. I'll create an issue.

@danisla
Copy link
Contributor Author

danisla commented Aug 5, 2018

I agree that the one time plugins may seem like they don’t give enough value. Many of my use cases are in the DevOps realm and involve creating and interacting with many clusters. I would hope that others would find that useful too.

kube-shell creates a new pod used for debugging services or the cluster itself and is not dependent on any existing pods. Whereas pod-shell locates an existing pod and runs a shell in it.

I think we should prioritize the tapping of other Krew-indices so that others can use Krew to manage their plugins without a centralized authority deciding a plugin’s relevance.

@ahmetb
Copy link
Member

ahmetb commented Aug 6, 2018

@danisla I think the problems in this PR are:

  1. We don't have well defined guidelines on how to enforce style on the centralized krew-index repo. Therefore we're trying to be careful of people putting dibs on generic words/names, or plugins that aren't super useful (such as two one-off commands combined together).

    Note that the plugin system is in its SUPER EARLY days, therefore we need input from "what is a plugin and what shouldn't be a plugin". My opinion is tipping the scale towards "tools that are used frequently as opposed to one-off install scripts".

  2. We're evaluating 7 plugins at once in one PR. :) I think some of them are fine. If you want to send them one by one I think we can get them in sooner.

@ahmetb
Copy link
Member

ahmetb commented Aug 6, 2018

I created kubernetes-sigs/krew#23 to keep track of custom index repository support. Please subscribe.

@ahmetb
Copy link
Member

ahmetb commented Aug 7, 2018

@danisla I still don't think the one-off installer plugins should go in. Plugins are more directed towards workflows and we probably want to have the first few plugins follow the best practices.

I'm not against you creating a plugin to simplify an installation step, but I think as long as someone copies a cmd from somewhere to install-cert-manager, they might as well just the 3 commands (not to mention 3 separate commands would explain what's happening better for tutorial/workshop purposes etc).

I think we should prioritize custom index repos for this. Feel free to remove one-off plugins and we can merge the remaining.

shortDescription: List nodes and run privileged pod with chroot
caveats: |
This plugin needs the following programs:
* kubectl
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

kubectl should not be listed as a dependency.

Copy link
Contributor

@lbb lbb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@ahmetb ahmetb merged commit 133f889 into kubernetes-sigs:master Aug 7, 2018
@ahmetb
Copy link
Member

ahmetb commented Aug 7, 2018

Due to lack of CI/CD I'm not sure if the plugins are working correctly. :) So please krew update and try installing them.

@ahmetb ahmetb mentioned this pull request Mar 6, 2019
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants