kosteev commented on a change in pull request #18981:
URL: https://github.com/apache/airflow/pull/18981#discussion_r731958751
##########
File path: airflow/providers/google/cloud/hooks/dataflow.py
##########
@@ -356,18 +356,23 @@ def _fetch_all_jobs(self) -> List[dict]:
.jobs()
.list(projectId=self._project_number, location=self._job_location)
)
- jobs: List[dict] = []
+ all_jobs: List[dict] = []
while request is not None:
response = request.execute(num_retries=self._num_retries)
- jobs.extend(response["jobs"])
+ if response is None:
Review comment:
is it possible that response object will be None here?
If this is not in the contract of discovery resource, I would not handle it
as it adds extra code and burden to test and maintain.
##########
File path: tests/providers/google/cloud/hooks/test_dataflow.py
##########
@@ -1697,6 +1697,28 @@ def test_fetch_list_job_messages_responses(self):
)
assert result == ["response_1"]
+ def test_fetch_all_jobs_when_no_jobs_returned(self):
+ # fmt: off
+ mock_list = (
+ self.mock_dataflow
+ .projects.return_value
+ .locations.return_value
+ .jobs.return_value
+ .list
+ )
+
+ # fmt: on
+ mock_list.return_value.execute.return_value = {}
Review comment:
`mock_list` variable is not used below. What do you think about merging
this statement into previous one?
##########
File path: tests/providers/google/cloud/hooks/test_dataflow.py
##########
@@ -1697,6 +1697,28 @@ def test_fetch_list_job_messages_responses(self):
)
assert result == ["response_1"]
+ def test_fetch_all_jobs_when_no_jobs_returned(self):
+ # fmt: off
+ mock_list = (
+ self.mock_dataflow
+ .projects.return_value
Review comment:
Nit: looks like redundant indentation. Did you use linter/pre-commit
hook from Airflow?
##########
File path: airflow/providers/google/cloud/hooks/dataflow.py
##########
@@ -356,18 +356,23 @@ def _fetch_all_jobs(self) -> List[dict]:
.jobs()
.list(projectId=self._project_number, location=self._job_location)
)
- jobs: List[dict] = []
+ all_jobs: List[dict] = []
while request is not None:
response = request.execute(num_retries=self._num_retries)
- jobs.extend(response["jobs"])
+ if response is None:
+ break
+ jobs = response.get("jobs")
+ if jobs is None:
+ break
+ all_jobs.extend(jobs)
Review comment:
would it work if we have just this line in previous implementation?
```
jobs.extend(response.get("jobs", []))
```
I am not sure about the contract of discovery service, but if the only thing
we need to handle is absence of "jobs" key and rest of logic will work the same
(listing will finish properly), I would avoid additional block of code and keep
it concise as it was before.
What do you think?
--
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]
