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

fix: avoid validating checksums for partial responses #361

Merged
merged 7 commits into from
Nov 1, 2022

Conversation

simonbohnen
Copy link
Contributor

@simonbohnen simonbohnen commented Oct 4, 2022

Thank you for opening a Pull Request! Before submitting your PR, there are a few things you can do to make sure it goes smoothly:

  • Make sure to open an issue as a bug/issue before writing your code! That way we can discuss the change, evaluate designs, and agree on the general idea
  • Ensure the tests and linter pass
  • Code coverage does not decrease (if any source code was changed)
  • Appropriate docs were updated (if necessary)

Fixes #362 🦕

@google-cla
Copy link

google-cla bot commented Oct 4, 2022

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

@product-auto-label product-auto-label bot added size: s Pull request size is small. api: storage Issues related to the googleapis/google-resumable-media-python API. labels Oct 4, 2022
@simonbohnen simonbohnen force-pushed the no-checksum-for-partial branch from 91f65af to 5c03176 Compare October 4, 2022 15:20
@simonbohnen simonbohnen marked this pull request as ready for review October 4, 2022 15:49
@simonbohnen simonbohnen requested review from a team as code owners October 4, 2022 15:49
@parthea
Copy link
Contributor

parthea commented Oct 4, 2022

Hi @simonbohnen,

Please could you sign the CLA?
https://cla.developers.google.com/about

@simonbohnen
Copy link
Contributor Author

Hi @simonbohnen,

Please could you sign the CLA? https://cla.developers.google.com/about

It looks like I did for my work account (SimonBohnenQC). Is there a way to re-run the check?
Screen Shot 2022-10-04 at 18 42 36

@parthea
Copy link
Contributor

parthea commented Oct 5, 2022

@cojenco Please could you take a look?

@cojenco
Copy link
Contributor

cojenco commented Oct 7, 2022

Hi @simonbohnen,
Please could you sign the CLA? https://cla.developers.google.com/about

It looks like I did for my work account (SimonBohnenQC). Is there a way to re-run the check? Screen Shot 2022-10-04 at 18 42 36

I force rescanned the CLA and it's passing now. Our team will work on reviewing your PR. Thanks for your contribution!

@cojenco cojenco added kokoro:force-run Add this label to force Kokoro to re-run the tests. owlbot:run Add this label to trigger the Owlbot post processor. labels Oct 7, 2022
@gcf-owl-bot gcf-owl-bot bot removed the owlbot:run Add this label to trigger the Owlbot post processor. label Oct 7, 2022
@yoshi-kokoro yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Oct 7, 2022
Copy link
Contributor

@cojenco cojenco left a comment

Choose a reason for hiding this comment

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

Thanks @simonbohnen for the fix. I agree we want to skip checksum validations for partial content responses, as noted in our documentation.

In addition to the comments, could you add a unit test for this? Something alongside the tests here. Let me know if you'd like assistance on testing.

google/resumable_media/requests/download.py Outdated Show resolved Hide resolved
google/resumable_media/requests/download.py Outdated Show resolved Hide resolved
@simonbohnen
Copy link
Contributor Author

Thanks @simonbohnen for the fix. I agree we want to skip checksum validations for partial content responses, as noted in our documentation.

In addition to the comments, could you add a unit test for this? Something alongside the tests here. Let me know if you'd like assistance on testing.

I'll add some tests next week👌🏼

@product-auto-label product-auto-label bot added size: m Pull request size is medium. and removed size: s Pull request size is small. labels Oct 24, 2022
@simonbohnen simonbohnen force-pushed the no-checksum-for-partial branch from 691b93c to 6bf2cf8 Compare October 24, 2022 12:48
@simonbohnen
Copy link
Contributor Author

Can you help me with this error? The commit messages look good to me 🤔
Screen Shot 2022-10-24 at 14 51 21

@simonbohnen simonbohnen requested a review from cojenco October 24, 2022 12:52
@cojenco cojenco changed the title Do not validate checksums for partial responses fix: avoid validating checksums for partial responses Oct 24, 2022
@cojenco cojenco added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Oct 24, 2022
@cojenco cojenco added the owlbot:run Add this label to trigger the Owlbot post processor. label Oct 24, 2022
@gcf-owl-bot gcf-owl-bot bot removed the owlbot:run Add this label to trigger the Owlbot post processor. label Oct 24, 2022
@yoshi-kokoro yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Oct 24, 2022
@cojenco cojenco added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Oct 28, 2022
@yoshi-kokoro yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Oct 28, 2022
Copy link
Contributor

@cojenco cojenco left a comment

Choose a reason for hiding this comment

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

Thanks for adding tests @simonbohnen

Besides the lint error on unused ret_val in the tests, LGTM. Try running nox -s lint to check linting.

@simonbohnen simonbohnen requested a review from cojenco October 29, 2022 08:26
@cojenco cojenco added kokoro:force-run Add this label to force Kokoro to re-run the tests. owlbot:run Add this label to trigger the Owlbot post processor. labels Nov 1, 2022
@gcf-owl-bot gcf-owl-bot bot removed the owlbot:run Add this label to trigger the Owlbot post processor. label Nov 1, 2022
@yoshi-kokoro yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Nov 1, 2022
Copy link
Contributor

@cojenco cojenco left a comment

Choose a reason for hiding this comment

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

LGTM, thanks again for your contribution!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api: storage Issues related to the googleapis/google-resumable-media-python API. size: m Pull request size is medium.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Checksums are validated for partial responses
5 participants