Skip to content
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

Build DagFactory DAGs when there is an invalid YAML in the DAGs folder #184

Merged
merged 8 commits into from
Oct 11, 2024

Conversation

quydx
Copy link
Contributor

@quydx quydx commented Mar 12, 2024

Before, when the Airflow DAGs folder had an invalid YAML file, no DagFactory DAGs would be loaded. This PR changes this behaviour to log any invalid YAML file paths but render valid DagFactory YAML-based DAGs.

Co-authored-by: Tatiana Al-Chueyr [email protected]

dagfactory/dagfactory.py Outdated Show resolved Hide resolved
@tatiana tatiana self-assigned this Aug 27, 2024
@tatiana
Copy link
Collaborator

tatiana commented Aug 27, 2024

@quy196hp Thank you very much for your contribution, and I am sorry for the delay in giving feedback.

Please, could you address the linting issue, so the checks pass?

==> Formatting with Black
would reformat /home/runner/work/dag-factory/dag-factory/dagfactory/dagfactory.py

Oh no! 💥 💔 💥
1 file would be reformatted, 5 files would be left unchanged.
make: *** [Makefile:38: fmt-check] Error 1

@phanikumv phanikumv added this to the DAG Factory 0.20.0 milestone Sep 6, 2024
dagfactory/dagfactory.py Outdated Show resolved Hide resolved
dagfactory/dagfactory.py Outdated Show resolved Hide resolved
dagfactory/dagfactory.py Outdated Show resolved Hide resolved
@kaxil kaxil force-pushed the master branch 2 times, most recently from 6e3ebdf to 60294b0 Compare September 7, 2024 20:02
Copy link
Collaborator

@kaxil kaxil left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The test needs fixing too:

  =========================== short test summary info ============================
  FAILED tests/test_dagfactory.py::test_load_yaml_dags_fail - Failed: DID NOT RAISE <class 'Exception'>
  ========================= 1 failed, 79 passed in 2.26s =========================

  ___________________________ test_load_yaml_dags_fail ___________________________
  
      def test_load_yaml_dags_fail():
          with pytest.raises(Exception):
  >           load_yaml_dags(
                  globals_dict=globals(),
                  dags_folder="tests/fixtures",
                  suffix=["invalid_yaml.yml"],
              )
  E           Failed: DID NOT RAISE <class 'Exception'>

@quydx
Copy link
Contributor Author

quydx commented Sep 12, 2024

Hi, sorrry for late response. About the failed test case, I see the test checks if there is an exception raises when loading invalid yaml files, but this PR seems to prevent the exception so can we remove this test case test_load_yaml_dags_fail @tatiana @kaxil ?

@tatiana
Copy link
Collaborator

tatiana commented Oct 10, 2024

@quydx I added tests in 90487b0 and e9a3c72. They are based on the PR #236

The idea is not to delete the test but to check if we're logging the error message as expected so that this type of error isn't hidden.

Additionally, I thought it was helpful to add an invalid yml to the /examples folder and guarantee that if we run airflow dags list-import-errors using that folder as the dags folder, the valid YMLs and DAGs would continue being imported without errors.

With these, the feature is fully covered with tests and we can guarantee the new behaviour is as intended.

@tatiana tatiana changed the title Handle error when load crash yaml Load DagFactory DAGs even when there is an invalid YAML in the DAGs folder Oct 11, 2024
@tatiana tatiana changed the title Load DagFactory DAGs even when there is an invalid YAML in the DAGs folder Load DagFactory DAGs when there is an invalid YAML in the DAGs folder Oct 11, 2024
@tatiana tatiana changed the title Load DagFactory DAGs when there is an invalid YAML in the DAGs folder Build DagFactory DAGs when there is an invalid YAML in the DAGs folder Oct 11, 2024
@tatiana tatiana merged commit 79cde23 into astronomer:main Oct 11, 2024
7 checks passed
@quydx
Copy link
Contributor Author

quydx commented Oct 11, 2024

Greate idea @tatiana , it seems more comprehensive way to handle this case. Things are good now

@tatiana tatiana mentioned this pull request Oct 17, 2024
tatiana added a commit that referenced this pull request Oct 22, 2024
### Added
- Support using envvar in config YAML by @tatiana in #236
- **Callback improvements**
- Support installed code via python callable string by @john-drews in
#221
- Add `callback_file` & `callback_name` to `default_args` DAG level by
@subbota19 in #218
- Cast callbacks to functions when set with `default_args` on TaskGroups
by @Baraldo and @pankajastro in #235

- **Telemetry**
- For more information, please, read the [Privacy
Notice](https://github.com/astronomer/dag-factory/blob/main/PRIVACY_NOTICE.md#collection-of-data).
  - Add scarf to readme for website analytics by @cmarteepants in #219
- Support telemetry during DAG parsing emitting data to Scarf by
@tatiana in #250.

### Fixed
- Build DAGs when tehre is an invalid YAML in the DAGs folder by @quydx
and @tatiana in #184

### Others
- Development tools
  - Fix make docker-run by @pankajkoti in #249
  - Add vim dot files to .gitignore by @tatiana in #228
  - Use Hatchling to modern package building by @kaxil in #208
- CI
  - Fix static check failures in PR #218 by @pankajkoti in #251
  - Fix pre-commit checks by @tatiana in #247
  - Remove tox and corresponding build jobs in CI by @pankajkoti in #248
- Install Airflow with different versions in the CI by @pankajkoti in
#237
  - Run pre-commit hooks on all existing files by @pankajkoti in #245
  - Add Python 3.11 and 3.12 to CI test pipeline by @pankajkoti in #229
- Tests
  - Fix duplicate test name by @pankajastro in #234
  - Add static check by @pankajastro in #231
  - Fix running tests locally (outside the CI) by @tatiana in #227
  - Add the task_2 back to dataset example by @cmarteepants in #204
  - Remove unnecessary config line by @jlaneve in #202
- Documentation
  - Update the license from MIT to Apache 2.0 by @pankajastro in #191
- Add registration icon and links to Airflow references by @cmarteepants
in #190
  - Update quickstart and add feature examples by @cmarteepants #189

### Breaking changes
- Removed support for Python 3.7
- The license was changed from MIT to Apache 2.0

Closes: #217
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants