apache
Move `register_views` to auth manager interface
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.rst or {issue_number}.significant.rst, in newsfragments.
I see the interfaces with this PR are much simpler. But for the coupling of Providers and Core, this smells like a breaking change. Do you have a proposal how to further release updates for provider compatible with Airflow 2.8/2.9/2.10? Is the core change intended to be for Airflow 3?
I see the interfaces with this PR are much simpler. But for the coupling of Providers and Core, this smells like a breaking change. Do you have a proposal how to further release updates for provider compatible with Airflow 2.8/2.9/2.10? Is the core change intended to be for Airflow 3?
Yes you are right, I should have mentioned it in the PR. I'll take care of that today
Do you have a proposal how to further release updates for provider compatible with Airflow 2.8/2.9/2.10?
I dont think this is a problem. The newly created method register_views in FAB auth manager calls underneath register_views in the security manager. Airflow 2 calls register_views from the security manager. Therefore, any modifications in the register_views method from security manager will be available in Airflow 2 and 3
The issue is all the way around, making sure that Airflow 3 can use older version of FAB provider. I'll handle it
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
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 a mechanism similar to AIP-68 will expose for plugins 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.
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 a mechanism similar to AIP-68 will expose for plugins 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