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 new plugin, kubectl resources #432

Closed
wants to merge 2 commits into from

Conversation

howardjohn
Copy link

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Jan 14, 2020
@k8s-ci-robot
Copy link
Contributor

Welcome @howardjohn!

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. 😃

@k8s-ci-robot k8s-ci-robot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Jan 14, 2020
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: howardjohn
To complete the pull request process, please assign ahmetb
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.

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

@ahmetb
Copy link
Member

ahmetb commented Jan 14, 2020

Thanks for submitting. Do you mind explaining how is it different than the several resource/allocation plugins we have?

@ahmetb
Copy link
Member

ahmetb commented Jan 14, 2020

I guess it is similar to view-allocations, just curious if it’s entirely overlapping or not.

@howardjohn
Copy link
Author

As far as I know this is the only one aggregating resource requests/limits with usage. I realize it isn't groundbreakingly innovative, I made it since I find it useful and someone opened an issue recommending me to add it to krew

@ahmetb
Copy link
Member

ahmetb commented Jan 14, 2020

For reference, I'm attaching screenshots:

  • kubectl view-allocations:
    image

  • kubectl resources (with "pods" section of the "view-allocations" in the same screen)
    image

  • kubectl view-allocations is also very similar to this, but it shows Node statuses.

The kubectl view-allocations has a funny separation of "cpu" "memory" and "pods", whereas kubectl resources is a cleaner representation of Pod resources.

Per our guidelines, this plugin likely would require renaming to accurately represent what it does (like pod-resources), or pod-usage. I'll have @corneliusweig also review and provide input.

@howardjohn
Copy link
Author

You can also aggregate by namespace or node for what its worth, with --by namespace or --by node, so pod-resources doesn't seem quite accurate

@ahmetb
Copy link
Member

ahmetb commented Jan 14, 2020

Yeah, in that case --by=node part is overlapping with resource-capacity plugin (also overlapping in name). I believe @corneliusweig opened the issue so we are not leaving any plugin authors uninformed about Krew, but let's see if it fits. Right now there's considerable overlap with existing plugins.

image

Copy link
Contributor

@corneliusweig corneliusweig left a comment

Choose a reason for hiding this comment

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

Hey @howardjohn, thanks for submitting this to krew!

Of the three alternatives, I like this plugin the most, because it produces a clean and noise-free report. However, the overlap with other plugins is substantial. Most of all, this is creates a naming problem. Just resources is too generic and many other names are already taken. Here you need to be a bit more creative (maybe resource-overview?).

Overall, I'm not 100% sure what to do here. So far we have pushed back plugins which are too similar to other ones. If you have something that clearly sets this plugin apart from the other ones, that would help a lot 😃.

Alternatively, if you plan to add something that makes your plugin unique, then let's wait for that before merging.

plugins/resources.yaml Outdated Show resolved Hide resolved
plugins/resources.yaml Outdated Show resolved Hide resolved
@howardjohn
Copy link
Author

I don't have any current plans to add any more features to the plugin (maybe a --watch flag, but not a high priority), but I am open to suggestions.

I am fine with changing the name, resource-overview sounds right. Let me know if that is sufficient or if I would need additional distinguishing

@ahmetb
Copy link
Member

ahmetb commented Jan 16, 2020

I'm good with both resource-overview and resource-usage (consider renaming your repo, and --help messages).

This would help it distinguish from resource-capacity which is geared towards showing cluster-wide total and percentages, despite both plugins are largely overlapping.

@corneliusweig
Copy link
Contributor

Then let's add this plugin to the index too. Please let us know if you plan to rename your repo as suggested.

Overview of Kubernetes resource requests, limits, and usage.
homepage: https://github.com/howardjohn/kubectl-resources
description: |
Overview of Kubernetes resource requests, limits, and usage. This plugin aggregates resource usage, similar to `kubectl top`,
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you wrap this at ~80 chars for easier reading on small terminals?

@howardjohn
Copy link
Author

Sounds good. I'll rename everything in my repo then send out an updated PR later. Thanks!

@howardjohn howardjohn closed this Mar 5, 2020
@saiharshitachava
Copy link

@howardjohn is this availble with krew yet?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/S Denotes a PR that changes 10-29 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Distribute via krew?
5 participants