fix sdk configuration to use $AIRFLOW_CONFIG env#64936
fix sdk configuration to use $AIRFLOW_CONFIG env#64936wjddn279 wants to merge 1 commit intoapache:mainfrom
Conversation
|
This fixes the |
Vamsi-klu
left a comment
There was a problem hiding this comment.
The AIRFLOW_CONFIG support looks correct and matches core's implementation at airflow-core/src/airflow/configuration.py:555-560. Good fix.
I have a couple of observations though.
The PR adds expand_env_var around AIRFLOW_HOME in get_sdk_expansion_variables (line 102), but the fallback path in get_airflow_config still uses the raw os.environ.get("AIRFLOW_HOME", os.path.expanduser("~/airflow")) without expand_env_var. So if someone sets AIRFLOW_HOME to something like $CUSTOM_DIR/airflow (without shell expansion), config value expansion would see the expanded path but get_airflow_config would see the unexpanded one.
Core solves this by having a dedicated get_airflow_home() function (configuration.py:550-552) that calls expand_env_var once, and both get_airflow_config and get_all_expansion_variables use that. It might be cleaner to do the same thing here in the SDK rather than sprinkling expand_env_var in two places with slightly different behavior.
On tests, both test cases cover the path where AIRFLOW_CONFIG is set. It would be good to also have one for the default fallback when AIRFLOW_CONFIG is not set, something like asserting that get_airflow_config() returns {AIRFLOW_HOME}/airflow.cfg when the env var is absent. That way if someone accidentally breaks the fallback path it gets caught.
Also small thing, the PR body says "closed:" but GitHub auto-close needs the keyword "closes:" (e.g. closes: #64918).
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Updates the Task SDK configuration resolution so get_airflow_config() respects the AIRFLOW_CONFIG environment variable (including env-var expansion), aligning behavior with Airflow’s documented configuration conventions (closes #64918).
Changes:
- Teach
get_airflow_config()to returnAIRFLOW_CONFIGwhen set, expanding environment variables in the path. - Expand environment variables in
AIRFLOW_HOMEwhen building SDK expansion variables. - Add tests covering
AIRFLOW_CONFIGprecedence and env-var expansion.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| task-sdk/src/airflow/sdk/configuration.py | Implement AIRFLOW_CONFIG override + env-var expansion; expand AIRFLOW_HOME for SDK expansion vars. |
| task-sdk/tests/task_sdk/test_configuration.py | Add unit tests validating AIRFLOW_CONFIG behavior and env-var expansion. |
| 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")) |
There was a problem hiding this 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.
| 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")) | |
| ) |
| with mock.patch.dict( | ||
| "os.environ", {"AIRFLOW_CONFIG": "$CUSTOM_DIR/airflow.cfg", "CUSTOM_DIR": "/resolved"} | ||
| ): | ||
| assert get_airflow_config() == "/resolved/airflow.cfg" |
There was a problem hiding this 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).
| assert get_airflow_config() == "/resolved/airflow.cfg" | |
| 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" |
closed: #64918
Was generative AI tooling used to co-author this PR?
{pr_number}.significant.rst, in airflow-core/newsfragments. You can add this file in a follow-up commit after the PR is created so you know the PR number.