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

Make run_after optional as part of API for Trigger Dag Run #46967

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

sunank200
Copy link
Collaborator

Currently run_after is defaulted to timezone.utcnow() in API. Now it is changed to optional with values - data_interval_end or utcnow() in the logic.

closes: #46650


^ 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.rst or {issue_number}.significant.rst, in newsfragments.

Copy link
Collaborator

@bugraoz93 bugraoz93 left a comment

Choose a reason for hiding this comment

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

Looks good!

@sunank200 sunank200 force-pushed the null-run-after-api branch 2 times, most recently from 4d42d9a to 70e9ec7 Compare February 21, 2025 18:33
Copy link
Member

@pierrejeambrun pierrejeambrun left a comment

Choose a reason for hiding this comment

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

I'll just mark it as 'request change' so we don't merge this by mistake because this will cause troubles on the UI in its current state.

The UI needs a non nullable date to work. before it was logical_date and a lot of things broke when we did the AIP-83 amendment.

To resolve that we agreed on using the run_after date because it's not supposed to be null. This will bring trouble on the UI side if this change go through, is that required, should we use another non nullable date for the UI ?

@bbovenzi
Copy link
Contributor

Can you update the title and description of this PR to say its for Trigger Dag Run? I was worried we were making it nullable everywhere for a second.

@pierrejeambrun
Copy link
Member

pierrejeambrun commented Feb 21, 2025

Can you update the title and description of this PR to say its for Trigger Dag Run? I was worried we were making it nullable everywhere for a second.

I got confused too, disregard my previous comment, that's just for the Trigger API, looks good thanks.

Copy link
Member

@pierrejeambrun pierrejeambrun left a comment

Choose a reason for hiding this comment

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

Looks good.

@sunank200 sunank200 changed the title Make run_after optional as part of API Make run_after optional as part of API for Trigger Dag Run Feb 21, 2025
@sunank200
Copy link
Collaborator Author

Can you update the title and description of this PR to say its for Trigger Dag Run? I was worried we were making it nullable everywhere for a second.

Changed it. I should have been more clear in my title.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:API Airflow's REST/HTTP API area:UI Related to UI/UX. For Frontend Developers.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Make run_after non public and move validation logic for the payloas to the Serializer
4 participants