kosteev commented on code in PR #23773:
URL: https://github.com/apache/airflow/pull/23773#discussion_r877529861
##########
airflow/providers/google/cloud/utils/credentials_provider.py:
##########
@@ -248,6 +250,16 @@ def get_credentials_and_project(self) ->
Tuple[google.auth.credentials.Credentia
project_id =
_get_project_id_from_service_account_email(self.target_principal)
+ if isinstance(credentials, compute_engine.Credentials):
+ try:
+ credentials.refresh(_http_client.Request())
+ except RefreshError as msg:
+ """
+ If the Compute Engine metadata service can't be reached in
this case the instance has not
+ credentials.
+ """
+ self._log_debug(msg)
Review Comment:
Well, then my take is that we have options:
- implement this change with refresh without try/except ->
get_credentials_and_project starts to retrieve credentials and some
clients/callers which didn't expect this may break (like breeze, and others)
- implement this refresh in different place, so that we fix an original
issue and not introduce breaking change (in some sense), e.g. caller that uses
"get_credentials_and_project" should do refresh
- have optional parameter in get_credentials_and_project if we want to call
refresh or not
I think returning different result in get_credentials_and_project method
depending on response error if we were able to retrieve credentials or not is
not good interface for caller.
I am curious to hear other opinions.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
