pierrejeambrun commented on code in PR #42973:
URL: https://github.com/apache/airflow/pull/42973#discussion_r1802543008
##########
airflow/api_fastapi/routes/public/dag_run.py:
##########
@@ -57,3 +57,19 @@ async def delete_dag_run(dag_id: str, dag_run_id: str,
session: Annotated[Sessio
)
session.delete(dag_run)
+
+
+@dag_run_router.patch("/{dag_run_id}",
responses=create_openapi_http_exception_doc([400, 401, 403, 404]))
Review Comment:
In the legacy api this is a `PUT` I believe. Also it is really dedicated to
the `update_dag_run_state`. It is not a general 'updage_dag_run`.
`@dag_run_router.put`
A general `PATCH` would be missing the `update_mask` etc... (cf legacy and
other PATCH endpoints)
##########
airflow/api_fastapi/serializers/dag_run.py:
##########
@@ -18,13 +18,28 @@
from __future__ import annotations
from datetime import datetime
+from enum import Enum
from pydantic import BaseModel, Field
from airflow.utils.state import DagRunState
from airflow.utils.types import DagRunTriggeredByType, DagRunType
+class DAGRunModifyFormStates(str, Enum):
Review Comment:
I think we can drop the `Form` in the name. This is not called from an http
form anymore. (I guess this is where the name is comming from)
```suggestion
class DAGRunModifyStates(str, Enum):
```
##########
airflow/api_fastapi/routes/public/dag_run.py:
##########
@@ -57,3 +57,19 @@ async def delete_dag_run(dag_id: str, dag_run_id: str,
session: Annotated[Sessio
)
session.delete(dag_run)
+
+
+@dag_run_router.patch("/{dag_run_id}",
responses=create_openapi_http_exception_doc([400, 401, 403, 404]))
+async def modify_dag_run(
Review Comment:
```suggestion
async def update_dag_run_state(
```
##########
airflow/api_fastapi/routes/public/dag_run.py:
##########
@@ -57,3 +57,19 @@ async def delete_dag_run(dag_id: str, dag_run_id: str,
session: Annotated[Sessio
)
session.delete(dag_run)
+
+
+@dag_run_router.patch("/{dag_run_id}",
responses=create_openapi_http_exception_doc([400, 401, 403, 404]))
+async def modify_dag_run(
+ dag_id: str, dag_run_id: str, state: DAGRunPatchBody, session:
Annotated[Session, Depends(get_session)]
+) -> DAGRunResponse:
+ """Modify a DAG Run."""
+ dag_run = session.scalar(select(DagRun).filter_by(dag_id=dag_id,
run_id=dag_run_id))
+ if dag_run is None:
+ raise HTTPException(
+ 404, f"The DagRun with dag_id: `{dag_id}` and run_id:
`{dag_run_id}` was not found"
+ )
+
+ setattr(dag_run, "state", state.state)
+
Review Comment:
Cf the legacy implementation. Setting a DagRun state has side effect and
should be using the utility functions `set_dag_run_state_to_success`,
`set_dag_run_state_to_queued`, etc... We cannot just set the `DagRun.state`
value in the db.
Also we should also use the DagBag for consistency
--
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-unsubscr...@airflow.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
