jedcunningham commented on code in PR #48070:
URL: https://github.com/apache/airflow/pull/48070#discussion_r2012229019
##########
airflow-core/src/airflow/dag_processing/collection.py:
##########
@@ -196,7 +196,7 @@ def _serialize_dag_capturing_errors(
min_update_interval=settings.MIN_SERIALIZED_DAG_UPDATE_INTERVAL,
session=session,
)
- if dag_was_updated:
+ if dag_was_updated and "FabAuthManager" in conf.get("core",
"auth_manager"):
_sync_dag_perms(dag, session=session)
else:
# Check and update DagCode
Review Comment:
or like this (can't suggest the update_source_code line, but you get the
idea):
```suggestion
if not dag_was_updated:
DagCode.update_source_code(dag.dag_id, dag.fileloc)
return []
if "FabAuthManager" in conf.get("core", "auth_manager"):
_sync_dag_perms(dag, session=session)
```
##########
dev/breeze/src/airflow_breeze/commands/kubernetes_commands.py:
##########
@@ -1038,6 +1038,8 @@ def _deploy_helm_chart(
"config.core.auth_manager=airflow.providers.fab.auth_manager.fab_auth_manager.FabAuthManager",
"--set",
f"config.api.base_url=http://localhost:{api_server_port}";,
+ "--set",
+
"config.database.external_db_managers=airflow.providers.fab.auth_manager.models.db.FABDBManager",
Review Comment:
```suggestion
```
Same with this.
##########
chart/values.yaml:
##########
@@ -2654,6 +2654,8 @@ config:
colored_console_log: 'False'
remote_logging: '{{- ternary "True" "False" (or
.Values.elasticsearch.enabled .Values.opensearch.enabled) }}'
auth_manager:
"airflow.providers.fab.auth_manager.fab_auth_manager.FabAuthManager"
+ database:
+ external_db_managers:
"airflow.providers.fab.auth_manager.models.db.FABDBManager"
Review Comment:
```suggestion
```
And this.
##########
dev/breeze/src/airflow_breeze/params/shell_params.py:
##########
@@ -522,7 +522,14 @@ def env_variables_for_docker_commands(self) -> dict[str,
str]:
_set_var(_env, "AIRFLOW__CELERY__BROKER_URL",
self.airflow_celery_broker_url)
_set_var(_env, "AIRFLOW__CORE__AUTH_MANAGER", self.auth_manager_path)
_set_var(_env, "AIRFLOW__CORE__EXECUTOR", self.executor)
- _set_var(_env, "AIRFLOW__CORE__SIMPLE_AUTH_MANAGER_USERS",
"admin:admin,viewer:viewer")
+ if self.auth_manager == FAB_AUTH_MANAGER:
+ _set_var(
+ _env,
+ "AIRFLOW__DATABASE__EXTERNAL_DB_MANAGERS",
+ "airflow.providers.fab.auth_manager.models.db.FABDBManager",
+ )
Review Comment:
```suggestion
```
I don't think we need this any longer, now that the FAB auth manager is able
to add it's own db manager.
##########
airflow-core/src/airflow/dag_processing/collection.py:
##########
@@ -196,7 +196,7 @@ def _serialize_dag_capturing_errors(
min_update_interval=settings.MIN_SERIALIZED_DAG_UPDATE_INTERVAL,
session=session,
)
- if dag_was_updated:
+ if dag_was_updated and "FabAuthManager" in conf.get("core",
"auth_manager"):
Review Comment:
This means we always run `update_source_code` if we don't have FAB. We
should refactor this either like this:
```suggestion
if dag_was_updated:
if "FabAuthManager" in conf.get("core", "auth_manager"):
```
--
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]
