-
Notifications
You must be signed in to change notification settings - Fork 17.4k
Move provider examples to their respective folders #57320
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
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 |
|---|---|---|
|
|
@@ -618,12 +618,12 @@ def collect_dags( | |
|
|
||
| files_to_parse = list_py_file_paths(dag_folder, safe_mode=safe_mode) | ||
|
|
||
| if include_examples: | ||
| from airflow import example_dags | ||
| # if include_examples: | ||
|
Contributor
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. This change felt bold. Which is why I commented it out. If we were to let DagBag load examples, then we are duplicating the provider examples loading in both Dagbundle and in dagbag. Will this affect backward compatibility?
Contributor
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. Even if it sfeels hard - I assume this is the core thing to remove. I assume this is a victim of the surgery but later all Dags will be loaded from standard provider directly. If there are legacy side effects we need to clean them up. So you are doing the "right" thing. |
||
| # from airflow import example_dags | ||
|
|
||
| example_dag_folder = next(iter(example_dags.__path__)) | ||
| # example_dag_folder = next(iter(example_dags.__path__)) | ||
|
|
||
| files_to_parse.extend(list_py_file_paths(example_dag_folder, safe_mode=safe_mode)) | ||
| # files_to_parse.extend(list_py_file_paths(example_dag_folder, safe_mode=safe_mode)) | ||
|
|
||
| for filepath in files_to_parse: | ||
| try: | ||
|
|
||
This file was deleted.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -73,7 +73,8 @@ class TestCliDags: | |
|
|
||
| @classmethod | ||
| def setup_class(cls): | ||
| parse_and_sync_to_db(os.devnull, include_examples=True) | ||
| with conf_vars({("core", "load_examples"): "True"}): | ||
| parse_and_sync_to_db(os.devnull, include_examples=True) | ||
| cls.parser = cli_parser.get_parser() | ||
|
|
||
| @classmethod | ||
|
|
@@ -216,9 +217,8 @@ def test_next_execution(self, tmp_path, stdout_capture): | |
|
|
||
| # Rebuild Test DB for other tests | ||
| clear_db_dags() | ||
| parse_and_sync_to_db(os.devnull, include_examples=True) | ||
| self.setup_class() | ||
|
Contributor
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. This PR changes the We have two options
Choosing (2)
Contributor
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. I'd also propose to choose 2. In my mind making code in a structure for testability is great but adding balconies for testing in productive code has the danger of future "mis-use"/"unintended use". Side effect is that we need this very much often, so in regards of convenience maybe a dedicated decorator might be nice (but not critical, just nice) and in regards of performance might be needed to cache examples to prevent heavy and often re-parse as this will maybe impact performance of testing if re-parsed often.
Contributor
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. Reading more context of the change I assume this is a bit of a mis-conception how the previous code was constructured. Calling the "setup_class()" or a re-sync of dags is conceptually wrong. Not for your change but in general. If the test "messes up" the dags and side effects should be prevented then a proper fixture is needed and it should not be made in a way that the initialization is called again... hard to get the context and error prone as missing might make other test cases broken... Not sure, might not be fixed here but would be a good issue in another PR to improve. |
||
|
|
||
| @conf_vars({("core", "load_examples"): "true"}) | ||
| def test_cli_report(self, stdout_capture): | ||
| args = self.parser.parse_args(["dags", "report", "--output", "json"]) | ||
| with stdout_capture as temp_stdout: | ||
|
|
@@ -228,7 +228,6 @@ def test_cli_report(self, stdout_capture): | |
| assert "airflow/example_dags/example_complex.py" in out | ||
| assert "example_complex" in out | ||
|
|
||
| @conf_vars({("core", "load_examples"): "true"}) | ||
| def test_cli_get_dag_details(self, stdout_capture): | ||
| args = self.parser.parse_args(["dags", "details", "example_complex", "--output", "yaml"]) | ||
| with stdout_capture as temp_stdout: | ||
|
|
@@ -245,7 +244,6 @@ def test_cli_get_dag_details(self, stdout_capture): | |
| for value in dag_details_values: | ||
| assert value in out | ||
|
|
||
| @conf_vars({("core", "load_examples"): "true"}) | ||
| def test_cli_list_dags(self, stdout_capture): | ||
| args = self.parser.parse_args(["dags", "list", "--output", "json"]) | ||
| with stdout_capture as temp_stdout: | ||
|
|
@@ -256,11 +254,12 @@ def test_cli_list_dags(self, stdout_capture): | |
| assert key in dag_list[0] | ||
| assert any("airflow/example_dags/example_complex.py" in d["fileloc"] for d in dag_list) | ||
|
|
||
| @conf_vars({("core", "load_examples"): "true"}) | ||
| def test_cli_list_local_dags(self, stdout_capture): | ||
| # Clear the database | ||
| clear_db_dags() | ||
| args = self.parser.parse_args(["dags", "list", "--output", "json", "--local"]) | ||
| args = self.parser.parse_args( | ||
| ["dags", "list", "--output", "json", "--local", "--bundle-name", "example_dags"] | ||
| ) | ||
| with stdout_capture as temp_stdout: | ||
| dag_command.dag_list_dags(args) | ||
| out = temp_stdout.getvalue() | ||
|
|
@@ -269,7 +268,7 @@ def test_cli_list_local_dags(self, stdout_capture): | |
| assert key in dag_list[0] | ||
| assert any("airflow/example_dags/example_complex.py" in d["fileloc"] for d in dag_list) | ||
| # Rebuild Test DB for other tests | ||
| parse_and_sync_to_db(os.devnull, include_examples=True) | ||
| self.setup_class() | ||
|
|
||
| @conf_vars({("core", "load_examples"): "false"}) | ||
| def test_cli_list_local_dags_with_bundle_name(self, configure_testing_dag_bundle, stdout_capture): | ||
|
|
@@ -290,9 +289,8 @@ def test_cli_list_local_dags_with_bundle_name(self, configure_testing_dag_bundle | |
| str(TEST_DAGS_FOLDER / "test_example_bash_operator.py") in d["fileloc"] for d in dag_list | ||
| ) | ||
| # Rebuild Test DB for other tests | ||
| parse_and_sync_to_db(os.devnull, include_examples=True) | ||
| self.setup_class() | ||
|
|
||
| @conf_vars({("core", "load_examples"): "true"}) | ||
| def test_cli_list_dags_custom_cols(self, stdout_capture): | ||
| args = self.parser.parse_args( | ||
| ["dags", "list", "--output", "json", "--columns", "dag_id,last_parsed_time"] | ||
|
|
@@ -306,7 +304,6 @@ def test_cli_list_dags_custom_cols(self, stdout_capture): | |
| for key in ["fileloc", "owners", "is_paused"]: | ||
| assert key not in dag_list[0] | ||
|
|
||
| @conf_vars({("core", "load_examples"): "true"}) | ||
| def test_cli_list_dags_invalid_cols(self, stderr_capture): | ||
| args = self.parser.parse_args(["dags", "list", "--output", "json", "--columns", "dag_id,invalid_col"]) | ||
| with stderr_capture as temp_stderr: | ||
|
|
@@ -350,9 +347,8 @@ def test_cli_list_dags_prints_local_import_errors( | |
|
|
||
| assert "Failed to load all files." in out | ||
| # Rebuild Test DB for other tests | ||
| parse_and_sync_to_db(os.devnull, include_examples=True) | ||
| self.setup_class() | ||
|
|
||
| @conf_vars({("core", "load_examples"): "true"}) | ||
| @mock.patch("airflow.models.DagModel.get_dagmodel") | ||
| def test_list_dags_none_get_dagmodel(self, mock_get_dagmodel, stdout_capture): | ||
| mock_get_dagmodel.return_value = None | ||
|
|
@@ -365,7 +361,6 @@ def test_list_dags_none_get_dagmodel(self, mock_get_dagmodel, stdout_capture): | |
| assert key in dag_list[0] | ||
| assert any("airflow/example_dags/example_complex.py" in d["fileloc"] for d in dag_list) | ||
|
|
||
| @conf_vars({("core", "load_examples"): "true"}) | ||
| def test_dagbag_dag_col(self, session): | ||
| dagbag = DBDagBag() | ||
| dag_details = dag_command._get_dagbag_dag_details( | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -349,7 +349,8 @@ def test_task_state(self): | |
| ) | ||
|
|
||
| def test_task_states_for_dag_run(self): | ||
| dag2 = DagBag().dags["example_python_operator"] | ||
| from airflow.providers.standard.example_dags.example_python_operator import dag as dag2 | ||
|
Contributor
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. That looks OK but is a hard wiring from core to standard provider. Have no better idea but smells a bit "not good". Might we need a fixture on a general level allowing dags to load w/o tainting individual tests with hard-wired dependencies? |
||
|
|
||
| lazy_deserialized_dag2 = LazyDeserializedDAG.from_dag(dag2) | ||
|
|
||
| SerializedDagModel.write_dag(lazy_deserialized_dag2, bundle_name="testing") | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -28,6 +28,7 @@ | |
| import zipfile | ||
| from copy import deepcopy | ||
| from datetime import datetime, timedelta, timezone | ||
| from pathlib import Path | ||
| from unittest import mock | ||
| from unittest.mock import patch | ||
|
|
||
|
|
@@ -41,6 +42,7 @@ | |
| from airflow.models.dag import DagModel | ||
| from airflow.models.dagwarning import DagWarning, DagWarningType | ||
| from airflow.models.serialized_dag import SerializedDagModel | ||
| from airflow.providers.standard import example_dags as standard_example_dags | ||
|
Contributor
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. Similar as above but a bit pmore problematic - if we make this a global import the python module loader needs to have the dependency available at moment of collecting tests already. Can you make this a local import of a small utility function in the test or better to be a fixture that loads it at test runtime to prevent a file level import? (Alongside with code in line 56 and then use it in line 216 and 482 etc) |
||
| from airflow.sdk import DAG, BaseOperator | ||
|
|
||
| from tests_common.pytest_plugin import AIRFLOW_ROOT_PATH | ||
|
|
@@ -51,7 +53,7 @@ | |
|
|
||
| pytestmark = pytest.mark.db_test | ||
|
|
||
| example_dags_folder = AIRFLOW_ROOT_PATH / "airflow-core" / "src" / "airflow" / "example_dags" / "standard" | ||
| standard_example_dags_folder = Path(standard_example_dags.__file__).parent | ||
|
|
||
| PY311 = sys.version_info >= (3, 11) | ||
| PY313 = sys.version_info >= (3, 13) | ||
|
|
@@ -211,9 +213,9 @@ def test_get_existing_dag(self, tmp_path): | |
| """ | ||
| Test that we're able to parse some example DAGs and retrieve them | ||
| """ | ||
| dagbag = DagBag(dag_folder=os.fspath(tmp_path), include_examples=True) | ||
| dagbag = DagBag(dag_folder=standard_example_dags_folder, include_examples=False) | ||
|
|
||
| some_expected_dag_ids = ["example_bash_operator", "example_branch_operator"] | ||
| some_expected_dag_ids = ["example_bash_operator", "example_python_operator"] | ||
|
|
||
| for dag_id in some_expected_dag_ids: | ||
| dag = dagbag.get_dag(dag_id) | ||
|
|
@@ -477,7 +479,7 @@ def process_file(self, filepath, only_if_updated=True, safe_mode=True): | |
| _TestDagBag.process_file_calls += 1 | ||
| super().process_file(filepath, only_if_updated, safe_mode) | ||
|
|
||
| dagbag = _TestDagBag(include_examples=True) | ||
| dagbag = _TestDagBag(dag_folder=standard_example_dags_folder) | ||
| dagbag.process_file_calls | ||
|
|
||
| # Should not call process_file again, since it's already loaded during init. | ||
|
|
@@ -489,9 +491,9 @@ def process_file(self, filepath, only_if_updated=True, safe_mode=True): | |
| ("file_to_load", "expected"), | ||
| ( | ||
| pytest.param( | ||
| pathlib.Path(example_dags_folder) / "example_bash_operator.py", | ||
| pathlib.Path(standard_example_dags_folder) / "example_bash_operator.py", | ||
| { | ||
| "example_bash_operator": f"{example_dags_folder.relative_to(AIRFLOW_ROOT_PATH) / 'example_bash_operator.py'}" | ||
| "example_bash_operator": f"{standard_example_dags_folder.relative_to(AIRFLOW_ROOT_PATH) / 'example_bash_operator.py'}" | ||
| }, | ||
| id="example_bash_operator", | ||
| ), | ||
|
|
@@ -553,7 +555,7 @@ def test_refresh_py_dag(self, mock_dagmodel, tmp_path): | |
| Test that we can refresh an ordinary .py DAG | ||
| """ | ||
| dag_id = "example_bash_operator" | ||
| fileloc = str(example_dags_folder / "example_bash_operator.py") | ||
| fileloc = str(standard_example_dags_folder / "example_bash_operator.py") | ||
|
|
||
| mock_dagmodel.return_value = DagModel() | ||
| mock_dagmodel.return_value.last_expired = datetime.max.replace(tzinfo=timezone.utc) | ||
|
|
@@ -567,7 +569,7 @@ def process_file(self, filepath, only_if_updated=True, safe_mode=True): | |
| _TestDagBag.process_file_calls += 1 | ||
| return super().process_file(filepath, only_if_updated, safe_mode) | ||
|
|
||
| dagbag = _TestDagBag(dag_folder=os.fspath(tmp_path), include_examples=True) | ||
| dagbag = _TestDagBag(dag_folder=standard_example_dags_folder, include_examples=False) | ||
|
|
||
| assert dagbag.process_file_calls == 1 | ||
| dag = dagbag.get_dag(dag_id) | ||
|
|
||
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.
THat is a good approach but I assume we need to make a lookup to all providers via plugins_manager. There could also be (in theory) providers not coming from Apache Airflow and they might reside in other hierarchy outside of "airflow" package
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.
Also some providers are nested one level down, like "common/sql" - so most probably via providers_manager we can discover the path/module name where stored... and if not we might need to extend provider.yaml to explicitly mark if examples are available.