This is an automated email from the ASF dual-hosted git repository. potiuk pushed a commit to branch fork-ci-test-merge-with-target in repository https://gitbox.apache.org/repos/asf/airflow.git
commit 302d22d73c0dddfaf3d95733f844b3c95063e193 Author: Jarek Potiuk <[email protected]> AuthorDate: Tue Nov 10 05:40:41 2026 -0700 Report fork CI's tested base and withhold CI passed on an old base Fork CI tests the head commit of a gated pull request, while project CI tests it merged into the target branch, so a pull request could go green on a stale base and merge on that result. When a head commit turns green, the fork CI monitor now compares it with the base branch: the merge base it was tested on, how many commits the base branch has since, and which of the pull request's files the base branch changed since. `CI passed` is reported only when the base is fewer than 200 commits behind and changed none of those files; otherwise it is posted as pending with a rebase hint, and the state comment lists the overlapping files. Every green state comment and the `CI passed` description name the base commit the run tested. Testing the merge commit in the fork itself was rejected: it would have to change every checkout of the fork run, there is no merge ref before the pull request exists, GitHub recomputes it asynchronously, and the monitor would have to trust the run's own report of what it tested. The design document lists the reasons. Generated-by: Claude Opus 5 --- contributing-docs/18_contribution_workflow.rst | 6 + .../doc/ci/08_fork_ci_for_external_contributors.md | 68 +++++++-- .../src/airflow_breeze/utils/fork_ci_messages.py | 101 ++++++++++--- .../src/airflow_breeze/utils/fork_ci_reconcile.py | 83 +++++++--- dev/breeze/tests/test_fork_ci_messages.py | 99 ++++++++++-- dev/breeze/tests/test_fork_ci_reconcile.py | 167 ++++++++++++++++----- 6 files changed, 421 insertions(+), 103 deletions(-) diff --git a/contributing-docs/18_contribution_workflow.rst b/contributing-docs/18_contribution_workflow.rst index 663808a4ed3..065dfb0cab1 100644 --- a/contributing-docs/18_contribution_workflow.rst +++ b/contributing-docs/18_contribution_workflow.rst @@ -188,6 +188,12 @@ does not run for 30 days is closed with an explanation; you can reopen it after files that decide which CI jobs run and where, such as ``.github/``, the breeze sources or ``scripts/ci/prek/``, a green fork run is not enough: a maintainer reviews the change and runs project CI for it. +Fork CI tests your branch as it is, while project CI tests it merged with the target branch. The +``CI passed`` check that merging requires is therefore reported only when your branch is fewer than +200 commits behind the target branch and the target branch has not changed any file your pull request +changes since your branch's base. Otherwise the pull request comment lists those files and asks you +to rebase on ``upstream/main`` (or the target branch) and push, which runs fork CI again. + **Joining the pilot.** Open a pull request that adds your GitHub login to the ``[pilot]`` table in ``.github/fork-ci-gate.toml``, with a join instant after the expected merge, for example the next day at 00:00 UTC: 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 1e73184cf61..abe89ba3a7c 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 @@ -1013,6 +1013,8 @@ is the source the ruleset accepts. pending or failing one would say, and it never competes with project CI's own check. - `green_unverified` gets no `CI passed`: the fork run used CI files the pull request changed, so it is not evidence. Once a maintainer adds `use project ci`, project CI reports the check. +- A `green` run whose base is not current gets a `pending` `CI passed` asking to rebase, and one + whose base could not be compared gets none (see "Tested base"). - When the head commit leaves `green`, for example a re-run in the fork that fails, a `success` posted earlier is overwritten with `pending` so the pull request cannot merge on it. - A pull request released with `use project ci` keeps any `success` the reconciler posted for @@ -1076,14 +1078,46 @@ conditions: completed, first attempt, its commit does not change `.github/workfl did project CI work by the tampering check's rules. Of its jobs only the gate and the jobs that also run in a gated run started, none failed, and at least one was cancelled. -### Far behind the base branch - -A fork run tests the pushed commit as it is, while project CI tests it merged with the base -branch. When a head commit turns green, the reconciler counts the commits it is behind its base -branch (`behind_by` of `GET /repos/apache/airflow/compare/<base>...<fork-owner>:<sha>`, once per -head commit, stored in the marker). From 200 commits behind, about four days of `main`, the -state comment says so and asks the contributor to rebase, since the merged result can still -fail. The count is advisory and does not change the state. +### Tested base + +A fork run tests the pushed commit as it is, while project CI's `pull_request` run tests GitHub's +merge commit of the pull request into its base branch. A fork run on an old base could otherwise go +green and merge although the merged result was never tested. So `CI passed` is only reported for a +fork run whose base is close enough to the base branch that the merge changes nothing the pull +request touches. + +When a head commit turns green, the reconciler compares it with the base branch, once per head +commit, and stores the result in the marker as `"base": {"sha", "behind", "overlap"}`: + +- `GET /repos/apache/airflow/compare/<base>...<fork-owner>:<sha>` gives the merge base, the commit + of the base branch the fork run tested on top of (`sha`), the commits the base branch has since + (`behind`), and the files the pull request changes. +- When `behind` is not zero, `GET /repos/apache/airflow/compare/<merge base>...<base>` gives the + files the base branch changed since. `overlap` is the paths in both lists, a renamed file + counting under both paths. + +`CI passed` is reported only when the base is current: fewer than `MAX_BEHIND_COMMITS` (200, about +four days of `main`) commits behind and no overlap. Otherwise the state stays `green` (the pull +request can be reviewed, and its status, labels and draft state are those of `green`), the state +comment lists the overlapping files or says how far behind the base is and gives the rebase +commands, and `CI passed` is posted as `pending` with a description saying to rebase. A rebase and +push starts a new fork run, which is compared again when it turns green. Every green state comment +names the base commit the run tested and how far behind it was, and the `CI passed` description +names it as well. + +The compare API lists at most 300 files. A pull request with more is listed with +`GET /repos/apache/airflow/pulls/<n>/files` instead (up to 3000); a base branch that changed 300 or +more files since the merge base cannot be checked and counts as overlapping, which a rebase +resolves. When the compare fails, `CI passed` is not reported, the failure is reported to Slack and +the next run compares again. A marker written before this check, with no `base`, is compared again +on the next run, so a pull request that already had `CI passed` loses it if its base turns out not +to be current. A retarget compares again, as it decides green trust again. + +This narrows the gap to project CI without closing it. A base change in other files can still break +the pull request, within at most 200 commits, which matches what project CI leaves open after its +run: a `pull_request` run is not repeated when the base branch moves, so the merged result at merge +time is not the one it tested either. Testing the merge commit in the fork itself was considered and +rejected; see "Known limitations". ### Unknown failures and attention items go to Slack @@ -1130,8 +1164,8 @@ Per run, with ~700 gated pull requests: ~28 GraphQL calls for pass 2, ~14 for fo ~1 for pass 1, plus one REST call per pull request in `setup_required`, a few paginated REST calls for the recent `pull_request` runs, one REST call per base branch for its workflow files, a jobs call and a compare call only per long `ci-amd.yml` run of a gated pull request, -one changed-files call per head commit that turns green, plus one mutation per actual state -change. Job statuses add a jobs call and a status call per pull request with a fork run in +one changed-files call and one or two compare calls per head commit that turns green, plus one +mutation per actual state change. Job statuses add a jobs call and a status call per pull request with a fork run in progress or a completed run not yet reported, and up to two status mutations per job, at most `MAX_JOB_STATUSES_PER_RUN` per run; see "Write budget". Pull requests that pass 1 labelled and the search does not list yet add one GraphQL call per 25. Well inside @@ -1335,6 +1369,17 @@ as a failed check rather than a crash. - An external pull request that legitimately changes workflow files, was released with `use project ci` and then had that label removed is closed on the next run, because both signals hold. Keeping `use project ci` on such a pull request is the intended handling. +- Fork CI tests the head commit, not the pull request merged into its base branch; "Tested base" + only bounds the difference. Testing the merged result in the fork was rejected because: + every job of the fork run checks out the pushed commit (about 60 `actions/checkout` steps across + `ci-amd.yml` and its reusable workflows, plus the image builds and caches keyed by the commit), + and each would have to check out one merge commit fixed for the whole run; the first push of a + branch happens before its pull request exists, so there is no `refs/pull/<n>/merge` to test yet; + GitHub computes that ref asynchronously after each push and replaces it whenever the base branch + moves, so a run would have to wait for it and could find its commit gone mid-run; and the + reconciler would have to trust the run's own report of which merge it tested. Merging the base + branch into the head in every job instead needs the shared history of both, which shallow + checkouts do not have. ## Testing @@ -1351,7 +1396,8 @@ as a failed check rather than a crash. close), a fork with a name other than `airflow`, marker round-trips, status desired-vs-current comparison, the unknown-failure collection, green trust (a change to CI-defining files keeps the status pending without drafting the pull request, decided once - per commit) and the gate + per commit), the tested base (`CI passed` only on a current base, the marker and comment naming + the base, compared again for a new commit, a retarget or a marker without a base) and the gate tampering check: a tampered run on an earlier commit is caught after a clean push, each signal alone does not close, short runs and gate-only jobs do not count, an in-progress non-gate job, a removed, duplicated or long-running gate job and an injected workflow with a 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 a68acd8d8ea..d4e5ec44cab 100644 --- a/dev/breeze/src/airflow_breeze/utils/fork_ci_messages.py +++ b/dev/breeze/src/airflow_breeze/utils/fork_ci_messages.py @@ -43,7 +43,6 @@ JOB_NAME_MAX_LENGTH = 100 # CI's gate job reports it; for a gated pull request, which never runs project CI, the reconciler # reports it from the fork run. CI_PASSED_CONTEXT = "CI passed" -CI_PASSED_DESCRIPTION = "Fork CI passed for this commit" CI_PASSED_REVOKED_DESCRIPTION = "Fork CI result no longer trusted for this commit" # Also required by the merge rulesets, so GitHub lists it as expected and waiting on every pull # request from the moment it is pushed. The reconciler reports it, linking the run: for a gated pull @@ -51,9 +50,12 @@ CI_PASSED_REVOKED_DESCRIPTION = "Fork CI result no longer trusted for this commi CI_RUN_STARTED_CONTEXT = "CI run started" CI_RUN_STARTED_DESCRIPTION = "CI runs in the contributor's fork - details open the run" CI_RUN_STARTED_PROJECT_DESCRIPTION = "Project CI runs in apache/airflow - details open the run" -# A fork run tests the head commit as it is, not merged with the base branch. Further behind than -# this, it says little about the result after merge. -BEHIND_WARNING_COMMITS = 200 +# A fork run tests the head commit as it is, not merged with the base branch as project CI does. From +# this many commits behind, `CI passed` is withheld until the branch is rebased, as it is when the +# base branch changed any file the pull request changes. +MAX_BEHIND_COMMITS = 200 +# Overlapping files named in the state comment; the marker keeps them all. +OVERLAP_LISTED = 10 MARKER_PREFIX = "<!-- fork-ci-gate: " MARKER_PATTERN = re.compile(r"<!-- fork-ci-gate: (?P<payload>.*?) -->") DESIGN_URL = ( @@ -82,13 +84,34 @@ WHY_FORK_CI = ( ) +@dataclass(frozen=True) +class BaseCheck: + """ + The base a green fork run tested, compared with the base branch when the run turned green. + + :param sha: the merge base of the head commit and the base branch, the base the run tested. + :param behind: commits on the base branch since ``sha``. + :param overlap: paths that both the pull request and the base branch since ``sha`` change; + None when GitHub listed too many changed files on either side to tell. + """ + + sha: str + behind: int + overlap: tuple[str, ...] | None = () + + @property + def is_current(self) -> bool: + """Whether the run tested close enough to the merged result for `CI passed`.""" + return self.behind < MAX_BEHIND_COMMITS and self.overlap == () + + @dataclass(frozen=True) class Marker: state: str sha: str since: datetime - # Commits the head commit was behind its base branch when its fork CI turned green. - behind: int | None = None + # Set when the head commit's fork CI turned green and its base could be compared. + base: BaseCheck | None = None # The completed fork run (``<run id>:<attempt>:<conclusion>``) whose jobs are all reported. jobs: str | None = None # Set only by a reconciler that ran the gate protection check for this head commit, so a @@ -98,8 +121,12 @@ class Marker: def render_marker(marker: Marker) -> str: data: dict[str, object] = {"state": marker.state, "sha": marker.sha, "since": marker.since.isoformat()} - if marker.behind is not None: - data["behind"] = marker.behind + if marker.base is not None: + data["base"] = { + "sha": marker.base.sha, + "behind": marker.base.behind, + "overlap": None if marker.base.overlap is None else list(marker.base.overlap), + } if marker.jobs is not None: data["jobs"] = marker.jobs if marker.gate_checked: @@ -114,11 +141,19 @@ def parse_marker(body: str) -> Marker | None: return None try: data = json.loads(match.group("payload")) + # A marker written before the base was checked carries only "behind"; its base is checked again. + base = data.get("base") return Marker( state=data["state"], sha=data["sha"], since=datetime.fromisoformat(data["since"]), - behind=data.get("behind"), + base=None + if base is None + else BaseCheck( + sha=base["sha"], + behind=base["behind"], + overlap=None if base["overlap"] is None else tuple(base["overlap"]), + ), jobs=data.get("jobs"), gate_checked=data.get("gate_checked") is True, ) @@ -178,25 +213,53 @@ _AUTHOR_DRAFT_TEXTS = { } -def behind_warning(behind: int, *, base_ref: str) -> str: +def tested_base_text(base: BaseCheck, *, base_ref: str) -> str: + text = ( + f"Fork CI tested this branch on top of `{base_ref}` at " + f"[`{base.sha[:10]}`](https://github.com/apache/airflow/commit/{base.sha}), " + f"{base.behind} commits behind `{base_ref}` when the run turned green. Fork CI tests the branch " + f"as it is, while project CI would test it merged with the current `{base_ref}`." + ) + if base.is_current: + return f"{text} No commit since changed a file this pull request changes." + if base.overlap is None: + reason = f"`{base_ref}` changed too many files since to tell whether it changed the same files." + elif base.overlap: + listed = ", ".join(f"`{path}`" for path in base.overlap[:OVERLAP_LISTED]) + more = len(base.overlap) - OVERLAP_LISTED + reason = f"`{base_ref}` changed files this pull request changes since: {listed}" + ( + f" and {more} more." if more > 0 else "." + ) + else: + reason = "That is too far behind to say much about the merged result." return ( - f"**This branch is {behind} commits behind `{base_ref}`.** Fork CI tests the branch as it is, " - f"while the merged result also contains those commits, so it can still fail after the merge. " - f"Rebase on `{base_ref}` and push to test it against the current code." + f"{text}\n\n**Rebase needed.** {reason} The `CI passed` check is not reported until fork CI is " + f"green for a branch rebased on `{base_ref}`:\n\n" + f"```bash\ngit fetch upstream\ngit rebase upstream/{base_ref}\ngit push --force-with-lease\n```" ) +def ci_passed_description(base: BaseCheck, *, base_ref: str) -> str: + return f"Fork CI passed; base {base_ref}@{base.sha[:10]} was {base.behind} commits behind, no overlap" + + +def rebase_needed_description(base: BaseCheck, *, base_ref: str) -> str: + if base.overlap is None: + why = f"{base_ref} changed too many files since to check" + elif base.overlap: + why = f"{base_ref} changed {len(base.overlap)} of the same files since" + else: + why = f"base is {base.behind} commits behind" + return f"Rebase on {base_ref} and push: {why}" + + def state_comment( marker: Marker, *, fork: str, run_url: str | None, base_ref: str = "main", author_draft: bool = False ) -> str: text = _AUTHOR_DRAFT_TEXTS.get(marker.state) if author_draft else None parts = [render_marker(marker), text or _STATE_TEXTS[marker.state], WHY_FORK_CI] - if ( - marker.state in ("green", "green_unverified") - and marker.behind is not None - and marker.behind >= BEHIND_WARNING_COMMITS - ): - parts.append(behind_warning(marker.behind, base_ref=base_ref)) + if marker.state in ("green", "green_unverified") and marker.base is not None: + parts.append(tested_base_text(marker.base, base_ref=base_ref)) if run_url: parts.append(f"Latest run: {run_url}") if marker.state == "setup_required": 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 6d666772e38..7b3324d4d0c 100644 --- a/dev/breeze/src/airflow_breeze/utils/fork_ci_reconcile.py +++ b/dev/breeze/src/airflow_breeze/utils/fork_ci_reconcile.py @@ -59,7 +59,6 @@ from airflow_breeze.utils.fork_ci_gate import ( from airflow_breeze.utils.fork_ci_github import GitHubClient, GitHubError from airflow_breeze.utils.fork_ci_messages import ( CI_PASSED_CONTEXT, - CI_PASSED_DESCRIPTION, CI_PASSED_REVOKED_DESCRIPTION, CI_RUN_STARTED_CONTEXT, CI_RUN_STARTED_DESCRIPTION, @@ -69,13 +68,16 @@ from airflow_breeze.utils.fork_ci_messages import ( STALE_JOB_STATUS, STATUS_CONTEXT, STATUS_STATES, + BaseCheck, Marker, + ci_passed_description, fork_actions_url, gate_modified_close_comment, inactivity_close_comment, job_status_context, nudge_comment, parse_marker, + rebase_needed_description, released_comment, state_comment, status_description, @@ -580,17 +582,24 @@ def _set_status( plan.status = desired -def _set_ci_passed(plan: Plan, pr: PullRequestSnapshot, *, passed: bool, target_url: str) -> None: +def _set_ci_passed( + plan: Plan, pr: PullRequestSnapshot, state: ForkState, base: BaseCheck | None, *, target_url: str +) -> None: """ Report fork CI to the `CI passed` check the merge ruleset requires. - Only success is ever posted: a gated pull request without the status cannot merge, which is - what a pending or failing one would say. A success that no longer holds for the head commit, - such as a failed re-run in the fork, is overwritten so the pull request cannot merge on it. + Success is posted only for a trusted green run whose base is current enough; a gated pull + request without the status cannot merge. A green run on an old base gets a pending status that + says to rebase. A success that no longer holds for the head commit, such as a failed re-run in + the fork, is overwritten so the pull request cannot merge on it. """ - if passed and pr.ci_passed_state != "success": - plan.ci_passed = ("success", target_url, CI_PASSED_DESCRIPTION) - elif not passed and pr.ci_passed_state == "success": + if state == ForkState.GREEN and base is not None and base.is_current: + if pr.ci_passed_state != "success": + plan.ci_passed = ("success", target_url, ci_passed_description(base, base_ref=pr.base_ref)) + elif state == ForkState.GREEN and base is not None: + if pr.ci_passed_state != "pending": + plan.ci_passed = ("pending", target_url, rebase_needed_description(base, base_ref=pr.base_ref)) + elif pr.ci_passed_state == "success": plan.ci_passed = ("pending", target_url, CI_PASSED_REVOKED_DESCRIPTION) @@ -614,7 +623,7 @@ def classify( repo: str, find_bypass_run: Callable[[], str | None], changes_ci_files: Callable[[], bool | None], - count_behind: Callable[[], int | None] = lambda: None, + compare_base: Callable[[], BaseCheck | None] = lambda: None, jobs_reported: str | None = None, head_shared_across_bases: bool = False, find_gate_change: Callable[[], str | None] = lambda: None, @@ -630,8 +639,9 @@ def classify( cannot be told for the snapshot's head commit; only called when a new head commit turns green in the fork. On None nothing is changed, since the decision would be recorded on a commit the answer may not belong to. - :param count_behind: how many commits the head commit is behind the base branch, None when - unknown; called at the same time as ``changes_ci_files``. + :param compare_base: compares the head commit's base with the base branch, None when that + cannot be read; called when a new head commit turns green, alongside ``changes_ci_files``, + and on later runs until it succeeds. :param jobs_reported: the completed fork run whose jobs are all reported as statuses, recorded in the marker so that later runs do not read its jobs again. :param head_shared_across_bases: another gated pull request has the same head commit and a @@ -754,7 +764,7 @@ def classify( plan.unknown.append(unknown_reason or "unknown fork state") return plan refresh_status = False - behind = marker.behind if marker and marker.sha == pr.head_sha else None + base = marker.base if marker and marker.sha == pr.head_sha else None if state == ForkState.GREEN: # The decision is read back from the commit status on the head commit, which only # apache/airflow's own tokens can write, never from a comment. The changed files depend @@ -784,9 +794,16 @@ def classify( if changes is None: return Plan() unverified = changes - behind = count_behind() + base = None if unverified: state = ForkState.GREEN_UNVERIFIED + if base is None: + base = compare_base() + if base is None and state == ForkState.GREEN: + plan.unknown.append( + f"fork CI is green, but the head commit could not be compared with `{pr.base_ref}`, " + "so `CI passed` waits for the next run" + ) plan.state = state since = marker.since if marker and marker.state == state.value else now @@ -822,7 +839,7 @@ def classify( target_url = fork.run_url or fork_actions_url(pr.head_repo) _set_status(plan, pr, state.value, target_url, force=refresh_status) - _set_ci_passed(plan, pr, passed=state == ForkState.GREEN, target_url=target_url) + _set_ci_passed(plan, pr, state, base, target_url=target_url) _set_ci_run_started(plan, pr, fork) green = state in (ForkState.GREEN, ForkState.GREEN_UNVERIFIED) _set_marker_comment( @@ -833,7 +850,7 @@ def classify( state.value, pr.head_sha, since, - behind=behind if green else None, + base=base if green else None, jobs=jobs_reported, gate_checked=gate_checked, ), @@ -2059,17 +2076,37 @@ def pull_request_changes_ci_files(client: GitHubClient, repo: str, number: int, return changes_ci_definition(changed_paths(files), file_count=len(files)) -def count_commits_behind(client: GitHubClient, repo: str, pr: PullRequestSnapshot) -> int | None: - """Commits on the base branch that the head commit does not have; None when not readable.""" +def compare_with_base(client: GitHubClient, repo: str, pr: PullRequestSnapshot) -> BaseCheck | None: + """ + Compare the base a fork run tested with the base branch; None when not readable. + + The fork run tests the head commit, so its base is the merge base with the base branch. Two + three-dot compares from it give the files the pull request changes and the files the base branch + changed since; a file in both means the merged result differs from what the fork run tested + in a file the pull request changes. Compares list at most 300 files (on their first page, which + ``per_page`` does not limit): a pull request with more is listed with the files endpoint instead, + while a base branch that changed more cannot be checked until the branch is rebased. + """ owner = (pr.head_repo or "").split("/", 1)[0] + base_ref = quote(pr.base_ref, safe="") try: - compare = client.rest_get( - f"repos/{repo}/compare/{quote(pr.base_ref, safe='')}...{owner}:{pr.head_sha}?per_page=1" + head_side = client.rest_get(f"repos/{repo}/compare/{base_ref}...{owner}:{pr.head_sha}?per_page=1") + merge_base = head_side["merge_base_commit"]["sha"] + behind = head_side["behind_by"] + base_side = ( + client.rest_get(f"repos/{repo}/compare/{merge_base}...{base_ref}?per_page=1") if behind else {} ) - except GitHubError as e: - console_print(f"[warning]#{pr.number}: could not count commits behind {pr.base_ref}: {e}[/]") + head_files = head_side.get("files") or [] + if len(head_files) >= COMPARE_FILES_CAP: + head_files = client.rest_get(f"repos/{repo}/pulls/{pr.number}/files?per_page=100", paginate=True) + except (GitHubError, KeyError, TypeError) as e: + console_print(f"[warning]#{pr.number}: could not compare with {pr.base_ref}: {e}[/]") return None - return compare.get("behind_by") + base_files = base_side.get("files") or [] + if len(head_files) >= PULL_REQUEST_FILES_CAP or len(base_files) >= COMPARE_FILES_CAP: + return BaseCheck(merge_base, behind, None) + overlap = set(changed_paths(head_files)) & set(changed_paths(base_files)) + return BaseCheck(merge_base, behind, tuple(sorted(overlap))) def find_heads_shared_across_bases(prs: Iterable[PullRequestSnapshot]) -> set[str]: @@ -2403,7 +2440,7 @@ def run_reconcile( changes_ci_files=functools.partial( pull_request_changes_ci_files, client, repo, pr.number, pr.head_sha ), - count_behind=functools.partial(count_commits_behind, client, repo, pr), + compare_base=functools.partial(compare_with_base, client, repo, pr), jobs_reported=jobs_reported, head_shared_across_bases=pr.head_sha in shared_heads, find_gate_change=functools.partial(gate_change_finder.find, pr), diff --git a/dev/breeze/tests/test_fork_ci_messages.py b/dev/breeze/tests/test_fork_ci_messages.py index 8d565cbecb4..b9b564120eb 100644 --- a/dev/breeze/tests/test_fork_ci_messages.py +++ b/dev/breeze/tests/test_fork_ci_messages.py @@ -25,24 +25,27 @@ import pytest from airflow_breeze.utils.fork_ci_messages import ( _STATE_TEXTS, _STATUS_DESCRIPTIONS, - BEHIND_WARNING_COMMITS, CI_CHANGE_MARKER, CI_MODIFICATION_MARKER, - CI_PASSED_DESCRIPTION, CI_PASSED_REVOKED_DESCRIPTION, FOOTER, GATED_NOTE_SENTENCE, JOB_NAME_MAX_LENGTH, JOB_STATUS_CONTEXT_PREFIX, + MAX_BEHIND_COMMITS, MAX_LISTED_CI_FILES, + OVERLAP_LISTED, + BaseCheck, Marker, ci_change_approval_comment, ci_modification_note_comment, + ci_passed_description, gate_modified_close_comment, inactivity_close_comment, job_status_context, nudge_comment, parse_marker, + rebase_needed_description, released_comment, render_marker, state_comment, @@ -57,8 +60,9 @@ MARKER = Marker(state="awaiting_push", sha="abc123", since=datetime(2026, 10, 15 @pytest.mark.parametrize( "marker", [ - pytest.param(MARKER, id="without-behind"), - pytest.param(replace(MARKER, behind=12), id="behind"), + pytest.param(MARKER, id="without-base"), + pytest.param(replace(MARKER, base=BaseCheck("def456", 12, ("a.py",))), id="base"), + pytest.param(replace(MARKER, base=BaseCheck("def456", 12, None)), id="base-unchecked"), pytest.param(replace(MARKER, jobs="123:SUCCESS"), id="jobs"), pytest.param(replace(MARKER, gate_checked=True), id="gate-checked"), ], @@ -67,27 +71,81 @@ def test_marker_round_trip(marker): assert parse_marker(f"text before\n{render_marker(marker)}\ntext after") == marker -def test_marker_without_behind_keeps_its_rendering(): - assert "behind" not in render_marker(MARKER) +def test_marker_without_base_keeps_its_rendering(): + assert "base" not in render_marker(MARKER) assert "jobs" not in render_marker(MARKER) +def test_marker_from_before_the_base_check_has_no_base(): + marker = parse_marker( + '<!-- fork-ci-gate: {"behind": 3, "sha": "a", "since": "2026-10-15T10:00:00+00:00", "state": "green"} -->' + ) + + assert marker is not None + assert marker.base is None + + @pytest.mark.parametrize( - ("state", "behind", "warned"), + ("base", "current"), [ - pytest.param("green", BEHIND_WARNING_COMMITS, True, id="green-far-behind"), - pytest.param("green_unverified", BEHIND_WARNING_COMMITS + 1, True, id="unverified-far-behind"), - pytest.param("green", BEHIND_WARNING_COMMITS - 1, False, id="green-close"), - pytest.param("green", None, False, id="green-not-counted"), - pytest.param("failed", BEHIND_WARNING_COMMITS, False, id="failed"), + pytest.param(BaseCheck("d", MAX_BEHIND_COMMITS - 1), True, id="close-no-overlap"), + pytest.param(BaseCheck("d", MAX_BEHIND_COMMITS), False, id="far-behind"), + pytest.param(BaseCheck("d", 1, ("a.py",)), False, id="overlap"), + pytest.param(BaseCheck("d", 1, None), False, id="unchecked"), + ], +) +def test_base_check_is_current(base, current): + assert base.is_current is current + + [email protected]( + ("state", "base", "expected", "unexpected"), + [ + pytest.param( + "green", + BaseCheck("d" * 40, 3), + [ + "`v3-1-test` at [`dddddddddd`](https://github.com/apache/airflow/commit/"; + "d" * 40, + "3 commits", + ], + ["Rebase needed"], + id="current", + ), + pytest.param( + "green", + BaseCheck("d", MAX_BEHIND_COMMITS), + ["Rebase needed", "too far behind", "git rebase upstream/v3-1-test"], + [], + id="far-behind", + ), + pytest.param( + "green_unverified", + BaseCheck("d", 4, ("a.py", "b.py")), + ["Rebase needed", "`a.py`, `b.py`."], + [], + id="overlap", + ), + pytest.param( + "green", + BaseCheck("d", 4, tuple(f"f{i:02}.py" for i in range(OVERLAP_LISTED + 2))), + [f"`f{OVERLAP_LISTED - 1:02}.py` and 2 more."], + [f"`f{OVERLAP_LISTED:02}.py`"], + id="overlap-shortened", + ), + pytest.param( + "green", BaseCheck("d", 4, None), ["Rebase needed", "too many files"], [], id="unchecked" + ), + pytest.param("failed", BaseCheck("d", MAX_BEHIND_COMMITS), [], ["tested this branch"], id="failed"), + pytest.param("green", None, [], ["tested this branch"], id="not-compared"), ], ) -def test_state_comment_warns_about_a_branch_far_behind(state, behind, warned): +def test_state_comment_says_which_base_was_tested(state, base, expected, unexpected): text = state_comment( - Marker(state, "a", MARKER.since, behind=behind), fork="u/airflow", run_url=None, base_ref="v3-1-test" + Marker(state, "a", MARKER.since, base=base), fork="u/airflow", run_url=None, base_ref="v3-1-test" ) - assert ("commits behind `v3-1-test`" in text) is warned + assert all(part in text for part in expected) + assert not any(part in text for part in unexpected) @pytest.mark.parametrize( @@ -164,7 +222,16 @@ def test_setup_texts_give_the_exact_command(text): def test_status_descriptions_fit_github_limit(): - descriptions = [*_STATUS_DESCRIPTIONS.values(), CI_PASSED_DESCRIPTION, CI_PASSED_REVOKED_DESCRIPTION] + base_ref = "providers-cncf-kubernetes/v10-22" + descriptions = [ + *_STATUS_DESCRIPTIONS.values(), + CI_PASSED_REVOKED_DESCRIPTION, + ci_passed_description(BaseCheck("d" * 40, 199), base_ref=base_ref), + *( + rebase_needed_description(BaseCheck("d" * 40, 9999, overlap), base_ref=base_ref) + for overlap in ((), ("a",) * 300, None) + ), + ] assert all(len(description) < 140 for description in descriptions) diff --git a/dev/breeze/tests/test_fork_ci_reconcile.py b/dev/breeze/tests/test_fork_ci_reconcile.py index 7b02221f062..cac8c08b9b5 100644 --- a/dev/breeze/tests/test_fork_ci_reconcile.py +++ b/dev/breeze/tests/test_fork_ci_reconcile.py @@ -36,6 +36,7 @@ from airflow_breeze.utils.fork_ci_messages import ( JOB_STATUS_CONTEXT_PREFIX, STALE_JOB_STATUS, STATUS_CONTEXT, + BaseCheck, Marker, parse_marker, render_marker, @@ -60,8 +61,8 @@ from airflow_breeze.utils.fork_ci_reconcile import ( build_slack_payload, changes_ci_definition, classify, + compare_with_base, compute_projection, - count_commits_behind, discover, drop_reported, fetch_fork_observations, @@ -97,6 +98,8 @@ CONFIG = GateConfig(enabled=True, enabled_since=datetime(2026, 10, 1, tzinfo=UTC RUN_URL = "https://github.com/someone/airflow/actions/runs/1"; PROJECT_RUN_URL = "https://github.com/apache/airflow/actions/runs/2"; GREEN = ForkObservation(suite_status="COMPLETED", suite_conclusion="SUCCESS", run_url=RUN_URL) +MERGE_BASE = "def4567890aa" +CURRENT_BASE = BaseCheck(MERGE_BASE, 3) def _pr(**overrides) -> PullRequestSnapshot: @@ -129,7 +132,7 @@ def _classify( bypass_run=None, changes_ci_files=False, config=CONFIG, - behind=None, + base=CURRENT_BASE, head_shared_across_bases=False, gate_change=None, ): @@ -143,7 +146,7 @@ def _classify( repo=REPO, find_bypass_run=find_bypass, changes_ci_files=changes, - count_behind=mock.Mock(return_value=behind), + compare_base=mock.Mock(return_value=base), head_shared_across_bases=head_shared_across_bases, find_gate_change=lambda: gate_change, ) @@ -472,66 +475,116 @@ UNVERIFIED_STATUS = ( ) -CI_PASSED = ("success", RUN_URL, "Fork CI passed for this commit") +CI_PASSED = ("success", RUN_URL, "Fork CI passed; base main@def4567890 was 3 commits behind, no overlap") CI_PASSED_REVOKED = ("pending", RUN_URL, "Fork CI result no longer trusted for this commit") +OVERLAPPING_BASE = BaseCheck(MERGE_BASE, 3, ("a.py",)) +REBASE_NEEDED = ("pending", RUN_URL, "Rebase on main and push: main changed 1 of the same files since") @pytest.mark.parametrize( - ("fork", "changes_ci_files", "ci_passed_state", "expected"), + ("fork", "changes_ci_files", "base", "ci_passed_state", "expected"), [ - pytest.param(GREEN, False, None, CI_PASSED, id="green-posts-success"), - pytest.param(GREEN, False, "success", None, id="green-already-posted"), - pytest.param(GREEN, False, "pending", CI_PASSED, id="green-after-revoke"), - pytest.param(GREEN, True, None, None, id="unverified-gets-none"), + pytest.param(GREEN, False, CURRENT_BASE, None, CI_PASSED, id="green-posts-success"), + pytest.param(GREEN, False, CURRENT_BASE, "success", None, id="green-already-posted"), + pytest.param(GREEN, False, CURRENT_BASE, "pending", CI_PASSED, id="green-after-revoke"), + pytest.param(GREEN, True, CURRENT_BASE, None, None, id="unverified-gets-none"), + pytest.param(GREEN, False, OVERLAPPING_BASE, None, REBASE_NEEDED, id="old-base-asks-to-rebase"), + pytest.param(GREEN, False, OVERLAPPING_BASE, "success", REBASE_NEEDED, id="old-base-revokes"), + pytest.param(GREEN, False, OVERLAPPING_BASE, "pending", None, id="old-base-already-pending"), + pytest.param(GREEN, False, None, None, None, id="base-not-compared-gets-none"), + pytest.param(GREEN, False, None, "success", CI_PASSED_REVOKED, id="base-not-compared-revokes"), pytest.param( ForkObservation(suite_status="COMPLETED", suite_conclusion="FAILURE", run_url=RUN_URL), False, + CURRENT_BASE, "success", CI_PASSED_REVOKED, id="failed-rerun-revokes", ), - pytest.param(GREEN, True, "success", CI_PASSED_REVOKED, id="unverified-after-retarget-revokes"), + pytest.param( + GREEN, True, CURRENT_BASE, "success", CI_PASSED_REVOKED, id="unverified-after-retarget-revokes" + ), pytest.param( ForkObservation(suite_status="IN_PROGRESS", run_url=RUN_URL), False, + CURRENT_BASE, None, None, id="running-gets-none", ), ], ) -def test_ci_passed_reports_trusted_green_only(fork, changes_ci_files, ci_passed_state, expected): - plan, _, _ = _classify(_pr(ci_passed_state=ci_passed_state), fork, changes_ci_files=changes_ci_files) +def test_ci_passed_reports_trusted_green_on_a_current_base_only( + fork, changes_ci_files, base, ci_passed_state, expected +): + plan, _, _ = _classify( + _pr(ci_passed_state=ci_passed_state), fork, changes_ci_files=changes_ci_files, base=base + ) assert plan.ci_passed == expected +def test_green_whose_base_cannot_be_compared_is_reported(): + plan, _, _ = _classify(_pr(), GREEN, base=None) + + assert plan.state == ForkState.GREEN + assert any("could not be compared with `main`" in item for item in plan.unknown) + + +KEPT_BASE = BaseCheck("kept", 300, None) + + @pytest.mark.parametrize( - ("pr_overrides", "behind", "expected_behind", "warned"), + ("pr_overrides", "compared", "expected_base"), [ - pytest.param({}, 250, 250, True, id="counted-on-decision"), - pytest.param({}, 3, 3, False, id="close-to-base"), + pytest.param({}, True, CURRENT_BASE, id="compared-on-decision"), pytest.param( - {"current_status": GREEN_STATUS, "marker": Marker("green", "abc123", NOW, behind=300)}, - None, - 300, - True, + {"current_status": GREEN_STATUS, "marker": Marker("green", "abc123", NOW, base=KEPT_BASE)}, + False, + KEPT_BASE, id="kept-from-the-marker", ), pytest.param( - {"current_status": GREEN_STATUS, "marker": Marker("green", "old-sha", NOW, behind=300)}, - None, - None, - False, + {"current_status": GREEN_STATUS, "marker": Marker("green", "old-sha", NOW, base=KEPT_BASE)}, + True, + CURRENT_BASE, id="not-kept-for-another-commit", ), + pytest.param( + {"current_status": GREEN_STATUS, "marker": Marker("green", "abc123", NOW)}, + True, + CURRENT_BASE, + id="compared-for-a-marker-without-base", + ), + pytest.param( + { + "current_status": GREEN_STATUS, + "marker": Marker("green", "abc123", NOW, base=KEPT_BASE), + "last_base_change_at": NOW, + "status_created_at": NOW - timedelta(hours=1), + }, + True, + CURRENT_BASE, + id="compared-again-after-retarget", + ), ], ) -def test_green_records_how_far_behind_the_base_branch(pr_overrides, behind, expected_behind, warned): - plan, _, _ = _classify(_pr(**pr_overrides), GREEN, behind=behind) +def test_green_records_the_base_it_tested(pr_overrides, compared, expected_base): + compare_base = mock.Mock(return_value=CURRENT_BASE) + plan = classify( + _pr(**pr_overrides), + GREEN, + config=CONFIG, + now=NOW, + repo=REPO, + find_bypass_run=lambda: None, + changes_ci_files=lambda: False, + compare_base=compare_base, + ) - assert parse_marker(plan.marker_comment).behind == expected_behind - assert ("commits behind `main`" in plan.marker_comment) is warned + assert compare_base.called is compared + assert parse_marker(plan.marker_comment).base == expected_base + assert f"[`{expected_base.sha[:10]}`]" in plan.marker_comment @pytest.mark.parametrize( @@ -2481,18 +2534,64 @@ def test_apply_plan_posts_ci_passed_under_its_own_context(client): assert mock.call(42, "abc123", *CI_PASSED, context="CI passed") in client.set_status.call_args_list +def _compare(behind, files, merge_base=MERGE_BASE): + return {"merge_base_commit": {"sha": merge_base}, "behind_by": behind, "files": files} + + +def _files(*paths): + return [{"filename": path} for path in paths] + + +HEAD_COMPARE = f"repos/{REPO}/compare/providers-fab%2Fv1-5...someone:abc123?per_page=1" +BASE_COMPARE = f"repos/{REPO}/compare/{MERGE_BASE}...providers-fab%2Fv1-5?per_page=1" +PR_FILES = f"repos/{REPO}/pulls/42/files?per_page=100" + + @pytest.mark.parametrize( - ("side_effect", "expected"), + ("responses", "expected", "expected_calls"), [ - pytest.param([{"behind_by": 17}], 17, id="counted"), - pytest.param(GitHubError("gh api failed: HTTP 500"), None, id="unreadable"), + pytest.param( + [_compare(0, _files("a.py"))], BaseCheck(MERGE_BASE, 0, ()), [HEAD_COMPARE], id="up-to-date" + ), + pytest.param( + [_compare(5, _files("a.py", "b.py")), _compare(0, _files("c.py"))], + BaseCheck(MERGE_BASE, 5, ()), + [HEAD_COMPARE, BASE_COMPARE], + id="no-overlap", + ), + pytest.param( + [ + _compare(5, [*_files("b.py"), {"filename": "new.py", "previous_filename": "a.py"}]), + _compare(0, _files("b.py", "a.py", "c.py")), + ], + BaseCheck(MERGE_BASE, 5, ("a.py", "b.py")), + [HEAD_COMPARE, BASE_COMPARE], + id="overlap-with-rename", + ), + pytest.param( + [_compare(5, _files("a.py")), _compare(0, _files(*(f"f{i}" for i in range(300))))], + BaseCheck(MERGE_BASE, 5, None), + [HEAD_COMPARE, BASE_COMPARE], + id="base-side-at-cap", + ), + pytest.param( + [ + _compare(5, _files(*(f"f{i}" for i in range(300)))), + _compare(0, _files("g.py", "f301")), + _files(*(f"f{i}" for i in range(400))), + ], + BaseCheck(MERGE_BASE, 5, ("f301",)), + [HEAD_COMPARE, BASE_COMPARE, PR_FILES], + id="large-pull-request-listed-in-full", + ), + pytest.param(GitHubError("gh api failed: HTTP 500"), None, [HEAD_COMPARE], id="unreadable"), ], ) -def test_count_commits_behind(client, side_effect, expected): - client.rest_get.side_effect = side_effect +def test_compare_with_base(client, responses, expected, expected_calls): + client.rest_get.side_effect = responses - assert count_commits_behind(client, REPO, _pr(base_ref="v3-1-test")) == expected - assert client.rest_get.call_args.args[0] == f"repos/{REPO}/compare/v3-1-test...someone:abc123?per_page=1" + assert compare_with_base(client, REPO, _pr(base_ref="providers-fab/v1-5")) == expected + assert [call.args[0] for call in client.rest_get.call_args_list] == expected_calls def _fork_job(name, status="completed", conclusion="success", url=None):
