-
Notifications
You must be signed in to change notification settings - Fork 12
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 support for complex file_id #156
Open
enkidulan
wants to merge
1
commit into
jowilf:main
Choose a base branch
from
enkidulan:support_complex_file_id
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
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.
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 only supports two cases:
folder/file
) and simple storage namestorage/folder
) and simple file_idWe could add support for both complex storage_name (
storage/folder
) and file_id (subfolder/file
)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.
Thank you for replying. Initially, I tried adding support for complex storage_name and file_id at the same time, but the challenge I ran into was that using
/
as a separator was insufficient to easily identify where the storage part ended and the file part began. At the time, I could not come up with a simple solution that would also be straightforward to the users. So I settled upon the simplestor
approach that covers the case I'm interested in.To give more context on the challenge of supporting complex storage_name and complex file_id in the same path, consider this example
storage/folder_a/folder_b/folder_c/file
. The parts [folder_a
,folder_b
,folder_c
] can belong to either the storage, the file, or both (in different combinations, where complexity increases with the number of sub-folders).One way to solve it is to introduce a special path separator that indicates that the left part is a storage and the right part is a file, something like
//
so users would usestorage/folder_a // folder_b/folder_c/file
. It's a simple approach, but it breaks backward compatibility.Another approach is similar to the current solution: test the combination of sub-paths against storage and return the first matching. However, this will require adding complex logic to rule out possible conflicts, not to mention getting the time complexity into at least O(N) space.
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 O(N) complexity is not bad assuming that N<= 10 for most cases. What I am worried about is whether this is supported by Apache libcloud.
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 don't see why libcloud would not support it, as far as the storage name is detected correctly. The tests are passing, and, in my fork, I have it running successfully against S3 as a storage.
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.
Great, in that case feel free to update and include some tests