diff --git a/build/shared/landing.mts b/build/shared/landing.mts index 3f83f43bc..0de64f7c7 100644 --- a/build/shared/landing.mts +++ b/build/shared/landing.mts @@ -98,6 +98,7 @@ export type CheckRun = { name: string; status: string; conclusion: string | null; + detailsUrl?: string; }; /** What GitHub reports about one commit status, which is the older kind. */ @@ -108,11 +109,25 @@ export type CommitStatus = { context: string; state: string }; * 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. + * The queue is itself a check, so its own run is left out. Waiting for it + * would be waiting for a job that cannot finish until it stops waiting. * @param {CheckRun[]} runs The check runs reported on the commit. * @param {CommitStatus[]} statuses The commit statuses reported on it. + * @param {string} ownRunId The workflow run doing the asking, if it is one. * @returns {string} The reason, or an empty string if there is none. */ -export function checksVerdict(runs: CheckRun[], statuses: CommitStatus[]) { +export function checksVerdict( + all: CheckRun[], + statuses: CommitStatus[], + ownRunId = '' +) { + const runs = + ownRunId === '' + ? all + : all.filter( + (run) => + !(run.detailsUrl ?? '').includes(`/actions/runs/${ownRunId}/`) + ); const pending = [ ...runs.filter((run) => run.status !== 'completed').map((run) => run.name), ...statuses diff --git a/build/shared/landing.test.mts b/build/shared/landing.test.mts index 4317d076c..0e3fcf5b1 100644 --- a/build/shared/landing.test.mts +++ b/build/shared/landing.test.mts @@ -241,3 +241,36 @@ describe('checksVerdict', () => { ); }); }); + +describe('checksVerdict and its own run', () => { + test('does not wait for the queue that is doing the asking', () => { + // The queue reports as a check itself, so counting it would mean waiting + // for a job that cannot finish until it stops waiting. + const own = { + name: 'Land', + status: 'in_progress', + conclusion: null, + detailsUrl: 'https://github.com/o/r/actions/runs/999/job/1', + }; + + deepStrictEqual( + checksVerdict( + [own, { name: 'Lint', status: 'completed', conclusion: 'success' }], + [], + '999' + ), + '' + ); + }); + + test('still waits for a different run', () => { + const other = { + name: 'Lint', + status: 'in_progress', + conclusion: null, + detailsUrl: 'https://github.com/o/r/actions/runs/1000/job/1', + }; + + match(checksVerdict([other], [], '999'), /have not finished: Lint/); + }); +}); diff --git a/build/tasks/land-pull-request.mts b/build/tasks/land-pull-request.mts index f2d7a83b8..23702ea0c 100644 --- a/build/tasks/land-pull-request.mts +++ b/build/tasks/land-pull-request.mts @@ -32,30 +32,69 @@ type PullRequest = { const [number, ...flags] = process.argv.slice(2); const dryRun = flags.includes('--dry-run'); -const gh = (...args: string[]) => - execFileSync('gh', args, { encoding: 'utf8', maxBuffer: 1 << 24 }).trim(); -const git = (...args: string[]) => - execFileSync('git', args, { encoding: 'utf8', maxBuffer: 1 << 24 }).trim(); +const run = (command: string, args: string[]) => + execFileSync(command, args, { encoding: 'utf8', maxBuffer: 1 << 24 }).trim(); + +/** + * Runs `gh`, and tries a second time before giving up. A read that fails + * because GitHub returned a 502 or throttled the request is not a reason to + * refuse to land, and it happens often enough to have happened here: an + * earlier version crashed on one and worked when run again unchanged. + * @param {...string} args What to pass to `gh`. + * @returns {string} Its output. + */ +const gh = (...args: string[]) => { + try { + return run('gh', args); + } catch (first) { + // A merge is not safe to repeat blindly: if the first attempt reached + // GitHub, the second would report an already-merged pull request as a + // failure. Reads are. + if (args.includes('PUT')) throw first; + + return run('gh', args); + } +}; + +const git = (...args: string[]) => run('git', args); + +/** + * Works something out once, the first time it is wanted. Asked for at the top + * of the file instead, these would run before anything could catch them + * failing, and a broken `gh` would print a stack trace rather than a reason. + * @param {() => T} work How to find the answer. + * @returns {() => T} A function returning it, computed at most once. + */ +const once = (work: () => T) => { + let answer: T | undefined; + + return () => { + answer ??= work(); + + return answer; + }; +}; // 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' +const repository = once( + () => + process.env.GITHUB_REPOSITORY ?? + git('remote', 'get-url', 'origin').match( + /github\.com[/:](?[^/]+\/[^/]+?)(?:\.git)?$/ + )?.groups?.repo ?? + '' +); + +const defaultBranch = once(() => + 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'); +const actor = once( + () => process.env.LAND_ACTOR ?? gh('api', 'user', '--jq', '.login') +); /** * Reads the pull request, waiting for GitHub to work out whether it merges. @@ -69,7 +108,7 @@ const readPull = async (pull: string): Promise => { JSON.parse( gh( 'api', - `repos/${REPOSITORY}/pulls/${pull}`, + `repos/${repository()}/pulls/${pull}`, '--jq', '{title, base: .base.ref, head: .head.sha, state, draft, mergeableState: .mergeable_state}' ) @@ -105,19 +144,20 @@ const checksRefuse = (sha: string) => JSON.parse( gh( 'api', - `repos/${REPOSITORY}/commits/${sha}/check-runs`, + `repos/${repository()}/commits/${sha}/check-runs`, '--jq', - '[.check_runs[] | {name, status, conclusion}]' + '[.check_runs[] | {name, status, conclusion, detailsUrl: .details_url}]' ) ), JSON.parse( gh( 'api', - `repos/${REPOSITORY}/commits/${sha}/status`, + `repos/${repository()}/commits/${sha}/status`, '--jq', '[.statuses[] | {context, state}]' ) - ) + ), + process.env.GITHUB_RUN_ID ?? '' ); /** @@ -129,14 +169,14 @@ const checksRefuse = (sha: string) => const actorRefuses = () => { const permission = gh( 'api', - `repos/${REPOSITORY}/collaborators/${ACTOR}/permission`, + `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`; + : `${actor()} has ${permission} access, which does not carry the right to push`; }; /** @@ -150,10 +190,10 @@ const refuse = (pull: PullRequest) => { } if (pull.mergeableState === 'dirty') { - return `it conflicts with ${DEFAULT_BRANCH}; rebase the branch onto it and push first`; + return `it conflicts with ${defaultBranch()}; rebase the branch onto it and push first`; } - if (pull.base !== DEFAULT_BRANCH) { + if (pull.base !== defaultBranch()) { return `it targets ${pull.base}; retarget it, or land its base first`; } @@ -161,87 +201,97 @@ const refuse = (pull: PullRequest) => { return 'GitHub has not worked out whether it merges yet; try again shortly'; } - const actor = actorRefuses(); + const unentitled = actorRefuses(); - if (actor !== '') return actor; + if (unentitled !== '') return unentitled; const checks = checksRefuse(pull.head); return checks === '' ? '' : `${checks}`; }; -if (number === undefined || !/^\d+$/.test(number)) { - console.error('Usage: nps "land [--dry-run]"'); - process.exitCode = 1; -} else { - const pull = await readPull(number); - const reason = refuse(pull); - - if (reason !== '') { - console.error(`#${number} cannot be landed: ${reason}.`); +// Anything unexpected -- a network failure that outlived its retry, a +// command that is not installed -- should say so in a line rather than +// print a stack trace at whoever applied a label. +try { + if (number === undefined || !/^\d+$/.test(number)) { + console.error('Usage: nps "land [--dry-run]"'); process.exitCode = 1; } else { - // Oldest first, so the landed message reads in the order the work was - // done rather than the order git lists it. - const shas = git( - 'rev-list', - '--reverse', - '--no-merges', - `origin/${DEFAULT_BRANCH}..${pull.head}` - ) - .split('\n') - .filter(Boolean); - const parts = shas.map((sha) => - partsOfMessage(git('log', '-1', '--format=%B', sha)) - ); - const message = composeLandingMessage( - parts, - `https://github.com/${REPOSITORY}/pull/${number}` - ); - const problems = validateCommitMessage(`${pull.title}\n\n${message}`); - const rule = '='.repeat(72); + const pull = await readPull(number); + const reason = refuse(pull); - console.log(`${rule}\n${pull.title}\n\n${message}\n${rule}`); - console.log( - `#${number}: ${shas.length} commit${shas.length === 1 ? '' : 's'}, ${pull.mergeableState}` - ); - - if (shas.length === 0) { - console.error(`\n#${number} has no commits over ${DEFAULT_BRANCH}.`); - process.exitCode = 1; - } else if (problems.length > 0) { - console.error( - '\nThe message this would land does not pass its own rules:' - ); - for (const problem of problems) console.error(` ${problem}`); + if (reason !== '') { + console.error(`#${number} cannot be landed: ${reason}.`); process.exitCode = 1; - } else if (dryRun) { - console.log('\nDry run; nothing landed.'); } else { - const merged = JSON.parse( - gh( - 'api', - '-X', - 'PUT', - `repos/${REPOSITORY}/pulls/${number}/merge`, - '-f', - 'merge_method=squash', - '-f', - `commit_title=${pull.title}`, - '-f', - `commit_message=${message}`, - '-f', - `sha=${pull.head}`, - '--jq', - '{merged, sha}' - ) + // Oldest first, so the landed message reads in the order the work was + // done rather than the order git lists it. + const shas = git( + 'rev-list', + '--reverse', + '--no-merges', + `origin/${defaultBranch()}..${pull.head}` + ) + .split('\n') + .filter(Boolean); + const parts = shas.map((sha) => + partsOfMessage(git('log', '-1', '--format=%B', sha)) + ); + const message = composeLandingMessage( + parts, + `https://github.com/${repository()}/pull/${number}` + ); + const problems = validateCommitMessage(`${pull.title}\n\n${message}`); + const rule = '='.repeat(72); + + console.log(`${rule}\n${pull.title}\n\n${message}\n${rule}`); + console.log( + `#${number}: ${shas.length} commit${shas.length === 1 ? '' : 's'}, ${pull.mergeableState}` ); - if (merged.merged === true) console.log(`\nLanded as ${merged.sha}`); - else { - console.error(`\n#${number} did not merge.`); + if (shas.length === 0) { + console.error(`\n#${number} has no commits over ${defaultBranch()}.`); process.exitCode = 1; + } else if (problems.length > 0) { + console.error( + '\nThe message this would land does not pass its own rules:' + ); + for (const problem of problems) console.error(` ${problem}`); + process.exitCode = 1; + } else if (dryRun) { + console.log('\nDry run; nothing landed.'); + } else { + const merged = JSON.parse( + gh( + 'api', + '-X', + 'PUT', + `repos/${repository()}/pulls/${number}/merge`, + '-f', + 'merge_method=squash', + '-f', + `commit_title=${pull.title}`, + '-f', + `commit_message=${message}`, + '-f', + `sha=${pull.head}`, + '--jq', + '{merged, sha}' + ) + ); + + if (merged.merged === true) console.log(`\nLanded as ${merged.sha}`); + else { + console.error(`\n#${number} did not merge.`); + process.exitCode = 1; + } } } } +} catch (error) { + console.error( + `Could not land #${number}: ${error instanceof Error ? error.message.split(String.fromCharCode(10))[0] : String(error)}` + ); + process.exitCode = 1; }