[GitHub] [airflow] zikun commented on a change in pull request #9473: [WIP] Dag Runs CRUD endpoints

[GitBox <gi...@apache.org>]

zikun commented on a change in pull request #9473:
URL: https://github.com/apache/airflow/pull/9473#discussion_r443626814



##########
File path: tests/api_connexion/endpoints/test_dag_run_endpoint.py
##########
@@ -76,10 +78,18 @@ def _create_test_dag_run(self, state='running', extra_dag=False):
 
 
 class TestDeleteDagRun(TestDagRunEndpoint):
-    @pytest.mark.skip(reason="Not implemented yet")
-    def test_should_response_200(self):
-        response = self.client.delete("api/v1/dags/TEST_DAG_ID}/dagRuns/TEST_DAG_RUN_ID")
+    @provide_session
+    def test_should_response_200(self, session):
+        session.add_all(self._create_test_dag_run())
+        session.commit()
+        response = self.client.delete("api/v1/dags/TEST_DAG_ID/dagRuns/TEST_DAG_RUN_ID_1")
         assert response.status_code == 204
+        response = self.client.get("api/v1/dags/TEST_DAG_ID/dagRuns/TEST_DAG_RUN_ID_1")
+        self.assertEqual(response.status_code, 404)
+
+    def test_should_response_404(self):
+        response = self.client.delete("api/v1/dags/INVALID_DAG_RUN/dagRuns/INVALID_DAG_RUN")
+        self.assertEqual(response.status_code, 404)

Review comment:
       ```suggestion
           assert response.status_code == 404
   ```

##########
File path: airflow/api_connexion/endpoints/dag_run_endpoint.py
##########
@@ -14,23 +14,28 @@
 # KIND, either express or implied.  See the License for the
 # specific language governing permissions and limitations
 # under the License.
+from flask import request
+from connexion import NoContent
+from sqlalchemy import and_, func 
 
-from sqlalchemy import func
-
-from airflow.api_connexion.exceptions import NotFound
+from airflow.api_connexion.exceptions import NotFound, AlreadyExists
 from airflow.api_connexion.schemas.dag_run_schema import (
     DAGRunCollection, dagrun_collection_schema, dagrun_schema,
 )
 from airflow.api_connexion.utils import conn_parse_datetime
-from airflow.models import DagRun
+from airflow.models import DagRun, DagModel
 from airflow.utils.session import provide_session
+from airflow.utils.types import DagRunType
 
 
-def delete_dag_run():
+@provide_session
+def delete_dag_run(dag_id, dag_run_id, session):
     """
     Delete a DAG Run
     """
-    raise NotImplementedError("Not implemented yet.")
+    if session.query(DagRun).filter(and_(DagRun.dag_id == dag_id, DagRun.run_id == dag_run_id)).delete() == 0:
+        raise NotFound("DAGRun not found")

Review comment:
       Maybe to be more precise and to be consistent with line 127:
   ```suggestion
           raise NotFound(f"DAGRun with DAG ID:{dag_id} and DAGRun ID:{dag_run_id} not found")
   ```

##########
File path: tests/api_connexion/endpoints/test_dag_run_endpoint.py
##########
@@ -354,7 +364,17 @@ def test_should_response_200(self):
 
 
 class TestPostDagRun(TestDagRunEndpoint):
-    @pytest.mark.skip(reason="Not implemented yet")
-    def test_should_response_200(self):
-        response = self.client.post("/dags/TEST_DAG_ID/dagRuns")
+    @provide_session
+    def test_should_response_200(self, session):
+        dag_instance = DagModel(dag_id="TEST_DAG_ID")
+        session.add(dag_instance)
+        session.commit()
+        response = self.client.post("api/v1/dags/TEST_DAG_ID/dagRuns", json={"dag_run_id": "TEST_DAG_RUN", "execution_date": self.default_time, "state": "failed"})
         assert response.status_code == 200
+    
+    def test_response_404(self):
+        response = self.client.post("api/v1/dags/TEST_DAG_ID/dagRuns", json={"dag_run_id": "TEST_DAG_RUN", "execution_date": self.default_time, "state": "failed"})
+        assert response.status_code == 404
+        self.assertEqual(
+            {'detail': None, 'status': 404, 'title': 'DAG with dag_id: TEST_DAG_ID not found', 'type': 'about:blank'}, response.json
+        )

Review comment:
       ```suggestion
           assert response.json == {
               'detail': None,
               'status': 404,
               'title': 'DAG with dag_id: TEST_DAG_ID not found',
               'type': 'about:blank',
           }
   ```

##########
File path: tests/api_connexion/endpoints/test_dag_run_endpoint.py
##########
@@ -76,10 +78,18 @@ def _create_test_dag_run(self, state='running', extra_dag=False):
 
 
 class TestDeleteDagRun(TestDagRunEndpoint):
-    @pytest.mark.skip(reason="Not implemented yet")
-    def test_should_response_200(self):
-        response = self.client.delete("api/v1/dags/TEST_DAG_ID}/dagRuns/TEST_DAG_RUN_ID")
+    @provide_session
+    def test_should_response_200(self, session):
+        session.add_all(self._create_test_dag_run())
+        session.commit()
+        response = self.client.delete("api/v1/dags/TEST_DAG_ID/dagRuns/TEST_DAG_RUN_ID_1")
         assert response.status_code == 204
+        response = self.client.get("api/v1/dags/TEST_DAG_ID/dagRuns/TEST_DAG_RUN_ID_1")
+        self.assertEqual(response.status_code, 404)

Review comment:
       ```suggestion
                   assert response.status_code == 404
   ```




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

For queries about this service, please contact Infrastructure at:
users@infra.apache.org