Support the execution of SSH commands with optional input. #1831
Conversation
gordonmessmer
force-pushed
the
scp-no-mktemp
branch
from
da89c33
to
c186d67
Compare
|
/approve |
|
Lint is complaining about some of your code? |
gordonmessmer
force-pushed
the
scp-no-mktemp
branch
from
c186d67
to
413d556
Compare
|
I've taken care of the lint problems, but I'm not sure how to address the CI failure. If I run "make test" on the unmodified "main" branch, I see what looks like the same failures as the tests in my branch. |
|
fails for me too ... can someone take a peek at this? |
gordonmessmer
force-pushed
the
scp-no-mktemp
branch
from
413d556
to
dbf804a
Compare
|
@baude @edsantiago PTAL |
|
@gordonmessmer could you write a more verbose commit message on why we need this PR (i'm not doubting we do)? when i review a PR, I really want to have the context (bigger picture) of why this is important, etc. And normally, when things come the team, I already have that ... so when it comes outside, it just really helps. I would appreciate that. I also want reviews from @edsantiago and @Luap99 but both are out today. |
gordonmessmer
force-pushed
the
scp-no-mktemp
branch
from
dbf804a
to
c21dfa6
Compare
|
Certainly! I've updated the commit message. Please let me know if any other changes would be helpful. |
pkg/ssh/ssh.go
Outdated
| return ExecWithInput(options, kind, nil) | ||
| } | ||
|
|
||
| func ExecWithInput(options *ConnectionExecOptions, kind EngineMode, input *os.File) (string, error) { |
There was a problem hiding this comment.
input should be io.Reader because the lower level APIs accept an io.Reader and this is much more flexible.
There was a problem hiding this comment.
That makes sense to me. Thanks.
There was a problem hiding this comment.
(I can't tell if the CI tests are meaningful at this point. I'll manually test podman-image-scp with these changes as soon as I get a chance.)
There was a problem hiding this comment.
I don't think there are any useful scp tests here.
gordonmessmer
force-pushed
the
scp-no-mktemp
branch
2 times, most recently
from
fa2ff70
to
8921971
Compare
pkg/ssh/ssh.go
Outdated
| return ExecWithInput(options, kind, nil) | ||
| } | ||
|
|
||
| func ExecWithInput(options *ConnectionExecOptions, kind EngineMode, input *io.Reader) (string, error) { |
There was a problem hiding this comment.
| func ExecWithInput(options *ConnectionExecOptions, kind EngineMode, input *io.Reader) (string, error) { | |
| func ExecWithInput(options *ConnectionExecOptions, kind EngineMode, input io.Reader) (string, error) { |
io.Reader is already an interface so this should not be a pointer to the interface as it adds extra indirection for no reason.
gordonmessmer
force-pushed
the
scp-no-mktemp
branch
from
8921971
to
4cf1cef
Compare
|
The PR has been rebased, and the fedora-39 test has passed. Pointers have been removed. (I'm afraid my experience with Go is still a bit limited.) If you have suggestions on what you'd like to see in the tests, I can do some additional work in that area. |
|
Regarding lint errors: this is another area where my inexperience with golang is showing. I kept golangConnectionExec and nativeConnectionExec because I wasn't sure if they were part of the public interface of the library. If they're not, I expect they could be removed. If they should be kept, then I'm not sure how to address the lint error. |
|
Functions which begin with a lower case letter are private so they can be removed. |
gordonmessmer
force-pushed
the
scp-no-mktemp
branch
from
4cf1cef
to
446d96e
Compare
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: gordonmessmer, Luap99, rhatdan 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 |
|
Lint is not happy. |
Because the SSH exec functions do not currently support execution with input, some users (such as the podman-image-scp) must operate in two phases, first copying a file to the remote host, and then executing a command with shell redirection and removing the temporary file. This API extension allows those users to simplify their code and avoid the use of temporary files. Signed-off-by: Gordon Messmer <gordon.messmer@gmail.com>
gordonmessmer
force-pushed
the
scp-no-mktemp
branch
from
446d96e
to
2681204
Compare
|
The CI result looks like it might be indicating three files with issues identified by lint, but I've installed gci and it only found whitespace issues in two files, so this push may or may not fix the lint issues. Is there any documentation that describes running tests locally? I haven't quite figured that part out, and it seems like it'd cut down some back-and-forth. If not, then I'll just wait for CI results. :) |
|
/lgtm |
|
/unhold |
These changes are needed to support containers/podman#21420
Supporting an input file for SSH commands can avoid the use of temporary files.