Add tls to ftp #1581
Add tls to ftp #1581
Conversation
fsspec/implementations/ftp_tls.py
Outdated
| self.ftp = FTP_TLS(timeout=self.timeout) | ||
| self.ftp.connect(self.host, self.port) | ||
| self.ftp.login(*self.cred) | ||
| self.ftp.prot_p() |
There was a problem hiding this comment.
Please take a look at this.
|
I've added tests (mostly copied from test_ftp.py, not all are yet working. Could use some help here :) |
|
Thanks for contributing! Before going further, do you think it's possible to provide the extra features as options on the original FTP filesystem rather than create a new variant that is largely the same? |
|
No worries, in a sense we could give two additional arguments on the init of FTPFileSystem:
Then we could modify the _connect to use either FTP or TLS_FTP and modify the init to activate prot_p if it's equal to True after setting the connection. How does this sound to you? |
|
Yes, exactly. For testing, you would need a secured server, of course, but you could essentially run all FTP tests with standard and secure servers. |
|
I've removed the the |
There was a problem hiding this comment.
Is this an exact copy of he insecure tests? You can use pytest.mark.parametrize to run each of the existing tests, once with secure=False and once with True.
There was a problem hiding this comment.
This is not possible since it will need a different fixture.
There was a problem hiding this comment.
It is totally possible to have a parametrised fixture, or two fixtures that you choose between. https://stackoverflow.com/a/29407931/3821154 shows an example.
|
Ping: is this PR still progressing? |
|
Thanks for the reminder, so far I've not had the time to further implement the tests. |
|
I've found some time to update the tests. The pyftpdlib server has been replaced with script that starts a pyftpdlib server that accepts TLS. Furtermore, docstrings and codestyle improvements have been made. @martindurant: Do you understand why the CI / downstream fails? |
Please merge from master and try again |
All checks have passed :) |
See #1580.
It is basically a copy of the FTP class in fsspec, with a modification to use FTP_TLS and one addtional change.
Please have a look at this.