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

Update view-secrets references #287

Merged
merged 3 commits into from
Oct 26, 2019
Merged

Update view-secrets references #287

merged 3 commits into from
Oct 26, 2019

Conversation

elsesiy
Copy link
Contributor

@elsesiy elsesiy commented Oct 22, 2019

Hi,
as announced to @ahmetb here, I re-wrote the view-secrets plugin.

This PR changes the existing references to the new repo.

@k8s-ci-robot
Copy link
Contributor

Welcome @elsesiy!

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
Copy link
Contributor

Thanks for your pull request. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please follow instructions at https://git.k8s.io/community/CLA.md#the-contributor-license-agreement to sign the CLA.

It may take a couple minutes for the CLA signature to be fully registered; after that, please reply here with a new comment and we'll verify. Thanks.


Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

@k8s-ci-robot k8s-ci-robot added the cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. label Oct 22, 2019
@k8s-ci-robot k8s-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Oct 22, 2019
@elsesiy
Copy link
Contributor Author

elsesiy commented Oct 22, 2019

/check-cla

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. and removed cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. labels Oct 22, 2019
plugins.md Outdated Show resolved Hide resolved
plugins/view-secret.yaml Outdated Show resolved Hide resolved
plugins/view-secret.yaml Outdated Show resolved Hide resolved
@ahmetb
Copy link
Member

ahmetb commented Oct 22, 2019

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


This pull request doesn't seem to be a straightforward version bump. I'll have a human review this.

Why wasn't this detected as a plugin version bump:

file plugins/view-secret.yaml is not a straightforward version bump: diff line unrecognized for version bumps: [+ homepage: https://github.com/elsesiy/kubectl-view-secret]

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! So cool that you take over maintaining this plugin.

In general this looks good. However I would recommend to keep the output backwards-compatible. Right now, if I run the old version without arguments, I get a list of keys in the secret. On the other hand, the new version will decode all fields right away and prefix the result with key=<decoded>.

Did you have a particular reason to change this behavior?

@ahmetb
Copy link
Member

ahmetb commented Oct 22, 2019

+1 for version/syntax parity. We don't know how many users exist out there, but if we can get the same syntax (or an error that shows the new syntax gracefully) we should wait on flipping the plugin manifest (this PR).

@elsesiy
Copy link
Contributor Author

elsesiy commented Oct 22, 2019

@corneliusweig @ahmetb My bad, I didn't even notice this because I didn't spend too much time trying to use the current version. In general I'm happy to add this but this somewhat changes how I anticipated using this plugin. Even though there might be instances where specifying the key makes sense, I see it more as an optional and not a mandatory attribute as this changes the workflow again to:

  1. kubectl view-secret <sec>
  2. Copy key
  3. kubectl view-secret <key from 2.>

I wanted to achieve simplicity by removing the friction of having to specifically providing either namespace or secret:key. Should I rename this plugin and submit it as a separate plugin?

@ahmetb
Copy link
Member

ahmetb commented Oct 22, 2019

We should keep the name the same, since I won't maintain view-secret anymore.

Is it not at all possible to accommodate something like:

  • kubectl view-secret <name>
  • kubectl view-secret <name.subkey>

at the same time? -n/--namespace should still exist separately.

@elsesiy
Copy link
Contributor Author

elsesiy commented Oct 22, 2019

I think that works. Another approach could be to use kubectl view-secret <name> --all
and then preserve the rest of the functionality as described above.
Which one do you prefer?

@ahmetb
Copy link
Member

ahmetb commented Oct 22, 2019

I don't follow, what does --all do?

@elsesiy
Copy link
Contributor Author

elsesiy commented Oct 23, 2019

--all/-a would output all key:value pairs from the provided secret whereas kubectl view-secret <name> would just print out the keys/follows the existing flow. But I do like your suggestion with secret.key a lot so I'll probably go ahead with this one

@ahmetb
Copy link
Member

ahmetb commented Oct 23, 2019

Why do you need —all? Existing plugin already prints all keys in a secret without needing any argument. Please try the existing plugin first. 🙃

@elsesiy
Copy link
Contributor Author

elsesiy commented Oct 23, 2019

Yes I've seen the current plugin, --all however would print keys=values and not just keys. Anyway as I said, I'll go with the name.key approach!

@ahmetb
Copy link
Member

ahmetb commented Oct 23, 2019

Yeah in case that makes sense. —all can print all secrets in the ns in k=v format.

@corneliusweig
Copy link
Contributor

corneliusweig commented Oct 23, 2019

So just to clarify to avoid misunderstandings about the semantics of --all:
@elsesiy you plan to reproduce the behavior of view-secrets where it will print a list of keys in a given secret, when called as kubectl view-secret my-secret. When called as kubectl view-secret my-secret my-key, it will decode the content under my-key in my-secret. That's the behavior of the old implementation.
In addition, you'll support a flag --all, which is used as kubectl view-secret my-secret --all. This will print all keys and decoded values inside a given secret. This is how your new implementation currently works.

Is that correct?

@elsesiy
Copy link
Contributor Author

elsesiy commented Oct 23, 2019

@corneliusweig yes that's exactly how I implemented it now to ensure backward-compatibility but also allow for easy decoding of all contents of a secret. I'll publish it as v0.2.0 and update this PR soon!

@corneliusweig
Copy link
Contributor

@elsesiy If you are revisiting your plugin, also consider if you really need the "k8s.io/cli-runtime/pkg/genericclioptions" and "k8s.io/utils/pointer" dependencies. You are not really making use of those, because you are shelling out to kubectl and the flags don't get passed along. I think you can make your binaries smaller by removing those deps (8MB compressed is quite hefty for what you are doing).

@elsesiy
Copy link
Contributor Author

elsesiy commented Oct 23, 2019

@corneliusweig Yeah I noticed that too and removed these dependencies too, thanks!!

@ahmetb
Copy link
Member

ahmetb commented Oct 23, 2019

/hold
Please let us know when you want to move forward with the new plugin. I'm putting this on hold since it seems like you wanna make some changes.

@k8s-ci-robot k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Oct 23, 2019
@ahmetb
Copy link
Member

ahmetb commented Oct 24, 2019

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


This pull request doesn't seem to be a straightforward version bump. I'll have a human review this.

Why wasn't this detected as a plugin version bump:

file plugins/view-secret.yaml is not a straightforward version bump: diff line unrecognized for version bumps: [+ homepage: https://github.com/elsesiy/kubectl-view-secret]

@elsesiy
Copy link
Contributor Author

elsesiy commented Oct 24, 2019

@corneliusweig @ahmetb I just pushed the changes as discussed, let me know if you have any remarks :)

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 @elsesiy, I see that you've improved your rewrite quite a bit already. There are two more things that need to be discussed and maybe fixed.

  1. When decoding a secret, you should not add a newline after the decoded value. Otherwise, the decoded content will have a newline and that makes a difference when used in a password, for example. (see Printing decoded secrets should not add a newline elsesiy/kubectl-view-secret#1)

  2. The output is still not quite the same. When running this on a secret with several keys, I get

    token
    ca.crt
    namespace
    

    with your version, but the old implementation prints

    Multiple sub keys found. Specify another argument, one of:
    -> ca.crt
    -> namespace
    -> token
    

    The message guides users what to do next and makes clear that what comes after is not the decoded secret. So your implementation should do that too.

plugins/view-secret.yaml Outdated Show resolved Hide resolved
@ahmetb
Copy link
Member

ahmetb commented Oct 26, 2019

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


This pull request doesn't seem to be a straightforward version bump. I'll have a human review this.

Why wasn't this detected as a plugin version bump:

file plugins/view-secret.yaml is not a straightforward version bump: diff line unrecognized for version bumps: [+ homepage: https://github.com/elsesiy/kubectl-view-secret]

@elsesiy
Copy link
Contributor Author

elsesiy commented Oct 26, 2019

@corneliusweig @ahmetb Alright, I changed the format to the original output format, log info messages to stderr and added the -q/--quiet flag. Please let me know if there's anything missing at this point!

@corneliusweig
Copy link
Contributor

@elsesiy great, this looks pretty good now! Thanks for bearing with us through this lengthy review round and for taking over maintenance of this plugin!

/approve
/lgtm

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

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: corneliusweig, elsesiy

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 Oct 26, 2019
@corneliusweig
Copy link
Contributor

/hold cancel

@k8s-ci-robot k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Oct 26, 2019
@k8s-ci-robot k8s-ci-robot merged commit baf63f5 into kubernetes-sigs:master Oct 26, 2019
@elsesiy elsesiy deleted the update-view-secret branch October 27, 2019 03:23
@elsesiy
Copy link
Contributor Author

elsesiy commented Oct 27, 2019

@corneliusweig Thank you, it was a good experience and sorry for not paying too much attention to the backwards-compatibility from the beginning! Won't happen again :) I think the plugins.md needs to be refreshed again, right?

@ahmetb
Copy link
Member

ahmetb commented Oct 27, 2019

I'll send a PR to re-generate that, you can do as well if you like.

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