diff --git a/.github/workflows/commit-queue.yml b/.github/workflows/commit-queue.yml new file mode 100644 index 000000000..3c13dd059 --- /dev/null +++ b/.github/workflows/commit-queue.yml @@ -0,0 +1,76 @@ +# Landing a pull request when it is labelled `commit-queue`. +# +# `pull_request_target` runs in the context of the base branch and can reach +# secrets, which `pull_request` cannot do for a fork. That is only safe +# because nothing here checks out or executes the pull request's code: the +# checkout below is the base branch, and the branch under review is fetched +# only so that its commit messages can be read. Never add a build, an install +# or a test step to this workflow -- those belong in the checks it waits for, +# which run without a token that can write anything. +# +# Actions are pinned by commit, never by tag. +name: Commit Queue + +on: + pull_request_target: + types: [labeled] + +permissions: + contents: read + +# Two labels applied in quick succession should not race each other into the +# same merge. +concurrency: + group: ${{ github.workflow }}-${{ github.event.pull_request.number }} + cancel-in-progress: false + +jobs: + land: + name: Land + if: github.event.label.name == 'commit-queue' + runs-on: ubuntu-latest + steps: + - name: Mint a token for the app + id: token + uses: actions/create-github-app-token@bcd2ba49218906704ab6c1aa796996da409d3eb1 # v3.2.0 + with: + app-id: ${{ secrets.LAND_APP_ID }} + private-key: ${{ secrets.LAND_APP_PRIVATE_KEY }} + + - name: Check out the base branch + uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 + with: + # The base, deliberately, not the pull request. Full history so that + # the range between the two can be read. + ref: ${{ github.event.pull_request.base.ref }} + fetch-depth: 0 + + - name: Set up Node.js runtime + uses: actions/setup-node@820762786026740c76f36085b0efc47a31fe5020 # v7.0.0 + with: + node-version-file: 'package.json' + + - name: Fetch the commits under review + env: + NUMBER: ${{ github.event.pull_request.number }} + run: git fetch --quiet origin "pull/${NUMBER}/head" + + - name: Land it + id: land + env: + GH_TOKEN: ${{ steps.token.outputs.token }} + # Whoever applied the label, whose right to push is checked before + # anything is merged. Applying a label needs only triage. + LAND_ACTOR: ${{ github.event.sender.login }} + NUMBER: ${{ github.event.pull_request.number }} + run: node build/tasks/land-pull-request.mts "${NUMBER}" + + - name: Say why it did not land + if: failure() + env: + GH_TOKEN: ${{ steps.token.outputs.token }} + NUMBER: ${{ github.event.pull_request.number }} + RUN: ${{ github.server_url }}/${{ github.repository }}/actions/runs/${{ github.run_id }} + run: | + gh pr edit "$NUMBER" --remove-label commit-queue + gh pr comment "$NUMBER" --body "The commit queue did not land this. See $RUN — the label has been removed, so re-apply it once the reason is dealt with." diff --git a/build/shared/landing.mts b/build/shared/landing.mts index 1b3d9ede5..3f83f43bc 100644 --- a/build/shared/landing.mts +++ b/build/shared/landing.mts @@ -92,3 +92,52 @@ export function composeLandingMessage(parts: CommitParts[], prUrl: string) { ), ].join('\n'); } + +/** What GitHub reports about one check run on a commit. */ +export type CheckRun = { + name: string; + status: string; + conclusion: string | null; +}; + +/** What GitHub reports about one commit status, which is the older kind. */ +export type CommitStatus = { context: string; state: string }; + +/** + * Says why the checks on a commit are not a reason to land it. Anything still + * running counts against it: a label applied while a check was in flight says + * nothing about how that check turned out. Neutral and skipped do not count + * against it, since neither is a complaint. + * @param {CheckRun[]} runs The check runs reported on the commit. + * @param {CommitStatus[]} statuses The commit statuses reported on it. + * @returns {string} The reason, or an empty string if there is none. + */ +export function checksVerdict(runs: CheckRun[], statuses: CommitStatus[]) { + const pending = [ + ...runs.filter((run) => run.status !== 'completed').map((run) => run.name), + ...statuses + .filter((status) => status.state === 'pending') + .map((status) => status.context), + ]; + + if (pending.length > 0) { + return `these have not finished: ${pending.sort().join(', ')}`; + } + + const failed = [ + ...runs + .filter( + (run) => + run.conclusion !== null && + !['success', 'neutral', 'skipped'].includes(run.conclusion) + ) + .map((run) => run.name), + ...statuses + .filter((status) => !['success', 'pending'].includes(status.state)) + .map((status) => status.context), + ]; + + return failed.length > 0 + ? `these did not pass: ${failed.sort().join(', ')}` + : ''; +} diff --git a/build/shared/landing.test.mts b/build/shared/landing.test.mts index 0da47c21e..4317d076c 100644 --- a/build/shared/landing.test.mts +++ b/build/shared/landing.test.mts @@ -10,6 +10,7 @@ import { execFileSync } from 'node:child_process'; import { describe, test } from 'node:test'; import { validateCommitMessage } from '@openinf/portal/build/commit-message'; import { + checksVerdict, composeLandingMessage, partsOfMessage, } from '@openinf/portal/build/landing'; @@ -175,3 +176,68 @@ describe('the message that comes out', () => { ); }); }); + +describe('checksVerdict', () => { + const run = ( + name: string, + conclusion: string | null, + status = 'completed' + ) => ({ + name, + status, + conclusion, + }); + + test('says nothing when everything passed', () => { + deepStrictEqual(checksVerdict([run('Lint and test', 'success')], []), ''); + }); + + test('counts anything still running against it', () => { + // A label applied while a check was in flight says nothing about how that + // check turned out. + match( + checksVerdict([run('Lint and test', null, 'in_progress')], []), + /have not finished: Lint and test/ + ); + }); + + test('reports a failure by name', () => { + match( + checksVerdict([run('CodeQL', 'failure')], []), + /did not pass: CodeQL/ + ); + }); + + test('treats neutral and skipped as no complaint', () => { + deepStrictEqual( + checksVerdict( + [run('Pages changed', 'neutral'), run('Deploy', 'skipped')], + [] + ), + '' + ); + }); + + test('reads the older commit statuses too', () => { + // Some services on this repository report these rather than check runs, + // so looking only at check runs would call a red commit green. + match( + checksVerdict([], [{ context: 'ci/legacy', state: 'failure' }]), + /did not pass: ci\/legacy/ + ); + }); + + test('holds a pending status back as well', () => { + match( + checksVerdict([], [{ context: 'ci/legacy', state: 'pending' }]), + /have not finished: ci\/legacy/ + ); + }); + + test('reports everything wrong, not just the first', () => { + match( + checksVerdict([run('A', 'failure'), run('B', 'timed_out')], []), + /did not pass: A, B/ + ); + }); +}); diff --git a/build/tasks/land-pull-request.mts b/build/tasks/land-pull-request.mts index 8941b58ed..f2d7a83b8 100644 --- a/build/tasks/land-pull-request.mts +++ b/build/tasks/land-pull-request.mts @@ -14,6 +14,7 @@ import { execFileSync } from 'node:child_process'; import { validateCommitMessage } from '@openinf/portal/build/commit-message'; import { + checksVerdict, composeLandingMessage, partsOfMessage, } from '@openinf/portal/build/landing'; @@ -28,9 +29,6 @@ type PullRequest = { mergeableState: string; }; -const REPOSITORY = 'OpenINF/openinf.github.io'; -const DEFAULT_BRANCH = 'live'; - const [number, ...flags] = process.argv.slice(2); const dryRun = flags.includes('--dry-run'); @@ -39,6 +37,26 @@ const gh = (...args: string[]) => const git = (...args: string[]) => execFileSync('git', args, { encoding: 'utf8', maxBuffer: 1 << 24 }).trim(); +// Nothing here names a repository. A workflow sets GITHUB_REPOSITORY, and a +// terminal has a remote to read it from, so a copy of this file lands in +// another repository without being edited first. +const REPOSITORY = + process.env.GITHUB_REPOSITORY ?? + git('remote', 'get-url', 'origin').match( + /github\.com[/:](?[^/]+\/[^/]+?)(?:\.git)?$/ + )?.groups?.repo ?? + ''; + +const DEFAULT_BRANCH = gh( + 'api', + `repos/${REPOSITORY}`, + '--jq', + '.default_branch' +); + +/** Who is asking for this to land: the labeller, or whoever is at the keyboard. */ +const ACTOR = process.env.LAND_ACTOR ?? gh('api', 'user', '--jq', '.login'); + /** * Reads the pull request, waiting for GitHub to work out whether it merges. * The answer is `unknown` for a moment after anything lands on the base, and @@ -75,6 +93,52 @@ const readPull = async (pull: string): Promise => { return found; }; +/** + * Fetches what GitHub reports about a commit, and asks whether it is a reason + * to land. Both kinds are read: some services on this repository still report + * the older commit statuses rather than check runs. + * @param {string} sha The commit the pull request is at. + * @returns {string} The reason not to land, or an empty string if there is none. + */ +const checksRefuse = (sha: string) => + checksVerdict( + JSON.parse( + gh( + 'api', + `repos/${REPOSITORY}/commits/${sha}/check-runs`, + '--jq', + '[.check_runs[] | {name, status, conclusion}]' + ) + ), + JSON.parse( + gh( + 'api', + `repos/${REPOSITORY}/commits/${sha}/status`, + '--jq', + '[.statuses[] | {context, state}]' + ) + ) + ); + +/** + * Says why the person asking is not entitled to land anything. Applying a + * label needs only triage, which does not carry the right to push -- so + * without this, the label would quietly hand out that right. + * @returns {string} The reason, or an empty string if there is none. + */ +const actorRefuses = () => { + const permission = gh( + 'api', + `repos/${REPOSITORY}/collaborators/${ACTOR}/permission`, + '--jq', + '.permission' + ); + + return ['admin', 'maintain', 'write'].includes(permission) + ? '' + : `${ACTOR} has ${permission} access, which does not carry the right to push`; +}; + /** * Says why a pull request cannot be landed as it stands. * @param {PullRequest} pull What GitHub reports about it. @@ -97,7 +161,13 @@ const refuse = (pull: PullRequest) => { return 'GitHub has not worked out whether it merges yet; try again shortly'; } - return ''; + const actor = actorRefuses(); + + if (actor !== '') return actor; + + const checks = checksRefuse(pull.head); + + return checks === '' ? '' : `${checks}`; }; if (number === undefined || !/^\d+$/.test(number)) {