-
Notifications
You must be signed in to change notification settings - Fork 13.7k
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.access_control can't sync when clean access_control #30340
Conversation
return True | ||
return False | ||
|
||
if dag.access_control or needs_perms(root_dag_id): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove this if block completely so we sync perms in both cases. This is effectively reverting #15464.
else: | ||
resource = self.get_resource(dag_resource_name) | ||
if resource: | ||
_revoke_all_stale_permissions(resource) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If a DAG’s access_control
is empty, it check whether there’s existing permission configurations and reset those
Do you have any plan to merge it? I need this one for my project :( |
This PR is ready for review |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. But I know very little about that part of the codebase. @ephraimbuddy @kaxil @uranusjr - I think you've been doing some "real fixes" around that - maybe second opinon there?
Awesome work, congrats on your first merged pull request! You are invited to check our Issue Tracker for additional contributions. |
* Reset permission if `access_control` is empty * Check `resource` before call `_revoke_all_stale_permissions` * Fix static checks (cherry picked from commit 2c0c8b8)
Update: #33632 |
Closes: #25149
access_control
is empty, permission-sync is skipped entirely. I think it should check whether there’s existing permission configurations and reset those if needed instead.