Skip to content
This repository was archived by the owner on May 6, 2026. It is now read-only.

fix: avoid validating checksums for partial responses#361

Merged
cojenco merged 7 commits into
googleapis:mainfrom
simonbohnen:no-checksum-for-partial
Nov 1, 2022
Merged

fix: avoid validating checksums for partial responses#361
cojenco merged 7 commits into
googleapis:mainfrom
simonbohnen:no-checksum-for-partial

Conversation

@simonbohnen

@simonbohnen simonbohnen commented Oct 4, 2022 •
edited
Loading

Copy link
Copy Markdown
Contributor

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

google-cla Bot commented Oct 4, 2022

Copy link
Copy Markdown

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 October 4, 2022 15:49
@parthea

parthea commented Oct 4, 2022

Copy link
Copy Markdown
Contributor

Hi @simonbohnen,

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

@simonbohnen

simonbohnen commented Oct 4, 2022

Copy link
Copy Markdown
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

parthea commented Oct 5, 2022

Copy link
Copy Markdown
Contributor

@cojenco Please could you take a look?

@cojenco

cojenco commented Oct 7, 2022

Copy link
Copy Markdown
Contributor

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
cojenco
cojenco suggested changes Oct 17, 2022
View reviewed changes

@cojenco cojenco left a comment

Copy link
Copy Markdown
Contributor

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.

Comment thread google/resumable_media/requests/download.py Outdated
Comment thread google/resumable_media/requests/download.py Outdated
@simonbohnen

simonbohnen commented Oct 21, 2022

Copy link
Copy Markdown
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

simonbohnen commented Oct 24, 2022

Copy link
Copy Markdown
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
cojenco
cojenco reviewed Oct 28, 2022
View reviewed changes

@cojenco cojenco left a comment

Copy link
Copy Markdown
Contributor

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
cojenco
cojenco approved these changes Nov 1, 2022
View reviewed changes

@cojenco cojenco left a comment

Copy link
Copy Markdown
Contributor

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!

@cojenco cojenco merged commit 354287f into googleapis:main Nov 1, 2022
@release-please release-please Bot mentioned this pull request Nov 1, 2022
Merged
@simonbohnen simonbohnen mentioned this pull request Jan 20, 2023
Merged
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

1 more reviewer

@cojenco cojenco cojenco approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@cojenco cojenco

Labels

api: storage Issues related to the googleapis/google-resumable-media-python API. size: m Pull request size is medium.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Checksums are validated for partial responses

5 participants

@simonbohnen @parthea @cojenco @yoshi-kokoro @sbqc