Copilot commented on code in PR #64936:
URL: https://github.com/apache/airflow/pull/64936#discussion_r3066473711
##########
task-sdk/tests/task_sdk/test_configuration.py:
##########
@@ -156,3 +156,19 @@ def test_user_config_overrides_provider_values(self):
custom_value = "my_custom.celery_executor"
with conf_vars({("celery", "celery_app_name"): custom_value}):
assert conf.get("celery", "celery_app_name") == custom_value
+
+
+class TestGetAirflowConfig:
+ """Tests for get_airflow_config respecting AIRFLOW_CONFIG env var."""
+
+ def test_returns_airflow_config_env_var(self):
+ """get_airflow_config returns AIRFLOW_CONFIG when set."""
+ with mock.patch.dict("os.environ", {"AIRFLOW_CONFIG":
"/custom/path/airflow.cfg"}):
+ assert get_airflow_config() == "/custom/path/airflow.cfg"
+
+ def test_expands_env_var_in_airflow_config(self):
+ """get_airflow_config expands env vars in AIRFLOW_CONFIG."""
+ with mock.patch.dict(
+ "os.environ", {"AIRFLOW_CONFIG": "$CUSTOM_DIR/airflow.cfg",
"CUSTOM_DIR": "/resolved"}
+ ):
+ assert get_airflow_config() == "/resolved/airflow.cfg"
Review Comment:
The new behavior covers `AIRFLOW_CONFIG`, but there is no test for the
fallback branch when `AIRFLOW_CONFIG` is unset and `AIRFLOW_HOME` contains env
vars (e.g. `$CUSTOM_DIR/airflow`). Adding a test for that case would prevent
regressions and would validate the fix suggested in the implementation
(expanding `AIRFLOW_HOME` in the fallback path).
```suggestion
assert get_airflow_config() == "/resolved/airflow.cfg"
def test_expands_env_var_in_airflow_home_fallback(self):
"""get_airflow_config expands env vars in AIRFLOW_HOME when
AIRFLOW_CONFIG is unset."""
with mock.patch.dict(
"os.environ", {"AIRFLOW_HOME": "$CUSTOM_DIR/airflow",
"CUSTOM_DIR": "/resolved"}, clear=True
):
assert get_airflow_config() == "/resolved/airflow/airflow.cfg"
```
##########
task-sdk/src/airflow/sdk/configuration.py:
##########
@@ -99,16 +100,19 @@ def get_sdk_expansion_variables() -> dict[str, Any]:
SDK only needs AIRFLOW_HOME for expansion. Core specific variables
(FERNET_KEY, JWT_SECRET_KEY, etc.) are not needed in the SDK.
"""
- airflow_home = os.environ.get("AIRFLOW_HOME",
os.path.expanduser("~/airflow"))
+ airflow_home = expand_env_var(os.environ.get("AIRFLOW_HOME",
os.path.expanduser("~/airflow")))
return {
"AIRFLOW_HOME": airflow_home,
}
def get_airflow_config() -> str:
"""Get path to airflow.cfg file."""
- airflow_home = os.environ.get("AIRFLOW_HOME",
os.path.expanduser("~/airflow"))
- return os.path.join(airflow_home, "airflow.cfg")
+ airflow_config_var = os.environ.get("AIRFLOW_CONFIG")
+ if airflow_config_var is None:
+ airflow_home = os.environ.get("AIRFLOW_HOME",
os.path.expanduser("~/airflow"))
Review Comment:
`get_airflow_config()` expands env vars for `AIRFLOW_CONFIG`, but when
`AIRFLOW_CONFIG` is unset it does *not* expand env vars in `AIRFLOW_HOME`
(e.g., `AIRFLOW_HOME=$CUSTOM_DIR/airflow` would return a path containing the
literal `$CUSTOM_DIR`). This is also inconsistent with
`get_sdk_expansion_variables()` which now expands `AIRFLOW_HOME`. Consider
applying `expand_env_var()` to the resolved `AIRFLOW_HOME` in this branch as
well before joining `airflow.cfg`.
```suggestion
airflow_home = expand_env_var(
os.environ.get("AIRFLOW_HOME", os.path.expanduser("~/airflow"))
)
```
--
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]
