diff --git a/.github/workflows/reviewers.yml b/.github/workflows/reviewers.yml index 6d23e1dde9b7d..57f91c0f2f10d 100644 --- a/.github/workflows/reviewers.yml +++ b/.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 }} diff --git a/ci/eval/compare/default.nix b/ci/eval/compare/default.nix index d0820618a0a06..19e8ce72c5d80 100644 --- a/ci/eval/compare/default.nix +++ b/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" '' diff --git a/ci/eval/compare/maintainers.nix b/ci/eval/compare/maintainers.nix index 9f19a4c382eb1..bacf112ffe90f 100644 --- a/ci/eval/compare/maintainers.nix +++ b/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; +} diff --git a/ci/request-reviews/default.nix b/ci/request-reviews/default.nix index 075ff52fd564a..2ebc34738a5db 100644 --- a/ci/request-reviews/default.nix +++ b/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 diff --git a/ci/request-reviews/get-code-owners.sh b/ci/request-reviews/get-code-owners.sh index 13a377429b923..e49bb54098283 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 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[@]}" diff --git a/ci/request-reviews/request-maintainers-reviews.sh b/ci/request-reviews/request-maintainers-reviews.sh new file mode 100755 index 0000000000000..b49f4e29fcc64 --- /dev/null +++ b/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" diff --git a/ci/request-reviews/request-reviewers.sh b/ci/request-reviews/request-reviewers.sh index 1c105e385d28c..23ad13819f11b 100755 --- a/ci/request-reviews/request-reviewers.sh +++ b/ci/request-reviews/request-reviewers.sh @@ -1,8 +1,16 @@ #!/usr/bin/env bash -# Request reviewers for a PR, reading line-separated usernames on stdin, +# Request reviewers for a PR, reading line-separated usernames/teams 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 @@ -30,59 +38,133 @@ baseRepo=$1 prNumber=$2 prAuthor=$3 +org="${baseRepo%/*}" +repo="${baseRepo#*/}" + tmp=$(mktemp -d) trap 'rm -rf "$tmp"' exit -declare -A users=() +declare -A users=() teams=() + while read -r handle && [[ -n "$handle" ]]; do - users[${handle,,}]= + 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 done +log "Checking users: ${!users[*]}" +log "Checking teams: ${!teams[*]}" + # Cannot request a review from the author if [[ -v users[${prAuthor,,}] ]]; then - log "One or more files are owned by the PR author, ignoring" + log "Avoiding review request for PR author $prAuthor" unset 'users[${prAuthor,,}]' fi -gh api \ - -H "Accept: application/vnd.github+json" \ - -H "X-GitHub-Api-Version: 2022-11-28" \ - "/repos/$baseRepo/pulls/$prNumber/reviews" \ - --jq '.[].user.login' > "$tmp/already-reviewed-by" +# 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 + } + } + } + } +} +' -# And we don't want to rerequest reviews from people who already reviewed -while read -r user; do +gh api graphql \ + -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" + +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 if [[ -v users[${user,,}] ]]; then - log "User $user is a potential reviewer, but has already left a review, ignoring" + log "Avoiding review request for user $user, who has already left a review" unset 'users[${user,,}]' fi -done < "$tmp/already-reviewed-by" +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" for user in "${!users[@]}"; do - 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" + 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 )" 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 -if [[ "${#users[@]}" -gt 10 ]]; then - log "Too many reviewers (${!users[*]}), skipping review requests" +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" exit 0 fi for user in "${!users[@]}"; do - log "Requesting review from: $user" + 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 - if ! response=$(jq -n --arg user "$user" '{ reviewers: [ $user ] }' | \ - effect gh api \ +for team in "${!teams[@]}"; do + log "Requesting review from team $team" + 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" \ - --input -); then - log "Failed to request review from $user: $response" + -f "team_reviewers[]=$team"); then + log "Failed to request review from team $team: $response" fi done diff --git a/pkgs/stdenv/generic/check-meta.nix b/pkgs/stdenv/generic/check-meta.nix index c59e92e114ea1..47174c6f21ece 100644 --- a/pkgs/stdenv/generic/check-meta.nix +++ b/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