Skip to content
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

Specify VersionId when possible on multipart downloads #254

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from

Conversation

mspdt22
Copy link

@mspdt22 mspdt22 commented Nov 9, 2022

This addresses #86 for versioned buckets

@cfxegbert
Copy link

cfxegbert commented Nov 9, 2022

How is this different than using VersionId in the extra_args parameter? VersionId is already in the ALLOWED_DOWNLOAD_ARGS list.

One useful feature is grabbing the version id at the beginning (if available) and using that version id throughout the multipart download.

@mspdt22
Copy link
Author

mspdt22 commented Nov 10, 2022

Thank you for taking a look!

How is this different than using VersionId in the extra_args parameter?

To clarify, are you proposing that the s3transfer library plumbs the VersionId from the HEAD request it already makes to get object size? Or that the s3transfer users themselves are responsible for making the HEAD request outside the s3transfer library and passing it to download_file()?

One useful feature is grabbing the version id at the beginning (if available) and using that version id throughout the multipart download.

Yes, agreed. This PR is adding the feature.

@cfxegbert
Copy link

Thank you for taking a look!

FYI, I do not have any permissions to do any sort of approval on this repo.

How is this different than using VersionId in the extra_args parameter?

To clarify, are you proposing that the s3transfer library plumbs the VersionId from the HEAD request it already makes to get object size? Or that the s3transfer users themselves are responsible for making the HEAD request outside the s3transfer library and passing it to download_file()?

Looking at the code the real bug is that extra_args are not passed along in MultipartDownload. If that is fixed, then VersionId can be passed as an extra_arg.

One useful feature is grabbing the version id at the beginning (if available) and using that version id throughout the multipart download.

Yes, agreed. This PR is adding the feature.

Looking at the documentation for VersionId this would be a breaking change because GetObjectVersion permission is required. A non-breaking change would be to extract the ETag from the head_object and use it with the IfMatch header.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants