-
Notifications
You must be signed in to change notification settings - Fork 15.9k
Fix custom timetable generate_run_id not called for manual triggers #56373
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
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 Contributors' Guide (https://github.com/apache/airflow/blob/main/contributing-docs/README.rst)
|
7d7c5b2 to
dbefee1
Compare
|
A little bump on this. Would love to get this in for 3.1.1 @kaxil |
pierrejeambrun
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 but I'll wait for another pair of eyes.
|
CI need fixing. |
They seem unrelated to my changes. I rebased but it didn't seem to retrigger ci |
In Airflow 3.x, commit 035060d (PR apache#46616) changed the trigger_dag module to use DagRun.generate_run_id() instead of dag.timetable.generate_run_id() for manual DAG runs. This bypassed custom timetable logic, causing a regression from Airflow 2.x behavior. This commit restores the use of dag.timetable.generate_run_id() for manual triggers, allowing custom timetables to control run_id generation for both scheduled and manually triggered runs. Changes: - airflow/api/common/trigger_dag.py: Changed to call dag.timetable.generate_run_id() with run_after and data_interval parameters instead of DagRun.generate_run_id() - tests/api_fastapi/core_api/routes/public/test_dag_run.py: Added test to verify custom timetable generate_run_id is called for manual triggers with both logical_date provided and null This pattern matches how manual triggers are handled in other parts of the codebase (e.g., assets.py, scheduler_job_runner.py). Fixes: apache#55908
13f3c50 to
5d7eef9
Compare
|
I just rebased the PR, lets see if the CI is happy now. |
|
static check need fixing, this is related to your code change. |
Sorry I misread the position of the ruff check. I have fixed it now. |
|
Need to fix test, otherwise this makes sense to me. |
|
When fixing the tests I realized I also needed to fix this in the fastapi endpoint. |
|
Awesome work, congrats on your first merged pull request! You are invited to check our Issue Tracker for additional contributions. |
…l triggers (#56373) (cherry picked from commit c28b211) Co-authored-by: Nils Werner <[email protected]>
…l triggers (apache#56373) (cherry picked from commit c28b211) Co-authored-by: Nils Werner <[email protected]>
…l triggers (#56373) (#56699) (cherry picked from commit c28b211) Co-authored-by: Nils Werner <[email protected]>
In Airflow 3.x, commit 035060d (PR #46616) changed the trigger_dag module to use DagRun.generate_run_id() instead of
dag.timetable.generate_run_id() for manual DAG runs. This bypassed custom timetable logic, causing a regression from Airflow 2.x behavior.
This commit restores the use of dag.timetable.generate_run_id() for manual triggers, allowing custom timetables to control run_id generation for both scheduled and manually triggered runs.
Changes:
This pattern matches how manual triggers are handled in other parts of the codebase (e.g., assets.py, scheduler_job_runner.py).
Fixes: #55908
^ Add meaningful description above
Read the Pull Request Guidelines for more information.
In case of fundamental code changes, an Airflow Improvement Proposal (AIP) is needed.
In case of a new dependency, check compliance with the ASF 3rd Party License Policy.
In case of backwards incompatible changes please leave a note in a newsfragment file, named
{pr_number}.significant.rstor{issue_number}.significant.rst, in airflow-core/newsfragments.