You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@airflow.apache.org by "jscheffl (via GitHub)" <gi...@apache.org> on 2026/02/14 17:44:01 UTC
[PR] Remove global variables in airflow.settings [airflow]
jscheffl opened a new pull request, #61917:
URL: https://github.com/apache/airflow/pull/61917
Another small increment to remove global statements for PR https://github.com/apache/airflow/pull/58116 - an alternative approach to #61356.
This removes global statements from settings.py where the a lot of state was hold in variables used with global and then a lot of code way relying to access these globals. Is a like a public interface and refactoring would be needed.
`global` is evil.
---
##### Was generative AI tooling used to co-author this PR?
- [ ] No (please specify the tool below)
---
* Read the **[Pull Request Guidelines](https://github.com/apache/airflow/blob/main/contributing-docs/05_pull_requests.rst#pull-request-guidelines)** for more information. Note: commit author/co-author name and email in commits become permanently public when merged.
* For fundamental code changes, an Airflow Improvement Proposal ([AIP](https://cwiki.apache.org/confluence/display/AIRFLOW/Airflow+Improvement+Proposals)) is needed.
* When adding dependency, check compliance with the [ASF 3rd Party License Policy](https://www.apache.org/legal/resolved.html#category-x).
* For significant user-facing changes create newsfragment: `{pr_number}.significant.rst` or `{issue_number}.significant.rst`, in [airflow-core/newsfragments](https://github.com/apache/airflow/tree/main/airflow-core/newsfragments).
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Remove global variables in airflow.settings [airflow]
Posted by "jscheffl (via GitHub)" <gi...@apache.org>.
jscheffl commented on code in PR #61917:
URL: https://github.com/apache/airflow/pull/61917#discussion_r3042856099
##########
providers/fab/src/airflow/providers/fab/auth_manager/models/db.py:
##########
@@ -66,7 +67,8 @@ class FABDBManager(BaseDBManager):
def create_db_from_orm(self):
super().create_db_from_orm()
- db, flask_app = _get_flask_db(settings.SQL_ALCHEMY_CONN)
+ sql_conn = settings.get_sql_alchemy_conn() if AIRFLOW_V_3_2_PLUS else settings.SQL_ALCHEMY_CONN
Review Comment:
Oh, meanwhile this needs adjustments assuming this is merged post 3.2.0 release!
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Remove global variables in airflow.settings [airflow]
Posted by "jscheffl (via GitHub)" <gi...@apache.org>.
jscheffl commented on PR #61917:
URL: https://github.com/apache/airflow/pull/61917#issuecomment-4307525174
Hi @ashb thanks for reversion. Wondering though, carefully ensured _full tests needed_ are executed prior merging. Do we need to add a test preventing regression on this? Seeing the error in the reversion PR is this code branch not executed in CI?
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Remove global variables in airflow.settings [airflow]
Posted by "jscheffl (via GitHub)" <gi...@apache.org>.
jscheffl commented on code in PR #61917:
URL: https://github.com/apache/airflow/pull/61917#discussion_r3107575681
##########
providers/fab/src/airflow/providers/fab/auth_manager/models/db.py:
##########
@@ -23,6 +23,7 @@
from airflow import settings
from airflow.providers.common.compat.sdk import AirflowException
from airflow.providers.fab.auth_manager.models import model_metadata
+from airflow.providers.fab.version_compat import AIRFLOW_V_3_2_1_PLUS
Review Comment:
Args there is still some version compat that needs fixing. Over-looked this.
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Remove global variables in airflow.settings [airflow]
Posted by "jscheffl (via GitHub)" <gi...@apache.org>.
jscheffl commented on code in PR #61917:
URL: https://github.com/apache/airflow/pull/61917#discussion_r3068252965
##########
airflow-core/src/airflow/settings.py:
##########
@@ -681,6 +702,16 @@ def __getattr__(name: str):
from airflow.exceptions import RemovedInAirflow4Warning
+ if name == "SQL_ALCHEMY_CONN":
+ warnings.warn(
+ "settings.SQL_ALCHEMY_CONN has been replaced by get_sql_alchemy_conn(). This shim is just for compatibility. "
+ "Please upgrade your provider or integration.",
+ RemovedInAirflow4Warning,
+ stacklevel=2,
+ )
+ return get_sql_alchemy_conn()
+ if name == "SQL_ALCHEMY_CONN_ASYNC":
+ return _AirflowSettings.sql_alchemy_conn_async
Review Comment:
applied, thanks.
##########
airflow-core/src/airflow/settings.py:
##########
@@ -439,14 +460,14 @@ def configure_orm(disable_connection_pool=False, pool_class=None):
engine_args = prepare_engine_args(disable_connection_pool, pool_class)
connect_args = _get_connect_args("sync")
- if SQL_ALCHEMY_CONN.startswith("sqlite"):
+ if _AirflowSettings.sql_alchemy_conn.startswith("sqlite"):
# FastAPI runs sync endpoints in a separate thread. SQLite does not allow
# to use objects created in another threads by default. Allowing that in test
# to so the `test` thread and the tested endpoints can use common objects.
connect_args["check_same_thread"] = False
engine = create_metadata_engine(
- SQL_ALCHEMY_CONN,
+ _AirflowSettings.sql_alchemy_conn,
Review Comment:
applied, thanks.
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Remove global variables in airflow.settings [airflow]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #61917:
URL: https://github.com/apache/airflow/pull/61917#discussion_r3066844273
##########
airflow-core/src/airflow/settings.py:
##########
@@ -681,6 +702,16 @@ def __getattr__(name: str):
from airflow.exceptions import RemovedInAirflow4Warning
+ if name == "SQL_ALCHEMY_CONN":
+ warnings.warn(
+ "settings.SQL_ALCHEMY_CONN has been replaced by get_sql_alchemy_conn(). This shim is just for compatibility. "
+ "Please upgrade your provider or integration.",
+ RemovedInAirflow4Warning,
+ stacklevel=2,
+ )
+ return get_sql_alchemy_conn()
+ if name == "SQL_ALCHEMY_CONN_ASYNC":
+ return _AirflowSettings.sql_alchemy_conn_async
Review Comment:
`settings.SQL_ALCHEMY_CONN_ASYNC` is still readable even after `_AirflowSettings.block_orm_access` is enabled. If `configure_vars()` already populated `sql_alchemy_conn_async`, task/DAG code could still retrieve the real async connection string and connect to the metadatabase, undermining `block_orm_access()`. Consider guarding this path the same as `SQL_ALCHEMY_CONN` (raise when blocked and/or return a blocked URL + emit a deprecation warning).
```suggestion
warnings.warn(
"settings.SQL_ALCHEMY_CONN_ASYNC has been replaced by get_sql_alchemy_conn_async(). "
"This shim is just for compatibility. Please upgrade your provider or integration.",
RemovedInAirflow4Warning,
stacklevel=2,
)
return get_sql_alchemy_conn_async()
```
##########
airflow-core/src/airflow/settings.py:
##########
@@ -498,7 +519,7 @@ def prepare_engine_args(disable_connection_pool=False, pool_class=None):
default_args = {}
for dialect, default in DEFAULT_ENGINE_ARGS.items():
- if SQL_ALCHEMY_CONN.startswith(dialect):
+ if _AirflowSettings.sql_alchemy_conn.startswith(dialect):
default_args = default.copy()
break
Review Comment:
`prepare_engine_args()` relies on `_AirflowSettings.sql_alchemy_conn.startswith(...)` without ensuring the connection string is configured or checking the `block_orm_access` flag. Using `get_sql_alchemy_conn()` (or a shared helper) here would give consistent error behavior and avoid accidental DB access when blocked.
##########
airflow-core/tests/unit/utils/test_db.py:
##########
@@ -485,18 +489,21 @@ def test_mysql_variants_detected(self, mocker):
def test_none_sql_alchemy_conn(self, mocker):
"""Test behavior when SQL_ALCHEMY_CONN is None."""
- mocker.patch.object(settings, "SQL_ALCHEMY_CONN", None)
+ mocker.patch.object(settings._AirflowSettings, "sql_alchemy_conn", None)
mock_dispose = mocker.patch.object(settings, "dispose_orm")
mock_configure = mocker.patch.object(settings, "configure_orm")
- with AutocommitEngineForMySQL():
- # Should be a no-op when connection string is None
- mock_dispose.assert_not_called()
- mock_configure.assert_not_called()
+ with pytest.raises(RuntimeError, match="connection string not configured"):
+ with AutocommitEngineForMySQL():
+ pass
+
+ # Should be a no-op when connection string is None
Review Comment:
This test now expects a `RuntimeError` when the connection string is unset, but the trailing comment still says it “should be a no-op when connection string is None”. Consider rewording the comment to reflect the new behavior (exception with no side effects), or adjust expectations if the intent is truly a no-op.
```suggestion
# Failure should not trigger any ORM disposal or reconfiguration side effects.
```
##########
task-sdk/src/airflow/sdk/execution_time/supervisor.py:
##########
@@ -319,8 +319,7 @@ def __getattr__(name: str):
settings.__getattr__ = __getattr__
- settings.SQL_ALCHEMY_CONN = conn
- settings.SQL_ALCHEMY_CONN_ASYNC = conn
+ settings.block_orm_access()
Review Comment:
`settings.block_orm_access()` currently only affects `get_sql_alchemy_conn()`/`SQL_ALCHEMY_CONN`. Since `SQL_ALCHEMY_CONN_ASYNC` is still returned by `airflow.settings.__getattr__` without checking the block flag, task/DAG code could still obtain an async metadb URL if it was set before blocking. Consider also blocking the async URL (e.g., update core to guard `SQL_ALCHEMY_CONN_ASYNC` when blocked, and/or ensure the async value is overwritten to the blocked URL here).
##########
airflow-core/src/airflow/settings.py:
##########
@@ -439,14 +460,14 @@ def configure_orm(disable_connection_pool=False, pool_class=None):
engine_args = prepare_engine_args(disable_connection_pool, pool_class)
connect_args = _get_connect_args("sync")
- if SQL_ALCHEMY_CONN.startswith("sqlite"):
+ if _AirflowSettings.sql_alchemy_conn.startswith("sqlite"):
# FastAPI runs sync endpoints in a separate thread. SQLite does not allow
# to use objects created in another threads by default. Allowing that in test
# to so the `test` thread and the tested endpoints can use common objects.
connect_args["check_same_thread"] = False
engine = create_metadata_engine(
- SQL_ALCHEMY_CONN,
+ _AirflowSettings.sql_alchemy_conn,
Review Comment:
`configure_orm()` uses `_AirflowSettings.sql_alchemy_conn` directly (e.g. `.startswith(...)`) and never consults `get_sql_alchemy_conn()` / `_AirflowSettings.block_orm_access`. This means ORM configuration can still proceed even when ORM access is meant to be blocked, and it can also fail with a confusing `'NoneType' object has no attribute 'startswith'` if `configure_vars()` hasn’t been called yet. Suggestion: fetch `conn_str = get_sql_alchemy_conn()` once at the start of `configure_orm()` and use that throughout.
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Remove global variables in airflow.settings [airflow]
Posted by "jscheffl (via GitHub)" <gi...@apache.org>.
jscheffl commented on code in PR #61917:
URL: https://github.com/apache/airflow/pull/61917#discussion_r3068253225
##########
airflow-core/src/airflow/settings.py:
##########
@@ -498,7 +519,7 @@ def prepare_engine_args(disable_connection_pool=False, pool_class=None):
default_args = {}
for dialect, default in DEFAULT_ENGINE_ARGS.items():
- if SQL_ALCHEMY_CONN.startswith(dialect):
+ if _AirflowSettings.sql_alchemy_conn.startswith(dialect):
default_args = default.copy()
break
Review Comment:
applied, thanks.
##########
task-sdk/src/airflow/sdk/execution_time/supervisor.py:
##########
@@ -319,8 +319,7 @@ def __getattr__(name: str):
settings.__getattr__ = __getattr__
- settings.SQL_ALCHEMY_CONN = conn
- settings.SQL_ALCHEMY_CONN_ASYNC = conn
+ settings.block_orm_access()
Review Comment:
applied, thanks.
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Remove global variables in airflow.settings [airflow]
Posted by "ashb (via GitHub)" <gi...@apache.org>.
ashb commented on PR #61917:
URL: https://github.com/apache/airflow/pull/61917#issuecomment-4304627337
I have reverted this as it broke everything at task run time.
I would like to point out: _This is still global_. module global or singleton class vars are still global.
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Remove global variables in airflow.settings [airflow]
Posted by "jscheffl (via GitHub)" <gi...@apache.org>.
jscheffl commented on code in PR #61917:
URL: https://github.com/apache/airflow/pull/61917#discussion_r3068253476
##########
airflow-core/tests/unit/utils/test_db.py:
##########
@@ -485,18 +489,21 @@ def test_mysql_variants_detected(self, mocker):
def test_none_sql_alchemy_conn(self, mocker):
"""Test behavior when SQL_ALCHEMY_CONN is None."""
- mocker.patch.object(settings, "SQL_ALCHEMY_CONN", None)
+ mocker.patch.object(settings._AirflowSettings, "sql_alchemy_conn", None)
mock_dispose = mocker.patch.object(settings, "dispose_orm")
mock_configure = mocker.patch.object(settings, "configure_orm")
- with AutocommitEngineForMySQL():
- # Should be a no-op when connection string is None
- mock_dispose.assert_not_called()
- mock_configure.assert_not_called()
+ with pytest.raises(RuntimeError, match="connection string not configured"):
+ with AutocommitEngineForMySQL():
+ pass
+
+ # Should be a no-op when connection string is None
Review Comment:
applied, thanks.
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Remove global variables in airflow.settings [airflow]
Posted by "jscheffl (via GitHub)" <gi...@apache.org>.
jscheffl merged PR #61917:
URL: https://github.com/apache/airflow/pull/61917
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Remove global variables in airflow.settings [airflow]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #61917:
URL: https://github.com/apache/airflow/pull/61917#issuecomment-4173745070
This pull request has been automatically marked as stale because it has not had recent activity. It will be closed in 5 days if no further activity occurs. Thank you for your contributions.
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Remove global variables in airflow.settings [airflow]
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on PR #61917:
URL: https://github.com/apache/airflow/pull/61917#issuecomment-4276722007
I agree globals are evit.
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org