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

Hash gzipped files if AWS_IS_GZIPPED is True #108

Closed
wants to merge 7 commits into from
Closed

Hash gzipped files if AWS_IS_GZIPPED is True #108

wants to merge 7 commits into from

Conversation

rehandalal
Copy link
Contributor

@rehandalal rehandalal commented Jul 31, 2017

Fixes #46.

This is a WIP but submitting for feedback.

@rehandalal
Copy link
Contributor Author

rehandalal commented Jul 31, 2017

Filed a PR in django-storages to fix the mtime in S3Boto3Storage which should make this work as expected.

Update: The PR has merged.

@rehandalal
Copy link
Contributor Author

This should be ready for review.

@rehandalal
Copy link
Contributor Author

@antonagestam any chance you could review this? I would love to get this in my project as things are really slow without it. thanks!

@mbeacom mbeacom requested a review from antonagestam September 6, 2017 18:34
@antonagestam
Copy link
Owner

Sorry, I'm not going to review any pull requests on this project until I get an indication from the roadies what their thoughts are on this jazzband/help#72
There's a few issues that needs to be sorted.

@antonagestam antonagestam removed their request for review September 6, 2017 22:22
@rehandalal
Copy link
Contributor Author

@antonagestam I 100% agree with you on having a process around contribution. It kind of sucks to have people committing code, free from any review, to master. That being said, I hope you'd be willing to reconsider not reviewing PRs as I feel like that only bolsters the notion that having a process like review of pull requests is tedious and holds up development.

@mbeacom
Copy link

mbeacom commented Sep 7, 2017

I apologize for requesting your review. I requested your review of the PR since it's your repository and you were involved with the original issue. Hopefully I didn't step on any toes. A process with guidelines would be ideal. In any case, I hope you get everything sorted out!

@antonagestam
Copy link
Owner

@rehandalal How is the performance of this looking in your tests? Also, is the gzipping deterministic so that it generates the same zipped content locally as it will for amazon remotely?

@antonagestam
Copy link
Owner

This is now published in the 0.6.0 release. Thank you again for the contribution!

@rehandalal
Copy link
Contributor Author

Thanks for publishing this.

To answer your questions:

  • Yes, the gzipping is deterministic.
  • On first run it appears to have little to no benefit, however subsequent runs are much faster as long as the cache has not expired.

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.

3 participants