From b5af85e10edeff23719eb3b05e259be7cfb97cb6 Mon Sep 17 00:00:00 2001 From: Wolfgang Walther Date: Tue, 28 Oct 2025 07:38:20 +0000 Subject: [PATCH] Revert "First-class GitHub team reviews" --- .github/workflows/reviewers.yml | 11 +- ci/eval/compare/default.nix | 9 +- ci/eval/compare/maintainers.nix | 52 ++----- ci/request-reviews/default.nix | 2 - ci/request-reviews/get-code-owners.sh | 40 +++++- .../request-maintainers-reviews.sh | 39 ----- ci/request-reviews/request-reviewers.sh | 134 ++++-------------- pkgs/stdenv/generic/check-meta.nix | 5 - 8 files changed, 85 insertions(+), 207 deletions(-) delete mode 100755 ci/request-reviews/request-maintainers-reviews.sh diff --git a/.github/workflows/reviewers.yml b/.github/workflows/reviewers.yml index 57f91c0f2f10..6d23e1dde9b7 100644 --- a/.github/workflows/reviewers.yml +++ b/.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 }} diff --git a/ci/eval/compare/default.nix b/ci/eval/compare/default.nix index 19e8ce72c5d8..d0820618a0a0 100644 --- a/ci/eval/compare/default.nix +++ b/ci/eval/compare/default.nix @@ -179,12 +179,8 @@ runCommand "compare" jq cmp-stats ]; - maintainers = builtins.toJSON maintainers.users; - teams = builtins.toJSON maintainers.teams; - passAsFile = [ - "maintainers" - "teams" - ]; + maintainers = builtins.toJSON maintainers; + passAsFile = [ "maintainers" ]; } '' mkdir $out @@ -227,5 +223,4 @@ runCommand "compare" fi cp "$maintainersPath" "$out/maintainers.json" - cp "$teamsPath" "$out/teams.json" '' diff --git a/ci/eval/compare/maintainers.nix b/ci/eval/compare/maintainers.nix index bacf112ffe90..9f19a4c382eb 100644 --- a/ci/eval/compare/maintainers.nix +++ b/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. - # 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 [ ]; + # 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 [ ]; } )) # 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"; - user = toString maintainer.githubId; + id = maintainer.githubId; + inherit (maintainer) github; packageName = pkg.name; - }); - - 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 + dueToFiles = pkg.filenames; + }) pkg.maintainers ) attrsWithModifiedFiles; - byType = lib.groupBy (ping: ping.type) maintainersToPing; + byMaintainer = lib.groupBy (ping: toString ping.id) listToPing; - 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))) - ]; + packagesPerMaintainer = lib.mapAttrs ( + maintainer: packages: map (pkg: pkg.packageName) packages + ) byMaintainer; in -{ - users = byUser; - teams = byTeam; -} +packagesPerMaintainer diff --git a/ci/request-reviews/default.nix b/ci/request-reviews/default.nix index 2ebc34738a5d..075ff52fd564 100644 --- a/ci/request-reviews/default.nix +++ b/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 diff --git a/ci/request-reviews/get-code-owners.sh b/ci/request-reviews/get-code-owners.sh index e49bb5409828..13a377429b92 100755 --- a/ci/request-reviews/get-code-owners.sh +++ b/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[@]}" diff --git a/ci/request-reviews/request-maintainers-reviews.sh b/ci/request-reviews/request-maintainers-reviews.sh deleted file mode 100755 index b49f4e29fcc6..000000000000 --- a/ci/request-reviews/request-maintainers-reviews.sh +++ /dev/null @@ -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" diff --git a/ci/request-reviews/request-reviewers.sh b/ci/request-reviews/request-reviewers.sh index 23ad13819f11..1c105e385d28 100755 --- a/ci/request-reviews/request-reviewers.sh +++ b/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 - 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 + users[${handle,,}]= 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 -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 \ +gh api \ -H "Accept: application/vnd.github+json" \ - --paginate \ - -f query="$all_reviewers_query" \ - -F owner="$org" \ - -F repo="$repo" \ - -F pr="$prNumber" \ - > "$tmp/already-reviewed-by-response" + -H "X-GitHub-Api-Version: 2022-11-28" \ + "/repos/$baseRepo/pulls/$prNumber/reviews" \ + --jq '.[].user.login' > "$tmp/already-reviewed-by" -readarray -t userReviewers < <(jq -r '.data.repository.pullRequest.reviews.nodes[].author.login' "$tmp/already-reviewed-by-response") -log "PR is already reviewed by these users: ${userReviewers[*]}" -for user in "${userReviewers[@]}"; do +# And we don't want to rerequest reviews from people who already reviewed +while read -r user; 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 - -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" +done < "$tmp/already-reviewed-by" for user in "${!users[@]}"; do - if ! gh api "/repos/$baseRepo/collaborators/$user" >&2; then - 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 )" + if ! gh api \ + -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 ), 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[*]}" -log "Would request reviews from teams: ${!teams[*]}" - -if (( ${#users[@]} + ${#teams[@]} > 10 )); then - log "Too many reviewers (users: ${!users[*]}, teams: ${!teams[*]}), skipping review requests" +if [[ "${#users[@]}" -gt 10 ]]; then + log "Too many reviewers (${!users[*]}), skipping review requests" exit 0 fi for user in "${!users[@]}"; do - log "Requesting review from user $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 + log "Requesting review from: $user" -for team in "${!teams[@]}"; do - log "Requesting review from team $team" - if ! response=$(effect gh api \ + if ! response=$(jq -n --arg user "$user" '{ reviewers: [ $user ] }' | \ + 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 - log "Failed to request review from team $team: $response" + --input -); then + log "Failed to request review from $user: $response" fi done diff --git a/pkgs/stdenv/generic/check-meta.nix b/pkgs/stdenv/generic/check-meta.nix index 47174c6f21ec..c59e92e114ea 100644 --- a/pkgs/stdenv/generic/check-meta.nix +++ b/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