-
Notifications
You must be signed in to change notification settings - Fork 1k
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
Infrastructure for writing to pluggable Galaxy file sources. #10152
Conversation
37c8f48
to
d950bdc
Compare
@@ -12,6 +12,7 @@ def __init__(self, label="FTP Directory", doc="Galaxy User's FTP Directory", roo | |||
root=root, | |||
label=label, | |||
doc=doc, | |||
writable=True, |
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.
Oh this is super cool! However, currently, our standard FTP server is usually not configured to allow downloads. We would need a second server with a different directory that only allows downloads and no uploads - is that correct @natefoo? If so, is it time to introduce ftp_download_*
options?
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 would think that would be a very natural place to start dumping large files (history exports, collection zips, etc.), so I think swapping the default here to True
makes sense. It can be overridden explicitly with the config file but it probably makes sense to have galaxy.yml options also.
I guess for options we should make ftp_upload_dir
an alias for ftp_dir
and user_library_dir
an alias for user_library_import_dir
and then add ftp_dir_writable
(default True) and user_library_dir_writable
(default False)?
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.
so I think swapping the default here to True makes sense.
Yes for sure. This is IMHO the easiest to set up globally and has the biggest impact.
But then I learned I can not download those datasets from the standard FTP directories :)
ftp_dir_writable
I'm not sure if this is enough. Would like to have @natefoo opinion here. Is it still recommended to make the upload-ftp upload-only? I don't think having two FTP servers running is a big deal. But maybe all of that is not necessary anymore?
Another aspect is that two different directories for up- and download might make also sense from the admin point of view.
We have cron jobs running cleaning old unused FTP uploads ... we probably want to handle "archives" (downloads) differently.
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'm happy to get @natefoo's insights also. But I will just say none of this prevents you from configuring two FTP directories - the whole thing is configurable now. I just think that is an advanced configuration and shouldn't be the default assumed.
If we're going to make a push toward documenting more advanced configurations - I don't think we should take user library import dir, ftp dir, and add a new fixed named thing like ftp download dir. We should instead work toward just documenting how to configure any of these things you want in a galaxy files configuration - including other directories with other options, etc..
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're going to make a push toward documenting more advanced configurations - I don't think we should take user library import dir, ftp dir, and add a new fixed named thing like ftp download dir. We should instead work toward just documenting how to configure any of these things you want in a galaxy files configuration - including other directories with other options, etc..
Ok, you convinced me. If we can easily use a second FTP this is not a problem at all. But the default configuration should be secure imho.
Thanks for this PR!
xref: #2968 |
d950bdc
to
04a4940
Compare
Taking this out of WIP - I think this is a good atomic improvement. Next steps I think would be:
|
04a4940
to
4647549
Compare
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.
Works flawlessly for me!
I restarted the failing test. @mvdbeek do you think we can get this in. It would be nice for the Beacon-COVID-19 project and would make my life a little bit easier. |
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.
This is truly awesome, thanks a lot @jmchilton!
🥇 ❤️ |
I thought that this was going to take a lot more work to build out some applications and get people excited - sounds like y'all already have that covered though. That is awesome - I'm excited to see how this is used! |
Mine will just be hacks, as usual, but this makes such hakes I think way easier :) |
Initial plugin implementation added to 20.09 branch with #9888