This is an automated email from the ASF dual-hosted git repository. potiuk pushed a commit to branch auto-approve-workflow-runs in repository https://gitbox.apache.org/repos/asf/airflow.git
commit 14d55dad1d68d808cdc3c395ee3255970edf51a4 Author: Shahar Epstein <[email protected]> AuthorDate: Fri Oct 23 20:55:17 2026 +0300 Keep one PR's API or git error from aborting the approval tick Each PR is now processed in its own try block. A requests.RequestException or subprocess.CalledProcessError for one PR no longer stops the remaining PRs or skips the job summary. The failed PR is recorded with failed=True and shown as "failed" in the summary, keeping its CI-time estimate when one was computed and giving a short, single-line form of the error as the reason. The workflow also tries to add the 'need workflow approval' label to it, so it is not lost once its runs leave the query window. Nothing is approved on failure. approve_workflow_runs exits 1 after writing the summary when any PR failed, so a persistent failure turns the scheduled run red instead of passing silently. --- contributing-docs/25_maintainer_pr_triage.md | 2 +- dev/breeze/doc/ci/05_workflows.md | 6 +- .../src/airflow_breeze/commands/ci_commands.py | 4 + .../airflow_breeze/utils/workflow_run_approval.py | 85 +++++++-- dev/breeze/tests/test_workflow_run_approval.py | 196 ++++++++++++++++++++- 5 files changed, 267 insertions(+), 26 deletions(-) diff --git a/contributing-docs/25_maintainer_pr_triage.md b/contributing-docs/25_maintainer_pr_triage.md index ad6ea684d37..e0bb3599f68 100644 --- a/contributing-docs/25_maintainer_pr_triage.md +++ b/contributing-docs/25_maintainer_pr_triage.md @@ -238,7 +238,7 @@ The triage workflow uses the following labels and states: | Label / State | Meaning | |---|---| | `ready for maintainer review` | PR has passed Stage 1 and is queued for human review. Applied only after the skill has verified that no workflow runs are awaiting approval. | -| `need workflow approval` | The PR's CI workflow runs wait for a maintainer, because the `Auto approve workflow runs` workflow did not approve them: a PR to a branch other than `main`, a change to CI definition files, a full-tests or dependency change, a change whose CI jobs are estimated at more than 310 job-minutes, or a change of more than 100 lines of source code (see [Pull Requests](05_pull_requests.rst)). Approve the runs after looking at the diff. The workflow removes the label w [...] +| `need workflow approval` | The PR's CI workflow runs wait for a maintainer, because the `Auto approve workflow runs` workflow did not approve them: a PR to a branch other than `main`, a change to CI definition files, a full-tests or dependency change, a change whose CI jobs are estimated at more than 310 job-minutes, or a change of more than 100 lines of source code (see [Pull Requests](05_pull_requests.rst)). The workflow also tries to add the label when processing the PR failed; its [...] | `closed because of multiple quality violations` | PR was closed because the author has multiple open PRs with quality issues. | | `suspicious changes detected` | PR (and all open PRs by the same author) was closed because the diff contained suspicious patterns (secret exfiltration, malicious CI modifications, etc.). | | `closed because of open PR limit` | PR was closed in the one-time closure that introduced the [open PR limit](32_open_pull_request_limit.rst). Applied by `dev/close_prs_over_open_pr_limit.py`, never by the triage skill. If the author reopens the PR, triage it like any other. | diff --git a/dev/breeze/doc/ci/05_workflows.md b/dev/breeze/doc/ci/05_workflows.md index b243f7afd85..066c8b9a013 100644 --- a/dev/breeze/doc/ci/05_workflows.md +++ b/dev/breeze/doc/ci/05_workflows.md @@ -124,8 +124,10 @@ approves a backlog of older waiting runs at once; those stay with maintainers. A head of several open PRs is never approved automatically, because its runs cannot be told apart by PR, and only runs started after the PR was opened and after its base branch last changed are approved (retargeting a PR does not start new runs). Other PRs get the -`need workflow approval` label and wait for a maintainer. The label is removed when -a later commit of the PR is approved automatically. +`need workflow approval` label and wait for a maintainer. When processing a PR fails (for example on +a GitHub API error), the workflow tries to add the label to it and the workflow run fails, so the +failure stays visible even if the label could not be added. The label is removed when a later commit +of the PR is approved automatically. The workflow uses only `main`'s code. It reads the PR's metadata and fetches its merge commit as git objects to list the changed files; it never checks out or runs the PR's code. The merge commit must diff --git a/dev/breeze/src/airflow_breeze/commands/ci_commands.py b/dev/breeze/src/airflow_breeze/commands/ci_commands.py index f2c19b0c810..b7d2943b483 100644 --- a/dev/breeze/src/airflow_breeze/commands/ci_commands.py +++ b/dev/breeze/src/airflow_breeze/commands/ci_commands.py @@ -1847,3 +1847,7 @@ def approve_workflow_runs( created_since = datetime.now(UTC) - timedelta(hours=max_run_age_hours) results = process_waiting_runs(api, created_since=created_since) write_step_summary(results, github_repository) + failed_prs = [result.pr_number for result in results if result.failed] + if failed_prs: + console_print(f"[error]Processing failed for PRs: {', '.join(f'#{n}' for n in failed_prs)}[/]") + sys.exit(1) diff --git a/dev/breeze/src/airflow_breeze/utils/workflow_run_approval.py b/dev/breeze/src/airflow_breeze/utils/workflow_run_approval.py index 1f59b9e948b..68646cf1536 100644 --- a/dev/breeze/src/airflow_breeze/utils/workflow_run_approval.py +++ b/dev/breeze/src/airflow_breeze/utils/workflow_run_approval.py @@ -32,6 +32,7 @@ from __future__ import annotations import os import re +import subprocess from dataclasses import dataclass, field from datetime import datetime from typing import Any @@ -51,6 +52,8 @@ NEED_WORKFLOW_APPROVAL_LABEL = "need workflow approval" # ``airflow-ctl/v0-1-test``, ...) run those branches' own, older workflows and always need a maintainer. AUTO_APPROVABLE_BASE_BRANCH = "main" +MAX_ERROR_TEXT_LENGTH = 200 + # Changes here can change which runner a job uses (``runs-on:``, ``runner-type`` computed by # selective checks, the scripts build-info runs) or what the workflows do, so they always need a # maintainer's look. @@ -410,6 +413,7 @@ class PullRequestResult: head_sha: str run_ids: list[int] decision: ApprovalDecision + failed: bool = False def process_waiting_runs(api: GitHubApi, *, created_since: datetime) -> list[PullRequestResult]: @@ -447,28 +451,54 @@ def process_waiting_runs(api: GitHubApi, *, created_since: datetime) -> list[Pul ) continue for pr in matching_prs: - if len(matching_prs) > 1: - # The runs cannot be told apart by PR, so one PR's decision must not approve them all. - decision = ApprovalDecision( - approve=False, reasons=["its head commit is the head of several open PRs"] - ) - else: - decision = decide_for_pull_request(api=api, pr=pr) - if decision.approve: - run_ids = find_runs_started_for_current_pr(api=api, pr=pr, runs=head_runs) - if not run_ids: + failed = False + decision: ApprovalDecision | None = None + try: + if len(matching_prs) > 1: + # The runs cannot be told apart by PR, so one PR's decision must not approve them all. decision = ApprovalDecision( - approve=False, - reasons=["its waiting runs started before the PR was opened or retargeted"], - estimate=decision.estimate, + approve=False, reasons=["its head commit is the head of several open PRs"] ) - run_ids = [run["id"] for run in head_runs] + else: + decision = decide_for_pull_request(api=api, pr=pr) + pr_run_ids = run_ids + if decision.approve: + pr_run_ids = find_runs_started_for_current_pr(api=api, pr=pr, runs=head_runs) + if not pr_run_ids: + decision = ApprovalDecision( + approve=False, + reasons=["its waiting runs started before the PR was opened or retargeted"], + estimate=decision.estimate, + ) + pr_run_ids = run_ids + apply_decision(api=api, pr=pr, run_ids=pr_run_ids, decision=decision) + except (requests.RequestException, subprocess.CalledProcessError) as e: + # One failing PR must not stop the others; the failure is reported through ``failed``. + failed = True + console_print(f"[warning]PR #{pr['number']}: processing failed: {escape(str(e))}[/]") + pr_run_ids = run_ids + # Without the label a PR that keeps failing drops out of the query window unnoticed. + try: + if not has_need_workflow_approval_label(pr): + api.add_label(pr["number"], NEED_WORKFLOW_APPROVAL_LABEL) + except requests.RequestException as label_error: + console_print( + f"[warning]PR #{pr['number']}: adding the label failed: {escape(str(label_error))}[/]" + ) + decision = ApprovalDecision( + approve=False, + reasons=[build_failure_reason(e)], + estimate=decision.estimate if decision else None, + ) results.append( PullRequestResult( - pr_number=pr["number"], head_sha=head_sha, run_ids=run_ids, decision=decision + pr_number=pr["number"], + head_sha=head_sha, + run_ids=pr_run_ids, + decision=decision, + failed=failed, ) ) - apply_decision(api=api, pr=pr, run_ids=run_ids, decision=decision) return results @@ -514,11 +544,25 @@ def decide_for_pull_request(*, api: GitHubApi, pr: dict[str, Any]) -> ApprovalDe ) +def build_failure_reason(error: Exception) -> str: + text = " ".join(str(error).split()) + if len(text) > MAX_ERROR_TEXT_LENGTH: + # The end of a failed git command holds its exit status, so keep both ends. + half = (MAX_ERROR_TEXT_LENGTH - len("...")) // 2 + text = f"{text[:half]}...{text[-half:]}" + reason = f"processing failed with {type(error).__name__}" + return f"{reason}: {text}" if text else reason + + +def has_need_workflow_approval_label(pr: dict[str, Any]) -> bool: + return any(label["name"] == NEED_WORKFLOW_APPROVAL_LABEL for label in pr.get("labels", [])) + + def apply_decision( *, api: GitHubApi, pr: dict[str, Any], run_ids: list[int], decision: ApprovalDecision ) -> None: pr_number = pr["number"] - has_label = any(label["name"] == NEED_WORKFLOW_APPROVAL_LABEL for label in pr.get("labels", [])) + has_label = has_need_workflow_approval_label(pr) console_print(f"[info]PR #{pr_number}: {format_estimate(decision.estimate)}[/]") if decision.approve: console_print( @@ -562,7 +606,12 @@ def write_step_summary(results: list[PullRequestResult], github_repository: str) "|---|---|---|---|---|", ] for result in results: - decision = "approved" if result.decision.approve else f"`{NEED_WORKFLOW_APPROVAL_LABEL}`" + if result.failed: + decision = "failed" + elif result.decision.approve: + decision = "approved" + else: + decision = f"`{NEED_WORKFLOW_APPROVAL_LABEL}`" lines.append( f"| [#{result.pr_number}](https://github.com/{github_repository}/pull/{result.pr_number}) " f"| `{result.head_sha[:10]}` | {decision} | {format_estimate(result.decision.estimate)} " diff --git a/dev/breeze/tests/test_workflow_run_approval.py b/dev/breeze/tests/test_workflow_run_approval.py index e472bc8c777..59c4783095d 100644 --- a/dev/breeze/tests/test_workflow_run_approval.py +++ b/dev/breeze/tests/test_workflow_run_approval.py @@ -21,18 +21,24 @@ from datetime import UTC, datetime from unittest import mock import pytest +import requests +from click.testing import CliRunner +from airflow_breeze.commands.ci_commands import approve_workflow_runs from airflow_breeze.utils.ci_minutes_estimate import CiMinutesEstimate, EstimatedJobGroup from airflow_breeze.utils.selective_checks import SelectiveChecks +from airflow_breeze.utils.shared_options import set_dry_run from airflow_breeze.utils.workflow_run_approval import ( MAX_AUTO_APPROVED_JOB_MINUTES, MAX_AUTO_APPROVED_SOURCE_LINES, + MAX_ERROR_TEXT_LENGTH, NEED_WORKFLOW_APPROVAL_LABEL, ApprovalDecision, ChangedFile, GitHubApi, PullRequestResult, apply_decision, + build_failure_reason, build_selective_checks, count_source_lines, decide_approval, @@ -433,6 +439,28 @@ def test_write_step_summary_lists_expected_jobs_and_time(tmp_path, monkeypatch): assert "| Provider tests (1 providers, 74 test files) | 110 | 16 |" in text +def test_write_step_summary_shows_failed_pull_requests_as_failed(tmp_path, monkeypatch): + summary = tmp_path / "summary.md" + monkeypatch.setenv("GITHUB_STEP_SUMMARY", str(summary)) + + write_step_summary( + [ + PullRequestResult( + pr_number=10, + head_sha="abcdef0123456789", + run_ids=[1], + decision=ApprovalDecision(approve=False, reasons=["processing failed"]), + failed=True, + ) + ], + "apache/airflow", + ) + + text = summary.read_text() + assert "| `abcdef0123` | failed |" in text + assert NEED_WORKFLOW_APPROVAL_LABEL not in text + + @pytest.mark.parametrize( ("paths", "approve"), [ @@ -530,11 +558,6 @@ def test_process_waiting_runs_does_not_approve_a_commit_shared_by_several_prs(mo def test_approve_workflow_runs_command_never_writes_in_dry_run( mock_api, mock_process, mock_summary, args, report_only ): - from click.testing import CliRunner - - from airflow_breeze.commands.ci_commands import approve_workflow_runs - from airflow_breeze.utils.shared_options import set_dry_run - try: result = CliRunner().invoke(approve_workflow_runs, ["--github-token", "token", *args]) finally: @@ -615,3 +638,166 @@ def test_escape_markdown(): assert escape_markdown("it changes .github/[x](http://evil)|`y`") == ( "it changes .github/\\[x\\]\\(http://evil\\)\\|\\`y\\`" ) + + [email protected]( + "error", + [ + pytest.param(requests.HTTPError("502 Server Error"), id="http-error"), + pytest.param(requests.ConnectionError("reset"), id="connection-error"), + pytest.param(subprocess.CalledProcessError(128, ["git", "diff"]), id="git-error"), + ], +) [email protected](f"{MODULE}.decide_for_pull_request", autospec=True) +def test_process_waiting_runs_continues_after_a_pull_request_fails(mock_decide, error): + api = mock.MagicMock(spec=GitHubApi) + api.github_repository = "apache/airflow" + api.get_last_base_change.return_value = None + api.get_runs_waiting_for_approval.return_value = [_run(1, "aaa"), _run(2, "bbb")] + api.get_open_pull_requests.return_value = [_pr(10, "aaa"), _pr(11, "bbb")] + mock_decide.side_effect = [error, ApprovalDecision(approve=True, reasons=["cheap"])] + + results = process_waiting_runs(api, created_since=CREATED_SINCE) + + assert [(r.pr_number, r.decision.approve, r.failed) for r in results] == [ + (10, False, True), + (11, True, False), + ] + assert type(error).__name__ in results[0].decision.reasons[0] + assert api.approve_run.call_args_list == [mock.call(2)] + + [email protected](f"{MODULE}.decide_for_pull_request", autospec=True) +def test_process_waiting_runs_records_a_failure_while_approving_as_not_approved(mock_decide): + api = mock.MagicMock(spec=GitHubApi) + api.github_repository = "apache/airflow" + api.get_last_base_change.return_value = None + api.get_runs_waiting_for_approval.return_value = [_run(1, "aaa"), _run(2, "aaa")] + api.get_open_pull_requests.return_value = [_pr(10, "aaa")] + api.approve_run.side_effect = [None, requests.HTTPError("409 Conflict")] + mock_decide.return_value = ApprovalDecision(approve=True, reasons=["cheap"]) + + results = process_waiting_runs(api, created_since=CREATED_SINCE) + + assert [(r.pr_number, r.run_ids, r.decision.approve, r.failed) for r in results] == [ + (10, [1, 2], False, True) + ] + assert api.approve_run.call_args_list == [mock.call(1), mock.call(2)] + api.add_label.assert_called_once_with(10, NEED_WORKFLOW_APPROVAL_LABEL) + + +def _api_with_prs(prs, runs): + api = mock.MagicMock(spec=GitHubApi) + api.github_repository = "apache/airflow" + api.get_last_base_change.return_value = None + api.get_runs_waiting_for_approval.return_value = runs + api.get_open_pull_requests.return_value = prs + return api + + [email protected](f"{MODULE}.decide_for_pull_request", autospec=True) +def test_process_waiting_runs_labels_failed_pull_requests_that_lack_the_label_without_approving(mock_decide): + api = _api_with_prs( + [_pr(10, "aaa"), _pr(11, "bbb", labels=[NEED_WORKFLOW_APPROVAL_LABEL])], + [_run(1, "aaa"), _run(2, "bbb")], + ) + mock_decide.side_effect = requests.HTTPError("502 Server Error") + + results = process_waiting_runs(api, created_since=CREATED_SINCE) + + assert [(r.failed, r.decision.approve) for r in results] == [(True, False), (True, False)] + assert api.add_label.call_args_list == [mock.call(10, NEED_WORKFLOW_APPROVAL_LABEL)] + api.approve_run.assert_not_called() + + [email protected](f"{MODULE}.decide_for_pull_request", autospec=True) +def test_process_waiting_runs_survives_a_failure_to_label_a_failed_pull_request(mock_decide): + api = _api_with_prs([_pr(10, "aaa"), _pr(11, "bbb")], [_run(1, "aaa"), _run(2, "bbb")]) + api.add_label.side_effect = requests.ConnectionError("reset") + mock_decide.side_effect = [ + requests.HTTPError("502 Server Error"), + ApprovalDecision(approve=True, reasons=["cheap"]), + ] + + results = process_waiting_runs(api, created_since=CREATED_SINCE) + + assert [(r.pr_number, r.failed) for r in results] == [(10, True), (11, False)] + assert api.add_label.call_args_list == [mock.call(10, NEED_WORKFLOW_APPROVAL_LABEL)] + assert api.approve_run.call_args_list == [mock.call(2)] + + [email protected](f"{MODULE}.decide_for_pull_request", autospec=True) +def test_process_waiting_runs_keeps_each_failed_pull_requests_own_estimate(mock_decide): + api = _api_with_prs([_pr(10, "aaa"), _pr(11, "bbb")], [_run(1, "aaa"), _run(2, "bbb")]) + api.approve_run.side_effect = requests.HTTPError("409 Conflict") + estimate = CiMinutesEstimate(job_groups=[]) + mock_decide.side_effect = [ + ApprovalDecision(approve=True, reasons=["cheap"], estimate=estimate), + requests.HTTPError("502 Server Error"), + ] + + results = process_waiting_runs(api, created_since=CREATED_SINCE) + + assert [(r.pr_number, r.failed) for r in results] == [(10, True), (11, True)] + assert results[0].decision.estimate is estimate + assert results[1].decision.estimate is None + + [email protected]( + ("error", "expected_reason"), + [ + pytest.param( + requests.HTTPError("502 Server\nError"), + "processing failed with HTTPError: 502 Server Error", + id="whitespace-collapsed", + ), + pytest.param( + requests.ConnectionError(), + "processing failed with ConnectionError", + id="empty-message", + ), + ], +) [email protected](f"{MODULE}.decide_for_pull_request", autospec=True) +def test_process_waiting_runs_describes_the_error_in_the_failure_reason(mock_decide, error, expected_reason): + api = _api_with_prs([_pr(10, "aaa")], [_run(1, "aaa")]) + mock_decide.side_effect = error + + results = process_waiting_runs(api, created_since=CREATED_SINCE) + + assert results[0].decision.reasons == [expected_reason] + + +def test_build_failure_reason_keeps_the_end_of_a_long_error(): + error = subprocess.CalledProcessError(128, ["git", "diff", "x" * 1000]) + + reason = build_failure_reason(error) + + assert reason.startswith("processing failed with CalledProcessError: Command '['git', 'diff'") + assert reason.endswith("returned non-zero exit status 128.") + assert "..." in reason + assert len(reason) == len("processing failed with CalledProcessError: ") + MAX_ERROR_TEXT_LENGTH - 1 + + [email protected](("failed", "exit_code"), [(False, 0), (True, 1)]) [email protected](f"{MODULE}.write_step_summary", autospec=True) [email protected](f"{MODULE}.process_waiting_runs", autospec=True) [email protected](f"{MODULE}.GitHubApi", autospec=True) +def test_approve_workflow_runs_command_exits_non_zero_after_summary_when_a_pr_failed( + mock_api, mock_process, mock_summary, failed, exit_code +): + results = [ + PullRequestResult( + pr_number=10, + head_sha="abc", + run_ids=[1], + decision=ApprovalDecision(approve=False, reasons=["processing failed"]), + failed=failed, + ) + ] + mock_process.return_value = results + + result = CliRunner().invoke(approve_workflow_runs, ["--github-token", "token"]) + + assert result.exit_code == exit_code, result.output + mock_summary.assert_called_once_with(results, mock.ANY)
