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.

Sign up for GitHub

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

Jump to bottom

Moving MD5 hash method from regression to storage helpers. #611

Merged
merged 2 commits into from
Feb 12, 2015
Merged

Moving MD5 hash method from regression to storage helpers. #611

merged 2 commits into from
Feb 12, 2015

Conversation

dhermes
Copy link
Contributor

@dhermes dhermes commented Feb 10, 2015

May make sense to swap out for hashlib.md5 (though they seem to
be equivalent).

In preparation for #547.

@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Feb 10, 2015
@dhermes
Copy link
Contributor Author

dhermes commented Feb 10, 2015

If we really want to do this, we'll need to support computing MD5 of files that don't fit in memory:
http://stackoverflow.com/a/1131255/1068170

This is done in a generic way in gsutil (though the method is quite a mouthful).

@dhermes
Copy link
Contributor Author

dhermes commented Feb 10, 2015

@tseaver The lint failure is a bug in my PR speedup code since among

gcloud/storage/_helpers.py
gcloud/storage/test__helpers.py
regression/storage.py

the abstract class is really never used.

We have 3 options:

  • I make a temporary commit disabling the PR speedup to just diff against all the files
  • I try to find a full-time fix (done in Repeating pylint after failures. #612)
  • Ignore and make sure it passes when merged

dhermes added a commit to dhermes/google-cloud-python that referenced this pull request Feb 10, 2015
@dhermes
Repeating pylint after failures.
b8d3caf 
This surfaced as a false negative in googleapis#611.
@dhermes dhermes mentioned this pull request Feb 10, 2015
Merged
@tseaver
Copy link
Contributor

tseaver commented Feb 10, 2015

LGTM. I'd say we should probably not set the envvars on Travis, to avoid these kinds of errors.

@dhermes
Copy link
Contributor Author

dhermes commented Feb 10, 2015

We don't set the env vars on merges to master. This only happens for PRs, which we want to be quick and snappy during review.

tseaver
tseaver reviewed Feb 11, 2015
View reviewed changes
gcloud/storage/_helpers.py
@@ -187,3 +190,14 @@ def _setter(self, value):
self._patch_properties({fieldname: value})

return property(_getter, _setter)


def _base64_md5hash(bytes_to_sign):

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

@dhermes
Copy link
Contributor Author

dhermes commented Feb 12, 2015

Went with 8192 for the chunk size since gsutil does:
https://github.com/GoogleCloudPlatform/gsutil/blob/bc083d405b92856a1aba46a65410bd1a1dbf65a6/gslib/util.py#L66

@tseaver PTAL

I also made a separate method for updating a hash object for when CRC32 comes around. (Sort of YAGNI, sort of not.)

@tseaver
Copy link
Contributor

tseaver commented Feb 12, 2015

@dhermes pylint failure due to partial-set checking.

@dhermes
Copy link
Contributor Author

dhermes commented Feb 12, 2015

Yes, recall that this PR is the reason for #612.

@dhermes
Moving MD5 hash method from regression to storage helpers.
e062c89 
May make sense to swap out for `hashlib.md5` (though they seem to
be equivalent).

In preparation for googleapis#547.
@dhermes
Making MD5 hash method take a buffer instead of bytes.
86b1f59 
This is to account for files too large to fit into memory.
@dhermes
Copy link
Contributor Author

dhermes commented Feb 12, 2015

@tseaver I just rebased on top of #612.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling 86b1f59 on dhermes:move-md5-to-library into df4c625 on GoogleCloudPlatform:master.

@tseaver
Copy link
Contributor

tseaver commented Feb 12, 2015

LGTM.

dhermes added a commit that referenced this pull request Feb 12, 2015
@dhermes
Merge pull request #611 from dhermes/move-md5-to-library
c88ba72 
Moving MD5 hash method from regression to storage helpers.
@dhermes dhermes merged commit c88ba72 into googleapis:master Feb 12, 2015
@dhermes
Copy link
Contributor Author

dhermes commented Feb 12, 2015

It seems coveralls has recovered from whatever was making it silently fail last night.

@dhermes dhermes deleted the move-md5-to-library branch February 12, 2015 19:32
@dhermes dhermes mentioned this pull request Feb 14, 2015
Merged
@dhermes dhermes mentioned this pull request Mar 9, 2015
Closed
vchudnov-g pushed a commit that referenced this pull request Sep 20, 2023
@gcf-owl-bot
docs: Add documentation for enums (#611)
8ff3a4f 
* docs: Add documentation for enums

fix: Add context manager return types

chore: Update gapic-generator-python to v1.8.1
PiperOrigin-RevId: 503210727

Source-Link: googleapis/googleapis@a391fd1

Source-Link: googleapis/googleapis-gen@0080f83
Copy-Tag: eyJwIjoiLmdpdGh1Yi8uT3dsQm90LnlhbWwiLCJoIjoiMDA4MGY4MzBkZWMzN2MzMzg0MTU3MDgyYmNlMjc5ZTM3MDc5ZWE1OCJ9

* 🦉 Updates from OwlBot post-processor

See https://github.com/googleapis/repo-automation-bots/blob/main/packages/owl-bot/README.md

Co-authored-by: Owl Bot <gcf-owl-bot[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
cla: yes This human has signed the Contributor License Agreement.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@dhermes @tseaver @coveralls @googlebot