-
Notifications
You must be signed in to change notification settings - Fork 57
feat: Quilt client part ii #2171
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: main
Are you sure you want to change the base?
Conversation
…t-encoding-part-ii
…into quilt-client-part-ii
…t-client-part-ii
…t-client-part-ii
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.
Thanks a lot for the PR, @liquid-helium, really nice! As usual, a few mostly minor comments and suggestions.
QuiltStoreBlob::new( | ||
blob, | ||
path.file_name() | ||
.and_then(|file_name| file_name.to_str()) |
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.
Important: We should have some way of handling duplicate filenames.
One idea would be to use relative paths to some specified path (we have already deduplicated the file paths).
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.
Good catch!
I've given this some thought as well, and it involves a trade-off. Using the full path is likely the most stable option, but it can include a lot of unnecessary details or even sensitive information, like an account number embedded in the full path. Since we allow reading files from different directories, relying on relative paths could lead to duplicates as well.
That's why I opted to use only file names for now. Looking ahead, when we introduce support for per-blob attributes, I hope the input will be 'rich' by default. This will enable users to assign custom identifiers, providing more flexibility.
But I am open to other approaches, just keeping it as for now and see where we go.
Warning: This PR modifies the Walrus CLI. Please consider the following:
|
Thanks @mlegner for the suggestions and comments, these are really helpful! PTAL. |
Description
Added QuiltClient to handle storing/reading quilts.
Contributes to WAL-858
Test plan
How did you test the new or updated feature?
Release notes
Check each box that your changes affect. If none of the boxes relate to your changes, release notes aren't required.
For each box you select, include information after the relevant heading that describes the impact of your changes that
a user might notice and any actions they must take to implement updates. (Add release notes after the colon for each item)