Revert "First-class GitHub team reviews"

.github/workflows/reviewers.yml

@@ -146,14 +146,19 @@ jobs:
       - name: Requesting maintainer reviews
         if: ${{ steps.app-token.outputs.token }}
         env:
-          GH_TOKEN: ${{ steps.app-token.outputs.token }}
+          GH_TOKEN: ${{ github.token }}
+          APP_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: result/bin/request-maintainers-reviews.sh "$ORG_ID" "$REPOSITORY" "$NUMBER" "$AUTHOR" comparison
+        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"
 
       - name: Log current API rate limits (app-token)
         if: ${{ steps.app-token.outputs.token }}

ci/eval/compare/default.nix

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

ci/eval/compare/maintainers.nix

@@ -51,16 +51,15 @@ 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)}/";
-        # meta.maintainers also contains all individual team members.
+        # TODO: Refactor this so we can ping entire teams instead of the individual members.
-        # We only want to ping individuals if they're added individually as maintainers, not via teams.
+        # Note that this will require keeping track of GH team IDs in "maintainers/teams.nix".
-        maintainers = package.meta.nonTeamMaintainers or [ ];
+        maintainers = package.meta.maintainers 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 != [ ] || pkg.teams != [ ]))
+    (lib.filter (pkg: pkg.maintainers != [ ]))
   ];
 
   relevantFilenames =
@@ -95,43 +94,20 @@ let
 
   attrsWithModifiedFiles = lib.filter (pkg: anyMatchingFiles pkg.filenames) attrsWithFilenames;
 
-  userPings =
+  listToPing = lib.concatMap (
     pkg:
     map (maintainer: {
-      type = "user";
+      id = maintainer.githubId;
-      user = toString maintainer.githubId;
+      inherit (maintainer) github;
       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;
 
-  byType = lib.groupBy (ping: ping.type) maintainersToPing;
+  byMaintainer = lib.groupBy (ping: toString ping.id) listToPing;
 
-  byUser = lib.pipe (byType.user or [ ]) [
+  packagesPerMaintainer = lib.mapAttrs (
-    (lib.groupBy (ping: ping.user))
+    maintainer: packages: map (pkg: pkg.packageName) packages
-    (lib.mapAttrs (_user: lib.map (pkg: pkg.packageName)))
+  ) byMaintainer;
-  ];
-  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,7 +17,6 @@ stdenvNoCC.mkDerivation {
       ./get-code-owners.sh
       ./request-reviewers.sh
       ./request-code-owner-reviews.sh
-      ./request-maintainers-reviews.sh
     ];
   };
   nativeBuildInputs = [ makeWrapper ];
@@ -27,7 +26,6 @@ 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 GitHub user/team handle per line
+# Get the code owners of the files changed by a PR, returning one username 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/team as the key for easy de-duplication
+# Associative array with the user as the key for easy de-duplication
 # Make sure to always lowercase keys to avoid duplicates with different casings
-declare -A finalOwners=()
+declare -A users=()
 
 for file in "${touchedFiles[@]}"; do
     result=$(codeowners --file "$tmp"/codeowners "$file")
@@ -59,9 +59,39 @@ for file in "${touchedFiles[@]}"; do
         # The first regex match is everything after the @
         entry=${BASH_REMATCH[1]}
 
-        finalOwners[${entry,,}]=
+        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
     done
 
 done
 
-printf "%s\n" "${!finalOwners[@]}"
+printf "%s\n" "${!users[@]}"

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

@@ -1,39 +0,0 @@
-#!/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"

ci/request-reviews/request-reviewers.sh

@@ -1,16 +1,8 @@
 #!/usr/bin/env bash
 
-# Request reviewers for a PR, reading line-separated usernames/teams on stdin,
+# Request reviewers for a PR, reading line-separated usernames on stdin,
 # filtering for valid reviewers before using the API endpoint to request reviews:
 # https://docs.github.com/en/rest/pulls/review-requests?apiVersion=2022-11-28#request-reviewers-for-a-pull-request
-#
-# Example:
-#   $ request-reviewers.sh NixOS/nixpkgs 123456 someUser <<-EOF
-#   someUser
-#   anotherUser
-#   NixOS/someTeam
-#   NixOS/anotherTeam
-#   EOF
 
 set -euo pipefail
 
@@ -38,133 +30,59 @@ baseRepo=$1
 prNumber=$2
 prAuthor=$3
 
-org="${baseRepo%/*}"
-repo="${baseRepo#*/}"
-
 tmp=$(mktemp -d)
 trap 'rm -rf "$tmp"' exit
 
-declare -A users=() teams=()
+declare -A users=()
-
 while read -r handle && [[ -n "$handle" ]]; do
-    if [[ "$handle" =~ (.*)/(.*) ]]; then
+    users[${handle,,}]=
-        if [[ "${BASH_REMATCH[1]}" != "$org" ]]; then
-            log "Team $handle is not part of the $org org, ignoring"
-        else
-            teams[${BASH_REMATCH[2],,}]=
-        fi
-    else
-        users[${handle,,}]=
-    fi
 done
 
-log "Checking users: ${!users[*]}"
-log "Checking teams: ${!teams[*]}"
-
 # Cannot request a review from the author
 if [[ -v users[${prAuthor,,}] ]]; then
-    log "Avoiding review request for PR author $prAuthor"
+    log "One or more files are owned by the PR author, ignoring"
     unset 'users[${prAuthor,,}]'
 fi
 
-# And we don't want to rerequest reviews from people or teams who already reviewed
+gh api \
-log -e "Checking if users/teams have already reviewed the PR"
-
-# shellcheck disable=SC2016
-# A graphql query to get all reviewers of a PR, including both users and teams
-# on behalf of which a review was done.
-# The REST API doesn't have an end-point for figuring out the on-behalfness of reviews
-all_reviewers_query='
-query($owner: String!, $repo: String!, $pr: Int!, $endCursor: String) {
-  repository(owner: $owner, name: $repo) {
-    pullRequest(number: $pr) {
-      reviews(first: 100, after: $endCursor) {
-        nodes {
-          author {
-            login
-          }
-          onBehalfOf(first: 100) {
-            nodes {
-              slug
-            }
-          }
-        }
-        pageInfo {
-          hasNextPage
-          endCursor
-        }
-      }
-    }
-  }
-}
-'
-
-gh api graphql \
     -H "Accept: application/vnd.github+json" \
-    --paginate \
+    -H "X-GitHub-Api-Version: 2022-11-28" \
-    -f query="$all_reviewers_query" \
+    "/repos/$baseRepo/pulls/$prNumber/reviews" \
-    -F owner="$org" \
+    --jq '.[].user.login' > "$tmp/already-reviewed-by"
-    -F repo="$repo" \
-    -F pr="$prNumber" \
-    > "$tmp/already-reviewed-by-response"
 
-readarray -t userReviewers < <(jq -r '.data.repository.pullRequest.reviews.nodes[].author.login' "$tmp/already-reviewed-by-response")
+# And we don't want to rerequest reviews from people who already reviewed
-log "PR is already reviewed by these users: ${userReviewers[*]}"
+while read -r user; do
-for user in "${userReviewers[@]}"; do
     if [[ -v users[${user,,}] ]]; then
-        log "Avoiding review request for user $user, who has already left a review"
+        log "User $user is a potential reviewer, but has already left a review, ignoring"
         unset 'users[${user,,}]'
     fi
-done
+done < "$tmp/already-reviewed-by"
-
-readarray -t teamReviewers < <(jq -r '.data.repository.pullRequest.reviews.nodes[].onBehalfOf.nodes[].slug' "$tmp/already-reviewed-by-response")
-log "PR is already reviewed by these teams: ${teamReviewers[*]}"
-for team in "${teamReviewers[@]}"; do
-    if [[ -v teams[${team,,}] ]]; then
-        log "Avoiding review request for team $team, who has already left a review"
-        unset 'teams[${team,,}]'
-    fi
-done
-
-log -e "Checking that all users/teams can be requested for review"
 
 for user in "${!users[@]}"; do
-    if ! gh api "/repos/$baseRepo/collaborators/$user" >&2; then
+    if ! gh api \
-        log "User $user cannot be requested for review because they don't exist or are not a repository collaborator, ignoring. Probably missed the automated invite to the maintainers team (see <https://github.com/NixOS/nixpkgs/issues/234293>)"
+        -H "Accept: application/vnd.github+json" \
+        -H "X-GitHub-Api-Version: 2022-11-28" \
+        "/repos/$baseRepo/collaborators/$user" >&2; then
+        log "User $user is not a repository collaborator, probably missed the automated invite to the maintainers team (see <https://github.com/NixOS/nixpkgs/issues/234293>), ignoring"
         unset 'users[$user]'
     fi
 done
-for team in "${!teams[@]}"; do
-    if ! gh api "/orgs/$org/teams/$team/repos/$baseRepo" >&2; then
-        log "Team $team cannot be requested for review because it doesn't exist or has no repository permissions, ignoring. Probably wasn't added to the nixpkgs-maintainers team (see https://github.com/NixOS/nixpkgs/tree/master/maintainers#maintainer-teams)"
-        unset 'teams[$team]'
-    fi
-done
 
-log "Would request reviews from users: ${!users[*]}"
+if [[ "${#users[@]}" -gt 10 ]]; then
-log "Would request reviews from teams: ${!teams[*]}"
+    log "Too many reviewers (${!users[*]}), skipping review requests"
-
-if (( ${#users[@]} + ${#teams[@]} > 10 )); then
-    log "Too many reviewers (users: ${!users[*]}, teams: ${!teams[*]}), skipping review requests"
     exit 0
 fi
 
 for user in "${!users[@]}"; do
-    log "Requesting review from user $user"
+    log "Requesting review from: $user"
-    if ! response=$(effect gh api \
-            --method POST \
-            "/repos/$baseRepo/pulls/$prNumber/requested_reviewers" \
-            -f "reviewers[]=$user"); then
-        log "Failed to request review from user $user: $response"
-    fi
-done
 
-for team in "${!teams[@]}"; do
+    if ! response=$(jq -n --arg user "$user" '{ reviewers: [ $user ] }' | \
-    log "Requesting review from team $team"
+        effect gh api \
-    if ! response=$(effect gh api \
             --method POST \
+            -H "Accept: application/vnd.github+json" \
+            -H "X-GitHub-Api-Version: 2022-11-28" \
             "/repos/$baseRepo/pulls/$prNumber/requested_reviewers" \
-            -f "team_reviewers[]=$team"); then
+            --input -); then
-        log "Failed to request review from team $team: $response"
+        log "Failed to request review from $user: $response"
     fi
 done

pkgs/stdenv/generic/check-meta.nix

@@ -397,7 +397,6 @@ 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;
@@ -671,10 +670,6 @@ 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