-
Notifications
You must be signed in to change notification settings - Fork 153
Add S3-Specific Directory Check #4623
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
base: master
Are you sure you want to change the base?
Add S3-Specific Directory Check #4623
Conversation
Thanks! I'll take a look shortly. |
Commenting here since this is going to affect the usage of our S3 storage quite heavily. Purely from a user experience point of view, this adds quite a long delay to browsing the files for us, as it seems to attempt to write 48 different test files:
vs
I suppose I'm not familiar with OSN, but would there be any alternative commands that could be used. Do any of the commands ( |
Hm, not ideal. I was thinking that Using |
Actually, thinking about it, running |
I just refreshed my memory a bit on this. The main reason why it doesn't use
However, checking the other commands, I noticed that
|
@robinkar I refactored to use |
Signed-off-by: Georgia Stuart <[email protected]>
Signed-off-by: Georgia Stuart <[email protected]>
Signed-off-by: Georgia Stuart <[email protected]>
Signed-off-by: Georgia Stuart <[email protected]>
Signed-off-by: Georgia Stuart <[email protected]>
2b6f867
to
043ac68
Compare
Seems to work nicely with our object storage at least (S3 and Swift). |
Thank you so much for looking into this @robinkar! I can't get to this right away, but I'll try to circle back shortly @georgiastuart. Seems like from @robinkar's last comment it's in a good state so I just need a bit to refresh my memory as well and get some other things off my plate. |
Sounds good! I'm happy to refactor more if there's a better way. |
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 could be touched up, but I'm good with this as it is.
Though I'd appreciate @robinkar's second opinion.
Left a small comment regarding the regex, but I'm fine with this. |
Co-authored-by: Robin Karlsson <[email protected]>
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 went ahead and made the change suggested by Robin.
Thanks for this addition @georgiastuart! It'll be in 4.1.
What problem this PR solves
This PR is primarily to fix a bug associated with Open Storage Network (OSN) S3 buckets. On OSN, users can't enumerate buckets in the root of the remote, so it leads to OOD believing that a bucket doesn't exist. For example, the bucket
georgia-osn
exists on my remote, but it doesn't register as valid sincelsf
on the parent is empty:But navigating to a "subdirectory" works fine since the parent returns a file listing on
lsf
:How this PR solves the problem
This PR simply adds an alternative route to verifying a bucket exists for S3 remotes. Instead of using
lsf
to enumerate the parent, it usesrclone test info --check-length
and approves any match with an entry that's notmaxFileLength = -1
.This has an unexpected side benefit of correctly rejecting an s3 file (not directory) for file browser navigation. Previously, it threw vague JSON error:
Now, it correctly identifies the file as not being a directory:
Improvements needed
I'm no Ruby expert, so this can probably be cleaned up. I went the straightforward route and duplicated the whole if-statement since efforts to reduce redundancy got more convoluted, somehow...