ci/github-script/merge: improve merge operation and error messages (#458412)

ci/github-script/bot.js

@@ -96,6 +96,47 @@ module.exports = async ({ github, context, core, dry }) => {
     return maintainerMaps[branch]
   }
 
+  // Caching the list of team members saves API requests when running the bot on the schedule and
+  // processing many PRs at once.
+  const members = {}
+  function getTeamMembers(team_slug) {
+    if (context.eventName === 'pull_request') {
+      // We have no chance of getting a token in the pull_request context with the right
+      // permissions to access the members endpoint below. Thus, we're pretending to have
+      // no members. This is OK; because this is only for the Test workflow, not for
+      // real use.
+      return []
+    }
+
+    if (!members[team_slug]) {
+      members[team_slug] = github.paginate(github.rest.teams.listMembersInOrg, {
+        org: context.repo.owner,
+        team_slug,
+        per_page: 100,
+      })
+    }
+
+    return members[team_slug]
+  }
+
+  // Caching users saves API requests when running the bot on the schedule and processing
+  // many PRs at once. It also helps to encapsulate the special logic we need, because
+  // actions/github doesn't support that endpoint fully, yet.
+  const users = {}
+  function getUser(id) {
+    if (!users[id]) {
+      users[id] = github
+        .request({
+          method: 'GET',
+          url: '/user/{id}',
+          id,
+        })
+        .then((resp) => resp.data)
+    }
+
+    return users[id]
+  }
+
   async function handlePullRequest({ item, stats, events }) {
     const log = (k, v) => core.info(`PR #${item.number} - ${k}: ${v}`)
 
@@ -121,6 +162,8 @@ module.exports = async ({ github, context, core, dry }) => {
       pull_request,
       events,
       maintainers,
+      getTeamMembers,
+      getUser,
     })
 
     // When the same change has already been merged to the target branch, a PR will still be

ci/github-script/merge.js

@@ -80,6 +80,7 @@ function runChecklist({
 
   return {
     checklist,
+    eligible,
     result,
   }
 }
@@ -109,10 +110,6 @@ async function handleMergeComment({ github, body, node_id, reaction }) {
   )
 }
 
-// Caching the list of team members saves API requests when running the bot on the schedule and
-// processing many PRs at once.
-const members = {}
-
 async function handleMerge({
   github,
   context,
@@ -122,31 +119,14 @@ async function handleMerge({
   pull_request,
   events,
   maintainers,
+  getTeamMembers,
+  getUser,
 }) {
   const pull_number = pull_request.number
 
-  function getTeamMembers(team_slug) {
+  const committers = new Set(
-    if (context.eventName === 'pull_request') {
+    (await getTeamMembers('nixpkgs-committers')).map(({ id }) => id),
-      // We have no chance of getting a token in the pull_request context with the right
+  )
-      // permissions to access the members endpoint below. Thus, we're pretending to have
-      // no members. This is OK; because this is only for the Test workflow, not for
-      // real use.
-      return new Set()
-    }
-
-    if (!members[team_slug]) {
-      members[team_slug] = github
-        .paginate(github.rest.teams.listMembersInOrg, {
-          org: context.repo.owner,
-          team_slug,
-          per_page: 100,
-        })
-        .then((members) => new Set(members.map(({ id }) => id)))
-    }
-
-    return members[team_slug]
-  }
-  const committers = await getTeamMembers('nixpkgs-committers')
 
   const files = await github.paginate(github.rest.pulls.listFiles, {
     ...context.repo,
@@ -168,14 +148,15 @@ async function handleMerge({
       // Ignore comments where the user has been deleted already.
       user &&
       // Ignore comments which had already been responded to by the bot.
-      !events.some(
+      (dry ||
-        ({ event, body }) =>
+        !events.some(
-          ['commented'].includes(event) &&
+          ({ event, body }) =>
-          // We're only testing this hidden reference, but not the author of the comment.
+            ['commented'].includes(event) &&
-          // We'll just assume that nobody creates comments with this marker on purpose.
+            // We're only testing this hidden reference, but not the author of the comment.
-          // Additionally checking the author is quite annoying for local debugging.
+            // We'll just assume that nobody creates comments with this marker on purpose.
-          body.match(new RegExp(`^<!-- comment: ${node_id} -->$`, 'm')),
+            // Additionally checking the author is quite annoying for local debugging.
-      ),
+            body.match(new RegExp(`^<!-- comment: ${node_id} -->$`, 'm')),
+        )),
   )
 
   async function merge() {
@@ -184,32 +165,9 @@ async function handleMerge({
       return 'Merge completed (dry)'
     }
 
-    // Using GraphQL's enablePullRequestAutoMerge mutation instead of the REST
+    // Using GraphQL mutations instead of the REST /merge endpoint, because the latter
-    // /merge endpoint, because the latter doesn't work with Merge Queues.
+    // doesn't work with Merge Queues. We now have merge queues enabled on all development
-    // This mutation works both with and without Merge Queues.
+    // branches, so we don't need a fallback for regular merges.
-    // It doesn't work when there are no required status checks for the target branch.
-    // All development branches have these enabled, so this is a non-issue.
-    try {
-      await github.graphql(
-        `mutation($node_id: ID!, $sha: GitObjectID) {
-          enablePullRequestAutoMerge(input: {
-            expectedHeadOid: $sha,
-            pullRequestId: $node_id
-          })
-          { clientMutationId }
-        }`,
-        { node_id: pull_request.node_id, sha: pull_request.head.sha },
-      )
-      return 'Enabled Auto Merge'
-    } catch (e) {
-      log('Auto Merge failed', e.response.errors[0].message)
-    }
-
-    // TODO: Observe whether the below is true and whether manual enqueue is actually needed.
-    // Auto-merge doesn't work if the target branch has already run all CI, in which
-    // case the PR must be enqueued explicitly.
-    // We now have merge queues enabled on all development branches, thus don't need a
-    // fallback after this.
     try {
       const resp = await github.graphql(
         `mutation($node_id: ID!, $sha: GitObjectID) {
@@ -227,6 +185,25 @@ async function handleMerge({
       return `[Queued](${resp.enqueuePullRequest.mergeQueueEntry.mergeQueue.url}) for merge`
     } catch (e) {
       log('Enqueing 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.
+    try {
+      await github.graphql(
+        `mutation($node_id: ID!, $sha: GitObjectID) {
+          enablePullRequestAutoMerge(input: {
+            expectedHeadOid: $sha,
+            pullRequestId: $node_id
+          })
+          { clientMutationId }
+        }`,
+        { node_id: pull_request.node_id, sha: pull_request.head.sha },
+      )
+      return 'Enabled Auto Merge'
+    } catch (e) {
+      log('Auto Merge failed', e.response.errors[0].message)
       throw new Error(e.response.errors[0].message)
     }
   }
@@ -265,7 +242,7 @@ async function handleMerge({
       }
     }
 
-    const { result, checklist } = runChecklist({
+    const { result, eligible, checklist } = runChecklist({
       committers,
       events,
       files,
@@ -295,6 +272,18 @@ async function handleMerge({
       '',
     ]
 
+    if (eligible.size > 0 && !eligible.has(comment.user.id)) {
+      const users = await Promise.all(
+        Array.from(eligible, async (id) => (await getUser(id)).login),
+      )
+      body.push(
+        '> [!TIP]',
+        '> Maintainers eligible to merge are:',
+        ...users.map((login) => `> - ${login}`),
+        '',
+      )
+    }
+
     if (result) {
       await react('ROCKET')
       try {