-
Notifications
You must be signed in to change notification settings - Fork 16.4k
Fix inconsistent returned value of airflow dags next-execution cli command
#30117
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
Fix inconsistent returned value of airflow dags next-execution cli command
#30117
Conversation
|
Congratulations on your first Pull Request and welcome to the Apache Airflow community! If you have any issues or are unsure about any anything please check our Contribution Guide (https://github.com/apache/airflow/blob/main/CONTRIBUTING.rst)
|
airflow/cli/commands/dag_command.py
Outdated
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.
Hmm, I think I prefer the previous range(1, num) way.
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 the range(1, num) way would require handling and printing None in 2 different places. 1 before the loop and 1 more within the for loop itself. I just think the handling all those cases within the loop would be more elegant. But if you insist ... 😅
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.
Personally I feel handling in two places is actually easier than having to check for 0 every time. If you really hate code duplication, you can define a local function to reuse the logic. But checking for the index in the loop almost always makes things unnecessarily complex.
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.
Right, will do!
…command This commit is to fix the inconsistent returned value of `airflow dags next-execution` cli command when the dag is paused and catchup is False
|
@uranusjr Just updated the PR's description. This PR is ready for review 🙇 |
huymq1710
left a comment
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.
LGTM
|
Awesome work, congrats on your first merged pull request! |
…command (#30117) * Fix inconsistent returned value of `airflow dags next-execution` cli command This commit is to fix the inconsistent returned value of `airflow dags next-execution` cli command when the dag is paused and catchup is False --------- Co-authored-by: Bui Duc Phong[ Bui Duc Phong ] <bui.duc.phong@linecorp.com> (cherry picked from commit c63836c)
…command (#30117) * Fix inconsistent returned value of `airflow dags next-execution` cli command This commit is to fix the inconsistent returned value of `airflow dags next-execution` cli command when the dag is paused and catchup is False --------- Co-authored-by: Bui Duc Phong[ Bui Duc Phong ] <bui.duc.phong@linecorp.com> (cherry picked from commit c63836c)
closes: #22474
airflow dags next-executioncli command when the dag is paused and catchup is FalseIn brief:
DagRunmodel. However, when the dag is paused, getting the next-execution run from theDagRunmodel would yield the next dag run time from the last executed dag run, which is incorrect as time passes.dag_model.next_dag_run(as in: dag.html L143) to make it more consistent with the UI.Modification:
DagModelto get a dag next execution info. This way we could:catchup=Falsecase and when the Dag is paused.Nonewhenargs.num_executionsis specified. Previous code will throw an error as follow: None type has no attribute logical_date ...This is my first PR, please let me know if there is anything I could do differently.