Fix: make dry run optional for patch task instance by Calder-Ty · Pull Request #34568 · apache/airflow

Calder-Ty · 2023-09-22T21:18:17Z

Fixes#34563. This sets Dry run to be optional as the docs indicate it should be and makes the default False per the docs.

^ 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.

This fixes an issue where dry_run is not actually and optional parameter in the patch task_instance api.

Calder-Ty · 2023-09-22T21:18:49Z

This is a reopened version of #34564 which was closed by mistake (by myself).

Calder-Ty · 2023-09-22T21:26:17Z

I didn't want this comment to be forgotten:

#34564 (review)

f0restron07 · 2023-09-24T07:03:47Z

Hi @Calder-Ty,

Thank you for addressing this issue and for reopening the pull request. Making dry_run optional and setting the default to False aligns the behavior of the code with the documentation, which will certainly prevent confusion and errors for API users in the future.

It is crucial that the API's behavior is consistent with the documentation to ensure a smooth and reliable user experience, and this change is a step in the right direction.

Looking at the commits, it seems you have kept the changes concise and directly related to the issue at hand, which is appreciated. It's also good to see that the checks have mostly passed, indicating that this change doesn't introduce any apparent regressions or new issues.

I look forward to seeing this change merged after the required reviews, and I believe it will be a valuable improvement to Apache Airflow.

Thank you again for your contribution!

Best regards,
f0restron

hussein-awala · 2023-09-24T14:09:37Z

@pierrejeambrun in the first PR, I commented with:

IMHO it's a problem in the doc, dry_run should be True by default.

WDYT? If you agree, should we fix the doc and the bug in the same PR?

pierrejeambrun · 2023-09-24T14:59:36Z

Yes, all other dry_run params default to True in other routes, the only 2 that do not in the doc are:

I think defaulting to true makes more sense, and is more conservative, unless explicitly stated by the user with a dry_run=False, we should run in dry run mode.

Also just for a consistency reason, dry_run should be True. Updating the doc and the schema in one place will fix both endpoints mentioned.

Yes this is a very small located change, updating both the schema and the API spec and putting this under bug fix seems fine to me :)

This updates the docs and the code so that they are in alignment while also being consistent with all other endpoints. All other Endpoints have dry run set to be True by default.

Calder-Ty · 2023-09-25T12:05:27Z

made changes to have docs and code aligned, as well as be consistent with the rest of the API (i.e Set dry_run to be true by default,

ephraimbuddy

We should only update here:

airflow/airflow/api_connexion/endpoints/task_instance_endpoint.py

Line 636 in d1b7bca

ifnotdata["dry_run"]:

to use data.get(‘dry_run’, False)

pierrejeambrun

LGTM, this is consistent with dry_run default value for other endpoints. (And the more conservative choice, I believe)

o-nikolas

Other than the static check failures (should be an easy fix to the generated ts) it lgtm.

I like the fix in the schema!

o-nikolas · 2023-09-27T17:16:45Z

Also, are we considering this change to the default a breaking change to the API or a bugfix?

CC @eladkal

Calder-Ty · 2023-09-27T17:24:15Z

Also, are we considering this change to the default a breaking change to the API or a bugfix?
CC @eladkal

Technically it is, however, I think we can be fairly sure no one was depending on the functionality since it was broken.

hussein-awala · 2023-09-27T19:02:48Z

Also, are we considering this change to the default a breaking change to the API or a bugfix?
CC @eladkal

Based on this issue #34563, we cannot use the endpoint without providing the dry_run value, so this PR technically adds a new default value for dry_run and not changing it from False to True.

pierrejeambrun · 2023-09-27T20:06:09Z

Yep, what Hussein said, not a breaking change. (Field was required before)

boring-cyborg · 2023-09-30T18:46:58Z

Awesome work, congrats on your first merged pull request! You are invited to check our Issue Tracker for additional contributions.

* fix: Make dry_run optional per docs This fixes an issue where dry_run is not actually and optional parameter in the patch task_instance api. * chore: remove formatting changes * fix: Make changes for api docs This updates the docs and the code so that they are in alignment while also being consistent with all other endpoints. All other Endpoints have dry run set to be True by default. * fix: Update static ts file for api change * fix: Remove dump_default (cherry picked from commit a4357ca)

Calder-Ty added 2 commits September 22, 2023 15:00

fix: Make dry_run optional per docs
616f131
This fixes an issue where dry_run is not actually and optional parameter in the patch task_instance api.

chore: remove formatting changes
926cbd8

Calder-Ty requested review from ephraimbuddy and pierrejeambrun as code owners September 22, 2023 21:18

boring-cyborgBot added the area:API Airflow's REST/HTTP API label Sep 22, 2023

Calder-Ty changed the title ~~Fix dry run~~Fix: make dry run optional for patch task instance Sep 24, 2023

pierrejeambrun added the type:bug-fix Changelog: Bug Fixes label Sep 24, 2023

pierrejeambrun added this to the Airflow 2.7.2 milestone Sep 24, 2023

fix: Make changes for api docs
d0b66a7
This updates the docs and the code so that they are in alignment while also being consistent with all other endpoints. All other Endpoints have dry run set to be True by default.

ephraimbuddy reviewed Sep 25, 2023
View reviewed changes

Comment threadairflow/api_connexion/openapi/v1.yaml
Comment threadairflow/api_connexion/schemas/task_instance_schema.py Outdated

pierrejeambrun approved these changes Sep 25, 2023
View reviewed changes

pierrejeambrun requested a review from hussein-awala September 25, 2023 20:52

o-nikolas approved these changes Sep 27, 2023
View reviewed changes

fix: Update static ts file for api change
54c25eb

Calder-Ty requested review from ashb, bbovenzi and ryanahamilton as code owners September 27, 2023 17:22

hussein-awala approved these changes Sep 27, 2023
View reviewed changes

Comment threadairflow/api_connexion/schemas/task_instance_schema.py Outdated

fix: Remove dump_default
895efde

pierrejeambrun merged commit a4357ca into apache:mainSep 30, 2023

ephraimbuddy mentioned this pull request Oct 9, 2023
Status of testing of Apache Airflow 2.7.2rc1 #34830
Closed
99 tasks

austinau mentioned this pull request Oct 24, 2023
fix: set dry_run to be optional. #35167
Merged

Uh oh!

Conversation

Calder-Ty commented Sep 22, 2023

Uh oh!

Calder-Ty commented Sep 22, 2023

Uh oh!

Calder-Ty commented Sep 22, 2023

Uh oh!

f0restron07 commented Sep 24, 2023

Uh oh!

hussein-awala commented Sep 24, 2023

Uh oh!

pierrejeambrun commented Sep 24, 2023• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Calder-Ty commented Sep 25, 2023

Uh oh!

ephraimbuddy left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

pierrejeambrun left a comment

Choose a reason for hiding this comment

Uh oh!

o-nikolas left a comment

Choose a reason for hiding this comment

Uh oh!

o-nikolas commented Sep 27, 2023

Uh oh!

Calder-Ty commented Sep 27, 2023

Uh oh!

hussein-awala commented Sep 27, 2023

Uh oh!

Uh oh!

pierrejeambrun commented Sep 27, 2023• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

boring-cyborgBot commented Sep 30, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

pierrejeambrun commented Sep 24, 2023•
edited
Loading

pierrejeambrun commented Sep 27, 2023•
edited
Loading