Copilot commented on code in PR #64863:
URL: https://github.com/apache/airflow/pull/64863#discussion_r3066496171
##########
dev/breeze/src/airflow_breeze/utils/workflow_status.py:
##########
@@ -93,7 +94,7 @@ def get_failed_jobs(run_id: int) -> list[str]:
)
if result.returncode != 0:
console.print(f"[red]Error fetching failed jobs:
{result.stderr}[/red]")
- return []
+ return [FAILED_JOBS_LOOKUP_ERROR]
Review Comment:
Returning a human-readable error string inside `list[str]` mixes two
different concepts (job names vs. lookup failure) and can confuse downstream
consumers that expect real job identifiers (e.g., rendering, comparisons, or
any logic that assumes entries map to actual jobs). A more robust approach is
to represent lookup failure explicitly (e.g., raise an exception that the
caller catches and treats as 'unknown', or change the return type to `list[str]
| None` / a small result object like `{jobs: list[str], lookup_ok: bool}`), so
the notification logic can avoid false recovery without treating an error
message as a job.
##########
dev/breeze/src/airflow_breeze/utils/workflow_status.py:
##########
@@ -31,6 +31,7 @@
from rich.console import Console
console = Console(width=400, color_system="standard")
+FAILED_JOBS_LOOKUP_ERROR = "ERROR: failed to fetch failed jobs; inspect the
workflow run directly"
Review Comment:
This message is likely user-facing (it may be displayed wherever failed jobs
are shown) but it lacks actionable context such as the run id and/or the
command that failed. Consider including the `run_id` (and/or a short hint like
`gh run view <run_id> --json jobs`) in the propagated error so the recipient
can immediately inspect the right workflow run without searching.
##########
dev/breeze/src/airflow_breeze/utils/workflow_status.py:
##########
@@ -31,6 +31,7 @@
from rich.console import Console
console = Console(width=400, color_system="standard")
+FAILED_JOBS_LOOKUP_ERROR = "ERROR: failed to fetch failed jobs; inspect the
workflow run directly"
Review Comment:
Returning a human-readable error string inside `list[str]` mixes two
different concepts (job names vs. lookup failure) and can confuse downstream
consumers that expect real job identifiers (e.g., rendering, comparisons, or
any logic that assumes entries map to actual jobs). A more robust approach is
to represent lookup failure explicitly (e.g., raise an exception that the
caller catches and treats as 'unknown', or change the return type to `list[str]
| None` / a small result object like `{jobs: list[str], lookup_ok: bool}`), so
the notification logic can avoid false recovery without treating an error
message as a job.
--
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]
