jason810496 commented on code in PR #69403:
URL: https://github.com/apache/airflow/pull/69403#discussion_r3978070268
##########
airflow-core/tests/unit/api_fastapi/execution_api/versions/head/test_task_instances.py:
##########
@@ -4505,6 +4505,130 @@ def test_ti_patch_rendered_map_index_empty_string(self,
client, session, create_
assert response.status_code == 422
+class TestTIDagRunNoteUpdate:
+ def setup_method(self):
+ clear_db_runs()
+
+ def teardown_method(self):
+ clear_db_runs()
+
+ def test_create_dag_run_note(self, client, session, create_task_instance):
+ ti = create_task_instance(
+ task_id="test_create_dag_run_note",
+ state=State.RUNNING,
+ session=session,
+ )
+ ti_id = ti.id
+ session.commit()
+
+ response = client.patch(
+ f"/execution/task-instances/{ti_id}/dag-run-note",
+ json={"note": "Created from task runtime"},
+ )
+
+ assert response.status_code == 204
+ assert response.text == ""
+
+ session.expire_all()
+ dag_run = session.get(TaskInstance, ti_id).dag_run
+ assert dag_run.note == "Created from task runtime"
+ assert dag_run.dag_run_note.user_id is None
+
+ def test_update_dag_run_note_clears_user_id(self, client, session,
create_task_instance):
Review Comment:
Should we not override the user_id in the patch route? It seems like a side
effect for me.
##########
task-sdk/src/airflow/sdk/execution_time/schema/versions/v2026_10_30.py:
##########
@@ -15,6 +15,15 @@
# specific language governing permissions and limitations
# under the License.
+"""
+In-progress supervisor schema version.
+
+A brand-new message body needs no field-level migration instructions here: a
lang-SDK
+pinned to an older version simply never sends it, so there is nothing to strip
on the
+way down. Only *changes* to bodies that already existed at an earlier dated
version
+require a ``VersionChange`` entry below.
+"""
+
Review Comment:
I'd like to remove the comment to avoid the "In-progress" still show-up in
the actual release.
```suggestion
```
##########
task-sdk/src/airflow/sdk/execution_time/task_runner.py:
##########
@@ -727,6 +728,15 @@ def get_previous_dagrun(self, state: str | None = None) ->
DagRun | None:
return response.dag_run
+ def update_dagrun_note(self, note: str | None) -> None:
+ """
+ Update the note for this task instance's DagRun.
+
+ A string sets or replaces the note and an empty string removes it.
``None`` is a
+ no-op, so an existing user-authored note is left untouched.
+ """
+ SUPERVISOR_COMMS.send(msg=UpdateDagRunNote(ti_id=self.id, note=note))
Review Comment:
How about adding a check to skip the call at all if the `note` is `None`?
Since the API is nop when the `note` in the payload is `None`. IMO, we can
skip the API call in the first place at the task-sdk client side.
##########
airflow-core/src/airflow/api_fastapi/execution_api/routes/task_instances.py:
##########
@@ -908,6 +910,49 @@ def _raise_ti_not_in_live_table(task_instance_id: UUID,
session: SessionDep) ->
)
+@ti_id_router.patch(
+ "/{task_instance_id}/dag-run-note",
+ status_code=status.HTTP_204_NO_CONTENT,
+ responses=create_openapi_http_exception_doc(
+ [
+ (status.HTTP_404_NOT_FOUND, "Task Instance not found"),
+ (HTTP_422_UNPROCESSABLE_CONTENT, "Invalid payload for the DagRun
note update"),
+ ]
+ ),
+)
+def update_dag_run_note(
+ task_instance_id: UUID,
+ body: DagRunNoteUpdatePayload,
+ session: SessionDep,
+) -> None:
+ """
+ Update the note for the DagRun associated with this task instance.
+
+ An empty note removes the existing note, matching the public API. A null
note is a
+ no-op so runtime callers can leave a user-authored note untouched.
+ """
+ bind_contextvars(ti_id=str(task_instance_id))
+
+ dag_run = session.scalar(
+ select(DR)
+ .join(TI, and_(TI.dag_id == DR.dag_id, TI.run_id == DR.run_id))
+ .options(joinedload(DR.dag_run_note))
+ .where(TI.id == task_instance_id)
+ )
+ if dag_run is None:
+ raise HTTPException(
+ status_code=status.HTTP_404_NOT_FOUND,
+ detail={"reason": "not_found", "message": "Task Instance not
found"},
+ )
+
+ if body.note is None:
+ return
+
+ # Reuse the public API note logic so both editing paths stay consistent.
Runtime notes carry
+ # no acting user, so they are stored unattributed (user_id=None).
+ patch_dag_run_note(dag_run=dag_run, note=body.note, user_id=None)
Review Comment:
How about just `patch_dag_run_note(dag_run=dag_run, note=body.note,
user_id=existing_user_id)`?
Or if we intentionally keep the `user_id=None`, I think we should at least
logs something like `patch ... DagRun.note, setting user_if from
{existing_user_id} to None` for the audit purpose.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
