jedcunningham commented on code in PR #30116:
URL: https://github.com/apache/airflow/pull/30116#discussion_r1140738795
##########
airflow/models/dagbag.py:
##########
@@ -702,7 +702,7 @@ def needs_perms(dag_id: str) -> bool:
return True
return False
- if dag.access_control or needs_perms(root_dag_id):
+ if dag.access_control or not needs_perms(root_dag_id):
Review Comment:
I don't think this logic is correct. You are saying "or dag perms already
exist", a.k.a sync perms for existing DAGs, but don't we need to do it for new
DAGs? You probably want to just remove this if block completely so we sync
perms in both cases. This is effectively reverting #15464.
I wonder if we can figure out a better way to check that isn't as costly?
I'm okay with extra cost for access_control, but ideally we don't impact DAGs
that don't use it.
Either way, a good first step is to write a test case that fails with the
current code in main. We will want it anyways before we merge this.
--
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]
