diff --git a/ci/README.md b/ci/README.md index be0e8e4c4fca..814299ed8182 100644 --- a/ci/README.md +++ b/ci/README.md @@ -52,6 +52,14 @@ To ensure security and a focused utility, the bot adheres to specific limitation - The user attempting to merge is a member of [@NixOS/nixpkgs-maintainers]. - The user attempting to merge is a maintainer of all packages touched by the PR. +Once these constraints are met, the bot picks a merge strategy based on the `no PR failures` commit status: + +- CI passing: the PR is added to the merge queue. +- CI unfinished (pending or missing status): the bot enables [Auto Merge], which queues the PR once required checks succeed. + Note that if CI later fails, nothing happens until it is fixed and passes. +- CI already failing (`error`/`failure` status): the bot does not enable Auto Merge, because it would never trigger, and fixing CI requires a new push that invalidates the merge command. + A fresh `@NixOS/nixpkgs-merge-bot merge` comment is needed once CI is green again. + ### Approving merge bot changes Changes to the bot can usually be approved by the [@NixOS/nixpkgs-ci] team, as with other CI changes. @@ -104,3 +112,4 @@ This script can also be run locally to print basic test cases. [@NixOS/nixpkgs-ci]: https://github.com/orgs/NixOS/teams/nixpkgs-ci [@NixOS/nixpkgs-core]: https://github.com/orgs/NixOS/teams/nixpkgs-core [RFC 172]: https://github.com/NixOS/rfcs/pull/172 +[Auto Merge]: https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/incorporating-changes-from-a-pull-request/automatically-merging-a-pull-request diff --git a/ci/github-script/merge.js b/ci/github-script/merge.js index ee861e21187d..e7f0cac8cc83 100644 --- a/ci/github-script/merge.js +++ b/ci/github-script/merge.js @@ -57,6 +57,9 @@ function runChecklist({ pull_request.user.login === 'r-ryantm', }, 'PR is not a draft': !pull_request.draft, + // CI state is intentionally *not* a checklist item: auto-merge exists precisely to + // cover unfinished CI, and an already-failed CI is reported via the merge message + // (see merge() below) rather than a blanket refusal. } if (user) { @@ -148,6 +151,14 @@ async function handleMerge({ // including an early exit when the first non-by-name file is found. if (files.length >= 100) return false + const noPrFailuresState = ( + await github.rest.repos.listCommitStatusesForRef({ + ...context.repo, + ref: pull_request.head.sha, + per_page: 100, + }) + ).data.find(({ context }) => context === 'no PR failures')?.state + // Only look through comments *after* the latest (force) push. const lastPush = events.findLastIndex( ({ event, sha, commit_id }) => @@ -173,10 +184,12 @@ async function handleMerge({ )), ) + // Returns `{ reaction, messages }`: the reaction to leave on the merge comment and the + // lines to append to the bot's reply. Throws only on an unexpected API error. async function merge() { if (dry) { core.info(`Merging #${pull_number}... (dry)`) - return ['Merge completed (dry)'] + return { reaction: 'ROCKET', messages: ['Merge completed (dry)'] } } // Using GraphQL mutations instead of the REST /merge endpoint, because the latter @@ -197,16 +210,37 @@ async function handleMerge({ { node_id: pull_request.node_id, sha: pull_request.head.sha }, ) log('merge', 'Queued for merge') - return [ - `:heavy_check_mark: [Queued](${resp.enqueuePullRequest.mergeQueueEntry.mergeQueue.url}) for merge (#306934)`, - ] + return { + reaction: 'ROCKET', + messages: [ + `:heavy_check_mark: [Queued](${resp.enqueuePullRequest.mergeQueueEntry.mergeQueue.url}) for merge (#306934)`, + ], + } } catch (e) { log('Enqueuing failed', e.response.errors[0].message) } - // If required status checks are not satisfied, yet, the above will fail. In this case - // we can enable auto-merge. We could also only use auto-merge, but this often gets - // stuck for no apparent reason. + // Enqueuing fails when the required status checks are not satisfied, yet. If CI has + // already failed, enabling auto-merge would be pointless: it would never fire, and + // fixing CI requires a new push, which invalidates this merge command anyway (we only + // act on comments after the latest push). So we don't enable auto-merge and instead + // ask for a fresh command once CI is green again. + if (['error', 'failure'].includes(noPrFailuresState)) { + log('merge', 'CI has failed, not enabling auto-merge') + return { + reaction: 'THUMBS_DOWN', + messages: [ + ':x: Pull Request could not be merged: CI has failed (#305350).', + '', + '> [!TIP]', + '> PRs cannot be merged while CI is failing.', + '> Once CI is passing, comment `@NixOS/nixpkgs-merge-bot merge` again.', + ], + } + } + + // CI has not finished yet, so we enable auto-merge. We could also only use auto-merge, + // but this often gets stuck for no apparent reason. try { await github.graphql( `mutation($node_id: ID!, $sha: GitObjectID) { @@ -219,12 +253,15 @@ async function handleMerge({ { node_id: pull_request.node_id, sha: pull_request.head.sha }, ) log('merge', 'Auto-merge enabled') - return [ - `:heavy_check_mark: Enabled Auto Merge (#306934)`, - '', - '> [!TIP]', - '> Sometimes GitHub gets stuck after enabling Auto Merge. In this case, leaving another approval should trigger the merge.', - ] + return { + reaction: 'ROCKET', + messages: [ + `:heavy_check_mark: Enabled Auto Merge (#306934)`, + '', + '> [!TIP]', + '> Sometimes GitHub gets stuck after enabling Auto Merge. In this case, leaving another approval should trigger the merge.', + ], + } } catch (e) { log('Auto Merge failed', e.response.errors[0].message) throw new Error(e.response.errors[0].message) @@ -308,10 +345,12 @@ async function handleMerge({ } if (result) { - await react('ROCKET') try { - body.push(...(await merge())) + const { reaction, messages } = await merge() + await react(reaction) + body.push(...messages) } catch (e) { + await react('THUMBS_DOWN') // Remove the HTML comment with node_id reference to allow retrying this merge on the next run. body.shift() body.push(`:x: Merge failed with: ${e} (#371492)`)