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


##########
airflow-ctl/src/airflowctl/api/operations.py:
##########
@@ -148,6 +150,33 @@ def __init_subclass__(cls, **kwargs):
             if callable(value):
                 setattr(cls, attr, 
_check_flag_and_exit_if_server_response_error(value))
 
+    def return_all_entries(
+        self,
+        *,
+        path: str,
+        total_entries: int,
+        data_model: type[BaseModel],
+        entry_list: list,
+        offset: int = 0,
+        limit: int = 50,
+        params: dict | None = None,
+        **kwargs,
+    ) -> list | ServerResponseError:
+        if params is None:
+            params = {}
+        params.update({"limit": limit}, **kwargs)
+        try:
+            while offset < total_entries:
+                params.update({"offset": offset})
+                self.response = self.client.get(path, params=params)
+                entry = data_model.model_validate_json(self.response.content)
+                offset = offset + limit  # default limit params = 50
+                entry_list.append(entry)
+
+            return entry_list
+        except ServerResponseError as e:
+            raise e

Review Comment:
   Yes but it should have been removed then, catching an exeption to re-raise 
it without further processing just achieves nothing, we can simply remove the 
try block.
   
   I'll open a follow up PR to remove that piece.



##########
airflow-ctl/tests/airflow_ctl/api/test_operations.py:
##########
@@ -114,6 +124,46 @@ def test_server_connection_refused(self):
         ):
             client.connections.get("1")
 
+    @pytest.mark.parametrize(
+        "total_entries, limit, expected_response",
+        [
+            (0, 0, (HelloCollectionResponse(hellos=[], total_entries=0))),
+            (1, 50, 
(HelloCollectionResponse(hellos=[HelloResponse(name="hello")], 
total_entries=1))),
+            (
+                150,
+                50,
+                (
+                    HelloCollectionResponse(
+                        hellos=[
+                            HelloResponse(name="hello"),
+                            HelloResponse(name="hello"),
+                            HelloResponse(name="hello"),
+                        ],
+                        total_entries=150,
+                    )
+                ),
+            ),
+            (
+                90,
+                50,
+                (
+                    HelloCollectionResponse(
+                        hellos=[HelloResponse(name="hello"), 
HelloResponse(name="hello")], total_entries=90
+                    )
+                ),
+            ),
+        ],
+    )
+    def test_execute_list(self, total_entries, limit, expected_response):
+        hello_response = []
+        if total_entries != 0:
+            update = (total_entries + limit - 1) // limit
+            hello_response.extend([HelloResponse(name="hello")] * update)
+        hello_collection_response = HelloCollectionResponse(
+            hellos=hello_response, total_entries=total_entries
+        )
+        assert expected_response == hello_collection_response
+

Review Comment:
   This test doesn't test anything at all. execute_list is never called



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

Reply via email to