Re: [PR] Aip 86: add deadlines to DagRunResponse [airflow]

["pierrejeambrun (via GitHub)" <gi...@apache.org>]

pierrejeambrun commented on code in PR #50957:
URL: https://github.com/apache/airflow/pull/50957#discussion_r2120350829


##########
airflow-core/src/airflow/api_fastapi/core_api/datamodels/dag_run.py:
##########
@@ -68,6 +69,7 @@ class DAGRunResponse(BaseModel):
     end_date: datetime | None
     data_interval_start: datetime | None
     data_interval_end: datetime | None
+    deadlines: list[DeadlineResponse] | None

Review Comment:
   Maybe I misunderstood something. As I understand it, there's no record in the db at all for deadlines unless the run is in progress?
   
   I assume we can always improve later and add that if people request it. (soft delete etc...)



##########
airflow-core/tests/unit/api_fastapi/core_api/routes/public/test_dag_run.py:
##########
@@ -166,6 +180,13 @@ def get_dag_versions_dict(dag_versions: list[DagVersion]) -> list[dict]:
     ]
 
 
+def convert_instrumented_list_to_dict(lst):
+    result = [obj.__dict__ for obj in lst]
+    for obj in result:
+        obj.pop("_sa_instance_state", None)
+    return result
+
+

Review Comment:
   Is that needed ?



##########
airflow-core/src/airflow/api_fastapi/core_api/datamodels/dag_run.py:
##########
@@ -68,6 +69,7 @@ class DAGRunResponse(BaseModel):
     end_date: datetime | None
     data_interval_start: datetime | None
     data_interval_end: datetime | None
+    deadlines: list[DeadlineResponse] | None

Review Comment:
   `deadlines: list[DeadlineResponse]` should be enough, the relationship should return an empty list when there is no related items



##########
airflow-core/src/airflow/api_fastapi/core_api/datamodels/dag_run.py:
##########
@@ -68,6 +69,7 @@ class DAGRunResponse(BaseModel):
     end_date: datetime | None
     data_interval_start: datetime | None
     data_interval_end: datetime | None
+    deadlines: list[DeadlineResponse] | None

Review Comment:
   From the API point of view, we can nest the `deadline` API inside the `dagrun` one. Because deadline can be considered a sub-resource of the dagrun, they only exist for their corresponding dagrun, and especially when the dagrun is in a specific state, otherwise they are removed. (But that's debatable, since all the API resource are for now flat at root level, we could do the same here).



-- 
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