Migrate public endpoint Set Task Instances State to FastAPI #44246
Conversation
omkar-foss
added
the
legacy api
omkar-foss
requested review from
bbovenzi,
pierrejeambrun,
ryanahamilton,
jscheffl and
ephraimbuddy
as code owners
| tags=["Task Instance"], prefix="/dags/{dag_id}/dagRuns/{dag_run_id}/taskInstances" | ||
| ) | ||
| task_instances_router = AirflowRouter(tags=["Task Instance"], prefix="/dags/{dag_id}") | ||
| task_instances_prefix = "/dagRuns/{dag_run_id}/taskInstances" |
There was a problem hiding this comment.
should we make task_instances_prefix all capitals to indicate constant?
| "task_instances": [ | ||
| { | ||
| "dag_id": "example_python_operator", | ||
| "dag_run_id": "TEST_DAG_RUN_ID", |
There was a problem hiding this comment.
| "dag_run_id": "TEST_DAG_RUN_ID", | |
| "dag_run_id": run_id, |
| @mock.patch("airflow.models.dag.DAG.set_task_instance_state") | ||
| def test_should_assert_call_mocked_api(self, mock_set_task_instance_state, test_client, session): | ||
| self.create_task_instances(session) | ||
| run_id = "TEST_DAG_RUN_ID" |
There was a problem hiding this comment.
| run_id = "TEST_DAG_RUN_ID" | |
| run_id = "TEST_DAG_RUN_ID" | |
| task_id = "print_the_context" |
There was a problem hiding this comment.
also,we denote them in capital letters to indicate that they are constants.
| session.query(TaskInstance) | ||
| .join(TaskInstance.dag_run) | ||
| .options(contains_eager(TaskInstance.dag_run)) | ||
| .filter(TaskInstance.task_id == "print_the_context") |
There was a problem hiding this comment.
| .filter(TaskInstance.task_id == "print_the_context") | |
| .filter(TaskInstance.task_id == task_id) |
| "/public/dags/example_python_operator/updateTaskInstancesState", | ||
| json={ | ||
| "dry_run": True, | ||
| "task_id": "print_the_context", |
There was a problem hiding this comment.
| "task_id": "print_the_context", | |
| "task_id": task_id", |
| "dag_id": "example_python_operator", | ||
| "dag_run_id": "TEST_DAG_RUN_ID", | ||
| "logical_date": "2020-01-01T00:00:00Z", | ||
| "task_id": "print_the_context", |
There was a problem hiding this comment.
| "task_id": "print_the_context", | |
| "task_id": task_id, |
| "/updateTaskInstancesState", | ||
| responses=create_openapi_http_exception_doc([status.HTTP_404_NOT_FOUND, status.HTTP_400_BAD_REQUEST]), | ||
| ) | ||
| def set_task_instances_state( |
There was a problem hiding this comment.
For consistency, I think it should be transformed as for dag_run into patch_dag_run.
A PATCH that can take only a state for now, but it will be easier to extend. It should also allow PATCHING the note.
There was a problem hiding this comment.
Just noticed you already have a PR opened for that.
| include_downstream: bool | ||
| include_future: bool | ||
| include_past: bool | ||
| new_state: str |
There was a problem hiding this comment.
It would be great to include editing the Task Instance note at the same time like we do for changing a dag run.
| task_id: str | ||
| dag_run_id: str = Field(validation_alias="run_id") | ||
| dag_id: str | ||
| logical_date: datetime |
There was a problem hiding this comment.
logical_date is no longer unique. Is it still necessary for serializing? @dstandish
| include_upstream: bool | ||
| include_downstream: bool | ||
| include_future: bool | ||
| include_past: bool |
There was a problem hiding this comment.
We can make all of these False by default
|
I think we should only merge one of this and #44223 and include setting a note and all the |
closes: #43752
related: #42370
This migrates the Set Task Instances State API from
api_connexiontoapi_fastapi.