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

Add FTPS support (#33) #739

Merged
merged 6 commits into from
Nov 25, 2022
Merged

Add FTPS support (#33) #739

merged 6 commits into from
Nov 25, 2022

Conversation

RachitSharma2001
Copy link
Contributor

@RachitSharma2001 RachitSharma2001 commented Nov 16, 2022

Title

Support for reading and writing files directly to/from ftps (ftp with tls)

Motivation

I have previously added support ftp servers (without tls encryption). Now, to add more complete ftp functionality, I have added support for ftp+tls servers.

from smart_open import open
with open('ftps://user:password@host:port/dir1/dir2/file') as fin:
     for line in fin:
          print(line)

Tests

I have edited the ftp integration tests to run an ftp and ftps server on different ports and then test reading, writing, and appending to each of them. (specific integration test here, pull up ftp and ftps servers here).

Copy link
Owner

@piskvorky piskvorky left a comment

Choose a reason for hiding this comment

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

Looks good but can you look into not requiring & running that external Docker image for the testing?

If you could fix that we'll include this PR in the upcoming smart_open release (next week or so). Thanks.

--env HOME_DIR=/home/user \
--env FTP_PORT=21 \
--env FTPS_PORT=90 \
rachitsharma2001/ftp_ftps
Copy link
Owner

@piskvorky piskvorky Nov 19, 2022

Choose a reason for hiding this comment

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

Running external docker images is scary, both from security and also maintenance perspective.

Any way to launch the FTP(S) server directly from Python here?

Copy link
Contributor Author

@RachitSharma2001 RachitSharma2001 Nov 19, 2022

Choose a reason for hiding this comment

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

Thank you for letting me know. I have removed the docker dependency and instead directly set up the ftp and ftps servers on ci using the vsftpd linux tool. Let me know if this looks good or if there is anything else I should add or change.

@piskvorky piskvorky added this to the 6.3.0 milestone Nov 19, 2022
@RachitSharma2001 RachitSharma2001 force-pushed the ftps branch 24 times, most recently from 5650e6f to 2c7a009 Compare November 19, 2022 20:17
@piskvorky
Copy link
Owner

Thanks for the quick resolution @RachitSharma2001.

@mpenkov can you please review?

It's simpler if we just run the whole script under sudo.
The docstring helps our users.
Also, we don't need read-text and write-text in FTP, that gets handled in a
different part of smart_open.
since helpers.sh runs under sudo, it doesn't have access to the
environment set in the workflow
looks like it _does_ get used after all
@mpenkov
Copy link
Collaborator

mpenkov commented Nov 25, 2022

Thank you for your work @RachitSharma2001 !

@mpenkov mpenkov merged commit a0bb975 into piskvorky:develop Nov 25, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants