uranusjr commented on a change in pull request #16404:
URL: https://github.com/apache/airflow/pull/16404#discussion_r650391513
##########
File path: airflow/models/connection.py
##########
@@ -18,6 +18,7 @@
import json
import warnings
+import logging
Review comment:
This won’t pass linters. Please run pre-commit locally and fix the
errors first.
##########
File path: airflow/models/connection.py
##########
@@ -391,7 +394,14 @@ def get_connection_from_secrets(cls, conn_id: str) ->
'Connection':
:return: connection
"""
for secrets_backend in ensure_secrets_loaded():
- conn = secrets_backend.get_connection(conn_id=conn_id)
- if conn:
- return conn
+ try:
+ conn = secrets_backend.get_connection(conn_id=conn_id)
+ if conn:
+ return conn
+ except Exception as e:
+ log.warning('Unable to retrieve connection from alternative
secret backend:'
+ f'\n{e}\n'
+ 'Checking default secrets backends'
+ )
Review comment:
Do not use f-strings to format log messages. [This blog post explains it
well.](https://www.siegescape.com/blog/2019/10/3/be-careful-with-your-f-strings).
In this particular instance, it’s best to use `logger.exception()` to also
retain the stack trace, which is useful for debugging.
##########
File path: airflow/models/connection.py
##########
@@ -391,7 +394,14 @@ def get_connection_from_secrets(cls, conn_id: str) ->
'Connection':
:return: connection
"""
for secrets_backend in ensure_secrets_loaded():
- conn = secrets_backend.get_connection(conn_id=conn_id)
- if conn:
- return conn
+ try:
+ conn = secrets_backend.get_connection(conn_id=conn_id)
+ if conn:
+ return conn
+ except Exception as e:
+ log.warning('Unable to retrieve connection from alternative
secret backend:'
+ f'\n{e}\n'
+ 'Checking default secrets backends'
+ )
Review comment:
Do not use f-strings to format log messages. [This blog post explains it
well.](https://www.siegescape.com/blog/2019/10/3/be-careful-with-your-f-strings)
In this particular instance, it’s best to use `logger.exception()` to also
retain the stack trace, which is useful for debugging.
##########
File path: airflow/providers/google/cloud/secrets/secret_manager.py
##########
@@ -101,9 +104,13 @@ def __init__(
"`connections_prefix`, `variables_prefix` and `sep` should
"
f"follows that pattern {SECRET_ID_PATTERN}"
)
- self.credentials, self.project_id = get_credentials_and_project_id(
- keyfile_dict=gcp_keyfile_dict, key_path=gcp_key_path,
scopes=gcp_scopes
- )
+ try:
+ self.credentials, self.project_id = get_credentials_and_project_id(
+ keyfile_dict=gcp_keyfile_dict, key_path=gcp_key_path,
scopes=gcp_scopes
+ )
+ except Exception as e:
+ log.exception(f'{e}')
Review comment:
`Logger.exception()` already logs the exception, so the `f'{e}'` message
is adding nothing useful. You should provide a readable string explaining why
the exception happens instead.
--
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.
For queries about this service, please contact Infrastructure at:
[email protected]
