-
Notifications
You must be signed in to change notification settings - Fork 236
feat: support operator_aliases for short operator names in YAML configs #753
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,3 +1,9 @@ | ||
| default_args: | ||
| start_date: "2025-01-01" | ||
| owner: "global_owner" | ||
|
|
||
| operator_aliases: | ||
| - operator_alias: BashOperator | ||
| operator_path: airflow.providers.standard.operators.bash.BashOperator | ||
| - operator_alias: PythonOperator | ||
| operator_path: airflow.providers.standard.operators.python.PythonOperator | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -400,6 +400,47 @@ def test_make_task_bad_operator(): | |
| td.make_task(operator, task_params) | ||
|
|
||
|
|
||
| def test_resolve_operator_alias_returns_full_path(): | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please add a
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It could be something along the lines: And confirming the expected operator was instantiated within
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Acknowledged, and will start working on this.
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The existing alias tests in test_dagbuilder.py inject operator_aliases directly into DagBuilder's default_config, which bypasses the propagation code in dagfactory.py. I missed that the two test files cover different layers. test_dagbuilder.py tests DagBuilder in isolation, while the DAGFactory-level behavior of reading aliases from defaults.yml and copying them into default_config needed a separate test in test_dagfactory.py. Added that now along with the Airflow 3 import path fix. |
||
| aliases = [{"operator_alias": "BashOperator", "operator_path": get_bash_operator_path()}] | ||
| resolved = DagBuilder._resolve_operator_alias("BashOperator", aliases) | ||
| assert resolved == get_bash_operator_path() | ||
|
|
||
|
|
||
| def test_resolve_operator_alias_passthrough_full_path(): | ||
| full_path = get_bash_operator_path() | ||
| resolved = DagBuilder._resolve_operator_alias(full_path, []) | ||
| assert resolved == full_path | ||
|
|
||
|
|
||
| def test_resolve_operator_alias_unknown_alias_passthrough(): | ||
| aliases = [{"operator_alias": "BashOperator", "operator_path": get_bash_operator_path()}] | ||
| resolved = DagBuilder._resolve_operator_alias("UnknownOperator", aliases) | ||
| assert resolved == "UnknownOperator" | ||
|
|
||
|
|
||
| def test_resolve_operator_alias_empty_aliases(): | ||
| resolved = DagBuilder._resolve_operator_alias("BashOperator", []) | ||
| assert resolved == "BashOperator" | ||
|
|
||
|
|
||
| def test_build_dag_with_operator_alias(): | ||
| aliases = [{"operator_alias": "BashOperator", "operator_path": get_bash_operator_path()}] | ||
| default_config_with_aliases = {**DEFAULT_CONFIG, "operator_aliases": aliases} | ||
| dag_config_with_alias = { | ||
| **DAG_CONFIG, | ||
| "tasks": [ | ||
| { | ||
| "task_id": "task_1", | ||
| "operator": "BashOperator", | ||
| "bash_command": "echo 1", | ||
| } | ||
| ], | ||
| } | ||
| td = dagbuilder.DagBuilder("test_dag", dag_config_with_alias, default_config_with_aliases) | ||
| result = td.build() | ||
| assert "task_1" in result["dag"].task_ids | ||
|
|
||
|
|
||
| def test_make_task_missing_required_param(): | ||
| td = dagbuilder.DagBuilder("test_dag", DAG_CONFIG, DEFAULT_CONFIG) | ||
| operator = get_bash_operator_path() | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we add example covering both Airflow 2 and Airflow 3 you can find example https://github.com/astronomer/dag-factory/tree/main/dev/dags
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, let me take a look.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @pankajastro. Quick clarification on covering both Airflow 2 and Airflow 3 here: operator_aliases is an alias-to-path map, so a single alias name like BashOperator can only point to one operator_path. That means I can't bind the same alias to both airflow.operators.bash.BashOperator (AF2) and airflow.providers.standard.operators.bash.BashOperator (AF3) in one list.
Which approach would you prefer?
Happy to go either way once I know your preference.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I mean we should add example yml in both Airflow3 and Airflow2 that would in CI as integration test