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

Add prune-unused plugin #162

Merged
merged 4 commits into from Jul 11, 2019

Conversation

Projects
None yet
4 participants
@etiennetremel
Copy link
Contributor

commented Jun 13, 2019

Add kubectl-prune plugin, unfortunately I wasn't able to install it using,

I have been trying to install it manually but for some reason I'm getting the following not executable warning. When I look at the file locally installed it is correctly executable, might work on your machine... otherwise will fix... hint on how to fix it are welcome :)

$ wget https://github.com/thecloudnatives/krew-prune/archive/v0.1.0.zip
$ kubectl krew install --manifest=prune.yaml --archive=/tmp/v0.1.0.zip

$ kubectl plugin list
The following compatible plugins are available:

/Users/bob/.krew/bin/kubectl-krew
/Users/bob/.krew/bin/kubectl-prune
  - warning: /Users/bob/.krew/bin/kubectl-prune identified as a kubectl plugin, but it is not executable

error: one plugin warning was found

Checklist for plugin developers:

  • [*] Read the Plugin Naming Guide (for new plugins)
  • Verify the installation from URL or a local archive works (kubectl krew install --manifest=[...] --archive=[...])
@k8s-ci-robot

This comment has been minimized.

Copy link

commented Jun 13, 2019

Welcome @etiennetremel!

It looks like this is your first PR to kubernetes-sigs/krew-index 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes-sigs/krew-index has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

@ahmetb

This comment has been minimized.

Copy link
Contributor

commented Jun 13, 2019

Thanks for your contribution.

Ideally for new plugins accepting namespace as an input, we want them to support -n/--namespace as non-positional arguments. Is it possible that you can implement this?

@corneliusweig
Copy link
Contributor

left a comment

Thanks for your addition to the krew index! There are a few issues which need to be sorted out.

  1. Plugin naming. The plugin naming guidelines demand that plugin names must be specific. For me, prune is too generic and could apply to all kinds of resources. What do you think about prune-configmaps? I understand that your tool does more than that, but it is descriptive (for the one case) and the generalization to secrets would be obvious.

  2. Plugin behavior. I had a look at your implementation and I fear it has some flaws. For example, it does not consider init-containers at all. If those mount a secret/cm that would wreak havoc. In addition, you only consider (running)pods+(suspended)cronJobs. What about deployments with zero active replicas? Same goes for StatefulSet, DaemonSet, ReplicaSet, ReplicationController, and Job resources. I think all these resources should be taken into account.

Show resolved Hide resolved plugins/prune.yaml Outdated
Show resolved Hide resolved plugins/prune.yaml Outdated

@k8s-ci-robot k8s-ci-robot added size/M and removed size/S labels Jun 14, 2019

@etiennetremel

This comment has been minimized.

Copy link
Contributor Author

commented Jun 14, 2019

@ahmetb I added the namespace flag capabilities.
@corneliusweig I improved the script to take in consideration all the resources you mentioned. Also splitted the prune command into to: prune-configmaps and prune-secrets. Let me know if there is anything else

@corneliusweig
Copy link
Contributor

left a comment

Hi @etiennetremel,

👍 for addressing the earlier comments so quickly. Is there a particular reason why you didn't include DaemonSet in the list of resources to check for usages of the cm/secret? Otherwise you might want to add that too.

Regarding your split of the original script into two independent ones: that was not the intention of my earlier comment. I'm a bit worried about having two almost identical plugins. My user expectation would be that this functionality is in one plugin and can be tuned with a flag or similar. @ahmetb WDYT?
That only leaves the question how to name such a plugin. You obviously didn't like prune-configmaps (for both), so how about prune-config-data or prune-app-data or prune-app-config. Then your command would read like

kubectl prune-app-config secrets -n default
# or
kubectl prune-app-config cm -n default
@etiennetremel

This comment has been minimized.

Copy link
Contributor Author

commented Jun 15, 2019

I added the Daemonset resource but before moving forward, let's align on the naming convention. Thanks for catching that @corneliusweig .
I'm not a huge fan of using prune-app-config as naming convention for that plugin. The app-config part doesn't mean anything related to Kubernetes resource to me. At first I used prune which as you mentioned is too generic. What do you thing about kubectl prune-resources secrets -n default or kubectl rm-unused secrets -n default, other ideas?

@corneliusweig

This comment has been minimized.

Copy link
Contributor

commented Jun 18, 2019

Hey @etiennetremel, given that there is already a plugin rm-standalone-pods, I think the most in-line name would be rm-unused. Given that this name is also not too specific, I propose the following:
Keep the scripts for secrets and configmaps separate, but call them kubectl-rm_unused-secrets and kubectl-rm_unused-configmaps, respectively. The kubectl plugin mechanism will call those scripts when executing kubectl rm-unused secrets. That way, future plugins can use the same base-name for other plugins (e.g. kubectl rm-unused volumes or so).
@ahmetb WDYT?

@ahmetb

This comment has been minimized.

Copy link
Contributor

commented Jun 18, 2019

Sounds good. My main concern was that in the future kubectl may introduce a command called prune very easily and we don’t want to be in that situation.

@ahmetb

This comment has been minimized.

Copy link
Contributor

commented Jun 27, 2019

Circling back here: What do you think about this sort of naming:

kubectl prune-unused configmap [...]
kubectl prune-unused secret [...]

this lets you distribute it as single plugin and still have both functionality. Naming-wise, I think prune-unused is better than rm-unused.

@etiennetremel

This comment has been minimized.

Copy link
Contributor Author

commented Jul 1, 2019

Great, prune-unused it is. I will make the change in the coming days. Thanks.

@etiennetremel

This comment has been minimized.

Copy link
Contributor Author

commented Jul 5, 2019

@corneliusweig, as you described, I'm a little confuse on how to have sub command working in this case.

My plugin directory structure look like this:

prune-unused
├── kubectl-prune_unused-configmaps
└── kubectl-prune_unused-secrets

Then I would assume something like this would be the configuration to map each script:

    uri: https://github.com/thecloudnatives/kubectl-plugins/archive/master.zip
    sha256: 4c84c36658ec414e99f4347a6c011dbe775c69d140768e04c3eceb2615481498
    files:
    - from: kubectl-plugins-*/prune-unused/*
      to: "."
    bin: ./kubectl-prune_unused-*

Is it possible to map sub command to a script as kubectl prune-unused secrets pointing to kubectl-prune_unused-secrets?
Or should I create 2 different plugin?

@ahmetb

This comment has been minimized.

Copy link
Contributor

commented Jul 5, 2019

We don’t support 2 plugin executables in one plugin at the moment.
It came up before and we decided it’s not a common case.

I recommend you write a small program in bash that accepts secret and configmap as a subcommand, then that program can call other executables you wrote in the same installation directory based on the subcommand.

Or you can just merge two executables into one. (:

@etiennetremel

This comment has been minimized.

Copy link
Contributor Author

commented Jul 11, 2019

Thanks @ahmetb for the clarification. I merged both scripts into a single one.

@ahmetb ahmetb changed the title Add kubectl-prune plugin Add prune-unused plugin Jul 11, 2019

@ahmetb

This comment has been minimized.

Copy link
Contributor

commented Jul 11, 2019

/lgtm
/approve

@k8s-ci-robot

This comment has been minimized.

Copy link

commented Jul 11, 2019

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ahmetb, etiennetremel

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 merged commit 0b99994 into kubernetes-sigs:master Jul 11, 2019

2 of 3 checks passed

tide Not mergeable.
Details
cla/linuxfoundation etiennetremel authorized
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
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.