-
Notifications
You must be signed in to change notification settings - Fork 106
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
Support cloud storage file transfers #2186
Support cloud storage file transfers #2186
Conversation
783bafb
to
fd19d97
Compare
Synced and resolved conflicts with master. |
fd19d97
to
0d40b44
Compare
from_fs = body_params.fetch(:from_fs, "fs") | ||
to_fs = body_params.fetch(:to_fs, "fs") |
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 wonder if we want to keep the same pattern of src
and dest
instead of to
and from
. They're the same semantically, I just wonder if we want to standardize the language everywhere.
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.
Using src
and dest
sounds better to me. Current naming of from
and to
is from clip_board.js
which uses that naming pattern. Is this something we want to standardize across both the backend and frontend, or would simply renaming the request parameters be sufficient?
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 at least changing new code is our best bet. We can keep the js as is and defer it's refactor.
So, we can't change what we're getting, but we can change what we name the internal variable sort of thing.
from_fs = body_params.fetch(:from_fs, "fs") | |
to_fs = body_params.fetch(:to_fs, "fs") | |
src_fs = body_params.fetch(:from_fs, "fs") | |
dest_fs = body_params.fetch(:to_fs, "fs") |
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.
Do we want to change to use src_fs
and dest_fs
as the names of the params in the request at the same time too as this PR introduces that? Easier to get that right from the beginning than change it later.
I.e. change only the body here:
ondemand/apps/dashboard/app/javascript/packs/files/file_ops.js
Lines 473 to 481 in bc73353
return fetch(transfersPath, { | |
method: 'post', | |
body: JSON.stringify({ | |
command: action, | |
files: files, | |
from_fs: from_fs, | |
to_fs: to_fs, | |
}), | |
headers: { 'X-CSRF-Token': csrf_token } |
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.
from_fs = body_params.fetch(:from_fs, "fs") | ||
to_fs = body_params.fetch(:to_fs, "fs") | ||
|
||
if from_fs == "fs" && to_fs == "fs" |
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 can defer this - but the string "fs"
is around enough that we likely need to make it a constant and refer to it instead of hoping we type fs
correctly all over the place.
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.
That would be a good idea. "fs"
definitely is used in quite a few different places.
What would be the correct place for that constant? RcloneUtil?
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.
That seems as good as any.
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.
Added the constant RcloneUtil::LOCAL_FS_NAME
. Only changed the places where this PR adds "fs"
.
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.
Strange, tests ran perfectly fine locally and worked fine in browser too, but somehow it's broken now. Will fix.
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.
Should be good now, tests passed locally since the tests using RemoteFile
ran first when I ran them locally, and same situation in browser.
https://github.com/OSC/ondemand/compare/bc733533fa94f53440ca97a6d74d8f51b6508e28..7f8652e21a65b205835b1af107fe0acd213574e5
0d40b44
to
4dbf2db
Compare
45e437d
to
bc73353
Compare
Resynced with master (to get #2220) and added |
991a6b0
to
7f8652e
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.
Looks good, thank you so much!
This PR adds support for moving, copying, deleting and renaming files and directories on cloud storages using Rclone, fixes #2129.
This adds
from_fs
andto_fs
optional parameters to the transfers API, which are used to define from and to which Rclone remote files are transferred. The default isfs
, which is treated as the local (posix) filesystem so that it is fully backwards compatible. The feature is enabled by settingOOD_FILES_APP_REMOTE_FILES
to true.Overwriting files is avoided so that it behaves in the same way as normal transfers on the local filesystem. That also simplifies the Rclone commands that are needed significantly. Allowlist paths are also checked for the local filesystem, and remotes of type local and alias are disabled when the allowlist is enabled (same behaviour as in #2119).
Short demo (includes #2140 for easier demonstration):
┆Issue is synchronized with this Asana task by Unito