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 aws-auth v0.3.5 #2159

Merged
merged 8 commits into from
Apr 12, 2022
Merged

Conversation

eytan-avisror
Copy link
Contributor

No description provided.

@ahmetb
Copy link
Member

ahmetb commented Apr 11, 2022

🤖 Beep beep! I’m a robot speaking on behalf of @ahmetb. 🤖


Thanks for submitting your kubectl plugin to Krew!
One of the krew-index maintainers will review it soon. Note that the reviews for new plugin submissions may take a few days.

In the meanwhile, here are a few tips to make your plugin manifest better:

  • Make sure your plugin follows the best practices.
  • Eliminate redundant wording form shortDescription (it should be max 50 characters).
  • Try to word wrap your description to 80-character lines (no usage examples, please).

Thanks for your patience!
/kind new-plugin

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/new-plugin labels Apr 11, 2022
@k8s-ci-robot
Copy link
Contributor

Welcome @eytan-avisror!

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/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Apr 11, 2022
@eytan-avisror
Copy link
Contributor Author

/assign @ahmetb

@ahmetb
Copy link
Member

ahmetb commented Apr 11, 2022

Have you read the naming guidelines? That name won't work.

@eytan-avisror
Copy link
Contributor Author

@ahmetb I have, can you be a bit more specific on why it won't work? is it too generic?

@ahmetb
Copy link
Member

ahmetb commented Apr 11, 2022

Has vendor prefix and is too generic. AWS or someone else can submit a plugin with the same name that works entirely differently.

@eytan-avisror
Copy link
Contributor Author

@ahmetb can you suggest a better name for the plugin? the purpose is easy modification/management of the 'aws-auth' configmap present on EKS clusters.
is aws-auth-manager any better?

@eytan-avisror
Copy link
Contributor Author

BTW @ahmetb
Not sure if it makes a difference but this tool is referenced by AWS best practices here: https://github.com/aws/aws-eks-best-practices/blob/master/content/security/docs/iam.md#use-tools-to-make-changes-to-the-aws-auth-configmap

And also the suggestion to add a krew plugin was by AWS here:
keikoproj/aws-auth#30

@eytan-avisror
Copy link
Contributor Author

@ahmetb aws-eks-auth-mapper kinda long, but perhaps more specific. would that be more acceptable?

@ahmetb
Copy link
Member

ahmetb commented Apr 11, 2022

edit-aws-auth can work too.
aws-auth can work if you can get thumbs up from AWS folks that they don't plan on using the name. I thought you might have a chance there since they're aware of this tool.

@ahmetb
Copy link
Member

ahmetb commented Apr 11, 2022

Also the fact that there's a notable existing user community and recognition from the vendor, we can consider this as an established solution and assign the name I think.

@eytan-avisror
Copy link
Contributor Author

Thanks @ahmetb
I've pinged the original requestor for the plugin (from AWS) to comment here

@jicowan
Copy link

jicowan commented Apr 11, 2022

I think the name, edit-aws-auth, while long, is fine. I was interested in a krew plugin because it provides a quick and easy way to discover and install the aws-auth binary which allows users to safely perform CRUD operations against the aws-auth ConfigMap.

@eytan-avisror
Copy link
Contributor Author

Thanks @jicowan
@ahmetb I can make the changes, is it OK though that the plugin name differs from the repo name & binary name?

@ahmetb
Copy link
Member

ahmetb commented Apr 11, 2022

Yeah, just make sure the -h/-help message doesn't confuse users with different names.

@jicowan
Copy link

jicowan commented Apr 11, 2022

I would prefer to keep it the same (aws-auth) but if we think it'll be too confusing I'm in favor of changing it.

@eytan-avisror
Copy link
Contributor Author

eytan-avisror commented Apr 11, 2022

@jicowan yeah I think it would actually be much nicer as a shorter name i.e. kubectl aws-auth ... - given that this is a pretty basic term on EKS clusters I don't see a reason why there might be a conflict with some other tooling/krew plugin.

@eytan-avisror
Copy link
Contributor Author

eytan-avisror commented Apr 11, 2022

I think according to @ahmetb it's not about it being 'confusing', but more about if you foresee any conflict with the name in the future e.g. AWS wanting to introduce a different tool with that name as a krew plugin being as the word auth is somewhat generic

@jicowan
Copy link

jicowan commented Apr 11, 2022

Ah, we have no intention of releasing a new CLI to modify the aws-auth ConfigMap. Rather, we are looking at ways we can assign Cluster access directly through IAM.

@ahmetb
Copy link
Member

ahmetb commented Apr 11, 2022

Fair, then we can assign the name aws-auth especially given the tool is already known, used by a nontrivial amount of users and endorsed.

@eytan-avisror
Copy link
Contributor Author

Thanks @ahmetb & @jicowan
Please let me know if there is anything else needed in order to get this merged.

spec:
version: v0.3.5
homepage: https://github.com/keikoproj/aws-auth
shortDescription: Makes the management of the aws-auth config map for EKS Kubernetes clusters easier
Copy link
Member

Choose a reason for hiding this comment

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

Please shorten.

Manage AWS auth-config ConfigMap

Copy link
Contributor Author

@eytan-avisror eytan-avisror Apr 12, 2022

Choose a reason for hiding this comment

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

done
Slightly modified to: Manage aws-auth ConfigMap

homepage: https://github.com/keikoproj/aws-auth
shortDescription: Makes the management of the aws-auth config map for EKS Kubernetes clusters easier
description: |
Useful for automation purposes, any workflow that needs
Copy link
Member

Choose a reason for hiding this comment

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

Rephrase to list what the plugin does; not what it's useful for.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

plugins/aws-auth.yaml Outdated Show resolved Hide resolved
@ahmetb
Copy link
Member

ahmetb commented Apr 12, 2022

/lgtm
/approve
Thanks!

Also, please consider setting up Krew release automation which helps you skip manually making updates to your Krew manifests on each new version and send a pull request. It’s a GitHub workflow bot that can run every time you push a new tag. Those PRs are automatically approved.

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Apr 12, 2022
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ahmetb, eytan-avisror

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 approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 12, 2022
@k8s-ci-robot k8s-ci-robot merged commit 49318b4 into kubernetes-sigs:master Apr 12, 2022
@eytan-avisror
Copy link
Contributor Author

Thanks @ahmetb !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/new-plugin lgtm "Looks good to me", indicates that a PR is ready to be merged. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants