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

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 googleapis/yoshi-go-admins as a code owner Dec 7, 2020
@google-cla google-cla bot added the cla: yes label Dec 7, 2020
@codyoss
Copy link
Member Author

@codyoss codyoss commented Dec 7, 2020

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

Copy link
Contributor

@tbpg tbpg left a comment

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

@codyoss
Copy link
Member Author

@codyoss 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.

@tbpg
tbpg approved these changes Dec 8, 2020
@gcf-merge-on-green
Copy link
Contributor

@gcf-merge-on-green gcf-merge-on-green bot commented Dec 9, 2020

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.

@codyoss codyoss requested a review from yoshi-approver as a code owner Dec 11, 2020
@gcf-merge-on-green gcf-merge-on-green bot merged commit 39cbab0 into googleapis:master Dec 11, 2020
3 checks passed
3 checks passed
@google-cla
cla/google All necessary CLAs are signed
@conventional-commit-lint-gcf
conventionalcommits.org
Details
@kokoro-team
kokoro Kokoro CI build successful
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

2 participants