pierrejeambrun commented on code in PR #50132:
URL: https://github.com/apache/airflow/pull/50132#discussion_r2144960748
##########
airflow-ctl/src/airflowctl/api/operations.py:
##########
@@ -148,6 +148,24 @@ 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, offset=0, params,
**kwargs
Review Comment:
Some parameter typing are missing.
##########
airflow-ctl/src/airflowctl/api/operations.py:
##########
@@ -148,6 +148,24 @@ 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, offset=0, params,
**kwargs
+ ) -> list | ServerResponseError:
+ params.update({"offset": 0})
+ params.update(**kwargs)
+ entry_list = []
+ try:
+ if total_entries == 0:
+ return [data_model.model_validate_json(self.response.content)]
+ while offset <= total_entries:
+ self.response = self.client.get(path, params=params)
+ entry = data_model.model_validate_json(self.response.content)
+ offset = offset + 50 # default limit params = 50
Review Comment:
limit should be a parameter, in case we want to fetch bigger pages, it can
default to 50.
##########
airflow-ctl/src/airflowctl/api/operations.py:
##########
@@ -148,6 +148,24 @@ 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, offset=0, params,
**kwargs
+ ) -> list | ServerResponseError:
+ params.update({"offset": 0})
+ params.update(**kwargs)
+ entry_list = []
+ try:
+ if total_entries == 0:
+ return [data_model.model_validate_json(self.response.content)]
+ while offset <= total_entries:
+ self.response = self.client.get(path, params=params)
+ entry = data_model.model_validate_json(self.response.content)
+ offset = offset + 50 # default limit params = 50
Review Comment:
I'm not sure if this will work. It does not seem that the `params` dict is
updated during the `while` loop, looking like the `offset` query param passed
will always be the same.
##########
airflow-ctl/src/airflowctl/api/operations.py:
##########
@@ -593,11 +611,18 @@ def get(self, pool_name: str) -> PoolResponse |
ServerResponseError:
except ServerResponseError as e:
raise e
- def list(self) -> PoolCollectionResponse | ServerResponseError:
+ def list(self) -> list | ServerResponseError:
"""List all pools."""
try:
self.response = self.client.get("pools")
- return
PoolCollectionResponse.model_validate_json(self.response.content)
+ total_entries =
PoolCollectionResponse.model_validate_json(self.response.content).total_entries
+ return super().return_all_entries(
+ path="pools",
+ total_entries=total_entries,
+ data_model=PoolCollectionResponse,
+ offset=0,
+ params={},
Review Comment:
params could default to `None` so we do not need to pass it like this, you
don't need to pass offset which default to 0.
##########
airflow-ctl/src/airflowctl/api/operations.py:
##########
@@ -148,6 +148,24 @@ 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, offset=0, params,
**kwargs
+ ) -> list | ServerResponseError:
+ params.update({"offset": 0})
Review Comment:
Why is that needed? Also shouldn't this be `"offset": offset` ? In case we
want to skip some elements at the start of the collection.
##########
airflow-ctl/src/airflowctl/api/operations.py:
##########
@@ -148,6 +148,24 @@ 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, offset=0, params,
**kwargs
+ ) -> list | ServerResponseError:
+ params.update({"offset": 0})
+ params.update(**kwargs)
+ entry_list = []
+ try:
+ if total_entries == 0:
+ return [data_model.model_validate_json(self.response.content)]
+ while offset <= total_entries:
Review Comment:
I think `<` is enough. We don't need to fetch another page if offset is 50
and the total entry is `50`.
--
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]
