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

FTP Added explicitSSL parameter to FtpsSettings (#976). #977

Merged
merged 4 commits into from Jun 5, 2018

Conversation

anzecesar
Copy link
Contributor

I've tested it agains the server, which requires encrypted data transfer and it works.

I'm not super sure how to add this case to the specs yet.

@anzecesar
Copy link
Contributor Author

This still needs to be rebased, once #975 is merged.

@ennru ennru added the p:ftp label May 28, 2018
@ennru
Copy link
Member

ennru commented May 28, 2018

I wonder if we could find a good way to hook into the creation of the ftp client so that any special configuration could be added.
For JMS we have a CustomDestination which allows for such a hook.

@anzecesar
Copy link
Contributor Author

@ennru Good point.

I'll take a look at the JMS code, when I get a chance.

@anzecesar
Copy link
Contributor Author

anzecesar commented May 28, 2018

@ennru Do you think adding a FTPSClient => Unit to the FtpsSettings instead of explicitSSL would do the trick?

Also, if you have any naming suggestions, I'm all ears :). extraConnectionOperations?

I feel like it needs to convey intent clearly, since FTPSClient => Unit could be used for anything :).

@ennru
Copy link
Member

ennru commented May 29, 2018

Yes, a general hook to allow for custom configuration of the FTP client is what I'd like to see. By default, it would just be empty.
What about configureConnection?

@@ -74,6 +77,9 @@ final case class FtpSettings(

def withPassiveMode(passiveMode: Boolean): FtpSettings =
copy(passiveMode = passiveMode)

def withConfigureConnection(configureConnection: FTPClient => Unit): FtpSettings =
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ennru I'm assuming this is intended mostly for the Java api. Should I make this take a Function[FTPClient, Unit]? Or do I need to use Void? I never made Java api in Scala before :).

I want to add an example to the docs, but I'm struggling with how to use this in Java 😅

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tnx. I was unable to convert that to Scala. Luckily, Java's lambda fit in elegantly.

@anzecesar
Copy link
Contributor Author

@ennru lmk if I should make any more changes. I'm using this in my project already, and am happy with it :).

Copy link
Member

@ennru ennru left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

@ennru
Copy link
Member

ennru commented Jun 5, 2018

But it has a problem with the Java API on Scala 2.11.

@ennru
Copy link
Member

ennru commented Jun 5, 2018

@anzecesar I added a Java-specific method taking a j.u.f.Consumer explicitly to support Java with the Scala 2.11, compiled code.

@ennru ennru added this to the 0.20 milestone Jun 5, 2018
@ennru ennru merged commit 30ffd79 into akka:master Jun 5, 2018
@ennru
Copy link
Member

ennru commented Jun 5, 2018

Thank you for your efforts, great to see this improving!

@anzecesar
Copy link
Contributor Author

@ennru Tnx. Happy to contribute :).

sfali pushed a commit to sfali/alpakka that referenced this pull request Jun 18, 2018
@sigurd-cp
Copy link

Yes, a general hook to allow for custom configuration of the FTP client is what I'd like to see. By default, it would just be empty. What about configureConnection?

Sounds interesting, do you have an example? In the FTPSClient the implicit flag is final (internally).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants