workflows/reviewers: First-class team package maintainer review requests

.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/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"

pkgs/stdenv/generic/check-meta.nix

@@ -397,6 +397,7 @@ let
         ];
       sourceProvenance = listOf attrs;
       maintainers = listOf (attrsOf any); # TODO use the maintainer type from lib/tests/maintainer-module.nix
+      nonTeamMaintainers = listOf (attrsOf any); # TODO use the maintainer type from lib/tests/maintainer-module.nix
       teams = listOf (attrsOf any); # TODO similar to maintainers, use a teams type
       priority = int;
       pkgConfigModules = listOf str;
@@ -670,6 +671,10 @@ let
       maintainers =
         attrs.meta.maintainers or [ ] ++ concatMap (team: team.members or [ ]) attrs.meta.teams or [ ];
 
+      # Needed for CI to be able to avoid requesting reviews from individual
+      # team members
+      nonTeamMaintainers = attrs.meta.maintainers or [ ];
+
       identifiers =
         let
           # nix-env writes a warning for each derivation that has null in its meta values, so