Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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 FTP support to jobstores #5142
Add FTP support to jobstores #5142
Changes from 4 commits
195fdc8
176381c
af46642
1ea4e3d
65e5564
5ba00b2
35fbe08
754b43c
5ed71cf
17536ba
13f4ac5
f234825
0f38d58
bc5da85
665e3bd
a7455a0
7d31e54
d2da46c
a2105af
66d3587
9f86277
91f1cbd
5884417
f490ed3
bbd321a
59e3fea
8fe81cc
f499caf
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we keep a class-level
FtpFsAccess
instance that is never destroyed, and theFtpFsAccess
instance caches open FTP connections to servers, when do we close our FTP connections? It might be never, and that's probably not what we want.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's weird that the
ftp
member is public, but the function that must be called before it is actually used has an underscore prefix.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you structured this as a method that returns an FtpFsAccess and not one that has a side effect of setting a class-level field, you wouldn't need all these asserts and you also wouldn't need to remember two member names.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The license at cwl-tes is also Apache 2.0. Should I add a section to the Toil license that specifically states all code under this class is under cwl-tes's Apache 2.0 license?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You don't need to repeat the license.
If there are code quotes, then you could mention the copyright of the original authors / copyright holders:
Otherwise if this implementation is different enough, you don't need to mention the authors.
I suggest this metric: if we wanted to change the license of Toil, would you have to get permission from these copyright holders because the code is essentially the same? Or is the code so adapted or different for other reasons that it is a new work?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One approach is just add their Apache 2.0 license's copyright line to Toil's LICENSE file along with the others.
You could also drop it in a different file and import it to here, and in that file have
Copyright 2017 Oregon Health and Science University
and the copyright boilerplate we use for our files. I think.Also the
#8
at the end of this link is pointing to the wrong line number.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's easier to do the latter by putting the code in a separate file, so I'll do that
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should have docstrings for this and all the other functions in the class, ideally. It's especially important with a parameter called
insecure
; what is not being secured, and what is it not being secured from? Does it mean we're vulnerable to password sniffing and someone tampering with the FTP data in transit, or are we now executing arbitrary code from every file we download, or what?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this using an undocumented
secure
parameter? What does that do?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ftp in this case is an FTP_TLS object, which supports connecting over TLS. It's not on the official FTP_TLS python documentation, nor is there an associated docstring, but the code itself simply sets up an SSL connection to the FTP server