This is an automated email from the ASF dual-hosted git repository. potiuk pushed a commit to branch backport-ci-sync-v3-4-test in repository https://gitbox.apache.org/repos/asf/airflow.git
commit dc6650c4c2a39f1287a62b7de2d7afcfdaf6069e Author: Jarek Potiuk <[email protected]> AuthorDate: Sun Oct 25 19:41:10 2026 +0100 Delete the project CI runs that only gate a pull request to fork CI (#75588) * Delete the project CI runs that only gate a pull request to fork CI A gated Tests (AMD) run lists every project CI job as a skipped check on the pull request, next to the fork's result. GitHub cannot delete or update single Actions check runs, so the fork CI monitor deletes the whole run once only the gate ran in it. Generated-by: Claude Opus 5 * Keep gated runs whose commit changes workflows as tampering evidence Generated-by: Claude Opus 5 * Tell maintainers to close and reopen a gated pull request to start project CI The fork CI monitor now deletes gated runs, so there is no run left to re-run after adding use project ci. Generated-by: Claude Opus 5 * Keep re-run gated runs, whose earlier attempts may have run project CI Generated-by: Claude Opus 5 (cherry picked from commit daa2a86fdbd69a1cd4039e59f5941daaa7f18eba) --- .github/workflows/fork-ci-monitor.yml | 8 +- contributing-docs/25_maintainer_pr_triage.md | 7 +- .../doc/ci/08_fork_ci_for_external_contributors.md | 28 +++-- .../src/airflow_breeze/utils/fork_ci_github.py | 7 ++ .../src/airflow_breeze/utils/fork_ci_messages.py | 3 +- .../src/airflow_breeze/utils/fork_ci_reconcile.py | 69 ++++++++++-- dev/breeze/tests/test_fork_ci_github.py | 10 ++ dev/breeze/tests/test_fork_ci_reconcile.py | 119 +++++++++++++++++++++ 8 files changed, 232 insertions(+), 19 deletions(-) diff --git a/.github/workflows/fork-ci-monitor.yml b/.github/workflows/fork-ci-monitor.yml index 2613f32a07b..e94f8fd0457 100644 --- a/.github/workflows/fork-ci-monitor.yml +++ b/.github/workflows/fork-ci-monitor.yml @@ -18,7 +18,8 @@ --- # Drives pull requests whose CI runs in contributor forks (AIP-120): labels, draft state, the # state comment, the `Fork CI / Tests (AMD)` commit status and closing. It is the only writer for -# those pull requests and never checks out or runs pull request code. See +# those pull requests, and deletes their project CI runs that only skipped every job. It never checks +# out or runs pull request code. See # dev/breeze/doc/ci/08_fork_ci_for_external_contributors.md. name: "Fork CI monitor" on: # yamllint disable-line rule:truthy @@ -31,8 +32,9 @@ on: # yamllint disable-line rule:truthy type: boolean default: false permissions: - # Listing the runs and jobs of the gate tampering check; unlisted scopes are none. - actions: read + # Listing the runs and jobs of the gate tampering check, and deleting the runs that only gated a + # pull request to fork CI; unlisted scopes are none. + actions: write contents: read # Labels, draft state, comments and closing of gated pull requests. pull-requests: write diff --git a/contributing-docs/25_maintainer_pr_triage.md b/contributing-docs/25_maintainer_pr_triage.md index ab12c1f877d..e5f5313c5b7 100644 --- a/contributing-docs/25_maintainer_pr_triage.md +++ b/contributing-docs/25_maintainer_pr_triage.md @@ -267,9 +267,10 @@ Everything else the skill does (quality criteria, pings, suspicious-change flags usual. The `Fork CI / Tests (AMD)` commit status shows the fork CI state and links to the run in the fork. To move a single pull request back to project CI, add `use project ci`; the reconciler releases it on its next run; removing `fork ci required` by hand is not enough, the next run -labels a gated pull request again. Adding the label does not start project CI by itself: re-run -the pull request's latest `Tests (AMD)` run (`gh run rerun <run-id> -R apache/airflow`, or -**Re-run all jobs** on the run's page), or ask the author to push. Pull requests that change CI-defining files stay in draft even +labels a gated pull request again. Adding the label does not start project CI by itself, and the +reconciler deletes the gated `Tests (AMD)` runs, so there is usually nothing to re-run: close and +reopen the pull request (`gh pr close <n> -R apache/airflow && gh pr reopen <n> -R apache/airflow`), +or ask the author to push. Pull requests that change CI-defining files stay in draft even when the fork run is green and are labelled `needs project ci`, for a maintainer to review and run project CI with `use project ci`. When the fork run is green and trusted, the reconciler also posts the `CI passed` status the merge ruleset requires, so these pull requests can be merged or diff --git a/dev/breeze/doc/ci/08_fork_ci_for_external_contributors.md b/dev/breeze/doc/ci/08_fork_ci_for_external_contributors.md index 6e352fea115..effe96065e7 100644 --- a/dev/breeze/doc/ci/08_fork_ci_for_external_contributors.md +++ b/dev/breeze/doc/ci/08_fork_ci_for_external_contributors.md @@ -221,14 +221,16 @@ Consequences for the other components: - The gate job reads the pull request's labels live from the API (one read-only call) rather than from the event payload, so labelling a pull request and pressing "Re-run all jobs" - switches it without a new push. + switches it without a new push. The reconciler deletes gated runs (see "Gated project CI runs + are deleted"), so for a gated pull request closing and reopening it takes the place of the + re-run. - Discovery in the reconciler also picks up open pull requests carrying `use fork ci` without `fork ci required`, regardless of `enabled`. - A pull request carrying both `fork ci required` and `use project ci` is *released*: the reconciler removes `fork ci required`, undrafts it if the bot drafted it, edits the state comment to say project CI is in use, and posts a `success` status with the description - "Project CI in use (label `use project ci`)". Project CI then runs on the next push or - re-run. + "Project CI in use (label `use project ci`)". Project CI then runs on the next push, or when + a maintainer closes and reopens the pull request. - Both override labels on one pull request resolve to project CI and are reported to Slack as a conflict. @@ -690,6 +692,20 @@ is the source the ruleset accepts. job must not report success there; it reports under another name when `build-info` is skipped, and the reconciler's status is the only `CI passed` the pull request has. +### Gated project CI runs are deleted + +A gated run of `ci-amd.yml` lists every project CI job on the pull request as a skipped check, +next to the fork's result in `Fork CI / Tests (AMD)`. GitHub cannot delete or update single check +runs created by GitHub Actions, so the reconciler deletes the whole run once it completed: the +`ci-amd.yml` `pull_request` runs of a gated pull request's branch from the tamper window in which +the gate, `Check if the run is needed` and `CI passed (project CI not run)` succeeded and every +other job was skipped. A run in which anything else happened, including a run the tampering check +counts as project CI work, is kept, and so is a run whose commit changes `.github/workflows/` or +whose change set is too large to check: such a commit could do project CI work under the gate's +name within its time limit, and its run is the evidence. A re-run is kept too: deleting a run +deletes every attempt, and an earlier attempt may have run project CI. Nothing is deleted for a +pull request in the `gate_bypassed` state. A dry run only reports the deletions. + ### Far behind the base branch A fork run tests the pushed commit as it is, while project CI tests it merged with the base @@ -745,11 +761,11 @@ budget stays observable. `.github/workflows/fork-ci-monitor.yml`: - `on: schedule: cron '*/15 * * * *'` and `workflow_dispatch` with a `dry-run` input. -- `permissions: actions: read, contents: read, pull-requests: write, statuses: write` with a +- `permissions: actions: write, contents: read, pull-requests: write, statuses: write` with a justification comment. Comments and labels on pull requests are covered by `pull-requests: write`; the commit status needs `statuses: write`; the gate tampering check - lists runs and their jobs, which needs `actions: read` because every scope a `permissions` - block leaves out is `none`. + lists runs and their jobs, and the reconciler deletes gated runs, which needs `actions: write` + because every scope a `permissions` block leaves out is `none`. - `runs-on: ubuntu-latest` (the breeze setup action frees disk space with `sudo` and `docker`, which `ubuntu-slim` does not provide), `timeout-minutes: 20`, `concurrency: fork-ci-monitor` with `cancel-in-progress: false` so runs never overlap. diff --git a/dev/breeze/src/airflow_breeze/utils/fork_ci_github.py b/dev/breeze/src/airflow_breeze/utils/fork_ci_github.py index 6f6203e7e78..73501627e77 100644 --- a/dev/breeze/src/airflow_breeze/utils/fork_ci_github.py +++ b/dev/breeze/src/airflow_breeze/utils/fork_ci_github.py @@ -187,6 +187,13 @@ class GitHubClient: {"state": "closed"}, ) + def delete_run(self, pr: int, run_id: int) -> None: + self._mutate( + f"#{pr}: delete workflow run {run_id}", + ["api", "-X", "DELETE", f"repos/{self.repo}/actions/runs/{run_id}"], + None, + ) + def set_status( self, pr: int, diff --git a/dev/breeze/src/airflow_breeze/utils/fork_ci_messages.py b/dev/breeze/src/airflow_breeze/utils/fork_ci_messages.py index f3e2cf76f2e..d28e5f54283 100644 --- a/dev/breeze/src/airflow_breeze/utils/fork_ci_messages.py +++ b/dev/breeze/src/airflow_breeze/utils/fork_ci_messages.py @@ -166,7 +166,8 @@ def released_comment(marker: Marker) -> str: return ( f"{render_marker(marker)}\n\n" "**Project CI in use.** A maintainer added the `use project ci` label, so CI for this pull " - "request runs in apache/airflow. It starts on the next push or re-run." + FOOTER + "request runs in apache/airflow. It starts on the next push, or when a maintainer closes and " + "reopens this pull request." + FOOTER ) diff --git a/dev/breeze/src/airflow_breeze/utils/fork_ci_reconcile.py b/dev/breeze/src/airflow_breeze/utils/fork_ci_reconcile.py index 3c4eeafb70d..61b44ff7899 100644 --- a/dev/breeze/src/airflow_breeze/utils/fork_ci_reconcile.py +++ b/dev/breeze/src/airflow_breeze/utils/fork_ci_reconcile.py @@ -88,6 +88,8 @@ GATED_RUN_JOBS = ( frozenset({"CI passed (project CI not run)", "CI passed (run cancelled)", "CI passed"}), ) GATED_RUN_JOB_NAMES = frozenset[str]().union(*GATED_RUN_JOBS) +CHECK_RUN_NEEDED_JOB_NAME = "Check if the run is needed" +PROJECT_CI_NOT_RUN_JOB_NAME = "CI passed (project CI not run)" BOT_LOGINS = frozenset({"github-actions", "github-actions[bot]"}) DISCOVERY_WINDOW = timedelta(hours=24) PASS_TWO_PAGE_SIZE = 25 @@ -156,6 +158,7 @@ class WorkflowRun: updated_at: datetime status: str event: str = "pull_request" + run_attempt: int = 1 @property def workflow_file(self) -> str: @@ -310,6 +313,25 @@ def jobs_show_project_ci(jobs: Iterable[dict[str, Any]], now: datetime) -> bool: ) +def is_gated_skip_run(jobs: Iterable[dict[str, Any]], now: datetime) -> bool: + """ + Whether a completed ci-amd.yml run is only the gate leaving the pull request to fork CI. + + Such a run lists every project CI job as a skipped check on the pull request, next to the + fork's result in the `Fork CI / Tests (AMD)` status. The gate and the check-run-needed job + succeeded, CI passed reported as not run, and every other job was skipped. + """ + jobs = list(jobs) + if jobs_show_project_ci(jobs, now): + return False + succeeded = {GATE_JOB_NAME, CHECK_RUN_NEEDED_JOB_NAME, PROJECT_CI_NOT_RUN_JOB_NAME} + if sorted(job["name"] for job in jobs if job["name"] in succeeded) != sorted(succeeded): + return False + return all( + job.get("conclusion") == ("success" if job["name"] in succeeded else "skipped") for job in jobs + ) + + def touches(files: Iterable[str], prefixes: tuple[str, ...]) -> bool: return any(path.startswith(prefixes) for path in files) @@ -920,6 +942,7 @@ def parse_workflow_run(run: dict[str, Any]) -> WorkflowRun: updated_at=parse_github_datetime(run["updated_at"]), # type: ignore[arg-type] status=run["status"], event=run["event"], + run_attempt=run.get("run_attempt") or 1, ) @@ -988,6 +1011,16 @@ class BypassFinder: ) self._did_project_ci: dict[int, bool] = {} self._changes_workflows: dict[tuple[str, str], bool | None] = {} + self._jobs: dict[int, list[dict[str, Any]]] = {} + + def _get_jobs(self, run: WorkflowRun) -> list[dict[str, Any]]: + if run.id not in self._jobs: + self._jobs[run.id] = self._client.rest_get( + f"repos/{self._repo}/actions/runs/{run.id}/jobs?filter=latest&per_page=100", + paginate=True, + items_key="jobs", + ) + return self._jobs[run.id] def _did_run_project_ci(self, run: WorkflowRun) -> bool: if run.id not in self._did_project_ci: @@ -998,15 +1031,33 @@ class BypassFinder: elif run.workflow_file != CI_WORKFLOW_FILE or not run.is_long_running(self._now): result = False else: - jobs = self._client.rest_get( - f"repos/{self._repo}/actions/runs/{run.id}/jobs?filter=latest&per_page=100", - paginate=True, - items_key="jobs", - ) - result = jobs_show_project_ci(jobs, self._now) + result = jobs_show_project_ci(self._get_jobs(run), self._now) self._did_project_ci[run.id] = result return self._did_project_ci[run.id] + def find_gated_runs(self, pr: PullRequestSnapshot) -> list[WorkflowRun]: + """ + Completed ci-amd.yml pull_request runs of the pull request's branch that only gated it. + + A commit that changes workflows could do project CI work under the gate's name within its + time limit, so its runs are kept as evidence, as are runs whose change set is too large + to check. The jobs read are those of the latest attempt, and deleting a run deletes every + attempt, so a re-run is kept: an earlier attempt may have run project CI. + """ + if not pr.head_repo: + return [] + return [ + run + for run in self._runs_by_branch.get((pr.head_repo.casefold(), pr.head_ref), []) + if run.workflow_file == CI_WORKFLOW_FILE + and run.event == "pull_request" + and run.status == "completed" + and run.run_attempt == 1 + and not self._did_run_project_ci(run) + and is_gated_skip_run(self._get_jobs(run), self._now) + and self._commit_changes_workflows(run, pr.base_ref) is False + ] + def _commit_changes_workflows(self, run: WorkflowRun, base_ref: str) -> bool | None: """Whether the run's commit changes workflows against the base; None when not knowable.""" key = (run.head_sha, base_ref) @@ -1270,6 +1321,12 @@ def run_reconcile( count_behind=functools.partial(count_commits_behind, client, repo, pr), ) apply_plan(client, pr, plan) + # The fork's result is in the `Fork CI / Tests (AMD)` status; the gated run's skipped + # checks only clutter the pull request. Runs with any project CI work stay for the + # tampering check. + if plan.state != ForkState.GATE_BYPASSED: + for run in bypass_finder.find_gated_runs(pr): + client.delete_run(pr.number, run.id) unknown.extend((pr.number, reason) for reason in plan.unknown) attention.extend((pr.number, item) for item in plan.attention) except Exception as e: diff --git a/dev/breeze/tests/test_fork_ci_github.py b/dev/breeze/tests/test_fork_ci_github.py index cf314e8e05d..d993add8bb5 100644 --- a/dev/breeze/tests/test_fork_ci_github.py +++ b/dev/breeze/tests/test_fork_ci_github.py @@ -127,10 +127,20 @@ def test_dry_run_does_not_mutate(mock_run): client.close(1) client.remove_label(1, "fork ci required") + client.delete_run(1, 7) mock_run.assert_not_called() [email protected]("airflow_breeze.utils.fork_ci_github.subprocess.run", autospec=True) +def test_delete_run(mock_run): + mock_run.return_value = _completed() + + GitHubClient().delete_run(1, 7) + + assert mock_run.call_args.args[0] == ["gh", "api", "-X", "DELETE", "repos/apache/airflow/actions/runs/7"] + + @mock.patch("airflow_breeze.utils.fork_ci_github.subprocess.run", autospec=True) def test_remove_label_quotes_the_name(mock_run): mock_run.return_value = _completed() diff --git a/dev/breeze/tests/test_fork_ci_reconcile.py b/dev/breeze/tests/test_fork_ci_reconcile.py index 3f7d56f7c14..aaa5e40bbc6 100644 --- a/dev/breeze/tests/test_fork_ci_reconcile.py +++ b/dev/breeze/tests/test_fork_ci_reconcile.py @@ -51,6 +51,7 @@ from airflow_breeze.utils.fork_ci_reconcile import ( fetch_fork_observations, fetch_workflow_state, find_state_comment, + is_gated_skip_run, jobs_show_project_ci, load_workflow_triggers, parse_pull_request, @@ -781,6 +782,7 @@ def _run( age_hours=1.0, event="pull_request", status="completed", + run_attempt=1, ): created = NOW - timedelta(hours=age_hours) return { @@ -795,6 +797,7 @@ def _run( "updated_at": (created + timedelta(minutes=minutes)).isoformat(), "status": status, "event": event, + "run_attempt": run_attempt, } @@ -989,6 +992,76 @@ def test_finder_reports_an_incomplete_run_listing(client, total_count, incomplet assert (finder.incomplete is not None) is incomplete +GATED_JOBS = [ + _job("Check if the run is needed", "success"), + _job("Fork CI gate", "success"), + _job("Build info", started=False), + _job("CI passed (project CI not run)", "success"), +] + + [email protected]( + ("jobs", "expected"), + [ + pytest.param(GATED_JOBS, True, id="gated"), + pytest.param([*GATED_JOBS[:2], _job("Build info", "success"), GATED_JOBS[3]], False, id="ci-ran"), + pytest.param( + [*GATED_JOBS[:3], _job("CI passed (run cancelled)", "success")], False, id="run-cancelled" + ), + pytest.param(GATED_JOBS[1:], False, id="check-run-needed-missing"), + pytest.param( + [GATED_JOBS[0], _job("Fork CI gate", "failure"), *GATED_JOBS[2:]], False, id="gate-failed" + ), + pytest.param( + [*GATED_JOBS[:2], _job("Build info", "cancelled"), GATED_JOBS[3]], False, id="cancelled" + ), + ], +) +def test_is_gated_skip_run(jobs, expected): + assert is_gated_skip_run(jobs, NOW) is expected + + [email protected]( + ("runs", "jobs", "compare_files", "expected_ids"), + [ + pytest.param([_run(7, minutes=1)], GATED_JOBS, ("README.md",), [7], id="gated"), + pytest.param([_run(7, minutes=1)], TAMPERED_JOBS, ("README.md",), [], id="project-ci-ran"), + pytest.param( + [_run(7, minutes=1, status="in_progress")], GATED_JOBS, ("README.md",), [], id="in-progress" + ), + pytest.param( + [_run(7, minutes=1, path=".github/workflows/codeql-analysis.yml")], + GATED_JOBS, + ("README.md",), + [], + id="other-workflow", + ), + pytest.param( + [_run(7, minutes=1, event="pull_request_review")], + GATED_JOBS, + ("README.md",), + [], + id="other-event", + ), + pytest.param( + [_run(7, minutes=1)], GATED_JOBS, (".github/workflows/ci-amd.yml",), [], id="workflows-changed" + ), + pytest.param([_run(7, minutes=1, run_attempt=2)], GATED_JOBS, ("README.md",), [], id="re-run"), + pytest.param( + [_run(7, minutes=1)], + GATED_JOBS, + tuple(f"f{i}.py" for i in range(300)), + [], + id="too-large-to-check", + ), + ], +) +def test_finder_finds_gated_runs(client, runs, jobs, compare_files, expected_ids): + finder = _finder(client, runs, jobs=jobs, compare_files=compare_files) + + assert [run.id for run in finder.find_gated_runs(_pr())] == expected_ids + + # --------------------------------------------------------------------------------------------- # GitHub data gathering and the run loop # --------------------------------------------------------------------------------------------- @@ -1397,6 +1470,52 @@ def test_run_reconcile_isolates_failures_and_reports_them(client, tmp_path): assert client.set_status.call_args.args[:3] == (2, "sha2", "error") [email protected]( + ("state", "deletes"), + [ + pytest.param(ForkState.AWAITING_PUSH, True, id="gated"), + pytest.param(ForkState.GATE_BYPASSED, False, id="gate-bypassed"), + ], +) [email protected]("airflow_breeze.utils.fork_ci_reconcile.classify", autospec=True) +def test_run_reconcile_deletes_gated_runs(mock_classify, client, tmp_path, state, deletes): + mock_classify.return_value = Plan(state=state) + client.graphql.side_effect = [ + _search_result(), + _search_result(), + _search_result( + _pr_node(1, headRefName="my-change", headRepository={"nameWithOwner": "someone/airflow"}) + ), + {"pr0": {"object": None}}, + ] + client.rest_get_pages.side_effect = lambda path: [ + {"total_count": 1, "workflow_runs": [_run(7, minutes=1)] if "event=pull_request&" in path else []} + ] + + def rest_get(path, paginate=False, items_key=None): + if path.endswith("/actions/workflows/ci-amd.yml"): + return {"state": "active"} + if path.endswith("jobs?filter=latest&per_page=100"): + return GATED_JOBS + if "/compare/" in path: + return {"files": [{"filename": "README.md"}]} + raise AssertionError(path) + + client.rest_get.side_effect = rest_get + (tmp_path / "ci-amd.yml").write_text("on: [pull_request]\n") + + run_reconcile( + client, + CONFIG, + now=NOW, + run_url="https://run";, + output_file=tmp_path / "s.json", + workflows_dir=tmp_path, + ) + + assert client.delete_run.call_args_list == ([mock.call(1, 7)] if deletes else []) + + @mock.patch("airflow_breeze.utils.fork_ci_reconcile.run_reconcile", autospec=True, return_value=True) def test_command_writes_has_alerts_output(mock_run, tmp_path, monkeypatch): github_output = tmp_path / "github_output"
