[JENKINS-53437] Added SFTP protocol implementation for Exec commands #21
Conversation
| String[] commandTokens = Util.replaceMacro(transfer.getExecCommand(), buildInfo.getEnvVars()).split("\n"); | ||
|
|
||
| for (String commandToken:commandTokens) { | ||
| String[] command = commandToken.split(" "); |
There was a problem hiding this comment.
Probably need to trim the commandToken before splitting on spaces
| switch (commandName) { | ||
| case "cd" : | ||
| firstArgument = command[1]; | ||
| changeDirectory(firstArgument); |
There was a problem hiding this comment.
Are the firstArugment, secondArgument, etc really needed? It seems redundant to have them
| firstArgument = command[1]; | ||
| changeDirectory(firstArgument); | ||
| break; | ||
| case "symlink" : |
There was a problem hiding this comment.
I think it would be better to put this under the "ln" case and check for -s
There was a problem hiding this comment.
Actually, it looks like symlink is an alias for ln. Looking at this command list here: https://kb.iu.edu/d/akqg
There was a problem hiding this comment.
Probably, it is. As it seems there's no official command list (even in IETF SFTP protocol drafts), so we've used Linux OpenSSH Client (aka sftp) as a reference. It supports both 'symlink' and 'ln' commands.
| } | ||
|
|
||
| public BapSshTransfer(final String sourceFiles, final String excludes, final String remoteDirectory, final String removePrefix, | ||
| final boolean remoteDirectorySDF, final boolean flatten, final String execCommand, final int execTimeout, | ||
| final boolean usePty, final boolean noDefaultExcludes, final boolean makeEmptyDirs, final String patternSeparator) { | ||
| final boolean usePty, final boolean noDefaultExcludes, final boolean makeEmptyDirs, final String patternSeparator, final boolean useSftpForExec) { |
There was a problem hiding this comment.
Adding to an existing constructor is not a good idea. Please add a setter and annotate it with @DataBoundSetter
| } | ||
|
|
||
| @DataBoundConstructor | ||
| public BapSshTransfer(final String sourceFiles, final String excludes, final String remoteDirectory, final String removePrefix, | ||
| final boolean remoteDirectorySDF, final boolean flatten, final boolean cleanRemote, final String execCommand, final int execTimeout, | ||
| final boolean usePty, final boolean noDefaultExcludes, final boolean makeEmptyDirs, final String patternSeparator) { | ||
| final boolean usePty, final boolean noDefaultExcludes, final boolean makeEmptyDirs, final String patternSeparator, final boolean useSftpForExec) { |
There was a problem hiding this comment.
Same comment as above, adding to the constructor is not needed if you add a setter and annotate it with @DataBoundSetter
| @@ -89,6 +89,10 @@ | |||
| <f:checkbox default="${defaults.transfer.useAgentForwarding}"/> | |||
| </f:entry> | |||
|
|
|||
| <f:entry title="${%useSftpForExec}" field="useSftpForExec"> | |||
There was a problem hiding this comment.
I think it is preferred to use an optionalProperty tag here instead of the checkbox.
There was a problem hiding this comment.
Hi! I'll give a brief comment to my colleague's commit. We considered that optionalProperty would fit better for optional value, not for a single flag (like checkbox). In our case this flag modifies behavior of processing Exec commands only. I suppose that adding separate field for SFTP commands in optionalProperty would result in overcomplexing of plugin and doesn't follow its logical structure (i.e. a) put some file and then b) execute some single set of commands via SSH or SFTP protocol).
There was a problem hiding this comment.
Sorry, I was looking at this wrong, you are correct, the checkbox is fine.
|
Looks ok in general (see other feedback). It would be nice to have a test to test the parsing of the different commands. |
…ransfer constructors restored.
Hello!
Please find attached the initial implementation of [JENKINS-53437] feature proposition (execution of Exec statements over SFTP protocol commands, not over SSH). Currently some basic but (probably) sufficient set of operations is implemented (cd, rm, rmdir, mkdir, symlink, ln).
Regards,
Alexander