Skip to content
This repository has been archived by the owner on Jan 16, 2023. It is now read-only.

Fix/AWS Multipart Hashes #202

Open
wants to merge 18 commits into
base: master
Choose a base branch
from

Conversation

sean-wallace
Copy link

@sean-wallace sean-wallace commented Jul 1, 2020

Here's a first draft of changes to handle hashing multipart uploads to match the ETag on S3. I'm really not sure about the structure of the change, let me know if you'd like it implemented some other way.

I wasn't able to get the live test running locally but I think it should pass. I tested with a project of mine and it seems to work.

Gzip support is not implemented at this point.

Fixes #201

@sean-wallace sean-wallace changed the title Fix/aws multipart hashes (Fixes #201) Fix/AWS Multipart Hashes Jul 2, 2020
@antonagestam
Copy link
Owner

Thanks for putting in the work on this! I ran the test suite locally on your branch the the suite is passing.

It would be really nice to maintain support for gzip, that should be especially relevant for projects with large files. Is it feasible to make that happen?

collectfast/strategies/boto3.py Outdated Show resolved Hide resolved
collectfast/strategies/boto3.py Outdated Show resolved Hide resolved
collectfast/strategies/boto3.py Outdated Show resolved Hide resolved
collectfast/tests/utils.py Outdated Show resolved Hide resolved
@sean-wallace sean-wallace marked this pull request as draft July 4, 2020 15:45
@sean-wallace
Copy link
Author

Ok, I think this is ready for another look. Pretty significant changes were necessary to implement the gzip stuff because of the way the Class inheritance is set up.

Do you mind if I squash the branch?

@sean-wallace sean-wallace marked this pull request as ready for review July 4, 2020 18:28
Copy link
Owner

@antonagestam antonagestam left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very nice work with moving this forward! I had a few more comments.

collectfast/strategies/boto3.py Outdated Show resolved Hide resolved
collectfast/strategies/boto3.py Outdated Show resolved Hide resolved
collectfast/strategies/boto3.py Outdated Show resolved Hide resolved
collectfast/strategies/boto3.py Show resolved Hide resolved
collectfast/strategies/boto3.py Outdated Show resolved Hide resolved
collectfast/strategies/boto3.py Outdated Show resolved Hide resolved
collectfast/strategies/boto3.py Outdated Show resolved Hide resolved
collectfast/strategies/boto3.py Outdated Show resolved Hide resolved
collectfast/strategies/boto3.py Outdated Show resolved Hide resolved
@sean-wallace
Copy link
Author

Very nice work with moving this forward! I had a few more comments.

Thanks very much for the thoughtful and constructive review -- I've made the changes generally as suggested. Let me know what you think.

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

Successfully merging this pull request may close these issues.

AWS Multipart Uploads Breaking Hash Comparisons
2 participants