diff --git a/.github/workflows/code_style.yml b/.github/workflows/code_style.yml index 7907a0086..9cb1befed 100644 --- a/.github/workflows/code_style.yml +++ b/.github/workflows/code_style.yml @@ -44,9 +44,13 @@ jobs: # runner, so it falls back to a GitHub-hosted one. Branch # pull requests, which are the ones a maintainer waits on, # keep the paid shape. + # Dependabot pull requests run on the hosted pool too (free on a public + # repository). The author is tested, not the actor, which changes when + # someone else pushes to the branch. runs-on: >- - ${{ github.event.pull_request.head.repo.fork && 'ubuntu-latest' - || 'ubicloud-standard-4' }} + ${{ (github.event.pull_request.head.repo.fork + || github.event.pull_request.user.login == 'dependabot[bot]') + && 'ubuntu-latest' || 'ubicloud-standard-4' }} timeout-minutes: 30 steps: - name: Checkout repository @@ -132,8 +136,9 @@ jobs: # pull requests, which are the ones a maintainer waits on, # keep the paid shape. runs-on: >- - ${{ github.event.pull_request.head.repo.fork && 'ubuntu-latest' - || 'ubicloud-standard-4' }} + ${{ (github.event.pull_request.head.repo.fork + || github.event.pull_request.user.login == 'dependabot[bot]') + && 'ubuntu-latest' || 'ubicloud-standard-4' }} timeout-minutes: 75 env: # sccache is the sole owner of compiler output: no cache step archives a diff --git a/.github/workflows/codescene-coverage.yml b/.github/workflows/codescene-coverage.yml index fbd6b9ad8..bf827eca2 100644 --- a/.github/workflows/codescene-coverage.yml +++ b/.github/workflows/codescene-coverage.yml @@ -65,9 +65,13 @@ jobs: # runner, so it falls back to a GitHub-hosted one. Branch # pull requests, which are the ones a maintainer waits on, # keep the paid shape. + # Dependabot pull requests run on the hosted pool too (free on a public + # repository). The author is tested, not the actor, which changes when + # someone else pushes to the branch. runs-on: >- - ${{ github.event.pull_request.head.repo.fork && 'ubuntu-latest' - || 'ubicloud-standard-4' }} + ${{ (github.event.pull_request.head.repo.fork + || github.event.pull_request.user.login == 'dependabot[bot]') + && 'ubuntu-latest' || 'ubicloud-standard-4' }} timeout-minutes: 90 needs: [compile-contracts] env: diff --git a/.github/workflows/e2e.yml b/.github/workflows/e2e.yml index 3dca29978..633e9692c 100644 --- a/.github/workflows/e2e.yml +++ b/.github/workflows/e2e.yml @@ -37,10 +37,14 @@ jobs: # fork cannot obtain an Ubicloud runner, so it runs # GitHub-hosted. A branch pull request, which is the one a # maintainer waits on, keeps the paid shape. + # Dependabot pull requests run on the hosted pool too (free on a public + # repository). The author is tested, not the actor, which changes when + # someone else pushes to the branch. runs-on: >- ${{ github.event_name == 'schedule' && 'ubuntu-latest' - || github.event.pull_request.head.repo.fork && 'ubuntu-latest' - || 'ubicloud-standard-4' }} + || (github.event.pull_request.head.repo.fork + || github.event.pull_request.user.login == 'dependabot[bot]') + && 'ubuntu-latest' || 'ubicloud-standard-4' }} timeout-minutes: 30 env: # sccache is the sole owner of compiler output: no cache step archives a @@ -147,8 +151,9 @@ jobs: # maintainer waits on, keeps the paid shape. runs-on: >- ${{ github.event_name == 'schedule' && 'ubuntu-latest' - || github.event.pull_request.head.repo.fork && 'ubuntu-latest' - || 'ubicloud-standard-2' }} + || (github.event.pull_request.head.repo.fork + || github.event.pull_request.user.login == 'dependabot[bot]') + && 'ubuntu-latest' || 'ubicloud-standard-2' }} timeout-minutes: 30 strategy: fail-fast: false diff --git a/.github/workflows/pr-label-classify.yml b/.github/workflows/pr-label-classify.yml index e7331e49e..d069dc173 100644 --- a/.github/workflows/pr-label-classify.yml +++ b/.github/workflows/pr-label-classify.yml @@ -16,6 +16,9 @@ jobs: # takes the smallest ARM shape: on the hosted pool this 13-20 s job # waited up to 2,098 s to start (ADR 0013). `pull_request_target` runs in # this repository's context for a fork too, so it needs no fork fallback. + # dependabot-runner: exempt. A few seconds of labelling on a `pull_request_target` + # event; a hosted arm would need a new hosted arm64 label in the registry and + # contracts for no measurable saving. runs-on: ubicloud-standard-2-arm timeout-minutes: 10 steps: diff --git a/.github/workflows/pr-label-scope.yml b/.github/workflows/pr-label-scope.yml index 5b6bd2193..f094d53c8 100644 --- a/.github/workflows/pr-label-scope.yml +++ b/.github/workflows/pr-label-scope.yml @@ -14,6 +14,9 @@ jobs: # architecture, so the smallest ARM shape: on the hosted pool this # 4-7 s job waited up to 1,495 s to start (ADR 0013). Like the classifier # it runs on `pull_request_target`, so a fork needs no fallback. + # dependabot-runner: exempt. A few seconds of labelling on a `pull_request_target` + # event; a hosted arm would need a new hosted arm64 label in the registry and + # contracts for no measurable saving. runs-on: ubicloud-standard-2-arm timeout-minutes: 10 steps: diff --git a/.github/workflows/regression-test-check.yml b/.github/workflows/regression-test-check.yml index 6db91b597..42b6e1e12 100644 --- a/.github/workflows/regression-test-check.yml +++ b/.github/workflows/regression-test-check.yml @@ -25,9 +25,13 @@ jobs: # the smallest ARM shape: on the hosted pool this 6-8 s job waited up to # 734 s to start (ADR 0013). A fork's pull request cannot obtain an # Ubicloud runner, so it falls back to a hosted one. + # Dependabot pull requests run on the hosted pool too (free on a public + # repository). The author is tested, not the actor, which changes when + # someone else pushes to the branch. runs-on: >- - ${{ github.event.pull_request.head.repo.fork && 'ubuntu-latest' - || 'ubicloud-standard-2-arm' }} + ${{ (github.event.pull_request.head.repo.fork + || github.event.pull_request.user.login == 'dependabot[bot]') + && 'ubuntu-latest' || 'ubicloud-standard-2-arm' }} timeout-minutes: 10 steps: - name: Checkout repository diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml index aaa6e8f07..e5d505ee9 100644 --- a/.github/workflows/test.yml +++ b/.github/workflows/test.yml @@ -89,10 +89,14 @@ jobs: # fork cannot obtain an Ubicloud runner, so it runs # GitHub-hosted. A branch pull request, which is the one a # maintainer waits on, keeps the paid shape. + # Dependabot pull requests run on the hosted pool too (free on a public + # repository). The author is tested, not the actor, which changes when + # someone else pushes to the branch. runs-on: >- ${{ github.event_name == 'schedule' && 'ubuntu-latest' - || github.event.pull_request.head.repo.fork && 'ubuntu-latest' - || 'ubicloud-standard-4' }} + || (github.event.pull_request.head.repo.fork + || github.event.pull_request.user.login == 'dependabot[bot]') + && 'ubuntu-latest' || 'ubicloud-standard-4' }} timeout-minutes: 90 env: # sccache is the sole owner of compiler output: no cache step archives a @@ -274,8 +278,9 @@ jobs: # maintainer waits on, keeps the paid shape. runs-on: >- ${{ github.event_name == 'schedule' && 'ubuntu-latest' - || github.event.pull_request.head.repo.fork && 'ubuntu-latest' - || 'ubicloud-standard-2' }} + || (github.event.pull_request.head.repo.fork + || github.event.pull_request.user.login == 'dependabot[bot]') + && 'ubuntu-latest' || 'ubicloud-standard-2' }} timeout-minutes: 45 env: # sccache is the sole owner of compiler output: no cache step archives a @@ -377,8 +382,9 @@ jobs: # maintainer waits on, keeps the paid shape. runs-on: >- ${{ github.event_name == 'schedule' && 'ubuntu-latest' - || github.event.pull_request.head.repo.fork && 'ubuntu-latest' - || 'ubicloud-standard-2' }} + || (github.event.pull_request.head.repo.fork + || github.event.pull_request.user.login == 'dependabot[bot]') + && 'ubuntu-latest' || 'ubicloud-standard-2' }} timeout-minutes: 20 env: # sccache is the sole owner of compiler output: no cache step archives a @@ -475,8 +481,9 @@ jobs: # This small member-specific matrix leg also exercises HTTP/2 policy. runs-on: >- ${{ github.event_name == 'schedule' && 'ubuntu-latest' - || github.event.pull_request.head.repo.fork && 'ubuntu-latest' - || 'ubicloud-standard-2' }} + || (github.event.pull_request.head.repo.fork + || github.event.pull_request.user.login == 'dependabot[bot]') + && 'ubuntu-latest' || 'ubicloud-standard-2' }} timeout-minutes: 30 env: # sccache is the sole owner of compiler output: no cache step archives a @@ -640,8 +647,9 @@ jobs: # maintainer waits on, keeps the paid shape. runs-on: >- ${{ github.event_name == 'schedule' && 'ubuntu-latest' - || github.event.pull_request.head.repo.fork && 'ubuntu-latest' - || 'ubicloud-standard-4' }} + || (github.event.pull_request.head.repo.fork + || github.event.pull_request.user.login == 'dependabot[bot]') + && 'ubuntu-latest' || 'ubicloud-standard-4' }} timeout-minutes: 75 env: # sccache is the sole owner of compiler output: no cache step archives a @@ -758,8 +766,9 @@ jobs: # maintainer waits on, keeps the paid shape. runs-on: >- ${{ github.event_name == 'schedule' && 'ubuntu-latest' - || github.event.pull_request.head.repo.fork && 'ubuntu-latest' - || 'ubicloud-standard-4' }} + || (github.event.pull_request.head.repo.fork + || github.event.pull_request.user.login == 'dependabot[bot]') + && 'ubuntu-latest' || 'ubicloud-standard-4' }} timeout-minutes: 45 steps: - name: Checkout repository diff --git a/docs/developers-guide.md b/docs/developers-guide.md index da33d8233..5d8c88cbc 100644 --- a/docs/developers-guide.md +++ b/docs/developers-guide.md @@ -357,12 +357,18 @@ to this repository and the fork's run does not reach it, so a lane that asks for one does not fail: it queues, waits, and is eventually reported as a stuck or cancelled check on somebody else's contribution, which reads as this repository being broken. Every lane a `pull_request` can dispatch therefore -names a GitHub-hosted runner for that case: +names a GitHub-hosted runner for that case. A pull request Dependabot opened +takes the same hosted arm: hosted minutes cost nothing on a public repository, +and Dependabot opens many pull requests, which therefore run on the hosted pool +without Ubicloud's cache store. The condition tests the pull request's author, +not `github.actor`, because the actor is whoever triggered the run, so a person +pushing to a Dependabot branch would flip the runner: ```yaml runs-on: >- - ${{ github.event.pull_request.head.repo.fork && 'ubuntu-latest' - || 'ubicloud-standard-4' }} + ${{ (github.event.pull_request.head.repo.fork + || github.event.pull_request.user.login == 'dependabot[bot]') + && 'ubuntu-latest' || 'ubicloud-standard-4' }} ``` A lane that also serves a cron composes both conditions in one expression, in @@ -371,33 +377,38 @@ that order, because `runs-on` is the only place either distinction can live: ```yaml runs-on: >- ${{ github.event_name == 'schedule' && 'ubuntu-latest' - || github.event.pull_request.head.repo.fork && 'ubuntu-latest' - || 'ubicloud-standard-2' }} + || (github.event.pull_request.head.repo.fork + || github.event.pull_request.user.login == 'dependabot[bot]') + && 'ubuntu-latest' || 'ubicloud-standard-2' }} ``` +The reader splits the chain on the `||` outside parentheses, so the hosted +condition's own `||` is not a chain separator, and it recognizes that condition +only as written (the fork field, then the Dependabot author). + `Job.runner_labels` reads a chain of any length and reports every distinct label, so a lane still answers `uses_ubicloud` and stays inside the timeout, sizing and sccache contracts. `Job.labels_for_event` answers which arm an event -selects, and it answers a fork condition false on purpose: these contracts ask -what a lane costs this repository, a fork's run costs nothing here, and reading -the fork arm as the pull-request answer would report every one of these lanes -as free while hiding the shape they buy for a branch pull request. A chain the -helpers cannot read, one with no fallback arm or naming a condition they cannot -evaluate, stays one opaque label rather than being split into arms nobody -checked. - -`tests/workflow_contracts/fork_fallback_test.py` asserts the field rather than -the shape, because the failure worth stopping looks right. Swapping -`head.repo.fork` for `head.repo.private` leaves an expression of the same shape -with the same two labels; it sends every branch pull request to a free runner -while a fork's still queues for a paid one, and nothing in the workflow shows -it. The contract also asserts the other direction, that a branch pull request -still selects the Ubicloud shape, so a lane rewritten to send every pull -request to `ubuntu-latest` fails rather than passing as a fallback. It reads -the arms in order, too: an arm ahead of the fork arm that selects Ubicloud on a -pull request takes a fork's run first, so it fails even though the fork field -and its hosted label are both present further along. An earlier cron arm is -fine, since a pull request cannot take it. +selects, and it answers a fork or Dependabot condition false on purpose: these +contracts ask what a lane costs this repository, a fork's run costs nothing +here, and reading the fork arm as the pull-request answer would report every +one of these lanes as free while hiding the shape they buy for a branch pull +request. A chain the helpers cannot read, one with no fallback arm or naming a +condition they cannot evaluate, stays one opaque label rather than being split +into arms nobody checked. + +`tests/workflow_contracts/fork_fallback_test.py` asserts the exact hosted +condition rather than the shape, because the failure worth stopping looks +right. Swapping `head.repo.fork` for `head.repo.private` leaves an expression +of the same shape with the same two labels; it sends every branch pull request +to a free runner while a fork's still queues for a paid one, and nothing in the +workflow shows it. The contract also asserts the other direction, that a branch +pull request still selects the Ubicloud shape, so a lane rewritten to send +every pull request to `ubuntu-latest` fails rather than passing as a fallback. +It reads the arms in order, too: an arm ahead of the fork arm that selects +Ubicloud on a pull request takes a fork's run first, so it fails even though +the fork field and its hosted label are both present further along. An earlier +cron arm is fine, since a pull request cannot take it. Reading either expression as one opaque label would drop the job out of the placement, timeout, sizing and sccache contracts at once, which is the failure diff --git a/tests/workflow_contracts/_fork_lanes.py b/tests/workflow_contracts/_fork_lanes.py index db0fa534e..63f985f19 100644 --- a/tests/workflow_contracts/_fork_lanes.py +++ b/tests/workflow_contracts/_fork_lanes.py @@ -20,7 +20,7 @@ import typing as typ from _runs_on import EVENT_CONDITION_RE, conditional_runs_on_arms -from _workflow_policy import FORK_CONDITION, UBICLOUD_LABEL_PREFIX +from _workflow_policy import FORK_CONDITION, HOSTED_CONDITION, UBICLOUD_LABEL_PREFIX if typ.TYPE_CHECKING: # pragma: no cover - typing only from _workflow_policy import Job @@ -126,7 +126,7 @@ def paid_arms_before_the_fork(declared: str) -> tuple[str, ...]: """ shadowing: list[str] = [] for condition, label in conditional_runs_on_arms(declared) or (): - if condition == FORK_CONDITION: + if condition in (FORK_CONDITION, HOSTED_CONDITION): break if label.startswith(UBICLOUD_LABEL_PREFIX) and _holds_on_a_pull_request( condition diff --git a/tests/workflow_contracts/_runs_on.py b/tests/workflow_contracts/_runs_on.py index d2774a79b..e81df5b78 100644 --- a/tests/workflow_contracts/_runs_on.py +++ b/tests/workflow_contracts/_runs_on.py @@ -50,6 +50,35 @@ r"^github\.event\.pull_request\.head\.repo\.(?P[a-z_]+)$" ) +#: The hosted arm's exact condition, parenthesized: a fork, or a pull request +#: Dependabot opened. Compared as written, because its inner `||` must not be +#: read as the chain's own separator and a looser pattern would accept a sibling +#: field or `github.actor` as the author. +HOSTED_CONDITION_RE: re.Pattern[str] = re.compile( + r"^\(github\.event\.pull_request\.head\.repo\.fork\s*\|\|\s*" + r"github\.event\.pull_request\.user\.login\s*==\s*'dependabot\[bot\]'\)$" +) + + +def _depth_change(character: str) -> int: + """Return how one character changes the parenthesis depth.""" + return {"(": 1, ")": -1}.get(character, 0) + + +def _split_chain(body: str) -> list[str]: + """Split an expression body on the `||` that sit outside parentheses.""" + parts: list[str] = [] + depth = 0 + start = 0 + for index, character in enumerate(body): + depth += _depth_change(character) + if depth == 0 and body.startswith("||", index): + parts.append(body[start:index].strip()) + start = index + 2 + parts.append(body[start:].strip()) + return parts + + def _expression_body(declared: str) -> str | None: """Return the inside of a `${{ ... }}` scalar, or None if it is not one. @@ -123,7 +152,7 @@ def _runs_on_chain(declared: str) -> tuple[tuple[str | None, str], ...] | None: body = _expression_body(declared) if body is None: return None - parts = [part.strip() for part in body.split("||")] + parts = _split_chain(body) if len(parts) < 2: return None arms = [ @@ -204,6 +233,7 @@ def _recognized_condition(condition: str) -> bool: return ( EVENT_CONDITION_RE.match(condition) is not None or HEAD_REPO_CONDITION_RE.match(condition) is not None + or HOSTED_CONDITION_RE.match(condition) is not None ) diff --git a/tests/workflow_contracts/_workflow_policy.py b/tests/workflow_contracts/_workflow_policy.py index 210d9eedb..15e26eb6d 100644 --- a/tests/workflow_contracts/_workflow_policy.py +++ b/tests/workflow_contracts/_workflow_policy.py @@ -32,6 +32,15 @@ #: The field that decides whether a pull request came from a fork. FORK_CONDITION = "github.event.pull_request.head.repo.fork" +#: The pull request's author, not `github.actor`: the actor is whoever +#: triggered the run, so a person pushing to a Dependabot branch would flip the +#: runner. Hosted minutes cost nothing on a public repository, so Dependabot's +#: pull requests take the hosted arm as a fork's do. +DEPENDABOT_CONDITION = "github.event.pull_request.user.login == 'dependabot[bot]'" + +#: The exact condition of the hosted arm: a fork, or Dependabot. +HOSTED_CONDITION = f"({FORK_CONDITION} || {DEPENDABOT_CONDITION})" + #: Prefix shared by every Ubicloud runner label. Match on the prefix, not on #: one exact label: the migration wave introduces `ubicloud-standard-2`, and a #: contract keyed to the current label would wave the new one through. diff --git a/tests/workflow_contracts/fork_fallback_test.py b/tests/workflow_contracts/fork_fallback_test.py index 996123e69..b668b7fb8 100644 --- a/tests/workflow_contracts/fork_fallback_test.py +++ b/tests/workflow_contracts/fork_fallback_test.py @@ -37,7 +37,7 @@ from _trigger_reading import runs_on_event, triggers from _workflow_files import jobs_of from _workflow_policy import ( - FORK_CONDITION, + HOSTED_CONDITION, UBICLOUD_LABEL_PREFIX, Job, conditional_runs_on_arms, @@ -156,10 +156,10 @@ def test_every_pull_request_lane_falls_back_for_a_fork(job: Job) -> None: Nothing about the workflow's appearance would show it. """ declared = runs_on_scalar(job) - assert FORK_CONDITION in declared, ( + assert HOSTED_CONDITION in declared, ( f"{job} can run on a fork's pull request and asks for " f"{job.runner_summary} without a fork fallback. Add " - f"`{FORK_CONDITION} && '{HOSTED_LABEL}'` ahead of the paid label: a " + f"`{HOSTED_CONDITION} && '{HOSTED_LABEL}'` ahead of the paid label: a " "fork cannot obtain an Ubicloud runner and its check will hang." ) @@ -167,7 +167,7 @@ def test_every_pull_request_lane_falls_back_for_a_fork(job: Job) -> None: def test_the_fork_arm_selects_a_hosted_runner(job: Job) -> None: """The fallback must be free, and it must be the fork arm's own label.""" declared = runs_on_scalar(job) - _, found, arm = declared.partition(FORK_CONDITION) + _, found, arm = declared.partition(HOSTED_CONDITION) assert found, f"{job} names no fork condition, so there is no fallback arm to read" assert arm.lstrip().startswith(f"&& '{HOSTED_LABEL}'"), ( f"{job} tests the fork field but does not hand a fork " @@ -186,7 +186,7 @@ def test_no_earlier_arm_hands_a_fork_a_paid_runner(job: Job) -> None: shadowing = paid_arms_before_the_fork(runs_on_scalar(job)) assert not shadowing, ( f"{job} selects a paid runner for a pull request before its fork arm " - f"is reached: {list(shadowing)}. Put `{FORK_CONDITION} && " + f"is reached: {list(shadowing)}. Put `{HOSTED_CONDITION} && " f"'{HOSTED_LABEL}'` ahead of every arm a pull request can take." ) diff --git a/tests/workflow_contracts/hosted_condition_test.py b/tests/workflow_contracts/hosted_condition_test.py new file mode 100644 index 000000000..b63cb266e --- /dev/null +++ b/tests/workflow_contracts/hosted_condition_test.py @@ -0,0 +1,72 @@ +"""Unit tests for the hosted arm's fork-or-Dependabot condition. + +A pull request from a fork, or one Dependabot opened, takes the hosted arm of a +lane's `runs-on`. The condition contains its own `||`, so the chain reader must +split only on the separators outside parentheses, and it must recognize the +condition only as written: the fork field, then the author login, never +`github.actor`, which is whoever triggered the run. + +Run via ``make test-workflow-contracts``. +""" + +from __future__ import annotations + +import pytest +from _fork_lanes import paid_arms_before_the_fork +from _workflow_policy import ( + FORK_CONDITION, + HOSTED_CONDITION, + Job, + conditional_runs_on_arms, +) + +PAID = "ubicloud-standard-2" + + +def _chain(condition: str) -> str: + """Return a folded lane `runs-on` carrying `condition` as its hosted arm.""" + return f"${{{{ {condition} && 'ubuntu-latest' || '{PAID}' }}}}" + + +def test_the_hosted_condition_is_one_arm_not_two() -> None: + """The condition's inner `||` is not a chain separator.""" + arms = conditional_runs_on_arms(_chain(HOSTED_CONDITION)) + assert arms == ((HOSTED_CONDITION, "ubuntu-latest"), (None, PAID)), arms + + +@pytest.mark.parametrize( + "condition", + [ + HOSTED_CONDITION.replace("pull_request.user.login", "actor"), + HOSTED_CONDITION.replace("||", "&&"), + HOSTED_CONDITION.replace("== 'dependabot[bot]'", "!= 'dependabot[bot]'"), + ], + ids=["actor-for-author", "and-for-or", "not-equal"], +) +def test_a_lookalike_condition_leaves_the_chain_unreadable(condition: str) -> None: + """The reader refuses a condition that is not exactly the hosted one. + + The variants are passed as written, so the refusal is the reader's and not + an artefact of added parentheses. + """ + assert conditional_runs_on_arms(_chain(condition)) is None, condition + + +def test_the_fork_field_alone_is_readable_but_not_the_hosted_condition() -> None: + """A fork-only arm parses, yet fails the hosted-condition policy.""" + arms = conditional_runs_on_arms(_chain(FORK_CONDITION)) + assert arms == ((FORK_CONDITION, "ubuntu-latest"), (None, PAID)), arms + assert arms[0][0] != HOSTED_CONDITION, "the fork-only arm met the hosted policy" + + +def test_no_arm_before_the_hosted_arm_may_be_paid() -> None: + """A paid arm ahead of the hosted arm takes a Dependabot run first.""" + declared = ( + f"${{{{ github.event_name == 'pull_request' && '{PAID}' " + f"|| {HOSTED_CONDITION} && 'ubuntu-latest' || '{PAID}' }}}}" + ) + assert paid_arms_before_the_fork(declared) == ( + f"github.event_name == 'pull_request' && '{PAID}'", + ), declared + job = Job("ci.yml", "build", {"runs-on": declared}) + assert job.uses_ubicloud, f"{declared!r} should still read as a paid lane"