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

Implement API endpoint for DAG deletion #17980

Merged
merged 5 commits into from
Sep 3, 2021
Merged

Conversation

uranusjr
Copy link
Member

@uranusjr uranusjr commented Sep 2, 2021

Close #17969.

I’m not sure about the error returned for DagFileExists. I was considering 409 Conflict, but Airflow (and Connexion) seems to define that to only represent conflicts on POST. I’m settling for 400 Bad Request, but I feel the current definition is too narrow. The definition in RFC 7231 is much broader and IMO applies here:

The 409 (Conflict) status code indicates that the request could not be completed due to a conflict with the current state of the target resource. This code is used in situations where the user might be able to resolve the conflict and resubmit the request.

https://datatracker.ietf.org/doc/html/rfc7231#section-6.5.8

Copy link
Contributor

@ephraimbuddy ephraimbuddy left a comment

Choose a reason for hiding this comment

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

LGTM just nitpicking :)

airflow/api_connexion/openapi/v1.yaml Show resolved Hide resolved
@uranusjr
Copy link
Member Author

uranusjr commented Sep 3, 2021

I added an except block to catch AiflowException and return 409. The 409 description in the OpenAPI schema is also rewritten slightly to make sense for non-creation conflicts.

Co-authored-by: Brent Bovenzi <brent.bovenzi@gmail.com>
@github-actions github-actions bot added the full tests needed We need to run full set of tests for this PR to merge label Sep 3, 2021
@github-actions
Copy link

github-actions bot commented Sep 3, 2021

The PR most likely needs to run full matrix of tests because it modifies parts of the core of Airflow. However, committers might decide to merge it quickly and take the risk. If they don't merge it quickly - please rebase it to the latest main at your convenience, or amend the last commit of the PR, and push it with --force-with-lease.

@uranusjr uranusjr merged commit 2cace94 into apache:main Sep 3, 2021
@uranusjr uranusjr deleted the api-dag-delete branch September 3, 2021 12:46
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 full tests needed We need to run full set of tests for this PR to merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Delete DAG REST API Functionality
4 participants