-
Notifications
You must be signed in to change notification settings - Fork 90
feat: add support for 'error_info' #307
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All returns from _parse_grpc_error_details
must return a 2-tuple.
Co-authored-by: Tres Seaver <tseaver@palladion.com>
We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for all the commit author(s) or Co-authors. If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google. ℹ️ Googlers: Go here for more info. |
We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for all the commit author(s) or Co-authors. If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google. ℹ️ Googlers: Go here for more info. |
We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for all the commit author(s) or Co-authors. If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google. ℹ️ Googlers: Go here for more info. |
@googlebot I consent. |
We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for all the commit author(s) or Co-authors. If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google. ℹ️ Googlers: Go here for more info. |
@atulep The CLA bot has (once again) lost its tiny mind. You'll need to remove the |
Hey @tseaver, it seems to complain because email used in the commit "@palladion.com" is different than the one you used to sign CLA with. The docs suggest "If the email address does not match an email found, please ask the contributor to either add their new email address to their CLA or rebase their commits with their correct email address." Can you please do that? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left one comment, other than that, looks good to me. :)
The error message is incorrect -- all my commits in the |
I checked the internal dashboards, and it shows you signed CLA with a "@gmail.com" address. This is the official instruction I was told to follow:
I know you're an active contributor, so this is really weird to see an CLA issue. Can you share a similar issue from the past where a Googler manually removed the CLA label? Thank you. |
@tswast, @busunkim96 Can one of you please tag in on the manual CLA flip? |
@googlebot I fixed it. |
@summer-ji-eng @tseaver - finally addressed your feedback. Thanks for your comments! |
Requested changes made in 331d6e3
@atulep the CLA flag has to be cleared again after pushing any commit, even a merge. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🚀
@tseaver I manually flipped the flag but it still blocks me from merging. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, just some nitpicking.
if not self._error_info: | ||
return None | ||
return self._error_info.reason |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: we can simplify this to
return self._error_info.reason if self._error_info else None
if not self._error_info: | ||
return None | ||
return self._error_info.domain |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here: we can use conditional expressions
if not self._error_info: | ||
return None | ||
return self._error_info.metadata |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And here, same as above.
error_info = list( | ||
filter( | ||
lambda detail: detail.get("@type", "") | ||
== "type.googleapis.com/google.rpc.ErrorInfo", | ||
details, | ||
) | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: it's more idiomatic to use list comprehensions than list
and filter
.
error_info_type = "type.googleapis.com/google.rpc.ErrorInfo"
error_info = [d for d in details if d.get("@type", "") == error_info_type]
Closing this in favor of #315. |
Adds
error_info
field toGoogleAPICallError
, as requested in #286.