-
Notifications
You must be signed in to change notification settings - Fork 16.4k
Move register_views to auth manager interface
#41777
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
Move register_views to auth manager interface
#41777
Conversation
|
I see the interfaces with this PR are much simpler. But for the coupling of Providers and Core, this smells like a breaking change. |
Yes you are right, I should have mentioned it in the PR. I'll take care of that today |
Yes |
I dont think this is a problem. The newly created method The issue is all the way around, making sure that Airflow 3 can use older version of FAB provider. I'll handle it |
|
I resolved the backward incompatibility issue. It should be all fine now. |
jscheffl
left a comment
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.
Did a review - if we would not want to replace FAB in Airflow 3 I would have made an approve, but as we actually want to get rid of it... Might be we need to introduce another method of integration that is future proof.
Will the AWS Auth Manager render the views inside Airflow UI or just add menu entries which point to an external site?
100% agree with you but the intent of this PR is not to drop FAB from Airflow. That requires a lot of work and will be done in separate PRs. The way I register views in the auth manager is the way it is done today so I dont add anything new. Definitely this will need to be reworked/redone later to complete AIP-79. And yes likely we will need to leverage the mechanism created in AIP-68 so that the auth manager can add new views to an Airflow environment |
I just want to clarify not to add a new interface and therefore add a new legacy - even if it is used the same way but unclean today. |
Nop, this already exist today. I just moved some code from security manager to auth manager. If you look here, we are already using appbuilder to register the view |
Auth managers have the possibility of implementing their own views. To do that, it had to be done in the security manager override. The security manager override had been creating to override/customize the security manager. This is something needed by the FAB auth manager, but other than that, the other auth managers should not have the need of creating a security manager override.
To avoid creating security manager override just for the sake of registering views specific to auth manager, moving this method to the interface will simplify things.
^ 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.rstor{issue_number}.significant.rst, in newsfragments.