[Reverted] First-class GitHub team reviews

.github/workflows/reviewers.yml

@@ -146,19 +146,14 @@ jobs:
       - name: Requesting maintainer reviews
         if: ${{ steps.app-token.outputs.token }}
         env:
-          GH_TOKEN: ${{ github.token }}
-          APP_GH_TOKEN: ${{ steps.app-token.outputs.token }}
+          GH_TOKEN: ${{ steps.app-token.outputs.token }}
           REPOSITORY: ${{ github.repository }}
+          ORG_ID: ${{ github.repository_owner_id }}
           NUMBER: ${{ github.event.number }}
           AUTHOR: ${{ github.event.pull_request.user.login }}
           # Don't request reviewers on draft PRs
           DRY_MODE: ${{ github.event.pull_request.draft && '1' || '' }}
-        run: |
-          # maintainers.json contains GitHub IDs. Look up handles to request reviews from.
-          # There appears to be no API to request reviews based on GitHub IDs
-          jq -r 'keys[]' comparison/maintainers.json \
-            | while read -r id; do gh api /user/"$id" --jq .login; done \
-            | GH_TOKEN="$APP_GH_TOKEN" result/bin/request-reviewers.sh "$REPOSITORY" "$NUMBER" "$AUTHOR"
+        run: result/bin/request-maintainers-reviews.sh "$ORG_ID" "$REPOSITORY" "$NUMBER" "$AUTHOR" comparison
 
       - name: Log current API rate limits (app-token)
         if: ${{ steps.app-token.outputs.token }}

ci/eval/compare/default.nix

@@ -179,8 +179,12 @@ runCommand "compare"
       jq
       cmp-stats
     ];
-    maintainers = builtins.toJSON maintainers;
-    passAsFile = [ "maintainers" ];
+    maintainers = builtins.toJSON maintainers.users;
+    teams = builtins.toJSON maintainers.teams;
+    passAsFile = [
+      "maintainers"
+      "teams"
+    ];
   }
   ''
     mkdir $out
@@ -223,4 +227,5 @@ runCommand "compare"
     fi
 
     cp "$maintainersPath" "$out/maintainers.json"
+    cp "$teamsPath" "$out/teams.json"
   ''

ci/eval/compare/maintainers.nix

@@ -51,15 +51,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.
+        maintainers = 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.maintainers != [ ] || pkg.teams != [ ]))
   ];
 
   relevantFilenames =
@@ -94,20 +95,43 @@ let
 
   attrsWithModifiedFiles = lib.filter (pkg: anyMatchingFiles pkg.filenames) attrsWithFilenames;
 
-  listToPing = lib.concatMap (
+  userPings =
     pkg:
     map (maintainer: {
-      id = maintainer.githubId;
-      inherit (maintainer) github;
+      type = "user";
+      user = toString maintainer.githubId;
       packageName = pkg.name;
-      dueToFiles = pkg.filenames;
-    }) pkg.maintainers
+    });
+
+  teamPings =
+    pkg: team:
+    if team ? github then
+      [
+        {
+          type = "team";
+          team = toString team.githubId;
+          packageName = pkg.name;
+        }
+      ]
+    else
+      userPings pkg team.members;
+
+  maintainersToPing = lib.concatMap (
+    pkg: userPings pkg pkg.maintainers ++ lib.concatMap (teamPings pkg) pkg.teams
   ) attrsWithModifiedFiles;
 
-  byMaintainer = lib.groupBy (ping: toString ping.id) listToPing;
+  byType = lib.groupBy (ping: ping.type) maintainersToPing;
 
-  packagesPerMaintainer = lib.mapAttrs (
-    maintainer: packages: map (pkg: pkg.packageName) packages
-  ) byMaintainer;
+  byUser = lib.pipe (byType.user or [ ]) [
+    (lib.groupBy (ping: ping.user))
+    (lib.mapAttrs (_user: lib.map (pkg: pkg.packageName)))
+  ];
+  byTeam = lib.pipe (byType.team or [ ]) [
+    (lib.groupBy (ping: ping.team))
+    (lib.mapAttrs (_team: lib.map (pkg: pkg.packageName)))
+  ];
 in
-packagesPerMaintainer
+{
+  users = byUser;
+  teams = byTeam;
+}

ci/request-reviews/default.nix

@@ -17,6 +17,7 @@ stdenvNoCC.mkDerivation {
       ./get-code-owners.sh
       ./request-reviewers.sh
       ./request-code-owner-reviews.sh
+      ./request-maintainers-reviews.sh
     ];
   };
   nativeBuildInputs = [ makeWrapper ];
@@ -26,6 +27,7 @@ stdenvNoCC.mkDerivation {
     for bin in *.sh; do
       mv "$bin" "$out/bin"
       wrapProgram "$out/bin/$bin" \
+        --set PAGER cat \
         --set PATH ${
           lib.makeBinPath [
             coreutils

ci/request-reviews/get-code-owners.sh

@@ -1,6 +1,6 @@
 #!/usr/bin/env bash
 
-# Get the code owners of the files changed by a PR, returning one username per line
+# Get the code owners of the files changed by a PR, returning one GitHub user/team handle per line
 
 set -euo pipefail
 
@@ -29,9 +29,9 @@ log "This PR touches ${#touchedFiles[@]} files"
 # remove code owners to avoid pinging them
 git -C "$gitRepo" show "$baseRef":"$ownersFile" > "$tmp"/codeowners
 
-# Associative array with the user as the key for easy de-duplication
+# Associative array with the user/team as the key for easy de-duplication
 # Make sure to always lowercase keys to avoid duplicates with different casings
-declare -A users=()
+declare -A finalOwners=()
 
 for file in "${touchedFiles[@]}"; do
     result=$(codeowners --file "$tmp"/codeowners "$file")
@@ -59,39 +59,9 @@ for file in "${touchedFiles[@]}"; do
         # The first regex match is everything after the @
         entry=${BASH_REMATCH[1]}
 
-        if [[ "$entry" =~ (.*)/(.*) ]]; then
-            # Teams look like $org/$team
-            org=${BASH_REMATCH[1]}
-            team=${BASH_REMATCH[2]}
-
-            # Instead of requesting a review from the team itself,
-            # we request reviews from the individual users.
-            # This is because once somebody from a team reviewed the PR,
-            # the API doesn't expose that the team was already requested for a review,
-            # so we wouldn't be able to avoid rerequesting reviews
-            # without saving some some extra state somewhere
-
-            # We could also consider implementing a more advanced heuristic
-            # in the future that e.g. only pings one team member,
-            # but escalates to somebody else if that member doesn't respond in time.
-            gh api \
-                --cache=1h \
-                -H "Accept: application/vnd.github+json" \
-                -H "X-GitHub-Api-Version: 2022-11-28" \
-                "/orgs/$org/teams/$team/members" \
-                --jq '.[].login' > "$tmp/team-members"
-            readarray -t members < "$tmp/team-members"
-            log "Team $entry has these members: ${members[*]}"
-
-            for user in "${members[@]}"; do
-                users[${user,,}]=
-            done
-        else
-            # Everything else is a user
-            users[${entry,,}]=
-        fi
+        finalOwners[${entry,,}]=
     done
 
 done
 
-printf "%s\n" "${!users[@]}"
+printf "%s\n" "${!finalOwners[@]}"

ci/request-reviews/request-maintainers-reviews.sh

@@ -0,0 +1,39 @@
+#!/usr/bin/env bash
+
+# Requests maintainer reviews for a PR
+
+set -euo pipefail
+SCRIPT_DIR=$(dirname "$0")
+
+if (( $# < 5 )); then
+    echo >&2 "Usage: $0 ORG_ID GITHUB_REPO PR_NUMBER AUTHOR COMPARISON_PATH"
+    exit 1
+fi
+orgId=$1
+baseRepo=$2
+prNumber=$3
+prAuthor=$4
+comparisonPath=$5
+
+org="${baseRepo%/*}"
+
+# maintainers.json/teams.json contains GitHub IDs. Look up handles to request reviews from.
+# There appears to be no API to request reviews based on IDs
+{
+  jq -r 'keys[]' "$comparisonPath"/maintainers.json \
+    | while read -r id; do
+      if login=$(gh api /user/"$id" --jq .login); then
+        echo "$login"
+      else
+        echo >&2 "Skipping user with id $id: $login"
+      fi
+    done
+  jq -r 'keys[]' "$comparisonPath"/teams.json \
+    | while read -r id; do
+      if slug=$(gh api /organizations/"$orgId"/team/"$id" --jq .slug); then
+        echo "$org/$slug"
+      else
+        echo >&2 "Skipping team with id $id: $slug"
+      fi
+    done
+} | "$SCRIPT_DIR"/request-reviewers.sh "$baseRepo" "$prNumber" "$prAuthor"