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(all): use CheckResponse for media downloads #773

Merged
merged 4 commits into from Dec 11, 2020
Merged

fix(all): use CheckResponse for media downloads #773

merged 4 commits into from Dec 11, 2020

Conversation

codyoss
Copy link
Member

@codyoss codyoss commented Dec 7, 2020

If a method supports media downloads use CheckResponse. This method
provided extra context to errors if the api follow
https://google.aip.dev/193. The storage api will continue to use
CheckMediaResponse as it does not conform to the aip due to legacy
reasons.

Fixes: #752

If a method supports media downloads use CheckResponse. This method
provided extra context to errors if the api follow
https://google.aip.dev/193. The storage api will continue to use
CheckMediaResponse as it does not conform to the aip due to legacy
reasons.

Fixes: #752
@codyoss codyoss requested a review from a team as a code owner December 7, 2020 17:01
@google-cla google-cla bot added the cla: yes This human has signed the Contributor License Agreement. label Dec 7, 2020
@codyoss
Copy link
Member Author

codyoss commented Dec 7, 2020

Ran the generator on cached discovery docs to generate the changes.

Copy link
Contributor

@tbpg tbpg left a comment

Choose a reason for hiding this comment

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

Just to confirm, do we have sufficient test coverage to be confident these are still working properly?

@codyoss
Copy link
Member Author

codyoss commented Dec 7, 2020

Yup, everything should work just fine. We could probably get away with removing CheckMediaResponse all together in the future. CheckResponse's only real difference is it will try to json.Unmarshall the value into something more descriptive. If the unmarshalling fails it falls back to the old behavior. At worst you lose some performance on error cases.

@codyoss codyoss added the automerge Merge the pull request once unit tests and other checks pass. label Dec 8, 2020
@gcf-merge-on-green
Copy link
Contributor

Merge-on-green attempted to merge your PR for 6 hours, but it was not mergeable because either one of your required status checks failed, or one of your required reviews was not approved. Learn more about your required status checks here: https://help.github.com/en/github/administering-a-repository/enabling-required-status-checks. You can remove and reapply the label to re-run the bot.

@gcf-merge-on-green gcf-merge-on-green bot removed the automerge Merge the pull request once unit tests and other checks pass. label Dec 9, 2020
@codyoss codyoss added kokoro:force-run Add this label to force Kokoro to re-run the tests. automerge Merge the pull request once unit tests and other checks pass. labels Dec 11, 2020
@codyoss codyoss removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Dec 11, 2020
@gcf-merge-on-green gcf-merge-on-green bot merged commit 39cbab0 into googleapis:master Dec 11, 2020
@gcf-merge-on-green gcf-merge-on-green bot removed the automerge Merge the pull request once unit tests and other checks pass. label Dec 11, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes This human has signed the Contributor License Agreement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

drive: FilesGetCall.Download did not unmarshal the body in the error response
2 participants