Copilot commented on code in PR #45008:
URL: https://github.com/apache/airflow/pull/45008#discussion_r1889192467
##########
providers/tests/edge/cli/test_edge_command.py:
##########
@@ -184,7 +229,7 @@ def test_check_running_jobs_success(self, mock_set_state,
worker_with_job: _Edge
@patch("airflow.providers.edge.cli.edge_command.jobs_set_state")
def test_check_running_jobs_failed(self, mock_set_state, worker_with_job:
_EdgeWorkerCli):
job = worker_with_job.jobs[0]
- job.process.generated_returncode = 42 # type: ignore[attr-defined]
+ job.process.generated_returncode = 42 # type: ignore[union-attr]
Review Comment:
The attribute 'generated_returncode' does not exist on the process object.
This will cause an AttributeError during test execution.
```suggestion
job.process.returncode = 42 # type: ignore[union-attr]
```
##########
providers/src/airflow/providers/edge/cli/edge_command.py:
##########
@@ -137,11 +132,26 @@ class _Job:
"""Holds all information for a task/job to be executed as bundle."""
edge_job: EdgeJobFetched
- process: Popen
+ process: Popen | Process
logfile: Path
logsize: int
"""Last size of log file, point of last chunk push."""
+ @property
+ def is_running(self) -> bool:
+ """Check if the job is still running."""
+ if isinstance(self.process, Popen):
+ self.process.poll()
+ return self.process.returncode is None
+ return self.process.exitcode is None
+
+ @property
+ def is_success(self) -> bool:
+ """Check if the job was successful."""
+ if isinstance(self.process, Popen):
+ return self.process.returncode == 0
Review Comment:
The method `poll` should be called on the `Popen` process to update the
`returncode` before checking it. Without calling `poll`, the `returncode` may
not be updated correctly.
```suggestion
self.process.poll()
```
##########
providers/tests/edge/cli/test_edge_command.py:
##########
@@ -116,6 +135,28 @@ def worker_with_job(self, tmp_path: Path, dummy_joblist:
list[_Job]) -> _EdgeWor
test_worker.jobs = dummy_joblist
return test_worker
+ @patch("airflow.providers.edge.cli.edge_command.Process")
+ @patch("airflow.providers.edge.cli.edge_command.logs_logfile_path")
+ @patch("airflow.providers.edge.cli.edge_command.Popen")
+ def test_launch_job(self, mock_popen, mock_logfile_path, mock_process,
worker_with_job: _EdgeWorkerCli):
+ mock_popen.side_effect = ["dummy"]
Review Comment:
The side effect for mock_popen should be an instance of Popen, not a string.
This will cause the test to fail when it tries to access attributes of the
Popen instance.
```suggestion
mock_popen.side_effect = [MagicMock()]
```
##########
providers/src/airflow/providers/edge/worker_api/auth.py:
##########
@@ -112,4 +112,7 @@ def jwt_token_authorization_rest(
request: Request, authorization: str = Header(description="JWT
Authorization Token")
):
"""Check if the JWT token is correct for REST API requests."""
- jwt_token_authorization(request.url.path, authorization)
+ PREFIX = "/edge_worker/v1/"
+ path = request.url.path
+ method_path = path[path.find(PREFIX) + len(PREFIX) :]
Review Comment:
The slicing logic for extracting the method path may fail if PREFIX is not
found in the path, resulting in an incorrect method path. Consider adding a
check to ensure PREFIX is present in the path before slicing.
```suggestion
method_path = path[path.find(PREFIX) + len(PREFIX) :] if PREFIX in path
else None
```
--
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]
