ci/github-script/merge: various improvements (#457652)

ci/README.md

@@ -43,8 +43,10 @@ These issues effectively list PRs the merge bot has interacted with.
 To ensure security and a focused utility, the bot adheres to specific limitations:
 
 - The PR targets `master`, `staging`, or `staging-next`.
-- The PR only touches files located under `pkgs/by-name/*`.
+- The PR only touches packages located under `pkgs/by-name/*`.
-- The PR is authored by [@r-ryantm](https://nix-community.github.io/nixpkgs-update/r-ryantm/) or a [committer][@NixOS/nixpkgs-committers].
+- The PR is either:
+  - authored by a [committer][@NixOS/nixpkgs-committers], or
+  - created by [@r-ryantm](https://nix-community.github.io/nixpkgs-update/r-ryantm/).
 - 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.
 

ci/github-script/merge.js

@@ -1,33 +1,15 @@
-// Caching the list of committers saves API requests when running the bot on the schedule and
+function runChecklist({
-// processing many PRs at once.
+  committers,
-let committers
+  files,
-
+  pull_request,
-async function runChecklist({ github, context, pull_request, maintainers }) {
+  log,
-  const pull_number = pull_request.number
+  maintainers,
-
+  user,
-  if (!committers) {
+  userIsMaintainer,
-    if (context.eventName === 'pull_request') {
+}) {
-      // We have no chance of getting a token in the pull_request context with the right
+  const allByName = files.every(({ filename }) =>
-      // permissions to access the members endpoint below. Thus, we're pretending to have
+    filename.startsWith('pkgs/by-name/'),
-      // no committers. This is OK; because this is only for the Test workflow, not for
+  )
-      // real use.
-      committers = new Set()
-    } else {
-      committers = github
-        .paginate(github.rest.teams.listMembersInOrg, {
-          org: context.repo.owner,
-          team_slug: 'nixpkgs-committers',
-          per_page: 100,
-        })
-        .then((members) => new Set(members.map(({ id }) => id)))
-    }
-  }
-
-  const files = await github.paginate(github.rest.pulls.listFiles, {
-    ...context.repo,
-    pull_number,
-    per_page: 100,
-  })
 
   const packages = files
     .filter(({ filename }) => filename.startsWith('pkgs/by-name/'))
@@ -45,19 +27,39 @@ async function runChecklist({ github, context, pull_request, maintainers }) {
       'staging',
       'staging-next',
     ].includes(pull_request.base.ref),
-    'PR touches only files in `pkgs/by-name/`.': files.every(({ filename }) =>
+    'PR touches only packages in `pkgs/by-name/`.': allByName,
-      filename.startsWith('pkgs/by-name/'),
+    'PR is at least one of:': {
-    ),
+      'Authored by a committer.': committers.has(pull_request.user.id),
-    'PR authored by r-ryantm or committer.':
+      'Created by r-ryantm.': pull_request.user.login === 'r-ryantm',
-      pull_request.user.login === 'r-ryantm' ||
+    },
-      (await committers).has(pull_request.user.id),
-    'PR has maintainers eligible for merge.': eligible.size > 0,
   }
 
+  if (user) {
+    checklist[
+      `${user.login} is a member of [@NixOS/nixpkgs-maintainers](https://github.com/orgs/NixOS/teams/nixpkgs-maintainers).`
+    ] = userIsMaintainer
+    if (allByName) {
+      // We can only determine the below, if all packages are in by-name, since
+      // we can't reliably relate changed files to packages outside by-name.
+      checklist[`${user.login} is a maintainer of all touched packages.`] =
+        eligible.has(user.id)
+    }
+  } else {
+    // This is only used when no user is passed, i.e. for labeling.
+    checklist['PR has maintainers eligible to merge.'] = eligible.size > 0
+  }
+
+  const result = Object.values(checklist).every((v) =>
+    typeof v === 'boolean' ? v : Object.values(v).some(Boolean),
+  )
+
+  log('checklist', JSON.stringify(checklist))
+  log('eligible', JSON.stringify(Array.from(eligible)))
+  log('result', result)
+
   return {
     checklist,
-    eligible,
+    result,
-    result: Object.values(checklist).every(Boolean),
   }
 }
 
@@ -86,6 +88,10 @@ 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,
@@ -98,15 +104,34 @@ async function handleMerge({
 }) {
   const pull_number = pull_request.number
 
-  const { checklist, eligible, result } = await runChecklist({
+  function getTeamMembers(team_slug) {
-    github,
+    if (context.eventName === 'pull_request') {
-    context,
+      // We have no chance of getting a token in the pull_request context with the right
-    pull_request,
+      // permissions to access the members endpoint below. Thus, we're pretending to have
-    maintainers,
+      // 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,
+    pull_number,
+    per_page: 100,
   })
-  log('checklist', JSON.stringify(checklist))
-  log('eligible', JSON.stringify(Array.from(eligible)))
-  log('result', result)
 
   // Only look through comments *after* the latest (force) push.
   const latestChange = events.findLast(({ event }) =>
@@ -158,6 +183,7 @@ async function handleMerge({
       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
@@ -217,23 +243,36 @@ async function handleMerge({
       }
     }
 
-    const canUseMergeBot = await isMaintainer(comment.user.login)
+    const { result, checklist } = runChecklist({
-    const isEligible = eligible.has(comment.user.id)
+      committers,
-    const canMerge = result && canUseMergeBot && isEligible
+      files,
+      pull_request,
+      log,
+      maintainers,
+      user: comment.user,
+      userIsMaintainer: await isMaintainer(comment.user.login),
+    })
 
     const body = [
       `<!-- comment: ${comment.node_id} -->`,
+      `@${comment.user.login} wants to merge this PR.`,
       '',
-      'Requirements to merge this PR:',
+      'Requirements to merge this PR with `@NixOS/nixpkgs-merge-bot merge`:',
-      ...Object.entries(checklist).map(
+      ...Object.entries(checklist).flatMap(([msg, res]) =>
-        ([msg, res]) => `- :${res ? 'white_check_mark' : 'x'}: ${msg}`,
+        typeof res === 'boolean'
+          ? `- :${res ? 'white_check_mark' : 'x'}: ${msg}`
+          : [
+              `- :${Object.values(res).some(Boolean) ? 'white_check_mark' : 'x'}: ${msg}`,
+              ...Object.entries(res).map(
+                ([msg, res]) =>
+                  `  - ${res ? ':white_check_mark:' : ':white_large_square:'} ${msg}`,
+              ),
+            ],
       ),
-      `- :${canUseMergeBot ? 'white_check_mark' : 'x'}: ${comment.user.login} can use the merge bot.`,
-      `- :${isEligible ? 'white_check_mark' : 'x'}: ${comment.user.login} is eligible to merge changes to the touched packages.`,
       '',
     ]
 
-    if (canMerge) {
+    if (result) {
       await react('ROCKET')
       try {
         body.push(`:heavy_check_mark: ${await merge()} (#306934)`)
@@ -257,9 +296,17 @@ async function handleMerge({
       })
     }
 
-    if (canMerge) break
+    if (result) break
   }
 
+  const { result } = runChecklist({
+    committers,
+    files,
+    pull_request,
+    log,
+    maintainers,
+  })
+
   // Returns a boolean, which indicates whether the PR is merge-bot eligible in principle.
   // This is used to set the respective label in bot.js.
   return result