vincbeck commented on code in PR #48070:
URL: https://github.com/apache/airflow/pull/48070#discussion_r2016763574
##########
airflow-core/src/airflow/utils/db_manager.py:
##########
@@ -144,13 +144,14 @@ class RunDBManager(LoggingMixin):
def __init__(self):
super().__init__()
self._managers: list[BaseDBManager] = []
- managers = conf.get("database", "external_db_managers").split(",")
-
+ managers_config = conf.get("database", "external_db_managers",
fallback=None)
+ if not managers_config:
+ return
+ managers = managers_config.split(",")
Review Comment:
If the config is None we do not want to stop. The auth manager can still add
its DB manager
```suggestion
if not managers_config:
managers = []
else:
managers = managers_config.split(",")
```
##########
devel-common/src/tests_common/test_utils/db.py:
##########
@@ -351,6 +353,7 @@ def clear_all():
clear_db_pools()
clear_db_connections(add_default_connections_back=True)
clear_db_deadline()
- clear_dag_specific_permissions()
+ if "FabAuthManager" in conf.get("core", "auth_manager"):
+ clear_dag_specific_permissions()
Review Comment:
Doing it in the function would avoid checking each time we are calling this
function
##########
airflow-core/tests/unit/models/test_dagbag.py:
##########
@@ -65,7 +66,8 @@ def db_clean_up():
db.clear_db_dags()
db.clear_db_runs()
db.clear_db_serialized_dags()
- db.clear_dag_specific_permissions()
+ if "FabAuthManager" in conf.get("core", "auth_manager"):
+ db.clear_dag_specific_permissions()
Review Comment:
Similar to one of my previous comments, if a function is specific to FAB, I
would handle it in the function and not in the function that is calling this
function. Example:
```
def clear_dag_specific_permissions():
if "FabAuthManager" in conf.get("core", "auth_manager"):
return
```
##########
airflow-core/tests/unit/utils/test_db.py:
##########
@@ -135,7 +140,10 @@ def test_check_migrations(self):
def test_upgradedb(self, mock_alembic_command):
upgradedb()
mock_alembic_command.upgrade.assert_called_with(mock.ANY,
revision="heads")
- assert mock_alembic_command.upgrade.call_count == 2
+ if "FabAuthManager" in conf.get("core", "auth_manager"):
+ assert mock_alembic_command.upgrade.call_count == 2
+ else:
+ assert mock_alembic_command.upgrade.call_count == 1
Review Comment:
We do not know if `FabAuthManager` is the auth manager in this test? It
seems to test that in a test, we should know. Why do not we parametrize this
test, and run it with simple auth manager and fab auth manager, so that we test
both conditions
--
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]
