FTP: Support custom SSH configuration #875

fmsbeekmans · 2018-04-04T16:05:48Z

Made it possible to use a custom SSH configuration.

An alternative would be the possibility to inject an SSHClient instance.

This should help with #874.
Worked together with @HarrisonBaxter

ennru · 2018-04-04T17:52:50Z

Looks good. Could you add it to the docs in ftp.md as well, please?

fmsbeekmans · 2018-04-05T10:54:17Z

Changed it such that you can use an SSHClient instead. This also allows reuse of an existing SSHClient instance. The method name in the scalaDsl is apply but in the javaDsl is withSshClient, is that considered idiomatic or should they be the same?

ennru

The compilation on Travis is broken as Java has issues with your changes.

ennru · 2018-04-07T20:08:12Z

ftp/src/main/scala/akka/stream/alpakka/ftp/javadsl/FtpApi.scala

+    * @param customSshClient custom ssh client
+    * @return A [[akka.stream.alpakka.ftp.javadsl.Sftp]]
+    */
+  def withSshClient(customSshClient: SSHClient): Sftp =


The withXyz methods are used have something to replace Scala's copy methods on case classes.
As constructor I'd recommend create where apply is used in Scala.

ennru · 2018-04-11T08:30:59Z

ftp/src/main/scala/akka/stream/alpakka/ftp/javadsl/FtpApi.scala

+   * @param customSshClient custom ssh client
+   * @return A [[akka.stream.alpakka.ftp.javadsl.FtpApi]] of [[net.schmizz.sshj.SSHClient]] with [[akka.stream.alpakka.ftp.impl.SftpSourceParams]]
+   */
+  def withSshClient(customSshClient: SSHClient): FtpApi[SSHClient] with SftpSourceParams =


Would it help the Java API to create a class Sftp and base the object on it?
I'd still like to call it create or createWithSshClient as a dual to Scala's apply.

Could you make it return a Sftp instance instead of FtpApi[SSHClient]?

In 013f9c9 we tried to create an SFTP class that the current SFTP object inherited from. This way all existing code using the SFTP object would still work.

Sadly Java doesn't play well with this, it is not able to resolve the methods inherited from the traits on companion object as static methods.

We could make a separate type for instead but it would look a bit awkward.

What about introducing
class SftpApi extends FtpApi[SSHClient] with SftpSourceParams
and extending and returning that?

HarrisonBaxter · 2018-04-19T09:48:06Z

@ennru is there more work needed for this, or does it look ok to merge?

HarrisonBaxter · 2018-04-19T13:40:49Z

Ferdy and I have done it like this to try and avoid breaking backward compatibility

…

On Thu, Apr 19, 2018 at 11:32 AM, Enno ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In ftp/src/main/scala/akka/stream/alpakka/ftp/javadsl/FtpApi.scala <#875 (comment)>: > @@ -238,4 +238,16 @@ sealed trait FtpApi[FtpClient] { _: FtpSourceFactory[FtpClient] => object Ftp extends FtpApi[FTPClient] with FtpSourceParams object Ftps extends FtpApi[FTPClient] with FtpsSourceParams -object Sftp extends FtpApi[SSHClient] with SftpSourceParams +object Sftp extends FtpApi[SSHClient] with SftpSourceParams { + + /** + * Java API: creates a [[akka.stream.alpakka.ftp.javadsl.FtpApi]] of [[net.schmizz.sshj.SSHClient]] with [[akka.stream.alpakka.ftp.impl.SftpSourceParams]] + * + * @param customSshClient custom ssh client + * @return A [[akka.stream.alpakka.ftp.javadsl.FtpApi]] of [[net.schmizz.sshj.SSHClient]] with [[akka.stream.alpakka.ftp.impl.SftpSourceParams]] + */ + def withSshClient(customSshClient: SSHClient): FtpApi[SSHClient] with SftpSourceParams = Could you make it return a Sftp instance instead of FtpApi[SSHClient]? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#875 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AQ8bMRhKYfyDMpqJOjkiDTCdF9iA-oeFks5tqGewgaJpZM4THE2l> .

fmsbeekmans · 2018-04-24T08:15:05Z

Hi @ennru , is there more work to be done before this can be merged?

HarrisonBaxter · 2018-04-24T13:58:25Z

We were hoping to achieve the same return type when using a custom ssh client as the default one. The proposed solution does not allow this in conjunction with keeping the backward compatibility. Update, sounds like a good idea actually

…

On Tue, Apr 24, 2018 at 10:34 AM, Enno ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In ftp/src/main/scala/akka/stream/alpakka/ftp/javadsl/FtpApi.scala <#875 (comment)>: > @@ -238,4 +238,16 @@ sealed trait FtpApi[FtpClient] { _: FtpSourceFactory[FtpClient] => object Ftp extends FtpApi[FTPClient] with FtpSourceParams object Ftps extends FtpApi[FTPClient] with FtpsSourceParams -object Sftp extends FtpApi[SSHClient] with SftpSourceParams +object Sftp extends FtpApi[SSHClient] with SftpSourceParams { + + /** + * Java API: creates a [[akka.stream.alpakka.ftp.javadsl.FtpApi]] of [[net.schmizz.sshj.SSHClient]] with [[akka.stream.alpakka.ftp.impl.SftpSourceParams]] + * + * @param customSshClient custom ssh client + * @return A [[akka.stream.alpakka.ftp.javadsl.FtpApi]] of [[net.schmizz.sshj.SSHClient]] with [[akka.stream.alpakka.ftp.impl.SftpSourceParams]] + */ + def withSshClient(customSshClient: SSHClient): FtpApi[SSHClient] with SftpSourceParams = What about introducing class SftpApi extends FtpApi[SSHClient] with SftpSourceParams and extending and returning that? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#875 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AQ8bMQx-li6-fd-i_1MABl5TOkWTLHaYks5trvGogaJpZM4THE2l> .

ennru

Yeah, this is much better than the generic type.

ennru · 2018-04-24T17:00:43Z

ftp/src/test/java/akka/stream/alpakka/ftp/examples/ConfigureCustomSSHClient.java

+
+    public ConfigureCustomSSHClient() {
+        SSHClient sshClient = new SSHClient(new DefaultConfig());
+        FtpApi<SSHClient> sftp = Sftp.create(sshClient);


You should change this to SftpApi as well.

ennru · 2018-04-24T17:01:57Z

ftp/src/main/scala/akka/stream/alpakka/ftp/scaladsl/FtpApi.scala

@@ -169,4 +169,18 @@ sealed trait FtpApi[FtpClient] { _: FtpSourceFactory[FtpClient] =>

 object Ftp extends FtpApi[FTPClient] with FtpSourceParams
 object Ftps extends FtpApi[FTPClient] with FtpsSourceParams
-object Sftp extends FtpApi[SSHClient] with SftpSourceParams
+class Sftp extends FtpApi[SSHClient] with SftpSourceParams


Would make sense to give it the same structure in the Scala API, WDYT?

ennru

LGTM

ennru · 2018-04-25T13:14:14Z

Thank you for getting this in place!

Made it possible to use custom SSH configuration.

5f81fbd

ennru added the p:ftp label Apr 4, 2018

ennru changed the title ~~Made it possible to use custom SSH configuration.~~ FTP: Support custom SSH configuration Apr 4, 2018

allow SSHClient instead of config

cf6925a

HarrisonBaxter added 3 commits April 5, 2018 16:53

refactored Sftp to be a class with a default instance

013f9c9

added readme and examples for custom ssh client

f81771b

made the ftp an object again in the java dsl

92af9ec

ennru reviewed Apr 7, 2018

View reviewed changes

ennru reviewed Apr 11, 2018

View reviewed changes

renamed withSshClient in the java api

b7b6cad

made SftpApi class to clean up the return type

fb6e938

ennru requested changes Apr 24, 2018

View reviewed changes

changed the scala dsl to be consistent with the java dsl

fbfa107

ennru approved these changes Apr 25, 2018

View reviewed changes

ennru merged commit bd2c8e7 into akka:master Apr 25, 2018

ennru added this to the 0.19 milestone Apr 25, 2018

ennru mentioned this pull request Apr 25, 2018

FTP: SecureRandom hangs on docker #874

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

FTP: Support custom SSH configuration #875

FTP: Support custom SSH configuration #875

fmsbeekmans commented Apr 4, 2018

ennru commented Apr 4, 2018

fmsbeekmans commented Apr 5, 2018

ennru left a comment

ennru Apr 7, 2018

ennru Apr 11, 2018

ennru Apr 19, 2018

fmsbeekmans Apr 20, 2018

ennru Apr 24, 2018

HarrisonBaxter commented Apr 19, 2018

HarrisonBaxter commented Apr 19, 2018 via email

fmsbeekmans commented Apr 24, 2018

HarrisonBaxter commented Apr 24, 2018 via email •

edited

ennru left a comment

ennru Apr 24, 2018

ennru Apr 24, 2018

ennru left a comment

ennru commented Apr 25, 2018

FTP: Support custom SSH configuration #875

FTP: Support custom SSH configuration #875

Conversation

fmsbeekmans commented Apr 4, 2018

ennru commented Apr 4, 2018

fmsbeekmans commented Apr 5, 2018

ennru left a comment

Choose a reason for hiding this comment

ennru Apr 7, 2018

Choose a reason for hiding this comment

ennru Apr 11, 2018

Choose a reason for hiding this comment

ennru Apr 19, 2018

Choose a reason for hiding this comment

fmsbeekmans Apr 20, 2018

Choose a reason for hiding this comment

ennru Apr 24, 2018

Choose a reason for hiding this comment

HarrisonBaxter commented Apr 19, 2018

HarrisonBaxter commented Apr 19, 2018 via email

fmsbeekmans commented Apr 24, 2018

HarrisonBaxter commented Apr 24, 2018 via email • edited

ennru left a comment

Choose a reason for hiding this comment

ennru Apr 24, 2018

Choose a reason for hiding this comment

ennru Apr 24, 2018

Choose a reason for hiding this comment

ennru left a comment

Choose a reason for hiding this comment

ennru commented Apr 25, 2018

HarrisonBaxter commented Apr 24, 2018 via email •

edited