-
Notifications
You must be signed in to change notification settings - Fork 14.6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix DAG-level permissions disappearance on DAG sync #32999
Conversation
Congratulations on your first Pull Request and welcome to the Apache Airflow community! If you have any issues or are unsure about any anything please check our Contribution Guide (https://github.com/apache/airflow/blob/main/CONTRIBUTING.rst)
|
How does this affect the issue #30340 is supposed to solve? Also cc @huymq1710 |
Seems like this PR does introduce that issue back |
9907c05
to
22f680e
Compare
I rebased it and marked is as 2.7.0. Indeed the #30340 removed syncing where it was still needed and reverting the change seems to solve the problem. I think this is a blocker for people who use it and want to migrate so having it fixed in 2.7.0 would be great. |
I am not sure however if that one is just a band-aid. Can someone - who knows how the logic of dag permissions syncing was supposed to work? I would like to understand it, because it seems that almost no-one knows how it should work and it is certainly not documented. A slimple logical description of what should happen here might be very useful. I am hppy to take a closer look and try to do it properly. but I need some guidance. I've never touched this part of code and it's a bit of mystery for me on how it should work - but I ema willing to learn @kaxil @jedcunningham @uranusjr. Maybe a birds eye view on that one ?? I think we dropped a ball on that one for 2.6.2 and mostly because we had too little expert knowledge there. |
I thin @SamWheating #33632 solution is a better solution. Closing this one in favour of it. |
Closing in favour of just merged #33632 |
Closes: #32839
Adds back the code that was removed in #30340
^ Add meaningful description above
Read the Pull Request Guidelines for more information.
In case of fundamental code changes, an Airflow Improvement Proposal (AIP) is needed.
In case of a new dependency, check compliance with the ASF 3rd Party License Policy.
In case of backwards incompatible changes please leave a note in a newsfragment file, named
{pr_number}.significant.rst
or{issue_number}.significant.rst
, in newsfragments.