feat: add cred info to auth related errors #2115
Conversation
arithmetic1728
force-pushed
the
cred_info
branch
2 times, most recently
from
137ee25
to
e909f85
Compare
product-auto-label
bot
added
size: m
arithmetic1728
force-pushed
the
cred_info
branch
2 times, most recently
from
96a3067
to
87a0f75
Compare
arithmetic1728
force-pushed
the
cred_info
branch
from
87a0f75
to
c33409d
Compare
arithmetic1728
force-pushed
the
cred_info
branch
from
7c9c167
to
f77061b
Compare
arithmetic1728
changed the title
arithmetic1728
added
the
do not merge
tests/integration/goldens/eventarc/google/cloud/eventarc_v1/services/eventarc/client.py
Show resolved
Hide resolved
arithmetic1728
removed
the
do not merge
| if not hasattr(cred, "get_cred_info"): | ||
| return | ||
|
|
||
| cred_info = cred.get_cred_info() # type: ignore |
There was a problem hiding this comment.
Please could you add a comment to clarify the reason that we have # type: ignore here, and include a link to a bug to follow up on it?
| self, | ||
| error: core_exceptions.GoogleAPICallError | ||
| ) -> None: | ||
| """Adds credential info string to error details for 401/403/404 errors. |
There was a problem hiding this comment.
Is this part of a client library AIP? https://google.aip.dev/client-libraries. If not, should we consider creating one? The AIPs are meant to help define requirements for client libraries which will also be used when we create client libraries for a new language like rust.
| return | ||
|
|
||
| cred = self._transport._credentials | ||
| if not hasattr(cred, "get_cred_info"): |
There was a problem hiding this comment.
Should we emit a warning/ update the error details to let users know that they can get more helpful error messages if they upgrade to a certain version of google-auth?
There was a problem hiding this comment.
Actually on second thought, we cannot do that since its not necessary that all credentials in all scenarios have cred info.
| return response | ||
| except core_exceptions.GoogleAPICallError as e: | ||
| self._add_cred_info_for_auth_errors(e) | ||
| raise e |
There was a problem hiding this comment.
Non-blocking comment. This PR touches a lot of duplicate code. Can we refactor this into a macro to avoid making the change in so many places? Feel free to file a bug to follow up on it later. If we address it now, then subsequent updates to the same code will be less toilsome.
|
|
||
| client._add_cred_info_for_auth_errors(error) | ||
| if show_cred_info: | ||
| assert error.details == [CRED_INFO_STRING] |
There was a problem hiding this comment.
Can we also add a test to ensure that error details that we receive from the API are not clobbered?
As an example, we could update the test_error_details showcase test which already has tests with error details
https://github.com/googleapis/gapic-generator-python/blob/main/tests/system/test_error_details.py
Adds cred info to error details for 401/403/404 errors. See go/python-auth-error-message-improvement.
Manually tested with KMS client: arithmetic1728/google-cloud-python#1