jedcunningham commented on code in PR #45009:
URL: https://github.com/apache/airflow/pull/45009#discussion_r1891059132
##########
airflow/api_fastapi/app.py:
##########
@@ -112,34 +112,29 @@ def get_auth_manager_cls() -> type[BaseAuthManager]:
return auth_manager_cls
-def init_auth_manager() -> BaseAuthManager:
- """
- Initialize the auth manager.
-
- Import the user manager class and instantiate it.
- """
+def create_auth_manager() -> BaseAuthManager:
+ """Create the auth manager."""
global auth_manager
auth_manager_cls = get_auth_manager_cls()
auth_manager = auth_manager_cls()
+ return auth_manager
+
+
+def init_auth_manager(app: FastAPI | None = None) -> BaseAuthManager:
+ """Initialize the auth manager."""
+ auth_manager = create_auth_manager()
auth_manager.init()
+
+ auth_manager_fastapi_app = auth_manager.get_fastapi_app()
+ if app and auth_manager_fastapi_app:
Review Comment:
```suggestion
if app and auth_manager_fastapi_app := auth_manager.get_fastapi_app():
```
nit; feel free to reject this one, definitely stylistic :)
##########
providers/src/airflow/providers/amazon/aws/auth_manager/aws_auth_manager.py:
##########
@@ -76,12 +76,12 @@ class AwsAuthManager(BaseAuthManager):
Leverages AWS services such as Amazon Identity Center and Amazon Verified
Permissions to perform
authentication and authorization in Airflow.
-
- :param appbuilder: the flask app builder
"""
- def __init__(self, appbuilder: AirflowAppBuilder) -> None:
- super().__init__(appbuilder)
+ appbuilder: AirflowAppBuilder | None = None
+
+ def __init__(self) -> None:
+ super().__init__()
Review Comment:
This change isn't backwards compatible with AF2.
##########
airflow/auth/managers/simple/simple_auth_manager.py:
##########
@@ -80,6 +81,9 @@ class
SimpleAuthManager(BaseAuthManager[SimpleAuthManagerUser]):
# Cache containing the password associated to a username
passwords: dict[str, str] = {}
+ # TODO: Needs to be deleted when Airflow 2 legacy UI is gone
+ appbuilder: AirflowAppBuilder | None = None
Review Comment:
Hmm, why does simple auth manager need this? If it's just for the
assignment, can we check before we set it? Or is this for typing/linting?
Having this in every auth manager, even those that have nothing to do with FAB,
doesn't feel very good imo.
##########
airflow/auth/managers/base_auth_manager.py:
##########
@@ -68,14 +68,8 @@ class BaseAuthManager(Generic[T], LoggingMixin):
Class to derive in order to implement concrete auth managers.
Auth managers are responsible for any user management related operation
such as login, logout, authz, ...
-
- :param appbuilder: the flask app builder
"""
- def __init__(self, appbuilder: AirflowAppBuilder | None = None) -> None:
Review Comment:
Probably worth a newsfragment for this change in the authmanager interface.
##########
providers/src/airflow/providers/fab/www/app.py:
##########
@@ -41,7 +42,7 @@
csrf = CSRFProtect()
-def create_app(config=None, testing=False):
+def create_app():
Review Comment:
I think you also need to rid of these kwargs in `cached_app`.
--
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]
