-
Notifications
You must be signed in to change notification settings - Fork 510
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 sshd
#462
Conversation
Welcome @ottoyiu! |
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: ottoyiu 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 |
I'll defer to @corneliusweig as he's reviewed similar plugins recently. |
Hey @ottoyiu, thanks for submitting this plugin to krew. As a first step, I'd like to understand what is the use-case for this plugin. Your landing page rightfully says that it should not be (ab)used for a remote shell or copying files. Instead, users should use In particular, for your plugin to work the container already needs to be capable of Additionally, you return the |
Hi @corneliusweig! Thanks for taking a look and reviewing my plugin. We have multiple Kubernetes clusters on-premise, that have One of the use-cases for this plugin for us, is having the ability to transfer existing Pod's data from one cluster to another using rsync over SSH. Sometimes, it is difficult for us to inject a sidecar that has access to the filePath/volume path in order for us to pull data off the Pod. An example of this is, an application dumping a coredump in a filepath that's not stored in a volume/filepath that a sidecar can access, yet we want to remotely pull it off the Pod and aggregate it without having to download it locally using Hope that clarifies the plugin somewhat. |
Thanks for clarifying this. Are there any other important use-cases that you want to solve with this plugin? If what you want to achieve is My concern with the current plugin is that it looks too much like a |
The main use-case is to enable any users to use an SSH client straight up to remote connect to a Pod, and also to allow any applications that use the SSH protocol to complete its task.
I plan to add examples of how to use Overall, I draw parallels of making this plugin more specific for a use-case like rsync to something as having |
I think increasingly it's becoming more clear the line is hard to draw, but in this space, there's increasingly more and more plugins –and this one is distinct enough. Since it's valid to a considerable subset of users, I feel like we should have this in krew-index. Many other plugins are also applicable to small set of users. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Alright, then let's add this to krew-index
.
Your manifest already looks quite good. There are just minor nits that need attention (see inline comments).
Finally, can you make sure that all text blocks wrap at ~80 chars, so that it prints nicely on smaller screens?
7c6691d
to
307f680
Compare
@ahmetb @corneliusweig thank you for accepting the plugin! I have made the requested changes. |
@alonyb It looks like something is wrong with out GitHub Workflow check: This probably happened because:
|
So the action does the following:
Looks like @alonyb Would this also work?
|
/lgtm |
@corneliusweig @ahmetb For these cases, I could make a validation for PR(maybe or sth to identify force-pushed) to comparing only the new hash
|
Perhaps I'm misunderstanding this statement. Squashing commits is rather standard for PRs to remove intermediate commits that pollute the commit history with unimportant commit messages. After a squash/rebase, it makes sense to force-push it back on the same branch as the opened PR. It feels pretty important to me that a workflow check would support this. If there's a better practice, I would love to know about it so that I can be a better contributor. Thanks :) |
This #476 should fix this, it's the solution I mentioned before
|
“Force pushes is a not a good practice” is simply inaccurate. :) |
kubectl sshd
kubectl sshd
Merged with admin privileges as the failing check is due to a bug in our CI. |
Requesting to add a new plugin:
kubectl sshd
.Thanks for the consideration :)