You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@airflow.apache.org by GitBox <gi...@apache.org> on 2021/09/02 08:13:53 UTC
[GitHub] [airflow] uranusjr opened a new pull request #17980: Implement API endpoint for DAG deletion
uranusjr opened a new pull request #17980:
URL: https://github.com/apache/airflow/pull/17980
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
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] bbovenzi commented on a change in pull request #17980: Implement API endpoint for DAG deletion
Posted by GitBox <gi...@apache.org>.
bbovenzi commented on a change in pull request #17980:
URL: https://github.com/apache/airflow/pull/17980#discussion_r701773251
##########
File path: airflow/api_connexion/endpoints/dag_endpoint.py
##########
@@ -100,3 +102,22 @@ def patch_dag(session, dag_id, update_mask=None):
setattr(dag, 'is_paused', patch_body['is_paused'])
session.commit()
return dag_schema.dump(dag)
+
+
+@security.requires_access([(permissions.ACTION_CAN_DELETE, permissions.RESOURCE_DAG)])
+@provide_session
+def delete_dag(dag_id: str, session: Session):
+ """Delete the specific DAG."""
+ # TODO: This function is shared with the /delete endpoint used by the web
+ # UI, so we're reusing it to simplify maintenance. Refactor the function to
+ # another place when the experimental/legacy API is removed.
+ from airflow.api.common.experimental import delete_dag
+
+ try:
+ delete_dag.delete_dag(dag_id, session=session)
+ except DagNotFound:
+ raise NotFound(f"Dag with id: '{dag_id}' not found")
+ except AirflowException:
+ raise AlreadyExists(detail=f"Task instances of fag with id: '{dag_id}' are still running")
Review comment:
```suggestion
raise AlreadyExists(detail=f"Task instances of dag with id: '{dag_id}' are still running")
```
typo
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] uranusjr commented on a change in pull request #17980: Implement API endpoint for DAG deletion
Posted by GitBox <gi...@apache.org>.
uranusjr commented on a change in pull request #17980:
URL: https://github.com/apache/airflow/pull/17980#discussion_r701716844
##########
File path: airflow/api_connexion/endpoints/dag_endpoint.py
##########
@@ -100,3 +102,20 @@ def patch_dag(session, dag_id, update_mask=None):
setattr(dag, 'is_paused', patch_body['is_paused'])
session.commit()
return dag_schema.dump(dag)
+
+
+@security.requires_access([(permissions.ACTION_CAN_DELETE, permissions.RESOURCE_DAG)])
+@provide_session
+def delete_dag(dag_id: str, session: Session):
+ """Delete the specific DAG."""
+ # TODO: This function is shared with the /delete endpoint used by the web
+ # UI, so we're reusing it to simplify maintenance. Refactor the function to
+ # another place when the experimental/legacy API is removed.
+ from airflow.api.common.experimental import delete_dag
+
+ try:
+ delete_dag.delete_dag(dag_id, session=session)
+ except DagNotFound:
+ raise NotFound(f"Dag with id: '{dag_id}' not found")
+
Review comment:
Yes except that brings back the 409 issue 🙃
It also feels curious to me the web UI does not seem to handle this exception. Do we just get a 500 when that happens?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] uranusjr commented on a change in pull request #17980: Implement API endpoint for DAG deletion
Posted by GitBox <gi...@apache.org>.
uranusjr commented on a change in pull request #17980:
URL: https://github.com/apache/airflow/pull/17980#discussion_r701542937
##########
File path: airflow/api_connexion/endpoints/dag_endpoint.py
##########
@@ -100,3 +102,23 @@ def patch_dag(session, dag_id, update_mask=None):
setattr(dag, 'is_paused', patch_body['is_paused'])
session.commit()
return dag_schema.dump(dag)
+
+
+@security.requires_access([(permissions.ACTION_CAN_DELETE, permissions.RESOURCE_DAG)])
+@provide_session
+def delete_dag(dag_id: str, session: Session):
+ """Delete the specific DAG."""
+ # TODO: This function is shared with the /delete endpoint used by the web
+ # UI, so we're reusing it to simplify maintenance. Refactor the function to
+ # another place when the experimental/legacy API is removed.
+ from airflow.api.common.experimental import delete_dag
+
+ try:
+ delete_dag.delete_dag(dag_id)
+ except DagNotFound:
+ raise NotFound(f"Dag with id: '{dag_id}' not found")
+ except DagFileExists:
+ detail = f"Dag with id: '{dag_id}' is still in DagBag. Remove the Dag file first."
+ raise BadRequest(detail=detail)
Review comment:
You’re right. I’m also going to remove the block from the web UI view (where I copied the implementation from).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] uranusjr commented on pull request #17980: Implement API endpoint for DAG deletion
Posted by GitBox <gi...@apache.org>.
uranusjr commented on pull request #17980:
URL: https://github.com/apache/airflow/pull/17980#issuecomment-912418689
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ephraimbuddy commented on a change in pull request #17980: Implement API endpoint for DAG deletion
Posted by GitBox <gi...@apache.org>.
ephraimbuddy commented on a change in pull request #17980:
URL: https://github.com/apache/airflow/pull/17980#discussion_r701737917
##########
File path: airflow/api_connexion/endpoints/dag_endpoint.py
##########
@@ -100,3 +102,20 @@ def patch_dag(session, dag_id, update_mask=None):
setattr(dag, 'is_paused', patch_body['is_paused'])
session.commit()
return dag_schema.dump(dag)
+
+
+@security.requires_access([(permissions.ACTION_CAN_DELETE, permissions.RESOURCE_DAG)])
+@provide_session
+def delete_dag(dag_id: str, session: Session):
+ """Delete the specific DAG."""
+ # TODO: This function is shared with the /delete endpoint used by the web
+ # UI, so we're reusing it to simplify maintenance. Refactor the function to
+ # another place when the experimental/legacy API is removed.
+ from airflow.api.common.experimental import delete_dag
+
+ try:
+ delete_dag.delete_dag(dag_id, session=session)
+ except DagNotFound:
+ raise NotFound(f"Dag with id: '{dag_id}' not found")
+
Review comment:
It handles the exception see: https://github.com/apache/airflow/blob/6db73a60c9f6e0ba46712cf44b9f1e506450fcae/airflow/www/views.py#L1572-L1578
I think 400 will be good to return when that happens since the user can do something to resolve it?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] uranusjr commented on a change in pull request #17980: Implement API endpoint for DAG deletion
Posted by GitBox <gi...@apache.org>.
uranusjr commented on a change in pull request #17980:
URL: https://github.com/apache/airflow/pull/17980#discussion_r701775568
##########
File path: airflow/api_connexion/endpoints/dag_endpoint.py
##########
@@ -100,3 +102,22 @@ def patch_dag(session, dag_id, update_mask=None):
setattr(dag, 'is_paused', patch_body['is_paused'])
session.commit()
return dag_schema.dump(dag)
+
+
+@security.requires_access([(permissions.ACTION_CAN_DELETE, permissions.RESOURCE_DAG)])
+@provide_session
+def delete_dag(dag_id: str, session: Session):
+ """Delete the specific DAG."""
+ # TODO: This function is shared with the /delete endpoint used by the web
+ # UI, so we're reusing it to simplify maintenance. Refactor the function to
+ # another place when the experimental/legacy API is removed.
+ from airflow.api.common.experimental import delete_dag
+
+ try:
+ delete_dag.delete_dag(dag_id, session=session)
+ except DagNotFound:
+ raise NotFound(f"Dag with id: '{dag_id}' not found")
+ except AirflowException:
+ raise AlreadyExists(detail=f"Task instances of fag with id: '{dag_id}' are still running")
Review comment:
Oops
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] jedcunningham commented on a change in pull request #17980: Implement API endpoint for DAG deletion
Posted by GitBox <gi...@apache.org>.
jedcunningham commented on a change in pull request #17980:
URL: https://github.com/apache/airflow/pull/17980#discussion_r701511972
##########
File path: airflow/api_connexion/endpoints/dag_endpoint.py
##########
@@ -100,3 +102,23 @@ def patch_dag(session, dag_id, update_mask=None):
setattr(dag, 'is_paused', patch_body['is_paused'])
session.commit()
return dag_schema.dump(dag)
+
+
+@security.requires_access([(permissions.ACTION_CAN_DELETE, permissions.RESOURCE_DAG)])
+@provide_session
+def delete_dag(dag_id: str, session: Session):
+ """Delete the specific DAG."""
+ # TODO: This function is shared with the /delete endpoint used by the web
+ # UI, so we're reusing it to simplify maintenance. Refactor the function to
+ # another place when the experimental/legacy API is removed.
+ from airflow.api.common.experimental import delete_dag
+
+ try:
+ delete_dag.delete_dag(dag_id)
+ except DagNotFound:
+ raise NotFound(f"Dag with id: '{dag_id}' not found")
+ except DagFileExists:
+ detail = f"Dag with id: '{dag_id}' is still in DagBag. Remove the Dag file first."
+ raise BadRequest(detail=detail)
Review comment:
I'm don't think this is actually raised any longer? Solves the 400/409 problem at least :)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] github-actions[bot] commented on pull request #17980: Implement API endpoint for DAG deletion
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #17980:
URL: https://github.com/apache/airflow/pull/17980#issuecomment-912441127
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] uranusjr merged pull request #17980: Implement API endpoint for DAG deletion
Posted by GitBox <gi...@apache.org>.
uranusjr merged pull request #17980:
URL: https://github.com/apache/airflow/pull/17980
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ephraimbuddy commented on a change in pull request #17980: Implement API endpoint for DAG deletion
Posted by GitBox <gi...@apache.org>.
ephraimbuddy commented on a change in pull request #17980:
URL: https://github.com/apache/airflow/pull/17980#discussion_r701676584
##########
File path: airflow/api_connexion/endpoints/dag_endpoint.py
##########
@@ -100,3 +102,20 @@ def patch_dag(session, dag_id, update_mask=None):
setattr(dag, 'is_paused', patch_body['is_paused'])
session.commit()
return dag_schema.dump(dag)
+
+
+@security.requires_access([(permissions.ACTION_CAN_DELETE, permissions.RESOURCE_DAG)])
+@provide_session
+def delete_dag(dag_id: str, session: Session):
+ """Delete the specific DAG."""
+ # TODO: This function is shared with the /delete endpoint used by the web
+ # UI, so we're reusing it to simplify maintenance. Refactor the function to
+ # another place when the experimental/legacy API is removed.
+ from airflow.api.common.experimental import delete_dag
+
+ try:
+ delete_dag.delete_dag(dag_id, session=session)
+ except DagNotFound:
+ raise NotFound(f"Dag with id: '{dag_id}' not found")
+
Review comment:
The delete_dag function also raises AirflowException when the dag still has running task instances. I think we should also handle the exception here and tell the user to mark those task instances as failed or successful before deleting, just like we do in the webserver. What do you think?
##########
File path: airflow/api_connexion/openapi/v1.yaml
##########
@@ -478,6 +478,23 @@ paths:
'404':
$ref: '#/components/responses/NotFound'
+ delete:
+ summary: Delete a DAG
Review comment:
```suggestion
summary: Delete a DAG
description: This endpoint will delete ALL metadata, DAGRuns, etc EXCEPT Log. This action cannot be undone
```
I think that we should add this description as we do in the UI, so users are aware of what this can cause
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org