-
Notifications
You must be signed in to change notification settings - Fork 48
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
feat: add configurable checksum support for uploads #139
Conversation
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.
Main request is to handle when metadata passes in md5 or crc32c AND user wants auto md5/crc32c enabled.
Otherwise implementation looks good.
return checksum_type | ||
|
||
|
||
def prepare_checksum_digest(digest_bytestring): |
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.
should be prefixed _
?
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.
Several other helper functions that are similarly independent of internal specifics are public in this file so I decided this one should be intentionally public as well.
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.
If that's the case should the file name be renamed to helpers.py
instead in the case an end-user wants to use them?
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 think that would be productive as it would commit us to a public interface and there is no expressed customer demand for that, but I wanted to follow the previous convention of not marking functions in this private module further private if they aren't extremely single-use and don't make strong assumptions about internal structure.
return checksum_type | ||
|
||
|
||
def prepare_checksum_digest(digest_bytestring): |
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.
If that's the case should the file name be renamed to helpers.py
instead in the case an end-user wants to use them?
PTAL (also, this revision unit and system tests for system-native strings (unicode in py3 and bytes in py2) to ensure everything works as users will actually use the software). |
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.
LGTM, thanks @andrewsg.
🤖 I have created a release \*beep\* \*boop\* --- ## [0.7.0](https://www.github.com/googleapis/google-resumable-media-python/compare/v0.6.0...v0.7.0) (2020-07-23) ### Features * add configurable checksum support for uploads ([#139](https://www.github.com/googleapis/google-resumable-media-python/issues/139)) ([68264f8](https://www.github.com/googleapis/google-resumable-media-python/commit/68264f811473a5aa06102912ea8d454b6bb59307)) ### Bug Fixes * accept `201 Created` as valid upload response ([#141](https://www.github.com/googleapis/google-resumable-media-python/issues/141)) ([00d280e](https://www.github.com/googleapis/google-resumable-media-python/commit/00d280e116d69f7f8d8a4b970bb1176e338f9ce0)), closes [#125](https://www.github.com/googleapis/google-resumable-media-python/issues/125) [#124](https://www.github.com/googleapis/google-resumable-media-python/issues/124) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please).
Adds configurable checksum support for MultipartUpload and ResumableUpload.
SimpleUpload is not supported because the GCS json API does not accept checksums defined in headers, only in metadata.
MultipartUpload is checked server-side as originally intended for uploads, and a failure will return a 400 error (InvalidResponse exception). ResumableUpload is checked locally as a workaround for having no way to submit the checksum for server-side uploads, as the metadata is transmitted only in the first chunk. A failure there will result in a DataCorruption exception.