First class team reviews (try #2)

ci/eval/compare/default.nix

@@ -161,12 +161,13 @@ let
     );
 
   inherit
-    (callPackage ./maintainers.nix { } {
+    (callPackage ./maintainers.nix {
       changedattrs = lib.attrNames (lib.groupBy (a: a.name) changedPackagePlatformAttrs);
       changedpathsjson = touchedFilesJson;
       removedattrs = lib.attrNames (lib.groupBy (a: a.name) removedPackagePlatformAttrs);
     })
-    maintainers
+    users
+    teams
     packages
     ;
 in
@@ -178,10 +179,12 @@ runCommand "compare"
       cmp-stats
       codeowners
     ];
-    maintainers = builtins.toJSON maintainers;
+    users = builtins.toJSON users;
+    teams = builtins.toJSON teams;
     packages = builtins.toJSON packages;
     passAsFile = [
-      "maintainers"
+      "users"
+      "teams"
       "packages"
     ];
   }
@@ -262,6 +265,7 @@ runCommand "compare"
 
     done
 
-    cp "$maintainersPath" "$out/maintainers.json"
+    cp "$usersPath" "$out/maintainers.json"
+    cp "$teamsPath" "$out/teams.json"
     cp "$packagesPath" "$out/packages.json"
   ''

ci/eval/compare/maintainers.nix

@@ -1,7 +1,5 @@
 {
   lib,
-}:
-{
   changedattrs,
   changedpathsjson,
   removedattrs,
@@ -51,15 +49,16 @@ let
         # updates to .json files.
         # TODO: Support by-name package sets.
         filenames = lib.optional (lib.length path == 1) "pkgs/by-name/${sharded (lib.head path)}/";
-        # TODO: Refactor this so we can ping entire teams instead of the individual members.
-        # Note that this will require keeping track of GH team IDs in "maintainers/teams.nix".
-        maintainers = package.meta.maintainers or [ ];
+        # meta.maintainers also contains all individual team members.
+        # We only want to ping individuals if they're added individually as maintainers, not via teams.
+        users = package.meta.nonTeamMaintainers or [ ];
+        teams = package.meta.teams or [ ];
       }
     ))
     # No need to match up packages without maintainers with their files.
     # This also filters out attributes where `packge = null`, which is the
     # case for libintl, for example.
-    (lib.filter (pkg: pkg.maintainers != [ ]))
+    (lib.filter (pkg: pkg.users != [ ] || pkg.teams != [ ]))
   ];
 
   relevantFilenames =
@@ -94,20 +93,44 @@ let
 
   attrsWithModifiedFiles = lib.filter (pkg: anyMatchingFiles pkg.filenames) attrsWithFilenames;
 
-  listToPing = lib.concatMap (
+  userPings =
     pkg:
     map (maintainer: {
-      id = maintainer.githubId;
-      inherit (maintainer) github;
+      type = "user";
+      userId = maintainer.githubId;
       packageName = pkg.name;
-      dueToFiles = pkg.filenames;
-    }) pkg.maintainers
+    });
+
+  teamPings =
+    pkg: team:
+    if team ? github then
+      [
+        {
+          type = "team";
+          teamId = team.githubId;
+          packageName = pkg.name;
+        }
+      ]
+    else
+      userPings pkg team.members;
+
+  maintainersToPing = lib.concatMap (
+    pkg: userPings pkg pkg.users ++ lib.concatMap (teamPings pkg) pkg.teams
   ) attrsWithModifiedFiles;
 
-  byMaintainer = lib.groupBy (ping: toString ping.id) listToPing;
+  byType = lib.groupBy (ping: ping.type) maintainersToPing;
+
+  byUser = lib.pipe (byType.user or [ ]) [
+    (lib.groupBy (ping: toString ping.userId))
+    (lib.mapAttrs (_user: lib.map (pkg: pkg.packageName)))
+  ];
+  byTeam = lib.pipe (byType.team or [ ]) [
+    (lib.groupBy (ping: toString ping.teamId))
+    (lib.mapAttrs (_team: lib.map (pkg: pkg.packageName)))
+  ];
 in
 {
-  maintainers = lib.mapAttrs (_: lib.catAttrs "packageName") byMaintainer;
-
-  packages = lib.catAttrs "packageName" listToPing;
+  users = byUser;
+  teams = byTeam;
+  packages = lib.catAttrs "name" attrsWithModifiedFiles;
 }

ci/github-script/bot.js

@@ -160,6 +160,29 @@ module.exports = async ({ github, context, core, dry }) => {
     return users[id]
   }
 
+  // Same for teams
+  const teams = {}
+  function getTeam(id) {
+    if (!teams[id]) {
+      teams[id] = github
+        .request({
+          method: 'GET',
+          url: '/organizations/{orgId}/team/{id}',
+          // TODO: Make this work without pull_requests payloads
+          orgId: context.payload.pull_request.base.user.id,
+          id,
+        })
+        .then((resp) => resp.data)
+        .catch((e) => {
+          // Team may have been deleted
+          if (e.status === 404) return null
+          throw e
+        })
+    }
+
+    return teams[id]
+  }
+
   async function handlePullRequest({ item, stats, events }) {
     const log = (k, v) => core.info(`PR #${item.number} - ${k}: ${v}`)
 
@@ -192,17 +215,49 @@ module.exports = async ({ github, context, core, dry }) => {
     })
 
     // Check for any human reviews other than the PR author, GitHub actions and other GitHub apps.
-    // Accounts could be deleted as well, so don't count them.
     const reviews = (
-      await github.paginate(github.rest.pulls.listReviews, {
-        ...context.repo,
-        pull_number,
-      })
-    ).filter(
+      await github.graphql(
+        `query($owner: String!, $repo: String!, $pr: Int!) {
+        repository(owner: $owner, name: $repo) {
+          pullRequest(number: $pr) {
+            # Unlikely that there's ever more than 100 reviews, so let's not bother,
+            # but once https://github.com/actions/github-script/issues/309 is resolved,
+            # it would be easy to enable pagination.
+            reviews(first: 100) {
+              nodes {
+                state
+                user: author {
+                  # Only get users, no bots
+                  ... on User {
+                    login
+                    # Set the id field in the resulting JSON to GraphQL's databaseId
+                    # databaseId in GraphQL-land is the same as id in REST-land
+                    id: databaseId
+                  }
+                }
+                onBehalfOf(first: 100) {
+                  nodes {
+                    slug
+                  }
+                }
+              }
+            }
+          }
+        }
+      }`,
+        {
+          owner: context.repo.owner,
+          repo: context.repo.repo,
+          pr: pull_number,
+        },
+      )
+    ).repository.pullRequest.reviews.nodes.filter(
       (r) =>
-        r.user &&
-        !r.user.login.endsWith('[bot]') &&
-        r.user.type !== 'Bot' &&
+        // The `... on User` makes it such that .login only exists for users,
+        // but we still need to filter the others out.
+        // Accounts could be deleted as well, so don't count them.
+        r.user?.login &&
+        // Also exclude author reviews, can't request their review in any case
         r.user.id !== pull_request.user?.id,
     )
 
@@ -402,6 +457,16 @@ module.exports = async ({ github, context, core, dry }) => {
           if (e.code !== 'ENOENT') throw e
         }
 
+        let team_maintainers = []
+        try {
+          team_maintainers = Object.keys(
+            JSON.parse(await readFile(`${pull_number}/teams.json`, 'utf-8')),
+          ).map((id) => parseInt(id))
+        } catch (e) {
+          // Older artifacts don't have the teams.json, yet.
+          if (e.code !== 'ENOENT') throw e
+        }
+
         // We set this label earlier already, but the current PR state can be very different
         // after handleReviewers has requested reviews, so update it in this case to prevent
         // this label from flip-flopping.
@@ -414,14 +479,15 @@ module.exports = async ({ github, context, core, dry }) => {
           pull_request,
           reviews,
           // TODO: Use maintainer map instead of the artifact.
-          maintainers: Object.keys(
+          user_maintainers: Object.keys(
             JSON.parse(
               await readFile(`${pull_number}/maintainers.json`, 'utf-8'),
             ),
           ).map((id) => parseInt(id)),
+          team_maintainers,
           owners,
-          getTeamMembers,
           getUser,
+          getTeam,
         })
       }
     }

ci/github-script/merge.js

@@ -171,7 +171,7 @@ async function handleMerge({
   async function merge() {
     if (dry) {
       core.info(`Merging #${pull_number}... (dry)`)
-      return 'Merge completed (dry)'
+      return ['Merge completed (dry)']
     }
 
     // Using GraphQL mutations instead of the REST /merge endpoint, because the latter