Copilot commented on code in PR #64926:
URL: https://github.com/apache/airflow/pull/64926#discussion_r3066490404
##########
airflow-core/src/airflow/models/serialized_dag.py:
##########
@@ -577,8 +578,17 @@ def write_dag(
if deadline_uuid_mapping is not None:
# All deadlines matched — reuse the UUIDs to preserve hash.
- # Clear the mapping since the alert rows already exist in
the DB;
- # no need to delete and recreate identical records.
+ # Update name in case it changed (it doesn't affect the
definition
+ # match or the hash, so existing rows won't be recreated,
but it
+ # must stay current in the DB).
+ for uuid_str, deadline_data in
deadline_uuid_mapping.items():
+ session.execute(
+ update(DeadlineAlertModel)
+ .where(DeadlineAlertModel.id == UUID(uuid_str))
+ .values(
+
name=deadline_data.get(DeadlineAlertFields.NAME),
+ )
+ )
dag.data["dag"]["deadline"] = existing_deadline_uuids
Review Comment:
This block executes `UPDATE deadline_alert SET name=...` even when the
serialized DAG hash is unchanged, but `write_dag()` can still return `False`
later (meaning "not written"). That makes the return value misleading and can
hide that a DB write occurred; consider tracking whether any name updates were
applied and returning `True` (or adjusting the control flow/docstring
accordingly).
##########
airflow-core/src/airflow/api_fastapi/core_api/datamodels/ui/deadline.py:
##########
@@ -53,7 +50,6 @@ class DeadlineAlertResponse(BaseModel):
id: UUID
name: str | None = None
- description: str | None = None
reference_type: str = Field(validation_alias=AliasPath("reference",
"reference_type"))
interval: float = Field(description="Interval in seconds between deadline
evaluations.")
Review Comment:
PR description says the API will expose both `name` and `description` on
`DeadlineAlert`, but this change removes `description` from the UI API response
models (and corresponding OpenAPI). Either update the PR description to match
the actual behavior, or reintroduce `description` in the response
models/OpenAPI if it should remain supported.
##########
airflow-core/src/airflow/models/serialized_dag.py:
##########
@@ -577,8 +578,17 @@ def write_dag(
if deadline_uuid_mapping is not None:
# All deadlines matched — reuse the UUIDs to preserve hash.
- # Clear the mapping since the alert rows already exist in
the DB;
- # no need to delete and recreate identical records.
+ # Update name in case it changed (it doesn't affect the
definition
+ # match or the hash, so existing rows won't be recreated,
but it
+ # must stay current in the DB).
+ for uuid_str, deadline_data in
deadline_uuid_mapping.items():
+ session.execute(
+ update(DeadlineAlertModel)
+ .where(DeadlineAlertModel.id == UUID(uuid_str))
+ .values(
+
name=deadline_data.get(DeadlineAlertFields.NAME),
+ )
+ )
Review Comment:
New behavior: when deadline UUIDs are reused, `write_dag()` now updates
`DeadlineAlertModel.name` even if the serialized DAG hash doesn't change. There
doesn't appear to be a unit test covering this path (e.g., name changes while
reference/interval/callback stay the same), so a regression test should be
added to ensure the DB row is updated and the returned value/side-effects are
as expected.
--
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]
