diff --git a/CHANGELOG.md b/CHANGELOG.md index 062316c..6bbc567 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,17 @@ # Top of tree +* `autoland` now works on branches without a merge queue. It asks GitHub + whether the target branch has one when it starts; without one, it + squash-merges each PR itself once the PR is approved, passing, and + mergeable, with the same commit message `stack-pr land` would use (the + commit's message without the stack-info trailer, numbered with the PR), + pinned to the commit it checked. A failed merge is retried after approval + and CI are checked again (`max_queue_retries`), and a branch that has fallen + behind a target that requires it to be up to date is reported and waited + on. A PR being merged this way shows as `Merging` rather than `In merge + queue`. `autoland.merge_queue` is no longer required: unset (the new + default), the mode is detected; `true` or `false` forces it. Runs on + branches with a merge queue behave as before. * `autoland` plans have a new step, `x `, which runs a command once the PRs before it have landed and stops the land unless it exits 0 — for automating what would otherwise be a manual `c` step, such as provisioning. diff --git a/README.md b/README.md index 8e61f89..a33877f 100644 --- a/README.md +++ b/README.md @@ -137,7 +137,7 @@ This will: ``` 3. Repeat for each PR in the stack -**Option C**: Using `stack-pr autoland` (if your repo uses GitHub merge queue): +**Option C**: Using `stack-pr autoland`: ```bash stack-pr autoland ``` @@ -169,11 +169,11 @@ That's it! `stack-pr` also has several other commands: -- `autoland` - land the whole stack automatically through the GitHub merge - queue: wait for approvals and CI (retrying flaky checks), enqueue each PR - bottom-to-top, rebase and re-submit the rest after each merge, and resume - cleanly after an interruption. Requires a repo that uses the GitHub merge - queue (see `autoland.merge_queue` below). +- `autoland` - land the whole stack automatically: wait for approvals and CI + (retrying flaky checks), merge each PR bottom-to-top — through the GitHub + merge queue where the target branch has one, or directly otherwise — rebase + and re-submit the rest after each merge, and resume cleanly after an + interruption (see `autoland.merge_queue` below). - `config` - set configuration values in the config file. Similar to `git config`, it takes a setting in the format `
.=` and updates the config file (`.stack-pr.cfg` by default). @@ -543,16 +543,31 @@ PR onto the right commit directly with `stack-pr adopt --commit `. #### autoland -Land the entire stack automatically through the GitHub merge queue, one PR at a -time from the bottom up. Run it from the repo root while on the stack's head -branch. For each PR it waits for approval and CI (re-running flaky checks), -adds the PR to the merge queue (retrying if the PR is booted), and after each +Land the entire stack automatically, one PR at a time from the bottom up. Run +it from the repo root while on the stack's head branch. For each PR it waits +for approval and CI (re-running flaky checks), merges the PR, and after each merge rebases and re-submits the rest of the stack. Progress is checkpointed so an interrupted run can be resumed. -> **Note**: `autoland` currently supports only repositories that use the GitHub -> merge queue. On other repositories it fails fast with a "not implemented" -> error; use `stack-pr land` instead. (Direct-merge autoland is future work.) +How a PR is merged depends on the target branch: + +- **With a merge queue**, the PR is added to the queue, and `autoland` waits for + it to merge, re-adding it (after checking approval and CI again) if it is + booted. The queue merges with its own configured method. +- **Without one**, `autoland` squash-merges the PR itself once GitHub reports it + mergeable, with the same commit message `stack-pr land` uses: the commit's + message without the stack-info trailer, with ` (#)` after the subject. + The merge is pinned to the commit `autoland` checked, so a push in between + fails it rather than landing unchecked code. A failed merge is retried (up to + `max_queue_retries` times) after checking approval and CI again. If the + branch must be up to date with the target before merging and has fallen + behind, `autoland` says so and waits for you to rebase and `stack-pr submit` + (or to update the branch on GitHub), then carries on. + +`autoland` asks GitHub which applies when it starts, and prints `Merging +directly into (no merge queue)` when there is no queue. Set +`autoland.merge_queue` to skip the question (see below). If GitHub can't +answer and the config doesn't say, `autoland` stops before doing anything. Options: @@ -644,7 +659,10 @@ max_queue_retries = 3 default_workflow = deploy.yaml ``` -- `merge_queue` (default `false`): must be `true` to enable `autoland`. +- `merge_queue` (default unset): whether the target branch merges through a + merge queue. Unset, `autoland` asks GitHub at the start of each run; `true` + or `false` skips the question and forces that mode — for when GitHub can't + say (e.g. the token can't read the branch's rules), or to skip the lookup. - `required_checks` (default empty): comma-separated CI check names that gate a merge. When empty, all reported (non-skipped) checks must pass. - `default_workflow` (default empty): when set, an interactive (`-i`) landing @@ -668,9 +686,10 @@ instead of one PR at a time: just as when landing them one at a time. 3. It makes one merge request on the top PR of the run, which merges it and every PR below it — all enqueued together in the merge queue, where the - repo has one. Each PR still lands as its own commit (made with the merge - queue's configured method, or squashed when there is no queue), and every - PR is closed as merged. + repo has one, or merged directly otherwise. Each PR still lands as its own + commit (made with the merge queue's configured method, or squashed when + there is no queue, with GitHub's default squash message rather than the + one `stack-pr land` uses), and every PR is closed as merged. 4. It rebases and re-submits the rest of the stack once, rather than after every PR. diff --git a/src/stack_pr/autoland/__init__.py b/src/stack_pr/autoland/__init__.py index ecd490f..2c03016 100644 --- a/src/stack_pr/autoland/__init__.py +++ b/src/stack_pr/autoland/__init__.py @@ -1,4 +1,4 @@ -"""stack-pr autoland: land a whole stack through the GitHub merge queue. +"""stack-pr autoland: land a whole stack, through the merge queue or directly. This package holds the full autoland engine. cli.py only wires up the subparser and dispatches into `run_autoland` to keep cli.py small. @@ -14,6 +14,7 @@ Modules, roughly bottom of the dependency graph first: +- ``merge_mode``: whether a run merges through the merge queue or directly. - ``options``: AutolandOptions, resolved from flags, config, and defaults. - ``runtime``: the console, sleep/wake resilience, and the retrying ``run``. - ``steps``: the AutolandStep base class, the registry of step types, and one @@ -29,7 +30,8 @@ - ``plan``: generating, formatting, parsing, and replanning landing plans. - ``display``: rendering a plan's progress. - ``repo``: the local clone (fetch, ancestry, landed SHAs, rebase + resubmit). -- ``waits``: waiting for approval, checks, the merge queue, and workflows. +- ``waits``: waiting for approval, checks, a merge (queued or direct), and + workflows. - ``native_stack``: merging a run of land steps as one GitHub stack. - ``services``: the tools a plan's execution uses (console, GitHub, sleep, the repo, the waits), and their production bindings. diff --git a/src/stack_pr/autoland/commands.py b/src/stack_pr/autoland/commands.py index c6779da..6b57ff1 100644 --- a/src/stack_pr/autoland/commands.py +++ b/src/stack_pr/autoland/commands.py @@ -4,6 +4,7 @@ import argparse import configparser +import dataclasses import json import os import signal @@ -12,8 +13,13 @@ from pathlib import Path from stack_pr import cli, git -from stack_pr.autoland import discovery, engine, runtime +from stack_pr.autoland import discovery, engine, gh, runtime from stack_pr.autoland.display import _describe_step, _escape_markup, print_status +from stack_pr.autoland.merge_mode import ( + MergeModeError, + describe_merge_mode, + resolve_merge_mode, +) from stack_pr.autoland.model import LandingContext, LandStep, PlanStep, StackEntry from stack_pr.autoland.options import AutolandOptions from stack_pr.autoland.plan import ( @@ -42,7 +48,10 @@ def register_parser( """Register the `autoland` subparser. Called from cli.create_argparser.""" p = subparsers.add_parser( "autoland", - help="Land the whole stack through the GitHub merge queue", + help=( + "Land the whole stack: through the GitHub merge queue where the " + "target branch has one, else by merging each PR directly" + ), parents=[common_parser], ) p.add_argument( @@ -60,7 +69,10 @@ def register_parser( "--max-queue-retries", type=int, default=None, - help="Max retries after a merge-queue boot (config: autoland.max_queue_retries).", + help=( + "Max retries after a merge-queue boot or a failed direct merge " + "(config: autoland.max_queue_retries)." + ), ) p.add_argument( "--poll-interval", @@ -185,14 +197,6 @@ def run_autoland( runtime.console.print("[red]-o/--output only applies to --status.[/red]") sys.exit(1) - # Merge-queue is the only supported strategy for now. Fail early otherwise. - if not opts.merge_queue: - raise NotImplementedError( - "stack-pr autoland currently supports only repositories that use " - "the GitHub merge queue. Enable it with:\n" - " stack-pr config autoland.merge_queue=true" - ) - # A file plan replaces the whole plan, so --count (which only shapes the # generated default) and --resume (which restores the plan from a # checkpoint) have nothing to act on. @@ -216,6 +220,8 @@ def run_autoland( ) sys.exit(1) + opts = _resolve_merge_mode(common, opts) + if opts.replan: _run_replan(common, opts) return @@ -225,6 +231,27 @@ def run_autoland( _run_fresh(common, opts) +def _resolve_merge_mode( + common: cli.CommonArgs, opts: AutolandOptions +) -> AutolandOptions: + """*opts* with the run's merge mode resolved, before anything lands. + + Exits if the mode can't be resolved. Says which mode the run uses, unless + it is the merge queue the config asked for, the one mode autoland used to + have. + """ + try: + mode = resolve_merge_mode(common.target, gh.github, configured=opts.merge_queue) + except MergeModeError as e: + runtime.console.print(f"[red]{_escape_markup(str(e))}[/red]") + sys.exit(1) + if not (mode.uses_queue and opts.merge_queue): + runtime.console.print( + f"[bold]{_escape_markup(describe_merge_mode(mode, common.target))}[/bold]" + ) + return dataclasses.replace(opts, merge_mode=mode) + + def _dispose_worktree( worktree: Worktree | None, opts: AutolandOptions, *, succeeded: bool ) -> None: diff --git a/src/stack_pr/autoland/display.py b/src/stack_pr/autoland/display.py index 56124a8..4fa3861 100644 --- a/src/stack_pr/autoland/display.py +++ b/src/stack_pr/autoland/display.py @@ -108,6 +108,7 @@ def _icon(outcome: StepOutcome) -> str: StepTone.AWAITING_CHECKS: "yellow", StepTone.AWAITING_USER: "yellow", StepTone.IN_QUEUE: "cyan", + StepTone.MERGING: "cyan", StepTone.AWAITING_WORKFLOW: "blue", StepTone.RUNNING: "blue", } diff --git a/src/stack_pr/autoland/gh.py b/src/stack_pr/autoland/gh.py index 9fc07ac..0b53b47 100644 --- a/src/stack_pr/autoland/gh.py +++ b/src/stack_pr/autoland/gh.py @@ -132,6 +132,46 @@ def enqueue(self, pr_number: int) -> None: ["gh", "pr", "merge", str(pr_number), "--squash"], quiet=False, retries=0 ) + def head_sha(self, pr_number: int) -> str: + """The commit at the head of the PR's branch.""" + sha = self._pr_view(pr_number, "headRefOid").get("headRefOid") + if not isinstance(sha, str) or not sha: + raise GitHubError(f"No head commit reported for PR #{pr_number}") + return sha + + def commit_message(self, sha: str) -> str: + """The full message of commit *sha*.""" + data = self._api("GET", f"commits/{sha}") + commit = data.get("commit") if isinstance(data, dict) else None + message = commit.get("message") if isinstance(commit, dict) else None + if not isinstance(message, str) or not message: + raise GitHubError(f"No commit message reported for {sha}") + return message + + def squash_merge( + self, pr_number: int, *, subject: str, body: str, head_sha: str + ) -> None: + """Squash-merge the PR straight into its base branch, as *subject* and + *body*. GitHub refuses if the PR's head is no longer *head_sha*.""" + runtime.run( + [ + "gh", + "pr", + "merge", + str(pr_number), + "--squash", + "--match-head-commit", + head_sha, + "--subject", + subject, + "--body-file", + "-", + ], + quiet=False, + input_data=body.encode(), + retries=0, + ) + # GitHub's native stacked PRs. A stack is an explicit server-side object — # GitHub does not infer one from a chain of PR bases — and once a PR is in # one, only the asynchronous merge API can merge it. See diff --git a/src/stack_pr/autoland/merge_mode.py b/src/stack_pr/autoland/merge_mode.py new file mode 100644 index 0000000..65ce64a --- /dev/null +++ b/src/stack_pr/autoland/merge_mode.py @@ -0,0 +1,57 @@ +"""How a run merges its PRs into the target branch: the merge queue, or directly. + +The mode is resolved once per run, before anything lands, and carried in +``AutolandOptions.merge_mode``. Everything that merges reads it from there — +landing one PR at a time (``waits.merge_and_wait``) and merging a run as a +GitHub stack (``native_stack``) — so they always agree. +""" + +from __future__ import annotations + +from enum import Enum +from typing import TYPE_CHECKING + +if TYPE_CHECKING: + from stack_pr.autoland.gh import GitHub + + +class MergeMode(str, Enum): + QUEUE = "queue" # add each PR to the target branch's merge queue + DIRECT = "direct" # squash-merge each PR straight into the target branch + + @property + def uses_queue(self) -> bool: + return self is MergeMode.QUEUE + + +class MergeModeError(Exception): + """The merge mode could not be resolved.""" + + +def resolve_merge_mode( + target: str, github: GitHub, *, configured: bool | None +) -> MergeMode: + """The run's merge mode. + + *configured* is ``autoland.merge_queue``: ``True`` or ``False`` forces the + mode, and ``None`` (unset) asks GitHub whether *target* has a merge queue. + Raises ``MergeModeError`` when GitHub can't say. + """ + if configured is not None: + return MergeMode.QUEUE if configured else MergeMode.DIRECT + has_queue = github.has_merge_queue(target) + if has_queue is None: + raise MergeModeError( + f"Could not tell whether {target} uses a merge queue. Say which " + "with one of:\n" + " stack-pr config autoland.merge_queue=true\n" + " stack-pr config autoland.merge_queue=false" + ) + return MergeMode.QUEUE if has_queue else MergeMode.DIRECT + + +def describe_merge_mode(mode: MergeMode, target: str) -> str: + """One line saying how the run merges, for the start of its output.""" + if mode.uses_queue: + return f"Merging into {target} through its merge queue" + return f"Merging directly into {target} (no merge queue)" diff --git a/src/stack_pr/autoland/native_stack.py b/src/stack_pr/autoland/native_stack.py index e7b15c5..99806cc 100644 --- a/src/stack_pr/autoland/native_stack.py +++ b/src/stack_pr/autoland/native_stack.py @@ -129,8 +129,12 @@ def _await_native_stack_merge( # PR that leaves it, so the lowest open PR leaving means the merge is # off. A request still "pending" hasn't reached the queue yet; without # a request id to ask, give it one interval to get there. - settled = not unread and ( - status == "enqueued" or (not uuid and awake_elapsed > 0) + # Without a queue there is nothing to be booted from: the request + # merges or fails, and GitHub says which. + settled = ( + opts.merge_mode.uses_queue + and not unread + and (status == "enqueued" or (not uuid and awake_elapsed > 0)) ) # A failed lookup (None) leaves the merge undecided; keep polling. if settled and github.in_merge_queue(still_open[0].pr_number) is False: @@ -161,8 +165,8 @@ def land_as_native_stack( Every PR still needs its own approval and passing checks, so those are waited for first, exactly as when landing one at a time. Then a single merge request on the top PR merges the whole run — through the merge queue - as one group, where the repo has one — and the stack above is rebased and - re-submitted once, not once per PR. + as one group, or directly, per ``opts.merge_mode`` — and the stack above is + rebased and re-submitted once, not once per PR. Anything that keeps the run from merging as a stack is not fatal: the result asks for the rest to be landed one at a time. A GitHub stack holding @@ -215,9 +219,12 @@ def land_as_native_stack( abort_reason = f"PR #{bottom.pr_number} failed to merge" if not abort_reason and bottom is not None: + merging = ( + PRState.IN_MERGE_QUEUE if opts.merge_mode.uses_queue else PRState.MERGING + ) for entry in entries: if entry.state != PRState.MERGED: - entry.state = PRState.IN_MERGE_QUEUE + entry.state = merging entry.error_message = "Merging as a stack..." ui.print( f"\n[bold cyan]Merging PRs #{bottom.pr_number}-#{top.pr_number} " @@ -241,13 +248,16 @@ def land_as_native_stack( ) if not failure: try: - # Ask GitHub rather than trust the config: the request body must - # match whether the branch really has a queue (see merge_async). - has_queue = github.has_merge_queue(common.target) - uuid = github.merge_async( - top.pr_number, - merge_queue=opts.merge_queue if has_queue is None else has_queue, - ) + # The request body must match whether the branch really has a + # queue (see merge_async). A direct run has found it has none, + # or was told so. A queue run asks again: merge_queue = true + # used to be required on any branch, queue or not. + if opts.merge_mode.uses_queue: + has_queue = github.has_merge_queue(common.target) + use_queue = True if has_queue is None else has_queue + else: + use_queue = False + uuid = github.merge_async(top.pr_number, merge_queue=use_queue) except RuntimeError as e: # 409: a merge request for this stack is already in flight (e.g. # from a run that was interrupted) — wait for it like our own. diff --git a/src/stack_pr/autoland/options.py b/src/stack_pr/autoland/options.py index 5a5dbd0..b996c30 100644 --- a/src/stack_pr/autoland/options.py +++ b/src/stack_pr/autoland/options.py @@ -7,6 +7,8 @@ from dataclasses import dataclass from pathlib import Path +from stack_pr.autoland.merge_mode import MergeMode + # --------------------------------------------------------------------------- # Defaults (overridable via [autoland] config or flags) # --------------------------------------------------------------------------- @@ -25,7 +27,9 @@ @dataclass class AutolandOptions: - merge_queue: bool + # ``autoland.merge_queue``: True or False forces the merge mode; None (unset) + # leaves it to be detected. See ``merge_mode.resolve_merge_mode``. + merge_queue: bool | None required_checks: list[str] poll_interval: int max_check_retries: int @@ -43,6 +47,9 @@ class AutolandOptions: plan_file: Path | None = None merge_as_stack: bool = True replan: bool = False + # How this run merges, resolved from ``merge_queue`` (and GitHub) before + # anything lands. + merge_mode: MergeMode = MergeMode.QUEUE @classmethod def from_config_and_args( @@ -67,7 +74,7 @@ def _int(flag_val: int | None, key: str, default: int) -> int: "autoland", "merge_as_stack", fallback=True ) return cls( - merge_queue=config.getboolean("autoland", "merge_queue", fallback=False), + merge_queue=config.getboolean("autoland", "merge_queue", fallback=None), required_checks=required_checks, poll_interval=_int( getattr(args, "poll_interval", None), diff --git a/src/stack_pr/autoland/pr_state.py b/src/stack_pr/autoland/pr_state.py index c6f0725..515c33e 100644 --- a/src/stack_pr/autoland/pr_state.py +++ b/src/stack_pr/autoland/pr_state.py @@ -13,6 +13,7 @@ class PRState(str, Enum): WAITING_FOR_APPROVAL = "waiting_for_approval" WAITING_FOR_CHECKS = "waiting_for_checks" IN_MERGE_QUEUE = "in_merge_queue" + MERGING = "merging" # being merged directly, with no merge queue WAITING_FOR_WORKFLOW = "waiting_for_workflow" MERGED = "merged" FAILED = "failed" diff --git a/src/stack_pr/autoland/services.py b/src/stack_pr/autoland/services.py index ff2a1cb..0e5ee54 100644 --- a/src/stack_pr/autoland/services.py +++ b/src/stack_pr/autoland/services.py @@ -72,7 +72,9 @@ def wait_for_mergeable( self, entry: StackEntry, *, opts: AutolandOptions ) -> MergeableResult: ... - def enqueue_and_wait(self, entry: StackEntry, *, opts: AutolandOptions) -> bool: ... + def merge_and_wait(self, entry: StackEntry, *, opts: AutolandOptions) -> bool: + """Merge the PR the run's way (``opts.merge_mode``: through the merge + queue, or directly), and wait until it has merged.""" def wait_for_workflow( self, @@ -220,8 +222,8 @@ def wait_for_mergeable( ) -> MergeableResult: return waits.wait_for_mergeable(entry, opts=opts, services=self.services) - def enqueue_and_wait(self, entry: StackEntry, *, opts: AutolandOptions) -> bool: - return waits.enqueue_and_wait(entry, opts=opts, services=self.services) + def merge_and_wait(self, entry: StackEntry, *, opts: AutolandOptions) -> bool: + return waits.merge_and_wait(entry, opts=opts, services=self.services) def wait_for_workflow( self, diff --git a/src/stack_pr/autoland/steps/land.py b/src/stack_pr/autoland/steps/land.py index 3b0738a..9491983 100644 --- a/src/stack_pr/autoland/steps/land.py +++ b/src/stack_pr/autoland/steps/land.py @@ -34,6 +34,7 @@ StepTone.AWAITING_CHECKS, ), PRState.IN_MERGE_QUEUE: (StepOutcome.ACTIVE, "In merge queue", StepTone.IN_QUEUE), + PRState.MERGING: (StepOutcome.ACTIVE, "Merging", StepTone.MERGING), PRState.WAITING_FOR_WORKFLOW: ( StepOutcome.ACTIVE, "Waiting for workflow", @@ -47,7 +48,7 @@ @register_step @dataclass class LandStep(AutolandStep): - """Land a PR in the stack through the merge queue. + """Land a PR in the stack, through the merge queue or directly. ``entry_index`` indexes into ``LandingContext.stack``, or is ``-1`` for a PR that has already landed: such a PR is no longer part of the stack, so @@ -146,8 +147,9 @@ def view( ) def execute(self, sc: StepContext) -> StepResult: - """Wait for approval and checks, merge through the queue, then rebase - and re-submit whatever is left of the stack above.""" + """Wait for approval and checks, merge (through the queue, or + directly), then rebase and re-submit whatever is left of the stack + above.""" ui, repo = sc.services.ui, sc.services.repo if self.already_landed: ui.print(f"\n[green]PR #{self.pr_number} already landed, skipping[/green]") @@ -179,7 +181,7 @@ def execute(self, sc: StepContext) -> StepResult: return StepResult.abort( f"PR #{entry.pr_number} checks failed after retries" ) - if not entry.is_merged() and not waits.enqueue_and_wait(entry, opts=opts): + if not entry.is_merged() and not waits.merge_and_wait(entry, opts=opts): return StepResult.abort(f"PR #{entry.pr_number} failed to merge") landed_sha = repo.landed_sha(sc.common, entry.pr_number) diff --git a/src/stack_pr/autoland/steps/view.py b/src/stack_pr/autoland/steps/view.py index 5d7490e..5c2d987 100644 --- a/src/stack_pr/autoland/steps/view.py +++ b/src/stack_pr/autoland/steps/view.py @@ -42,6 +42,7 @@ class StepTone(Enum): AWAITING_CHECKS = "awaiting_checks" AWAITING_USER = "awaiting_user" IN_QUEUE = "in_queue" + MERGING = "merging" # being merged directly, with no merge queue AWAITING_WORKFLOW = "awaiting_workflow" RUNNING = "running" # running a command of its own diff --git a/src/stack_pr/autoland/waits.py b/src/stack_pr/autoland/waits.py index 31cbe71..e771739 100644 --- a/src/stack_pr/autoland/waits.py +++ b/src/stack_pr/autoland/waits.py @@ -1,4 +1,4 @@ -"""Waiting for approval, checks, the merge queue, and workflows. +"""Waiting for approval, checks, a merge (queued or direct), and workflows. Every wait polls through the ``Services`` it is given: GitHub, the console, the clock, and the local repo. ``services.PollingWaits`` binds them as the @@ -12,6 +12,7 @@ from dataclasses import dataclass from typing import TYPE_CHECKING, TypeVar +from stack_pr import cli from stack_pr.autoland.checks import CheckStatus, evaluate_checks from stack_pr.autoland.display import _REVIEW_DECISION_DISPLAY from stack_pr.autoland.gh import MergeQueuePollResult, _sha_eq @@ -19,7 +20,6 @@ from stack_pr.autoland.options import AutolandOptions if TYPE_CHECKING: - from stack_pr import cli from stack_pr.autoland.services import Repo, Services # --------------------------------------------------------------------------- @@ -313,6 +313,8 @@ def wait_for_mergeable( ) -> MergeableResult: """Wait until GitHub reports the PR as mergeable.""" ui, github = services.ui, services.github + uses_queue = opts.merge_mode.uses_queue + reported_behind = False while True: data = _poll_read(github.merge_state, entry.pr_number, services) if data is None: @@ -347,8 +349,32 @@ def wait_for_mergeable( services.sleep(opts.poll_interval) continue + # Without a queue, a branch that must be up to date with its base before + # it merges stays BEHIND until someone updates it; autoland doesn't + # push to a PR on its own, so it says what to do and waits. + if not uses_queue and merge_state == "BEHIND": + entry.error_message = "Branch is behind its base — waiting for an update" + if not reported_behind: + reported_behind = True + ui.print( + f"\n[bold yellow]PR #{entry.pr_number} is behind its base " + "branch, which must be up to date before it merges. Rebase " + "the stack and run `stack-pr submit` (or update the branch " + "on GitHub); autoland will resume automatically.[/bold yellow]" + ) + ui.print( + f"[dim]PR #{entry.pr_number}: behind its base — " + f"polling in {opts.poll_interval}s[/dim]" + ) + services.sleep(opts.poll_interval) + continue + # UNKNOWN can also mean "already in the merge queue". - if merge_state == "UNKNOWN" and github.in_merge_queue(entry.pr_number) is True: + if ( + uses_queue + and merge_state == "UNKNOWN" + and github.in_merge_queue(entry.pr_number) is True + ): ui.print( f"[cyan]PR #{entry.pr_number} is already in the merge queue — " "skipping enqueue[/cyan]" @@ -366,7 +392,7 @@ def wait_for_mergeable( def _rewait_after_retry( entry: StackEntry, *, opts: AutolandOptions, services: Services ) -> bool: - """Re-verify approval and checks before a queue retry.""" + """Re-verify approval and checks before retrying a merge.""" if not wait_for_approval(entry, opts=opts, services=services): return False entry.check_retries = 0 @@ -458,3 +484,116 @@ def enqueue_and_wait( ) services.sleep(opts.poll_interval) awake_elapsed += opts.poll_interval + + +def merge_and_wait( + entry: StackEntry, *, opts: AutolandOptions, services: Services +) -> bool: + """Merge the PR the way the run merges (``opts.merge_mode``), and wait until + it has merged.""" + if opts.merge_mode.uses_queue: + return enqueue_and_wait(entry, opts=opts, services=services) + return merge_directly(entry, opts=opts, services=services) + + +# After a direct merge, how many times (and how many seconds apart) to read the +# PR's state before deciding GitHub didn't merge it after all. GitHub usually +# reports the merge at once; this only covers it lagging behind. +_MERGE_CONFIRM_READS = 6 +_MERGE_CONFIRM_INTERVAL = 5 + + +def _squash_merge(entry: StackEntry, services: Services) -> None: + """Squash-merge the PR as ``stack-pr land`` would: as its commit message + without the stack-info trailer, with the PR number in the subject. + + Pinned to the head commit the message was read from, so a push since then + fails the merge instead of landing code under another commit's message. + """ + github = services.github + head = github.head_sha(entry.pr_number) + subject, body = cli.squash_commit_message( + github.commit_message(head), entry.pr_number + ) + github.squash_merge(entry.pr_number, subject=subject, body=body, head_sha=head) + + +def _confirm_merged(entry: StackEntry, *, reads: int, services: Services) -> str: + """The PR's state after a merge attempt: "MERGED" or "CLOSED" as soon as + GitHub reports it, else what the last of *reads* reads said ("" if it + failed).""" + state = "" + for attempt in range(reads): + if attempt: + services.sleep(_MERGE_CONFIRM_INTERVAL) + state = _poll_read(services.github.pr_state, entry.pr_number, services) or "" + if state in ("MERGED", "CLOSED"): + break + return state + + +def merge_directly( + entry: StackEntry, *, opts: AutolandOptions, services: Services +) -> bool: + """Squash-merge the PR straight into its base branch, with no merge queue. + + Waits for it to be mergeable first. A merge that fails, or that GitHub + doesn't report as merged, is retried up to ``max_queue_retries`` times, + after approval and checks are verified again. + """ + ui = services.ui + while True: + mergeable_result = wait_for_mergeable(entry, opts=opts, services=services) + if not mergeable_result.ready: + return False + if mergeable_result.already_merged: + entry.state = PRState.MERGED + entry.error_message = "" + ui.print( + f"\n[bold green]PR #{entry.pr_number} already merged![/bold green]" + ) + return True + + entry.state = PRState.MERGING + entry.error_message = "Merging..." + ui.print(f"\n[bold cyan]Merging PR #{entry.pr_number}[/bold cyan]") + + try: + _squash_merge(entry, services) + except RuntimeError as e: + failure = str(e) + # The merge may have gone through even though gh reported an error + # (e.g. a lost response), so look before retrying. + reads = 1 + else: + failure = "GitHub did not report the PR as merged" + reads = _MERGE_CONFIRM_READS + + state = _confirm_merged(entry, reads=reads, services=services) + if state == "MERGED": + entry.state = PRState.MERGED + entry.error_message = "" + ui.print(f"\n[bold green]PR #{entry.pr_number} merged![/bold green]") + return True + if state == "CLOSED": + entry.state = PRState.FAILED + entry.error_message = "PR was closed" + return False + + entry.error_message = f"Failed to merge: {failure}" + ui.print(f"[red]Failed to merge PR #{entry.pr_number}: {failure}[/red]") + if entry.queue_retries >= opts.max_queue_retries: + entry.state = PRState.FAILED + entry.error_message = ( + f"Failed to merge after {opts.max_queue_retries + 1} attempts: " + f"{failure}" + ) + return False + entry.queue_retries += 1 + ui.print( + f"[yellow]Retrying the merge of PR #{entry.pr_number} " + f"(attempt {entry.queue_retries + 1}/{opts.max_queue_retries + 1})" + "[/yellow]" + ) + if not _rewait_after_retry(entry, opts=opts, services=services): + return False diff --git a/src/stack_pr/cli.py b/src/stack_pr/cli.py index cce04f4..d619cae 100755 --- a/src/stack_pr/cli.py +++ b/src/stack_pr/cli.py @@ -1567,6 +1567,18 @@ def rebase_pr(e: StackEntry, remote: str, target: str, *, verbose: bool) -> None force_push_with_lease([f"{e.head}:{e.head}"], remote, target, verbose=verbose) +def squash_commit_message(commit_msg: str, pr_id: str | int) -> tuple[str, str]: + """The subject and body of the squash commit that lands a PR. + + It is the PR's commit message and nothing else: the stack-info trailer is + dropped, and the PR number is appended to the subject. The body is a single + space when the message has none. + """ + lines = remove_stack_info(commit_msg).splitlines() + title = f"{lines[0]} (#{pr_id})" + return title, "\n".join(lines[1:]) or " " + + def land_pr(e: StackEntry, remote: str, target: str, *, verbose: bool) -> None: log(b("Landing ") + e.pprint(links=False), level=2) # Rebase the head branch to the most recent 'origin/main' @@ -1581,16 +1593,7 @@ def land_pr(e: StackEntry, remote: str, target: str, *, verbose: bool) -> None: # Switch PR base branch to 'main' edit_pr_base(e.pr, target, verbose=verbose) - # Form the commit message: it should contain the original commit message - # and nothing else. - pr_body = remove_stack_info(e.commit.commit_msg()) - - # Since title is passed separately, we need to strip the first line from the - # body: - lines = pr_body.splitlines() - pr_id = last(e.pr) - title = f"{lines[0]} (#{pr_id})" - pr_body = "\n".join(lines[1:]) or " " + title, pr_body = squash_commit_message(e.commit.commit_msg(), last(e.pr)) run_shell_command( ["gh", "pr", "merge", e.pr, "--squash", "-t", title, "-F", "-"], input=pr_body.encode(), diff --git a/tests/autoland_fakes.py b/tests/autoland_fakes.py index 5b46bc8..d595da2 100644 --- a/tests/autoland_fakes.py +++ b/tests/autoland_fakes.py @@ -129,9 +129,9 @@ class FakeWaits: approval: Outcome = ok checks: Outcome = ok mergeable: Outcome = ok - queue: Outcome = merges + merge: Outcome = merges workflow_succeeds: bool = True - # Each wait, in order: "approval", "checks", "mergeable", or "queue". + # Each wait, in order: "approval", "checks", "mergeable", or "merge". calls: list[str] = field(default_factory=list) # The entry of each wait, in the same order as ``calls``. entries: list[StackEntry] = field(default_factory=list) @@ -158,8 +158,8 @@ def wait_for_mergeable( ) -> MergeableResult: return MergeableResult(ready=self._wait("mergeable", entry, self.mergeable)) - def enqueue_and_wait(self, entry: StackEntry, *, opts: AutolandOptions) -> bool: - return self._wait("queue", entry, self.queue) + def merge_and_wait(self, entry: StackEntry, *, opts: AutolandOptions) -> bool: + return self._wait("merge", entry, self.merge) def wait_for_workflow( self, @@ -274,8 +274,10 @@ class FakeGitHub: A PR is open unless it is in ``merged`` or ``closed``. By default it is approved, has no checks, is mergeable, is merged as soon as it is - enqueued, and stays in the merge queue while open. Every call is logged - in ``calls`` as ``(method, args)``. + enqueued or merged directly, and stays in the merge queue while open. Its + head commit is ``head-``, with the message in ``messages`` (default: a + title, a body, and a stack-info trailer). Every call is logged in + ``calls`` as ``(method, args)``. ``script(method, *replies)`` answers the next calls of *method*, one reply each, before it goes back to answering from the fake's state. A @@ -293,6 +295,10 @@ def __init__(self) -> None: self.pr_checks: dict[int, list[dict]] = {} self.merge_commits: dict[int, str] = {} self.enqueued: list[int] = [] + # PR -> its head commit's message, and (PR, subject, body, head) for + # each direct merge. + self.messages: dict[int, str] = {} + self.squash_merges: list[tuple[int, str, str, str]] = [] self.booted: set[int] = set() # The latest runs of every workflow, newest first, and GitHub's answer # to whether (ancestor, descendant) is one (unknown if absent). @@ -305,6 +311,8 @@ def __init__(self) -> None: self.created: list[list[int]] = [] self.unstacked: list[int] = [] self.merge_requests: list[int] = [] + # Whether each merge request asked for the merge queue. + self.merge_via_queue: list[bool] = [] # (PR, new base) for each base change, and the base changes made by # the time of each merge request. self.base_changes: list[tuple[int, str]] = [] @@ -386,6 +394,26 @@ def poll_merge(self, pr: int) -> MergeQueuePollResult: return MergeQueuePollResult(booted=True) return MergeQueuePollResult() + # --- merging directly ------------------------------------------------- + + @_scriptable + def head_sha(self, pr: int) -> str: + return f"head-{pr}" + + @_scriptable + def commit_message(self, sha: str) -> str: + pr = int(sha.removeprefix("head-")) + return self.messages.get( + pr, + f"Title {pr}\n\nBody of {pr}.\n\n" + f"stack-info: PR: https://github.com/o/r/pull/{pr}, branch: b{pr}", + ) + + @_scriptable + def squash_merge(self, pr: int, *, subject: str, body: str, head_sha: str) -> None: + self.squash_merges.append((pr, subject, body, head_sha)) + self.merged.add(pr) + # --- workflows ------------------------------------------------------- @_scriptable @@ -434,6 +462,7 @@ def set_base(self, pr: int, base: str) -> None: @_scriptable def merge_async(self, pr: int, *, merge_queue: bool) -> str: self.merge_requests.append(pr) + self.merge_via_queue.append(merge_queue) self.bases_at_merge.append(list(self.base_changes)) if self.merge_error: raise RuntimeError(self.merge_error) diff --git a/tests/autoland_scenarios.py b/tests/autoland_scenarios.py index eb51468..f968feb 100644 --- a/tests/autoland_scenarios.py +++ b/tests/autoland_scenarios.py @@ -104,6 +104,7 @@ def view(self, fields: str) -> dict[str, Any]: "mergeStateStatus": self.merge_state, "mergeable": "MERGEABLE", "mergeCommit": {"oid": self.merge_commit} if self.merge_commit else None, + "headRefOid": commit_sha(self.number), } return {f: values[f] for f in fields.split(",")} @@ -127,6 +128,12 @@ class FakeWorld: stack_merge_message: str = "" # An ``x`` step's command line -> the status it exits with. commands: dict[str, int] = field(default_factory=dict) + # Whether the target branch has a merge queue; whether asking GitHub that + # fails; and how many direct merges (``gh pr merge`` without a queue) fail + # before they go through. + has_queue: bool = True + queue_lookup_fails: bool = False + merge_failures: int = 0 main_head: str = field(default_factory=lambda: sha("main-0")) stacks: dict[int, list[int]] = field(default_factory=dict) @@ -191,7 +198,10 @@ def _gh(self, args: list[str], stdin: str | None) -> str: # noqa: PLR0911 return json.dumps(self._checks(self.prs[int(args[2])])) if sub == ["pr", "merge"]: pr = self.prs[int(args[2])] - pr.queued_for = pr.queue_ticks + if self.has_queue: + pr.queued_for = pr.queue_ticks + else: + self._merge_directly(pr, args) return "" if sub == ["pr", "edit"]: return "" @@ -244,13 +254,38 @@ def _workflow_runs(self, name: str) -> list[dict[str, str]]: ) return [dict(r) for r in runs[:10]] + def _merge_directly(self, pr: FakePR, args: list[str]) -> None: + """``gh pr merge`` on a branch without a queue merges at once, as the + commit it is pinned to.""" + assert args[args.index("--match-head-commit") + 1] == commit_sha(pr.number) + if self.merge_failures: + self.merge_failures -= 1 + raise subprocess.CalledProcessError( + 1, + ["gh", *args], + stderr="GraphQL: Base branch was modified. Review and try the " + "merge again. (mergePullRequest)", + ) + self.merge(pr) + + def _commit(self, commit: str) -> dict: + """A PR's commit, with the message stack-pr gave it.""" + pr = next(p for p in self.prs.values() if commit_sha(p.number) == commit) + trailer = f"stack-info: PR: {PR_URL.format(pr.number)}, branch: {pr.branch}" + return {"sha": commit, "commit": {"message": f"{pr.title}\n\n{trailer}"}} + def _graphql(self, args: list[str]) -> dict: params = dict(a.split("=", 1) for a in args[3::2]) if "number" in params: queued = self.prs[int(params["number"])].queued_for is not None entry = {"id": "MQE_1", "state": "QUEUED"} if queued else None return {"data": {"repository": {"pullRequest": {"mergeQueueEntry": entry}}}} - return {"data": {"repository": {"mergeQueue": {"id": "MQ_1"}}}} + if self.queue_lookup_fails: + raise subprocess.CalledProcessError( + 1, ["gh", *args], stderr="HTTP 502: Bad Gateway" + ) + queue = {"id": "MQ_1"} if self.has_queue else None + return {"data": {"repository": {"mergeQueue": queue}}} def _stack_json(self, number: int) -> dict: return { @@ -261,7 +296,9 @@ def _stack_json(self, number: int) -> dict: ], } - def _rest(self, method: str, path: str, body: dict | None) -> Any: + def _rest(self, method: str, path: str, body: dict | None) -> Any: # noqa: PLR0911 + if method == "GET" and path.startswith("commits/"): + return self._commit(path.split("/", 1)[1]) if method == "GET" and path.startswith("stacks?pull_request="): pr = int(path.split("=", 1)[1]) return [self._stack_json(n) for n, prs in self.stacks.items() if pr in prs] @@ -275,7 +312,8 @@ def _rest(self, method: str, path: str, body: dict | None) -> Any: self.stacks.pop(int(path.split("/")[1])) return None if method == "PUT" and path.endswith("/merge-async"): - return self._merge_async(int(path.split("/")[1])) + assert body is not None + return self._merge_async(int(path.split("/")[1]), body) if method == "GET" and "/merge-async/" in path: prs = self.merge_requests[path.rsplit("/", 1)[1]] if self.stack_merge == "fail": @@ -287,7 +325,14 @@ def _rest(self, method: str, path: str, body: dict | None) -> Any: return {"status": "merged" if merged else "enqueued"} raise AssertionError(f"unexpected gh api call: {method} {path}") - def _merge_async(self, top: int) -> dict: + def _merge_async(self, top: int, body: dict) -> dict: + # GitHub rejects a request that doesn't match whether there is a queue. + expected = ( + {"merge_action": "merge_queue"} + if self.has_queue + else {"merge_action": "direct_merge", "merge_method": "squash"} + ) + assert body == expected, body native = next(prs for prs in self.stacks.values() if top in prs) prs = [ n for n in native[: native.index(top) + 1] if self.prs[n].state == "OPEN" @@ -297,6 +342,9 @@ def _merge_async(self, top: int) -> dict: if self.stack_merge == "fail": for n in self.stack_merge_lands: self.merge(self.prs[n]) + elif not self.has_queue: + for n in prs: + self.merge(self.prs[n]) else: for n in prs: self.prs[n].queued_for = self.prs[n].queue_ticks @@ -323,8 +371,8 @@ class Scenario: plan: str | None = None # Extra autoland flags (the plan file and the state file are added). flags: list[str] = field(default_factory=list) - # [autoland] config, on top of the defaults below. - config: dict[str, str] = field(default_factory=dict) + # [autoland] config, on top of the defaults below; None unsets a default. + config: dict[str, str | None] = field(default_factory=dict) # What the user types at each prompt; input after that is EOF. answers: list[str] = field(default_factory=list) workflows: dict[str, str] = field(default_factory=dict) @@ -333,6 +381,12 @@ class Scenario: stack_merge_message: str = "" # An ``x`` step's command line -> the status it exits with. commands: dict[str, int] = field(default_factory=dict) + # Whether the target branch has a merge queue (the default config also + # says it does), whether asking GitHub that fails, and how many direct + # merges fail before one goes through. + has_queue: bool = True + queue_lookup_fails: bool = False + merge_failures: int = 0 # PRs that landed before this run (and so are no longer in the stack). landed: list[int] = field(default_factory=list) # Builds the checkpoint of an earlier run, which this run resumes or @@ -371,11 +425,13 @@ class ScenarioResult: """What a user could observe of a run, normalized for golden files.""" out: str # the console output of the run - checkpoint: str # the last checkpoint the run wrote + # The last checkpoint the run wrote, or None if it stopped before writing + # one (and so did everything below, but --status). + checkpoint: str | None status_text: str # `autoland --status`, over that checkpoint status_json: str # `autoland --status -o json`, over that checkpoint - plan: str # the checkpointed plan, as the -i editor shows it - plan_round_trip: str # that text, parsed and formatted again + plan: str | None # the checkpointed plan, as the -i editor shows it + plan_round_trip: str | None # that text, parsed and formatted again exit_code: int | None @@ -421,6 +477,8 @@ def sleep(_seconds: int) -> float: return 0.0 mocker.patch.object(runtime, "resilient_sleep", side_effect=sleep) + # Retries of a transient gh failure back off in real time; don't wait. + mocker.patch.object(shell_commands, "RETRY_DELAY", 0) # Subprocesses: git, gh, and the scenario's commands go to the world; # anything else is a test bug. @@ -488,13 +546,17 @@ def run_scenario( stack_merge_lands=scenario.stack_merge_lands, stack_merge_message=scenario.stack_merge_message, commands=dict(scenario.commands), + has_queue=scenario.has_queue, + queue_lookup_fails=scenario.queue_lookup_fails, + merge_failures=scenario.merge_failures, ) for number in scenario.landed: world.merge(world.prs[number]) _install_seams(mocker, monkeypatch, scenario, world, tmp_path) state_path = AutolandCheckpointer.default_path(BRANCH) config = configparser.ConfigParser() - config["autoland"] = {**DEFAULT_CONFIG, **scenario.config} + settings = {**DEFAULT_CONFIG, **scenario.config} + config["autoland"] = {k: v for k, v in settings.items() if v is not None} argv = list(scenario.flags) plan_file = tmp_path / "plan.autoland-plan" @@ -520,14 +582,24 @@ def save(self: AutolandCheckpointer, ctx: LandingContext) -> None: capsys.readouterr() exit_code = _autoland(argv, config) out = capsys.readouterr().out - assert saved, "the run wrote no checkpoint" # --status, as it would read the run's last checkpoint. - state_path.write_text(saved[-1]) + if saved: + state_path.write_text(saved[-1]) assert _autoland(["--status"], config) is None status_text = capsys.readouterr().out assert _autoland(["--status", "-o", "json"], config) is None status_json = capsys.readouterr().out + if not saved: + return ScenarioResult( + out=_normalize(out, tmp_path), + checkpoint=None, + status_text=_normalize(status_text, tmp_path), + status_json=_normalize(status_json, tmp_path), + plan=None, + plan_round_trip=None, + exit_code=exit_code, + ) _checkpointer, ctx = AutolandCheckpointer.load(state_path) plan_text = format_plan_for_editor(ctx.stack, ctx.plan) diff --git a/tests/golden/autoland/direct_merge_as_stack.checkpoint.json b/tests/golden/autoland/direct_merge_as_stack.checkpoint.json new file mode 100644 index 0000000..82e810a --- /dev/null +++ b/tests/golden/autoland/direct_merge_as_stack.checkpoint.json @@ -0,0 +1,67 @@ +{ + "version": 1, + "branch": "feat", + "base": "main", + "plan_file": "/plan.autoland-plan", + "current_step": 4, + "last_landed_sha": "1a935560d359b44bfc714d90b02f064b6167c8be", + "abort_reason": "", + "stack": [ + { + "pr_url": "https://github.com/o/r/pull/101", + "pr_number": 101, + "branch": "TestBot/stack/101", + "title": "Add the widget model", + "review_decision": "APPROVED", + "state": "merged", + "check_retries": 0, + "queue_retries": 0, + "error_message": "" + }, + { + "pr_url": "https://github.com/o/r/pull/102", + "pr_number": 102, + "branch": "TestBot/stack/102", + "title": "Expose widgets over the API", + "review_decision": "APPROVED", + "state": "merged", + "check_retries": 0, + "queue_retries": 0, + "error_message": "" + }, + { + "pr_url": "https://github.com/o/r/pull/103", + "pr_number": 103, + "branch": "TestBot/stack/103", + "title": "Show widgets in the dashboard", + "review_decision": "APPROVED", + "state": "merged", + "check_retries": 0, + "queue_retries": 0, + "error_message": "" + } + ], + "plan": [ + { + "type": "land", + "entry_index": 0, + "pr_number": 101 + }, + { + "type": "land", + "entry_index": 1, + "pr_number": 102 + }, + { + "type": "workflow", + "workflow": "deploy.yaml", + "state": "succeeded", + "error_message": "" + }, + { + "type": "land", + "entry_index": 2, + "pr_number": 103 + } + ] +} diff --git a/tests/golden/autoland/direct_merge_as_stack.out.txt b/tests/golden/autoland/direct_merge_as_stack.out.txt new file mode 100644 index 0000000..54e86fa --- /dev/null +++ b/tests/golden/autoland/direct_merge_as_stack.out.txt @@ -0,0 +1,64 @@ +Merging directly into main (no merge queue) + +Discovering stack... + + +Autoland plan · 4 steps · 0 done · 4 remaining + +→ ⬜ 1. Pending | Land | #101 - Add the widget model + ⬜ 2. Pending | Land | #102 - Expose widgets over the API + ⬜ 3. Pending | Workflow | deploy.yaml + ⬜ 4. Pending | Land | #103 - Show widgets in the dashboard +Steps 1-2 (#101, #102) will merge together as a GitHub stack. +State file: /home/.stack-pr/autoland/feat.json + + +============================================================ +Landing 2 PRs as a GitHub stack: #101, #102 +============================================================ +PR #101 is mergeable (mergeStateStatus=CLEAN) + +Merging PRs #101-#102 as a stack + +PRs #101, #102 merged! + +Rebasing stack on origin/main... +$ git fetch origin main +$ git rebase origin/main +Re-submitting stack... + +PR #101 already merged, skipping + +PR #102 already merged, skipping + +============================================================ +Step 3/4: Workflow checkpoint — deploy.yaml +============================================================ + +Waiting for workflow: deploy.yaml +Target SHA: 9cc0a8c62f2d +Workflow deploy.yaml: waiting (0m) — polling in 120s + +Workflow deploy.yaml completed with SHA 9cc0a8c62f2d + +============================================================ +Step 4/4: Landing PR #103 — Show widgets in the dashboard +============================================================ +PR #103 is mergeable (mergeStateStatus=CLEAN) + +Merging PR #103 +$ gh pr merge 103 --squash --match-head-commit 4fdac901a32a008e208f052358166d2d269e920a --subject Show widgets in the dashboard (#103) --body-file - + +PR #103 merged! + + + +Autoland plan · 4 steps · 4 done · 0 remaining + + ✅ 1. Merged | Land | #101 - Add the widget model + ✅ 2. Merged | Land | #102 - Expose widgets over the API + ✅ 3. Workflow complete | Workflow | deploy.yaml + ✅ 4. Merged | Land | #103 - Show widgets in the dashboard + +All PRs landed successfully! + diff --git a/tests/golden/autoland/direct_merge_as_stack.plan.txt b/tests/golden/autoland/direct_merge_as_stack.plan.txt new file mode 100644 index 0000000..224d552 --- /dev/null +++ b/tests/golden/autoland/direct_merge_as_stack.plan.txt @@ -0,0 +1,17 @@ +# Autoland plan — edit steps below. +# l [] = land that PR (a number, or its URL); a bare 'l' +# lands the next PR in the stack instead +# w = wait for a workflow to complete +# c [] = pause for manual confirmation; the optional +# condition names what to verify before proceeding +# (e.g. 'c QA sign-off complete') +# x = run a command, and stop unless it exits 0; it runs +# without a shell (use 'x bash -c ...' for pipes) +# +# Lines starting with # are comments and are ignored. +# Blank lines are ignored. +# +l 101 # Add the widget model +l 102 # Expose widgets over the API +w deploy.yaml +l 103 # Show widgets in the dashboard diff --git a/tests/golden/autoland/direct_merge_as_stack.status.json b/tests/golden/autoland/direct_merge_as_stack.status.json new file mode 100644 index 0000000..8da4e92 --- /dev/null +++ b/tests/golden/autoland/direct_merge_as_stack.status.json @@ -0,0 +1,62 @@ +{ + "status": "stopped", + "branch": "feat", + "base": "main", + "state_file": "/home/.stack-pr/autoland/feat.json", + "state_file_exists": true, + "lock_file": "/home/.stack-pr/autoland/feat.json.lock", + "pid": null, + "last_saved": "", + "abort_reason": null, + "resume_command": "stack-pr autoland --resume", + "plan": { + "total": 4, + "done": 4, + "remaining": 0, + "steps": [ + { + "number": 1, + "type": "land", + "outcome": "done", + "status": "Merged", + "is_next": false, + "pr_number": 101, + "pr_url": "https://github.com/o/r/pull/101", + "title": "Add the widget model", + "detail": null + }, + { + "number": 2, + "type": "land", + "outcome": "done", + "status": "Merged", + "is_next": false, + "pr_number": 102, + "pr_url": "https://github.com/o/r/pull/102", + "title": "Expose widgets over the API", + "detail": null + }, + { + "number": 3, + "type": "workflow", + "outcome": "done", + "status": "Workflow complete", + "is_next": false, + "workflow": "deploy.yaml", + "detail": null + }, + { + "number": 4, + "type": "land", + "outcome": "done", + "status": "Merged", + "is_next": false, + "pr_number": 103, + "pr_url": "https://github.com/o/r/pull/103", + "title": "Show widgets in the dashboard", + "detail": null + } + ] + }, + "other_runs": [] +} diff --git a/tests/golden/autoland/direct_merge_as_stack.status.txt b/tests/golden/autoland/direct_merge_as_stack.status.txt new file mode 100644 index 0000000..f697329 --- /dev/null +++ b/tests/golden/autoland/direct_merge_as_stack.status.txt @@ -0,0 +1,17 @@ +Autoland status + + Status: Stopped — not running, can be resumed + Branch: feat + Base: main + State file: /home/.stack-pr/autoland/feat.json + Last saved: + + +Autoland plan · 4 steps · 4 done · 0 remaining + + ✅ 1. Merged | Land | #101 - Add the widget model + ✅ 2. Merged | Land | #102 - Expose widgets over the API + ✅ 3. Workflow complete | Workflow | deploy.yaml + ✅ 4. Merged | Land | #103 - Show widgets in the dashboard + +Resume with: stack-pr autoland --resume diff --git a/tests/golden/autoland/direct_merge_retry.checkpoint.json b/tests/golden/autoland/direct_merge_retry.checkpoint.json new file mode 100644 index 0000000..042ce07 --- /dev/null +++ b/tests/golden/autoland/direct_merge_retry.checkpoint.json @@ -0,0 +1,45 @@ +{ + "version": 1, + "branch": "feat", + "base": "main", + "plan_file": null, + "current_step": 2, + "last_landed_sha": "9cc0a8c62f2d99c85284a96fe68191bd3843f14b", + "abort_reason": "", + "stack": [ + { + "pr_url": "https://github.com/o/r/pull/101", + "pr_number": 101, + "branch": "TestBot/stack/101", + "title": "Add the widget model", + "review_decision": "APPROVED", + "state": "merged", + "check_retries": 0, + "queue_retries": 1, + "error_message": "" + }, + { + "pr_url": "https://github.com/o/r/pull/102", + "pr_number": 102, + "branch": "TestBot/stack/102", + "title": "Expose widgets over the API", + "review_decision": "APPROVED", + "state": "merged", + "check_retries": 0, + "queue_retries": 0, + "error_message": "" + } + ], + "plan": [ + { + "type": "land", + "entry_index": 0, + "pr_number": 101 + }, + { + "type": "land", + "entry_index": 1, + "pr_number": 102 + } + ] +} diff --git a/tests/golden/autoland/direct_merge_retry.out.txt b/tests/golden/autoland/direct_merge_retry.out.txt new file mode 100644 index 0000000..e8d27fc --- /dev/null +++ b/tests/golden/autoland/direct_merge_retry.out.txt @@ -0,0 +1,54 @@ +Merging directly into main (no merge queue) + +Discovering stack... + + +Autoland plan · 2 steps · 0 done · 2 remaining + +→ ⬜ 1. Pending | Land | #101 - Add the widget model + ⬜ 2. Pending | Land | #102 - Expose widgets over the API +State file: /home/.stack-pr/autoland/feat.json + + +============================================================ +Step 1/2: Landing PR #101 — Add the widget model +============================================================ +PR #101 is mergeable (mergeStateStatus=CLEAN) + +Merging PR #101 +$ gh pr merge 101 --squash --match-head-commit 1a5a0dd7ff70fb1f21a0ca5f5ca31aa189d0188d --subject Add the widget model (#101) --body-file - +Failed to merge PR #101: Command failed (1): gh pr merge 101 --squash --match-head-commit 1a5a0dd7ff70fb1f21a0ca5f5ca31aa189d0188d --subject Add the widget model (#101) --body-file - +GraphQL: Base branch was modified. Review and try the merge again. (mergePullRequest) +Retrying the merge of PR #101 (attempt 2/2) +PR #101 is mergeable (mergeStateStatus=CLEAN) + +Merging PR #101 +$ gh pr merge 101 --squash --match-head-commit 1a5a0dd7ff70fb1f21a0ca5f5ca31aa189d0188d --subject Add the widget model (#101) --body-file - + +PR #101 merged! + +Rebasing stack on origin/main... +$ git fetch origin main +$ git rebase origin/main +Re-submitting stack... + +============================================================ +Step 2/2: Landing PR #102 — Expose widgets over the API +============================================================ +PR #102 is mergeable (mergeStateStatus=CLEAN) + +Merging PR #102 +$ gh pr merge 102 --squash --match-head-commit ca434a493f6745a2ec227ae2034f6b5e46c62268 --subject Expose widgets over the API (#102) --body-file - + +PR #102 merged! + + + +Autoland plan · 2 steps · 2 done · 0 remaining + + ✅ 1. Merged | Land | #101 - Add the widget model + ↳ retries CI 0 / MQ 1 + ✅ 2. Merged | Land | #102 - Expose widgets over the API + +All PRs landed successfully! + diff --git a/tests/golden/autoland/direct_merge_retry.plan.txt b/tests/golden/autoland/direct_merge_retry.plan.txt new file mode 100644 index 0000000..8e9ad6a --- /dev/null +++ b/tests/golden/autoland/direct_merge_retry.plan.txt @@ -0,0 +1,15 @@ +# Autoland plan — edit steps below. +# l [] = land that PR (a number, or its URL); a bare 'l' +# lands the next PR in the stack instead +# w = wait for a workflow to complete +# c [] = pause for manual confirmation; the optional +# condition names what to verify before proceeding +# (e.g. 'c QA sign-off complete') +# x = run a command, and stop unless it exits 0; it runs +# without a shell (use 'x bash -c ...' for pipes) +# +# Lines starting with # are comments and are ignored. +# Blank lines are ignored. +# +l 101 # Add the widget model +l 102 # Expose widgets over the API diff --git a/tests/golden/autoland/direct_merge_retry.status.json b/tests/golden/autoland/direct_merge_retry.status.json new file mode 100644 index 0000000..97adeb4 --- /dev/null +++ b/tests/golden/autoland/direct_merge_retry.status.json @@ -0,0 +1,42 @@ +{ + "status": "stopped", + "branch": "feat", + "base": "main", + "state_file": "/home/.stack-pr/autoland/feat.json", + "state_file_exists": true, + "lock_file": "/home/.stack-pr/autoland/feat.json.lock", + "pid": null, + "last_saved": "", + "abort_reason": null, + "resume_command": "stack-pr autoland --resume", + "plan": { + "total": 2, + "done": 2, + "remaining": 0, + "steps": [ + { + "number": 1, + "type": "land", + "outcome": "done", + "status": "Merged", + "is_next": false, + "pr_number": 101, + "pr_url": "https://github.com/o/r/pull/101", + "title": "Add the widget model", + "detail": "retries CI 0 / MQ 1" + }, + { + "number": 2, + "type": "land", + "outcome": "done", + "status": "Merged", + "is_next": false, + "pr_number": 102, + "pr_url": "https://github.com/o/r/pull/102", + "title": "Expose widgets over the API", + "detail": null + } + ] + }, + "other_runs": [] +} diff --git a/tests/golden/autoland/direct_merge_retry.status.txt b/tests/golden/autoland/direct_merge_retry.status.txt new file mode 100644 index 0000000..aa93a71 --- /dev/null +++ b/tests/golden/autoland/direct_merge_retry.status.txt @@ -0,0 +1,16 @@ +Autoland status + + Status: Stopped — not running, can be resumed + Branch: feat + Base: main + State file: /home/.stack-pr/autoland/feat.json + Last saved: + + +Autoland plan · 2 steps · 2 done · 0 remaining + + ✅ 1. Merged | Land | #101 - Add the widget model + ↳ retries CI 0 / MQ 1 + ✅ 2. Merged | Land | #102 - Expose widgets over the API + +Resume with: stack-pr autoland --resume diff --git a/tests/golden/autoland/direct_one_at_a_time.checkpoint.json b/tests/golden/autoland/direct_one_at_a_time.checkpoint.json new file mode 100644 index 0000000..9dcbf97 --- /dev/null +++ b/tests/golden/autoland/direct_one_at_a_time.checkpoint.json @@ -0,0 +1,51 @@ +{ + "version": 1, + "branch": "feat", + "base": "main", + "plan_file": "/plan.autoland-plan", + "current_step": 3, + "last_landed_sha": "9cc0a8c62f2d99c85284a96fe68191bd3843f14b", + "abort_reason": "", + "stack": [ + { + "pr_url": "https://github.com/o/r/pull/101", + "pr_number": 101, + "branch": "TestBot/stack/101", + "title": "Add the widget model", + "review_decision": "APPROVED", + "state": "merged", + "check_retries": 0, + "queue_retries": 0, + "error_message": "" + }, + { + "pr_url": "https://github.com/o/r/pull/102", + "pr_number": 102, + "branch": "TestBot/stack/102", + "title": "Expose widgets over the API", + "review_decision": "APPROVED", + "state": "merged", + "check_retries": 0, + "queue_retries": 0, + "error_message": "" + } + ], + "plan": [ + { + "type": "land", + "entry_index": 0, + "pr_number": 101 + }, + { + "type": "workflow", + "workflow": "deploy.yaml", + "state": "succeeded", + "error_message": "" + }, + { + "type": "land", + "entry_index": 1, + "pr_number": 102 + } + ] +} diff --git a/tests/golden/autoland/direct_one_at_a_time.out.txt b/tests/golden/autoland/direct_one_at_a_time.out.txt new file mode 100644 index 0000000..5554017 --- /dev/null +++ b/tests/golden/autoland/direct_one_at_a_time.out.txt @@ -0,0 +1,63 @@ +Merging directly into main (no merge queue) + +Discovering stack... + + +Autoland plan · 3 steps · 0 done · 3 remaining + +→ ⬜ 1. Pending | Land | #101 - Add the widget model + ⬜ 2. Pending | Workflow | deploy.yaml + ⬜ 3. Pending | Land | #102 - Expose widgets over the API +State file: /home/.stack-pr/autoland/feat.json + + +============================================================ +Step 1/3: Landing PR #101 — Add the widget model +============================================================ +PR #101: Checks in progress... — polling in 120s +PR #101 is mergeable (mergeStateStatus=CLEAN) + +Merging PR #101 +$ gh pr merge 101 --squash --match-head-commit 1a5a0dd7ff70fb1f21a0ca5f5ca31aa189d0188d --subject Add the widget model (#101) --body-file - + +PR #101 merged! + +Rebasing stack on origin/main... +$ git fetch origin main +$ git rebase origin/main +Re-submitting stack... + +============================================================ +Step 2/3: Workflow checkpoint — deploy.yaml +============================================================ + +Waiting for workflow: deploy.yaml +Target SHA: 78e4ec1063c7 +Workflow deploy.yaml: waiting (0m) — polling in 120s + +Workflow deploy.yaml completed with SHA 78e4ec1063c7 + +============================================================ +Step 3/3: Landing PR #102 — Expose widgets over the API +============================================================ +PR #102 is not yet approved (REVIEW_REQUIRED). Waiting... +PR #102: waiting for approval — polling in 120s +PR #102: waiting for approval — polling in 120s +PR #102 is now approved +PR #102 is mergeable (mergeStateStatus=CLEAN) + +Merging PR #102 +$ gh pr merge 102 --squash --match-head-commit ca434a493f6745a2ec227ae2034f6b5e46c62268 --subject Expose widgets over the API (#102) --body-file - + +PR #102 merged! + + + +Autoland plan · 3 steps · 3 done · 0 remaining + + ✅ 1. Merged | Land | #101 - Add the widget model + ✅ 2. Workflow complete | Workflow | deploy.yaml + ✅ 3. Merged | Land | #102 - Expose widgets over the API + +All PRs landed successfully! + diff --git a/tests/golden/autoland/direct_one_at_a_time.plan.txt b/tests/golden/autoland/direct_one_at_a_time.plan.txt new file mode 100644 index 0000000..5219d0e --- /dev/null +++ b/tests/golden/autoland/direct_one_at_a_time.plan.txt @@ -0,0 +1,16 @@ +# Autoland plan — edit steps below. +# l [] = land that PR (a number, or its URL); a bare 'l' +# lands the next PR in the stack instead +# w = wait for a workflow to complete +# c [] = pause for manual confirmation; the optional +# condition names what to verify before proceeding +# (e.g. 'c QA sign-off complete') +# x = run a command, and stop unless it exits 0; it runs +# without a shell (use 'x bash -c ...' for pipes) +# +# Lines starting with # are comments and are ignored. +# Blank lines are ignored. +# +l 101 # Add the widget model +w deploy.yaml +l 102 # Expose widgets over the API diff --git a/tests/golden/autoland/direct_one_at_a_time.status.json b/tests/golden/autoland/direct_one_at_a_time.status.json new file mode 100644 index 0000000..6a785a6 --- /dev/null +++ b/tests/golden/autoland/direct_one_at_a_time.status.json @@ -0,0 +1,51 @@ +{ + "status": "stopped", + "branch": "feat", + "base": "main", + "state_file": "/home/.stack-pr/autoland/feat.json", + "state_file_exists": true, + "lock_file": "/home/.stack-pr/autoland/feat.json.lock", + "pid": null, + "last_saved": "", + "abort_reason": null, + "resume_command": "stack-pr autoland --resume", + "plan": { + "total": 3, + "done": 3, + "remaining": 0, + "steps": [ + { + "number": 1, + "type": "land", + "outcome": "done", + "status": "Merged", + "is_next": false, + "pr_number": 101, + "pr_url": "https://github.com/o/r/pull/101", + "title": "Add the widget model", + "detail": null + }, + { + "number": 2, + "type": "workflow", + "outcome": "done", + "status": "Workflow complete", + "is_next": false, + "workflow": "deploy.yaml", + "detail": null + }, + { + "number": 3, + "type": "land", + "outcome": "done", + "status": "Merged", + "is_next": false, + "pr_number": 102, + "pr_url": "https://github.com/o/r/pull/102", + "title": "Expose widgets over the API", + "detail": null + } + ] + }, + "other_runs": [] +} diff --git a/tests/golden/autoland/direct_one_at_a_time.status.txt b/tests/golden/autoland/direct_one_at_a_time.status.txt new file mode 100644 index 0000000..7aa811c --- /dev/null +++ b/tests/golden/autoland/direct_one_at_a_time.status.txt @@ -0,0 +1,16 @@ +Autoland status + + Status: Stopped — not running, can be resumed + Branch: feat + Base: main + State file: /home/.stack-pr/autoland/feat.json + Last saved: + + +Autoland plan · 3 steps · 3 done · 0 remaining + + ✅ 1. Merged | Land | #101 - Add the widget model + ✅ 2. Workflow complete | Workflow | deploy.yaml + ✅ 3. Merged | Land | #102 - Expose widgets over the API + +Resume with: stack-pr autoland --resume diff --git a/tests/golden/autoland/merge_mode_unknown.out.txt b/tests/golden/autoland/merge_mode_unknown.out.txt new file mode 100644 index 0000000..d4e9be3 --- /dev/null +++ b/tests/golden/autoland/merge_mode_unknown.out.txt @@ -0,0 +1,3 @@ +Could not tell whether main uses a merge queue. Say which with one of: + stack-pr config autoland.merge_queue=true + stack-pr config autoland.merge_queue=false diff --git a/tests/golden/autoland/merge_mode_unknown.status.json b/tests/golden/autoland/merge_mode_unknown.status.json new file mode 100644 index 0000000..126d2d0 --- /dev/null +++ b/tests/golden/autoland/merge_mode_unknown.status.json @@ -0,0 +1,14 @@ +{ + "status": "none", + "branch": "feat", + "base": null, + "state_file": "/home/.stack-pr/autoland/feat.json", + "state_file_exists": false, + "lock_file": "/home/.stack-pr/autoland/feat.json.lock", + "pid": null, + "last_saved": null, + "abort_reason": null, + "resume_command": null, + "plan": null, + "other_runs": [] +} diff --git a/tests/golden/autoland/merge_mode_unknown.status.txt b/tests/golden/autoland/merge_mode_unknown.status.txt new file mode 100644 index 0000000..1858cf1 --- /dev/null +++ b/tests/golden/autoland/merge_mode_unknown.status.txt @@ -0,0 +1,2 @@ +No autoland in progress for branch feat. +State file (not present): /home/.stack-pr/autoland/feat.json diff --git a/tests/test_autoland.py b/tests/test_autoland.py index f873a22..186cec8 100644 --- a/tests/test_autoland.py +++ b/tests/test_autoland.py @@ -43,6 +43,7 @@ _plan_rows, _StepRow, ) +from stack_pr.autoland.merge_mode import MergeMode from stack_pr.autoland.model import ( ConfirmStep, LandingContext, @@ -163,26 +164,62 @@ def test_default_workflow_absent_is_none() -> None: assert opts.default_workflow is None -# --- merge-queue gate ---------------------------------------------------- +# --- merge mode at the entry point ------------------------------------------- -def _forbid_landing_flows(monkeypatch: pytest.MonkeyPatch) -> None: - """Make the fresh/resume/replan flows fail the test if reached, so a test of - run_autoland's early exits can never run a real land.""" +def _stub_landing_flows(monkeypatch: pytest.MonkeyPatch) -> list[AutolandOptions]: + """Replace the fresh/resume/replan flows so a test of run_autoland's entry + can never run a real land. A fresh run records its options; the others fail + the test.""" + fresh_runs: list[AutolandOptions] = [] def forbidden(*_args: object, **_kwargs: object) -> None: - pytest.fail("run_autoland reached a landing flow") + pytest.fail("run_autoland reached the resume/replan flow") - for flow in ("_run_fresh", "_run_resume", "_run_replan"): - monkeypatch.setattr(commands, flow, forbidden) + monkeypatch.setattr( + commands, "_run_fresh", lambda _common, opts: fresh_runs.append(opts) + ) + monkeypatch.setattr(commands, "_run_resume", forbidden) + monkeypatch.setattr(commands, "_run_replan", forbidden) + return fresh_runs + + +def _printed(console: Mock) -> str: + return "\n".join(str(a) for c in console.print.call_args_list for a in c.args) + + +def test_run_autoland_without_merge_queue_lands_directly( + monkeypatch: pytest.MonkeyPatch, autoland_console: Mock +) -> None: + fresh_runs = _stub_landing_flows(monkeypatch) + github = FakeGitHub() + github.script("has_merge_queue", False) + monkeypatch.setattr(gh, "github", github) + cfg = configparser.ConfigParser() # no [autoland] -> merge_queue unset + + commands.run_autoland(common_args(), _args(), cfg) + assert github.called("has_merge_queue") == [("main",)] + assert [opts.merge_mode for opts in fresh_runs] == [MergeMode.DIRECT] + assert "Merging directly into main (no merge queue)" in _printed(autoland_console) -def test_run_autoland_requires_merge_queue(monkeypatch: pytest.MonkeyPatch) -> None: - _forbid_landing_flows(monkeypatch) - cfg = configparser.ConfigParser() # no [autoland] -> merge_queue False - with pytest.raises(NotImplementedError): + +def test_run_autoland_stops_when_the_merge_mode_is_unknown( + monkeypatch: pytest.MonkeyPatch, autoland_console: Mock +) -> None: + fresh_runs = _stub_landing_flows(monkeypatch) + github = FakeGitHub() + github.script("has_merge_queue", None) + monkeypatch.setattr(gh, "github", github) + cfg = configparser.ConfigParser() + + with pytest.raises(SystemExit) as exc: commands.run_autoland(common_args(), _args(), cfg) + assert exc.value.code == 1 + assert fresh_runs == [] + assert "stack-pr config autoland.merge_queue=false" in _printed(autoland_console) + # --- check evaluation (pure: takes the check list) ------------------------ @@ -1116,7 +1153,7 @@ def test_confirm_step_banner_is_a_single_line() -> None: def test_execute_plan_lands_a_pinned_step_that_is_still_open() -> None: - fakes = FakeServices(waits=FakeWaits(queue=ok)) + fakes = FakeServices(waits=FakeWaits(merge=ok)) stack = _pinned_stack([103]) plan = parse_plan("l 102\nl 103\n", stack, pr_is_merged=lambda pr: pr == 102) @@ -1127,7 +1164,7 @@ def test_execute_plan_lands_a_pinned_step_that_is_still_open() -> None: ) assert fakes.waits is not None - assert fakes.waits.calls == ["approval", "checks", "queue"] + assert fakes.waits.calls == ["approval", "checks", "merge"] assert all(entry is stack[0] for entry in fakes.waits.entries) @@ -1977,7 +2014,7 @@ class _StackLanding: def __init__(self, plan_text: str, prs: list[int], **opts: object) -> None: self.github = FakeGitHub() - self.waits = FakeWaits(queue=self._land_one) + self.waits = FakeWaits(merge=self._land_one) self.fakes = FakeServices(github=self.github, waits=self.waits) stack = _pinned_stack(prs) self._ctx = LandingContext(stack=stack, plan=parse_plan(plan_text, stack)) @@ -2002,7 +2039,7 @@ def rebases(self) -> int: return self.fakes.repo.rebases def landed_one_by_one(self) -> list[int]: - return self.waits.waited("queue") + return self.waits.waited("merge") def _land_with_fake_github(plan_text: str, prs: list[int], **opts: object): # noqa: ANN202 @@ -2254,7 +2291,7 @@ def test_merge_queue_wait_polls_again_after_a_failed_read(mocker) -> None: # no ) entry = _pinned_stack([101])[0] - assert _real_github().waits.enqueue_and_wait(entry, opts=_opts()) + assert _real_github().waits.merge_and_wait(entry, opts=_opts()) assert entry.state == PRState.MERGED @@ -2264,7 +2301,7 @@ def test_merge_queue_wait_still_times_out_while_reads_fail(mocker) -> None: # n entry = _pinned_stack([101])[0] opts = _opts(poll_interval=60, merge_timeout=300) - assert not _real_github().waits.enqueue_and_wait(entry, opts=opts) + assert not _real_github().waits.merge_and_wait(entry, opts=opts) assert entry.error_message == "Timed out waiting for merge queue" diff --git a/tests/test_autoland_golden.py b/tests/test_autoland_golden.py index 8fa3879..8fcd42b 100644 --- a/tests/test_autoland_golden.py +++ b/tests/test_autoland_golden.py @@ -237,6 +237,48 @@ def _one_at_a_time_prs() -> list[FakePR]: landed=[101, 102], checkpoint=_replan_checkpoint, ), + # No merge queue on the target branch, and none configured: autoland + # detects that and merges each PR directly, with a workflow between. + Scenario( + name="direct_one_at_a_time", + prs=_one_at_a_time_prs(), + stack=[101, 102], + plan="l\nw deploy.yaml\nl\n", + config={"merge_queue": None}, + has_queue=False, + workflows={"deploy.yaml": "success"}, + ), + # A run of land steps merges as one GitHub stack, directly rather than + # through a queue. + Scenario( + name="direct_merge_as_stack", + prs=_prs(101, 102, 103), + stack=[101, 102, 103], + plan="l\nl\nw deploy.yaml\nl\n", + config={"merge_queue": None, "merge_as_stack": "true"}, + has_queue=False, + workflows={"deploy.yaml": "success"}, + ), + # The first direct merge fails (the base moved under it); after approval + # and checks are verified again, the retry goes through. + Scenario( + name="direct_merge_retry", + prs=_prs(101, 102), + stack=[101, 102], + config={"merge_queue": None}, + has_queue=False, + merge_failures=1, + ), + # GitHub can't say whether the target branch has a merge queue, and the + # config doesn't either: autoland stops before doing anything. + Scenario( + name="merge_mode_unknown", + prs=_prs(101, 102), + stack=[101, 102], + config={"merge_queue": None}, + queue_lookup_fails=True, + expect_exit=1, + ), ] @@ -272,6 +314,9 @@ def test_golden_scenario( for suffix, attr in ARTIFACTS.items(): path = GOLDEN_DIR / f"{scenario.name}.{suffix}" actual = getattr(result, attr) + if actual is None: # the run stopped before it wrote a checkpoint + assert not path.exists(), f"{path.name} is stale; delete it" + continue if UPDATE: path.parent.mkdir(parents=True, exist_ok=True) path.write_text(actual) diff --git a/tests/test_autoland_merge_mode.py b/tests/test_autoland_merge_mode.py new file mode 100644 index 0000000..97ca486 --- /dev/null +++ b/tests/test_autoland_merge_mode.py @@ -0,0 +1,217 @@ +"""Landing without a merge queue: resolving the merge mode, and merging directly.""" + +from __future__ import annotations + +import argparse +import configparser +import dataclasses + +import pytest + +from stack_pr.autoland import native_stack +from stack_pr.autoland.merge_mode import MergeMode, MergeModeError, resolve_merge_mode +from stack_pr.autoland.model import LandingContext, PRState +from stack_pr.autoland.options import AutolandOptions +from tests.autoland_fakes import ( + OPTS, + FakeGitHub, + FakeServices, + FakeSleep, + FakeWaits, + stack, +) +from tests.helpers import common_args + +DIRECT = dataclasses.replace(OPTS, merge_mode=MergeMode.DIRECT) + + +# --- resolving the mode ------------------------------------------------------ + + +@pytest.mark.parametrize( + ("value", "expected"), [("true", True), ("false", False), (None, None)] +) +def test_merge_queue_config_is_an_optional_override( + value: str | None, expected: bool | None +) -> None: + cfg = configparser.ConfigParser() + cfg["autoland"] = {} if value is None else {"merge_queue": value} + + opts = AutolandOptions.from_config_and_args(cfg, argparse.Namespace()) + + assert opts.merge_queue is expected + + +@pytest.mark.parametrize( + ("configured", "expected"), [(True, MergeMode.QUEUE), (False, MergeMode.DIRECT)] +) +def test_configured_mode_wins_without_asking_github( + configured: bool, expected: MergeMode +) -> None: + github = FakeGitHub() + + assert resolve_merge_mode("main", github, configured=configured) is expected # type: ignore[arg-type] + assert github.calls == [] + + +@pytest.mark.parametrize( + ("has_queue", "expected"), [(True, MergeMode.QUEUE), (False, MergeMode.DIRECT)] +) +def test_unset_mode_follows_whether_the_target_has_a_queue( + has_queue: bool, expected: MergeMode +) -> None: + github = FakeGitHub() + github.script("has_merge_queue", has_queue) + + assert resolve_merge_mode("release", github, configured=None) is expected # type: ignore[arg-type] + assert github.called("has_merge_queue") == [("release",)] + + +def test_unset_mode_fails_when_github_cannot_say() -> None: + github = FakeGitHub() + github.script("has_merge_queue", None) + + with pytest.raises(MergeModeError, match=r"autoland\.merge_queue=false"): + resolve_merge_mode("main", github, configured=None) # type: ignore[arg-type] + + +# --- merging one PR directly --------------------------------------------------- + + +def _merge(fakes: FakeServices, opts: AutolandOptions = DIRECT): # noqa: ANN202 + """Merge PR #101, whose checks pass, the way *opts* says.""" + fakes.github.pr_checks[101] = [{"name": "ci", "bucket": "pass"}] + entry = stack(101)[0] + return fakes.services.waits.merge_and_wait(entry, opts=opts), entry + + +def test_direct_merge_squashes_with_the_stack_pr_land_message() -> None: + fakes = FakeServices.polling() + + merged, entry = _merge(fakes) + + assert merged + assert entry.state == PRState.MERGED + # The commit message without its stack-info trailer, numbered like + # `stack-pr land` numbers it, pinned to the head it was read from. + assert fakes.github.squash_merges == [ + (101, "Title 101 (#101)", "\nBody of 101.", "head-101") + ] + assert fakes.github.enqueued == [] + + +def test_queue_mode_still_enqueues() -> None: + fakes = FakeServices.polling() + + merged, _entry = _merge(fakes, OPTS) + + assert merged + assert fakes.github.enqueued == [101] + assert fakes.github.squash_merges == [] + + +def test_direct_merge_retries_a_failed_merge_after_rechecking() -> None: + fakes = FakeServices.polling() + fakes.github.script("squash_merge", RuntimeError("Base branch was modified")) + + merged, entry = _merge(fakes, dataclasses.replace(DIRECT, max_queue_retries=1)) + + assert merged + assert entry.queue_retries == 1 + assert len(fakes.github.squash_merges) == 1 + # Approval and checks were verified again before the retry. + assert len(fakes.github.called("review_decision")) == 1 + assert len(fakes.github.called("checks")) == 1 + assert "Base branch was modified" in fakes.ui.output + + +def test_direct_merge_gives_up_after_its_retries() -> None: + fakes = FakeServices.polling() + fakes.github.script( + "squash_merge", RuntimeError("not mergeable"), RuntimeError("not mergeable") + ) + + merged, entry = _merge(fakes, dataclasses.replace(DIRECT, max_queue_retries=1)) + + assert not merged + assert entry.state == PRState.FAILED + assert entry.error_message == "Failed to merge after 2 attempts: not mergeable" + + +def test_direct_merge_that_errored_but_went_through_is_not_retried() -> None: + fakes = FakeServices.polling() + + def lost_response(pr: int, **_kwargs: object) -> None: + fakes.github.merged.add(pr) + raise RuntimeError("unexpected EOF") + + fakes.github.script("squash_merge", lost_response) + + merged, entry = _merge(fakes) + + assert merged + assert entry.queue_retries == 0 + + +def test_direct_merge_waits_for_github_to_report_it() -> None: + fakes = FakeServices.polling() + fakes.github.script("pr_state", "OPEN", "OPEN") # lagging behind the merge + + merged, entry = _merge(fakes) + + assert merged + assert entry.queue_retries == 0 + assert len(fakes.github.squash_merges) == 1 + + +def test_direct_merge_of_a_closed_pr_fails() -> None: + fakes = FakeServices.polling() + fakes.github.closed.add(101) + + merged, entry = _merge(fakes) + + assert not merged + assert entry.error_message == "PR was closed" + assert fakes.github.squash_merges == [] + + +def test_direct_merge_waits_out_a_branch_that_is_behind() -> None: + sleep = FakeSleep() + fakes = FakeServices.polling(sleep=sleep) + behind = {"state": "OPEN", "mergeStateStatus": "BEHIND", "mergeable": "MERGEABLE"} + fakes.github.script("merge_state", behind, behind) + + merged, _entry = _merge(fakes) + + assert merged + assert len(sleep.slept) == 2 + # Says why it is waiting, and what to do, once. + assert fakes.ui.output.count("stack-pr submit") == 1 + + +# --- merging a run as a GitHub stack, directly -------------------------------- + + +def test_direct_stack_merge_is_not_mistaken_for_a_boot() -> None: + github = FakeGitHub() + github.booted = {101, 102} # nothing is ever in a queue + github.lands = [] # the request is still pending at the first poll + github.script("merge_async_status", ("pending", "")) + + def merge_while_asleep(_seconds: int) -> float: + github.merged.update({101, 102}) + return 0.0 + + fakes = FakeServices(github=github, sleep=merge_while_asleep, waits=FakeWaits()) + entries = stack(101, 102) + + result = native_stack.land_as_native_stack( + entries, + ctx=LandingContext(stack=entries), + common=common_args(), + opts=DIRECT, + services=fakes.services, + ) + + assert result.landed + assert github.merge_via_queue == [False] diff --git a/tests/test_autoland_step_execution.py b/tests/test_autoland_step_execution.py index 8850c3f..63b446c 100644 --- a/tests/test_autoland_step_execution.py +++ b/tests/test_autoland_step_execution.py @@ -102,7 +102,7 @@ def test_land_step_lands_and_rebases_the_stack_above() -> None: ) assert result == StepResult(landed_sha="sha101") - assert waits.calls == ["approval", "checks", "queue"] + assert waits.calls == ["approval", "checks", "merge"] assert entries[0].is_merged() assert repo.lookups == [101] assert repo.rebases == 1 @@ -168,9 +168,9 @@ def test_already_landed_step_reports_its_sha_only_at_the_end_of_the_prefix( [ (FakeWaits(approval=fails), "PR #101 was closed while waiting for approval"), (FakeWaits(checks=fails), "PR #101 checks failed after retries"), - (FakeWaits(queue=fails), "PR #101 failed to merge"), + (FakeWaits(merge=fails), "PR #101 failed to merge"), ], - ids=["approval", "checks", "queue"], + ids=["approval", "checks", "merge"], ) def test_land_step_aborts_when_a_wait_fails(waits: FakeWaits, reason: str) -> None: repo = FakeRepo(shas={101: "sha101"}) diff --git a/tests/test_autoland_steps.py b/tests/test_autoland_steps.py index 0533187..3278f88 100644 --- a/tests/test_autoland_steps.py +++ b/tests/test_autoland_steps.py @@ -229,6 +229,7 @@ def test_land_view_of_a_pending_pr() -> None: "In merge queue", StepTone.IN_QUEUE, ), + (PRState.MERGING, StepOutcome.ACTIVE, "Merging", StepTone.MERGING), ( PRState.WAITING_FOR_WORKFLOW, StepOutcome.ACTIVE,