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.

Sign up for GitHub

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

Jump to bottom

Consolidate to one schedule param #25410

Merged
merged 38 commits into from
Aug 10, 2022
Merged

Consolidate to one schedule param #25410

merged 38 commits into from
Aug 10, 2022

Conversation

dstandish
Copy link
Contributor

Consolidate the three scheduling params to one param schedule

@dstandish dstandish changed the title One true schedule param Consolidate to one schedule param Jul 29, 2022
@dstandish dstandish force-pushed the one-true-schedule-param branch from f032e8c to 9bfff65 Compare July 29, 2022 20:54
@dstandish dstandish force-pushed the one-true-schedule-param branch from 9bfff65 to ba7c23a Compare August 6, 2022 19:31
@dstandish dstandish marked this pull request as ready for review August 7, 2022 19:54
@dstandish dstandish force-pushed the one-true-schedule-param branch from ac75dcc to a35d59e Compare August 7, 2022 20:38
@uranusjr
Copy link
Member

uranusjr commented Aug 8, 2022

Wow, 356 files changed. Would it be a good idea to split out the part that consolidates arguement handling, and change the examples and documentation in a later PR?

@potiuk
Copy link
Member

potiuk commented Aug 8, 2022 •
edited
Loading

Wow, 356 files changed. Would it be a good idea to split out the part that consolidates arguement handling, and change the examples and documentation in a later PR?

I'd prefer to keep those together, even if it's a big change. There is a little value in splitting them, Especially with the new Github Review experience with "change file tree" it's easy to separate out the "code" from "documentation changes during review - and keeping docs and code changes in the same PR makes it easier to cherry-pick and reason about.

@potiuk
Copy link
Member

potiuk commented Aug 8, 2022

image

@potiuk
Copy link
Member

potiuk commented Aug 8, 2022

@dstandish - the failing test in "providers 2.2" indicates backwards-incompatible changes in providers I am afraid.

@dstandish
Copy link
Contributor Author

@uranusjr @potiuk re separation... yeah since it would just be splitting out files, rather than e.g. steps in series of changes, not sure it would help review all that much. but also... the main reason i included example dags is because in past at least we had CI tests that checked for warnings, and every example dag would produce a deprecation warning if i did not update them.

@dstandish
Copy link
Contributor Author

er i guess it could help just because in a big "example dags" PR you don't really need to scrutinize anything as carefully, and in a "core" change PR you would know to look at everything 🤷

@dstandish
Copy link
Contributor Author

@dstandish - the failing test in "providers 2.2" indicates backwards-incompatible changes in providers I am afraid.

Ah... yes... this makes sense.... because... airflow 2.2 won't have new schedule param.... will have to come up with something for backcompat in the example dags stored in providers.

@dstandish dstandish requested a review from mik-laj as a code owner August 8, 2022 19:58
@dstandish dstandish force-pushed the one-true-schedule-param branch from 423c682 to 3106e5a Compare August 8, 2022 20:02
jedcunningham
jedcunningham reviewed Aug 8, 2022
View reviewed changes
docs/apache-airflow/dag-run.rst Outdated Show resolved Hide resolved
docs/apache-airflow/dag-run.rst Outdated Show resolved Hide resolved
docs/apache-airflow/faq.rst Outdated Show resolved Hide resolved
docs/apache-airflow/faq.rst Outdated Show resolved Hide resolved
newsfragments/25410.significant.rst Outdated Show resolved Hide resolved
scripts/in_container/verify_providers.py Outdated Show resolved Hide resolved
@dstandish dstandish force-pushed the one-true-schedule-param branch from 76e84ff to b74ca05 Compare August 9, 2022 00:20
uranusjr
uranusjr reviewed Aug 9, 2022
View reviewed changes
newsfragments/25410.significant.rst Outdated Show resolved Hide resolved
newsfragments/25410.significant.rst Outdated Show resolved Hide resolved
newsfragments/25410.significant.rst Outdated Show resolved Hide resolved
newsfragments/25410.significant.rst Outdated Show resolved Hide resolved
newsfragments/25410.significant.rst Outdated Show resolved Hide resolved
newsfragments/25410.significant.rst Outdated Show resolved Hide resolved
dstandish and others added 20 commits August 10, 2022 09:18
@dstandish
remove schedule_interval param from example dags
b32a0aa
@dstandish
fixup! remove schedule_interval param from example dags
c1d0c34
@dstandish
fix test
7bc7f76
@dstandish @jedcunningham
Update scripts/in_container/verify_providers.py
721881e 
Co-authored-by: Jed Cunningham <66968678+jedcunningham@users.noreply.github.com>
@dstandish @jedcunningham
Update docs/apache-airflow/faq.rst
6d00cef 
Co-authored-by: Jed Cunningham <66968678+jedcunningham@users.noreply.github.com>
@dstandish @jedcunningham
Update docs/apache-airflow/dag-run.rst
fd808bd 
Co-authored-by: Jed Cunningham <66968678+jedcunningham@users.noreply.github.com>
@dstandish @jedcunningham
Update docs/apache-airflow/dag-run.rst
fd3d9e9 
Co-authored-by: Jed Cunningham <66968678+jedcunningham@users.noreply.github.com>
@dstandish
add example
b49c39b
@dstandish
simplify example
2509e8e
@dstandish @jedcunningham
Update newsfragments/25410.significant.rst
17531f8 
Co-authored-by: Jed Cunningham <66968678+jedcunningham@users.noreply.github.com>
@dstandish
add test
8e77857
@uranusjr @dstandish
Simplify and fix test
46ee72c
@dstandish
uppercase
babf5c5
@dstandish
fix log testing
7858b38
@uranusjr @dstandish
Unify comment format
e5e50a3
@uranusjr @dstandish
Fix casing an wording
eaca727 
Most significantly, use "argument" instead of "param" or "parameter"
where applicable, to avoid possible confusion, since we already have a
DAG argument called "params".
@uranusjr @dstandish
More casing and wording fixes
b168c2f
@uranusjr @dstandish
A few more formatting fixes
d0559ac
@uranusjr @dstandish
Some more TaskDag conversions
86238c6
@dstandish
remove redundant param
6f0003b
@dstandish dstandish force-pushed the one-true-schedule-param branch from c1ad946 to 6f0003b Compare August 10, 2022 16:20
@dstandish dstandish merged commit 1b412c9 into apache:main Aug 10, 2022
@dstandish dstandish deleted the one-true-schedule-param branch August 10, 2022 17:12
@ephraimbuddy ephraimbuddy added the type:new-feature Changelog: New Features label Aug 15, 2022
This was referenced Aug 23, 2022
Merged
Merged
@jedcunningham jedcunningham added the type:improvement Changelog: Improvements label Sep 12, 2022
@ephraimbuddy ephraimbuddy added this to the Airflow 2.4.0 milestone Sep 14, 2022
This was referenced Sep 28, 2022
Closed
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jedcunningham jedcunningham jedcunningham approved these changes

@eladkal eladkal eladkal left review comments

@uranusjr uranusjr uranusjr left review comments

@turbaszek turbaszek Awaiting requested review from turbaszek

@potiuk potiuk Awaiting requested review from potiuk potiuk is a code owner

@kaxil kaxil Awaiting requested review from kaxil

@ashb ashb Awaiting requested review from ashb ashb is a code owner

@XD-DENG XD-DENG Awaiting requested review from XD-DENG XD-DENG is a code owner

@mik-laj mik-laj Awaiting requested review from mik-laj

Assignees
No one assigned
Labels
area:API Airflow's REST/HTTP API area:serialization kind:documentation type:improvement Changelog: Improvements type:new-feature Changelog: New Features
Projects
None yet
Milestone
Airflow 2.4.0
Development

Successfully merging this pull request may close these issues.
6 participants
@dstandish @uranusjr @potiuk @eladkal @jedcunningham @ephraimbuddy