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

Show import error for 'airflow dags list' CLI command #21991

Merged
merged 6 commits into from
Mar 8, 2022

Conversation

ephraimbuddy
Copy link
Contributor

When there's an import error in a dag, the dag doesn't show up in the list of dags
and there's no indication that there's an import error.
This PR fixes that

airflow/cli/commands/dag_command.py Outdated Show resolved Hide resolved
airflow/cli/commands/dag_command.py Outdated Show resolved Hide resolved
@mik-laj
Copy link
Member

mik-laj commented Mar 4, 2022

Won't that cause problem for scripts that process the output of this command? I think we should display import errors on stderr.

@mik-laj
Copy link
Member

mik-laj commented Mar 4, 2022

Related PR: #15087

@ephraimbuddy ephraimbuddy force-pushed the dag-list-import-error branch from 2424a13 to d0ad999 Compare March 7, 2022 20:34
@ephraimbuddy
Copy link
Contributor Author

Related PR: #15087

What's your opinion about having --show-import-error option? I think printing the error in stderr without any option is OK.
Happy to hear what you think then I would work on it and add tests

@mik-laj
Copy link
Member

mik-laj commented Mar 8, 2022

@ephraimbuddy, For me, displaying an error on stderr without any options is also OK. In the above-mentioned PR, I proposed to add a separate command as this would allow us to write scripts that process these import errors, but we can do it as a separate contribution.

@ephraimbuddy
Copy link
Contributor Author

@mik-laj I have created a separate command for import errors(PR: #22084). Would wait for that to be merged and update this PR to just say that we have import errors and ask the users to run airflow dags list-import-errors to see the errors. WDYT?

ephraimbuddy and others added 4 commits March 8, 2022 20:12
When there's an import error in a dag, the dag doesn't show up in the list of dags
and there's no indication that there's an import error.
This PR fixes that
airflow/cli/commands/dag_command.py Outdated Show resolved Hide resolved
@github-actions github-actions bot added the okay to merge It's ok to merge this PR as it does not require more tests label Mar 8, 2022
@github-actions
Copy link

github-actions bot commented Mar 8, 2022

The PR is likely OK to be merged with just subset of tests for default Python and Database versions without running the full matrix of tests, because it does not modify the core of Airflow. If the committers decide that the full tests matrix is needed, they will add the label 'full tests needed'. Then you should rebase to the latest main or amend the last commit of the PR, and push it with --force-with-lease.

@ephraimbuddy ephraimbuddy merged commit 8482b69 into apache:main Mar 8, 2022
@ephraimbuddy ephraimbuddy deleted the dag-list-import-error branch March 8, 2022 23:43
@ephraimbuddy ephraimbuddy added the type:improvement Changelog: Improvements label Apr 8, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:CLI okay to merge It's ok to merge this PR as it does not require more tests type:improvement Changelog: Improvements
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants