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

added plugin pod-inspect #1021

Merged
merged 3 commits into from
Feb 13, 2021
Merged

added plugin pod-inspect #1021

merged 3 commits into from
Feb 13, 2021

Conversation

jpriebe
Copy link
Contributor

@jpriebe jpriebe commented Feb 9, 2021

No description provided.

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Feb 9, 2021
@k8s-ci-robot k8s-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Feb 9, 2021
@jpriebe
Copy link
Contributor Author

jpriebe commented Feb 9, 2021

/assign @chriskim06

@ahmetb
Copy link
Member

ahmetb commented Feb 9, 2021

/hold
You have manually removed the pull request template which asks you to read the naming guidelines.

@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 Feb 9, 2021
@jpriebe
Copy link
Contributor Author

jpriebe commented Feb 9, 2021

@ahmetb - I'm not sure what I did wrong here. Yes, I removed the template, but that appeared to be a comment that doesn't need to stay in the request once it's filed. I did read the naming guidelines. Did I violate the guidelines? I've already had to rename my plugin twice to try to get something that works with kubectl and that (I thought) met the guidelines.

@ahmetb
Copy link
Member

ahmetb commented Feb 9, 2021

Punctuation
Plugin names must be all lowercase and separate words with hyphens. Don’t use camelCase, PascalCase, or snake_case; use kebab-case.

https://krew.sigs.k8s.io/docs/developer-guide/develop/naming-guide/

@jpriebe
Copy link
Contributor Author

jpriebe commented Feb 10, 2021

When I first read the naming guidelines, I was still planning to use "pod-inspect". I then tried "pod-inspect", and it didn't work (I put kubectl-pod-inspect in my path, but kubectl wouldn't recognize it), so I renamed it "pod_inspect", forgetting item #1 in the naming guidelines. My apologies.

I understand that krew does some symlink magic, but if I use kebab-case, then my plugin won't work outside of a krew context (without asking my users to do some extra work to rename the executable or symlink it or whatever).

Do you have any guidance so that a developer can build a pluging that easily works both inside and outside of krew?

@chriskim06
Copy link
Member

You can leave the binary name snake cased, it's just the plugin name should be kebab cased. There's a few plugins like this, cert-manager is one that I can immediately think of.

@jpriebe
Copy link
Contributor Author

jpriebe commented Feb 10, 2021

@chriskim06 - great idea. I'm refactoring now. One thing I noticed: the go snippet to detect whether running as a plugin on this page: https://krew.sigs.k8s.io/docs/developer-guide/develop/best-practices/ doesn't seem to actually do anything different when the command is run directly vs as a plugin.

I'd really like to detect that so that I can output appropriate usage for the person who happens to run my plugin directly. I find for kubectl plugin beginners, this dichotomy is kind of confusing. Especially with the "-"/"_" naming issues.

If you've got any other techniques for accurately detecting kubectl-pod_inspect vs kubectl pod-inspect invocations, I'd love to see them. I'll spend a little time experimenting myself. Maybe I can detect that my process was execve'd by a process called with a different command line?

@chriskim06
Copy link
Member

hmm I'm not sure, afaik none of our plugins do this currently. I would think most people would invoke your plugin via kubectl, so it wouldn't be an issue.

@ahmetb
Copy link
Member

ahmetb commented Feb 10, 2021

If you install your plugin with Krew, that snippet will contain the name of the symlink (kubectl-foo_bar) we create for your plugin in os.Args[0].

So if you distribute your binary without kubectl- prefix, you can easily tell the difference. Does that help?

@jpriebe jpriebe changed the title added plugin pod_inspect added plugin pod-inspect Feb 10, 2021
@jpriebe
Copy link
Contributor Author

jpriebe commented Feb 10, 2021

Updated pull request with renamed plugin. I found a couple of thing:

  • there doesn't seem to be a way to reliably distinguish between a user calling your plugin directly or calling it via kubectl; because of this, I opted to craft my usage text to reflect the common case of calling it through kubectl
  • cobra really does not like putting "kubectl " in front of your Use: string. It will create some highly inaccurate usage strings from that, and you have to jump through a lot of hoops to override the template. Not sure if there's a better approach.

@jpriebe
Copy link
Contributor Author

jpriebe commented Feb 10, 2021

@ahmetb - that's an option I didn't think about - I was fixated on calling my binary "kubectl-SOMETHING". I don't think it's terrible the way I've structured it, but maybe at some point in the future, I'll change the binary name so I can tell the difference. I've spent WAY too much time refactoring this thing, and I'm not inclined to change anything needlessly at this point.

plugins/pod-inspect.yaml Outdated Show resolved Hide resolved
@ahmetb
Copy link
Member

ahmetb commented Feb 11, 2021

/lgtm
/approve
/hold
cc: @corneliusweig any objections?

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

[APPROVALNOTIFIER] This PR is APPROVED

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

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 Feb 11, 2021
@corneliusweig
Copy link
Contributor

This looks like a fine plugin! Thanks for building and sharing! 🎉
/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 Feb 13, 2021
@k8s-ci-robot k8s-ci-robot merged commit 886ac58 into kubernetes-sigs:master Feb 13, 2021
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.

None yet

5 participants