-
Notifications
You must be signed in to change notification settings - Fork 114
capture version_id after upload #524
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
|
@martindurant do you know by chance who is maintaining the project and could release this change? |
|
@shcheklein thanks for contribution! Populating the updated I'll have more cycles next week to take a deeper review but some initial things we probably want to address now are:
|
a9618f5 to
4eaa094
Compare
4eaa094 to
70c345d
Compare
|
Hi @kyleknap , thanks for the feedback. I've addressed all the items. Azurite doesn't support versioning - thus we use mocks in tests. I think that should be fine. |
|
Thanks @shcheklein! I agree mocks suffice in this case. I'll take a deeper look at the PR this week. |
kyleknap
left a comment
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 looks good! I pulled down the PR and tried it out and it was working well. I just had one question related to append blobs that I ran into.
| if self.fs.version_aware: | ||
| self.version_id = response.get("version_id") |
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.
For append blobs, were you able to get version id's returned for the upload blob method call? Trying it out myself, I'm not sure if an x-ms-version-id header is even returned back when a PUT is called on an append blob. So the version_id was consistently None even though for block blobs it was being returned back. That being said for HEAD calls on the append blob, the x-ms-version-id was present.
I'm thinking if the API is not returning version id's for PUTs on append blobs, let's remove this logic for now to avoid code paths that won't be hit and avoid having tests implying this should work. We can always add it in if version id's are returned in the future.
Similar to fsspec/gcsfs#718
To be able to refer to a file properly (in case multiple uploads happening simultaneously to the same path), we want to grab generation from the response. S3 already does this.