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

Raise error when DagRun fails while running dag test #36517

Merged
merged 2 commits into from
Jan 2, 2024

Conversation

kaxil
Copy link
Member

@kaxil kaxil commented Dec 31, 2023

Motivation:

Currently, when using airflow dags test, there is no easy way to know programmatically if a DagRun fails since the state is not stored in DB. The way to do know relies on log lines as below:

state=$(airflow dags test exception_dag | grep "DagRun Finished" | awk -F, '{for(i=1;i<=NF;i++) if ($i ~ / state=/) print $i}' | awk -F= '{print $2}') if [[ $state == "failed" ]]; then exit 1 else exit 0 fi

This PR will return an exit code 1 when airflow dags test command if DagRun fails and makes it easy to integrate in CI for testing.

@kaxil kaxil added this to the Airflow 2.8.1 milestone Dec 31, 2023
@kaxil kaxil force-pushed the raise-error-dag-test branch 2 times, most recently from 8919958 to 3df3eba Compare December 31, 2023 17:05
Copy link
Contributor

@jscheffl jscheffl left a comment

Choose a reason for hiding this comment

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

I am a bit wondering why we add another CLI parameter. If the DAG test fails and exit code 0 is returned I'd rather see this as a bug.

Is there a reason adding a new CLI param? Please convince me. Else it would even be leaner w/o additional parameter

@jscheffl jscheffl added the type:bug-fix Changelog: Bug Fixes label Dec 31, 2023
@dirrao
Copy link
Contributor

dirrao commented Jan 1, 2024

I am a bit wondering why we add another CLI parameter. If the DAG test fails and exit code 0 is returned I'd rather see this as a bug.

Is there a reason adding a new CLI param? Please convince me. Else it would even be leaner w/o additional parameter

I agree with @jscheffl. The default behavior of the cli command should be the status of the DAG run/runs.

@potiuk
Copy link
Member

potiuk commented Jan 1, 2024

Once concern is backwards compatibility (and I think that's where @kaxil's solution came from).

But yeah, I think you are right @jscheffl @dirrao - probably return value is not teally compatibility promise (and we can treat it as bugfix). We've already done the same in #35378 (and It was me who proposed skipping the flag and treating it as bugfix :D ... Now I remember .

@jscheffl
Copy link
Contributor

jscheffl commented Jan 1, 2024

Once concern is backwards compatibility (and I think that's where @kaxil's solution came from).

But yeah, I think you are right @jscheffl @dirrao - probably return value is not teally compatibility promise (and we can treat it as bugfix). We've already done the same in #35378 (and It was me who proposed skipping the flag and treating it as bugfix :D ... Now I remember .

So if we had it two times already would be even a good "quality excercise" to double check that all CLI commands exit with a proper error code in case of error :-D

@kaxil
Copy link
Member Author

kaxil commented Jan 2, 2024

I am a bit wondering why we add another CLI parameter. If the DAG test fails and exit code 0 is returned I'd rather see this as a bug.

Yeah I tried to think more on it to keep backwards-compat but I think we are ok -- will remove the param and make it the default behavior

**Motivation**:

Currently, when using `airflow dags test`, there is no easy way to know programmatically if a DagRun fails since the state is not stored in DB. The way to do know relies on log lines as below:

```bash
state=$(airflow dags test exception_dag | grep "DagRun Finished" | awk -F, '{for(i=1;i<=NF;i++) if ($i ~ / state=/) print $i}' | awk -F= '{print $2}') if [[ $state == "failed" ]]; then exit 1 else exit 0 fi
```

This PR adds `--fail-on-dagrun-failure` flag to `airflow dags test` command which will return an exit code of 1 if DagRun fails and makes it easy to integrate in CI for testing.
@kaxil
Copy link
Member Author

kaxil commented Jan 2, 2024

@jscheffl Done

Copy link
Contributor

@jscheffl jscheffl left a comment

Choose a reason for hiding this comment

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

Thanks for the fix!

@kaxil kaxil merged commit 383ad31 into apache:main Jan 2, 2024
50 of 51 checks passed
@kaxil kaxil deleted the raise-error-dag-test branch January 2, 2024 10:38
@kaxil kaxil changed the title Allow raising error if DagRun fails when running dag test Raise error when DagRun fails while running dag test Jan 2, 2024
@kaxil
Copy link
Member Author

kaxil commented Jan 2, 2024

The failed test is unrelated to the PR and succeeds locally:

=========================== short test summary info ============================
FAILED tests/cli/test_cli_parser.py::TestCliSubprocess::test_airflow_config_contains_providers - AssertionError: assert 'celery_config_options' in ''
 +  where '' = <bound method Path.read_text of PosixPath('/root/airflow/airflow.cfg')>()
 +    where <bound method Path.read_text of PosixPath('/root/airflow/airflow.cfg')> = PosixPath('/root/airflow/airflow.cfg').read_text
===== 1 failed, 304 passed, 1050 skipped, 1 xfailed, 9 warnings in 18.30s ======

ephraimbuddy pushed a commit that referenced this pull request Jan 11, 2024
**Motivation**:

Currently, when using `airflow dags test`, there is no easy way to know programmatically if a DagRun fails since the state is not stored in DB. The way to do know relies on log lines as below:

```bash
state=$(airflow dags test exception_dag | grep "DagRun Finished" | awk -F, '{for(i=1;i<=NF;i++) if ($i ~ / state=/) print $i}' | awk -F= '{print $2}') if [[ $state == "failed" ]]; then exit 1 else exit 0 fi
```

This PR adds will return an exit code 1 when `airflow dags test` command if DagRun fails and makes it easy to integrate in CI for testing.

(cherry picked from commit 383ad31)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:CLI type:bug-fix Changelog: Bug Fixes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants