potiuk commented on code in PR #71798:
URL: https://github.com/apache/airflow/pull/71798#discussion_r4057159562
##########
providers/fab/tests/unit/fab/auth_manager/test_security.py:
##########
@@ -1055,7 +1055,28 @@ def
test_create_dag_specific_permissions_airflow3(session, security_manager, mon
security_manager.create_dag_specific_permissions()
-def test_get_all_permissions(security_manager):
+def
test_create_dag_specific_permissions_skips_dag_with_bad_access_control(security_manager,
monkeypatch):
+ """A single DAG with an invalid access_control must not abort syncing the
remaining DAGs."""
+ bad_dag = DAG("bad_access_control", schedule=None,
access_control={"NonExistentRole": {ACTION_CAN_READ}})
+ good_dag = DAG("good_access_control", schedule=None,
access_control={"Public": {ACTION_CAN_READ}})
+
+ import airflow.providers.fab.auth_manager.security_manager
+
+ _iter_dags_mock = mock.Mock(return_value=[bad_dag, good_dag])
Review Comment:
Nit: `mock.Mock(return_value=...)` without `spec`. It mirrors the
neighbouring `_iter_dags` test, so no need to change it here; just noting it.
##########
providers/fab/src/airflow/providers/fab/auth_manager/security_manager/override.py:
##########
@@ -1008,7 +1008,14 @@ def create_dag_specific_permissions(self) -> None:
self._merge_perm(action_name, dag_resource_name)
if dag.access_control is not None:
- self.sync_perm_for_dag(dag.dag_id, dag.access_control)
+ try:
+ self.sync_perm_for_dag(dag.dag_id, dag.access_control)
+ except FabException:
+ self.log.exception(
+ "Failed to sync permissions for DAG '%s'; skipping it
and continuing with "
Review Comment:
Prose convention in the repo (including log strings) is "Dag", so: `"Failed
to sync permissions for Dag '%s'; skipping it and continuing with the remaining
Dags. ..."`.
##########
providers/fab/tests/unit/fab/auth_manager/test_security.py:
##########
@@ -1055,7 +1055,28 @@ def
test_create_dag_specific_permissions_airflow3(session, security_manager, mon
security_manager.create_dag_specific_permissions()
-def test_get_all_permissions(security_manager):
+def
test_create_dag_specific_permissions_skips_dag_with_bad_access_control(security_manager,
monkeypatch):
Review Comment:
This hunk replaces the `def test_get_all_permissions(security_manager):`
line, so that existing test disappears and its body (the
`assert_queries_count(1)` block, the `isinstance(perms, set)` checks and the
`("can_read", "Connections")` assertion) now runs as the tail of this new test.
Please add the new test as its own function and restore `def
test_get_all_permissions(security_manager):` in front of the original body.
--
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]
