From 0371b7fb4b8d3f69f6cf7f1a7192a3c55b362006 Mon Sep 17 00:00:00 2001 From: Silvan Mosberger Date: Fri, 3 Jan 2025 02:52:13 +0100 Subject: [PATCH 1/4] ci/request-reviews: split off a more reusable reviewer processing script We can reuse the new process-reviewers.json part for requesting reviews from maintainers. --- ci/request-reviews/default.nix | 1 + ci/request-reviews/get-reviewers.sh | 42 +++---------------- ci/request-reviews/process-reviewers.sh | 55 +++++++++++++++++++++++++ ci/request-reviews/request-reviews.sh | 3 +- 4 files changed, 64 insertions(+), 37 deletions(-) create mode 100755 ci/request-reviews/process-reviewers.sh diff --git a/ci/request-reviews/default.nix b/ci/request-reviews/default.nix index b51d896539d88..f5f8dc1deb004 100644 --- a/ci/request-reviews/default.nix +++ b/ci/request-reviews/default.nix @@ -15,6 +15,7 @@ stdenvNoCC.mkDerivation { root = ./.; fileset = lib.fileset.unions [ ./get-reviewers.sh + ./process-reviewers.sh ./request-reviews.sh ./verify-base-branch.sh ./dev-branches.txt diff --git a/ci/request-reviews/get-reviewers.sh b/ci/request-reviews/get-reviewers.sh index 1107edd9e6f16..f447e2a7e597e 100755 --- a/ci/request-reviews/get-reviewers.sh +++ b/ci/request-reviews/get-reviewers.sh @@ -1,8 +1,6 @@ #!/usr/bin/env bash -# Get the code owners of the files changed by a PR, -# suitable to be consumed by 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 +# Get the code owners of the files changed by a PR, returning one username per line set -euo pipefail @@ -10,18 +8,15 @@ log() { echo "$@" >&2 } -if (( "$#" < 7 )); then - log "Usage: $0 GIT_REPO OWNERS_FILE BASE_REPO BASE_REF HEAD_REF PR_NUMBER PR_AUTHOR" +if (( "$#" < 4 )); then + log "Usage: $0 GIT_REPO OWNERS_FILE BASE_REF HEAD_REF" exit 1 fi gitRepo=$1 ownersFile=$2 -baseRepo=$3 -baseRef=$4 -headRef=$5 -prNumber=$6 -prAuthor=$7 +baseRef=$3 +headRef=$4 tmp=$(mktemp -d) trap 'rm -rf "$tmp"' exit @@ -98,29 +93,4 @@ for file in "${touchedFiles[@]}"; do done -# Cannot request a review from the author -if [[ -v users[${prAuthor,,}] ]]; then - log "One or more files are owned by the PR author, ignoring" - 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 who already reviewed -while read -r user; do - if [[ -v users[${user,,}] ]]; then - log "User $user is a code owner but has already left a review, ignoring" - unset 'users[${user,,}]' - fi -done < "$tmp/already-reviewed-by" - -# Turn it into a JSON for the GitHub API call to request PR reviewers -jq -n \ - --arg users "${!users[*]}" \ - '{ - reviewers: $users | split(" "), - }' +printf "%s\n" "${!users[@]}" diff --git a/ci/request-reviews/process-reviewers.sh b/ci/request-reviews/process-reviewers.sh new file mode 100755 index 0000000000000..ec929e25b9005 --- /dev/null +++ b/ci/request-reviews/process-reviewers.sh @@ -0,0 +1,55 @@ +#!/usr/bin/env bash + +# Process reviewers for a PR, reading line-separated usernames on stdin, +# returning a JSON suitable to be consumed by 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 + +set -euo pipefail + +log() { + echo "$@" >&2 +} + +if (( "$#" < 3 )); then + log "Usage: $0 BASE_REPO PR_NUMBER PR_AUTHOR" + exit 1 +fi + +baseRepo=$1 +prNumber=$2 +prAuthor=$3 + +tmp=$(mktemp -d) +trap 'rm -rf "$tmp"' exit + +declare -A users=() +while read -r handle && [[ -n "$handle" ]]; do + users[$handle]= +done + +# Cannot request a review from the author +if [[ -v users[${prAuthor,,}] ]]; then + log "One or more files are owned by the PR author, ignoring" + 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 who already reviewed +while read -r user; do + if [[ -v users[${user,,}] ]]; then + log "User $user is a code owner but has already left a review, ignoring" + unset 'users[${user,,}]' + fi +done < "$tmp/already-reviewed-by" + +# Turn it into a JSON for the GitHub API call to request PR reviewers +jq -n \ + --arg users "${!users[*]}" \ + '{ + reviewers: $users | split(" "), + }' diff --git a/ci/request-reviews/request-reviews.sh b/ci/request-reviews/request-reviews.sh index b21354560242a..986f3684b42a2 100755 --- a/ci/request-reviews/request-reviews.sh +++ b/ci/request-reviews/request-reviews.sh @@ -78,7 +78,8 @@ if ! "$SCRIPT_DIR"/verify-base-branch.sh "$tmp/nixpkgs.git" "$headRef" "$baseRep fi log "Getting code owners to request reviews from" -"$SCRIPT_DIR"/get-reviewers.sh "$tmp/nixpkgs.git" "$ownersFile" "$baseRepo" "$baseBranch" "$headRef" "$prNumber" "$prAuthor" > "$tmp/reviewers.json" +"$SCRIPT_DIR"/get-reviewers.sh "$tmp/nixpkgs.git" "$ownersFile" "$baseBranch" "$headRef" | \ + "$SCRIPT_DIR"/process-reviewers.sh "$baseRepo" "$prNumber" "$prAuthor" > "$tmp/reviewers.json" log "Requesting reviews from: $(<"$tmp/reviewers.json")" From 0ebab0bccadbd38d9bc0aae3e53e9493afa9ec0b Mon Sep 17 00:00:00 2001 From: Silvan Mosberger Date: Fri, 3 Jan 2025 03:07:38 +0100 Subject: [PATCH 2/4] workflows/eval: Reuse process-reviewers.sh Filters out the PR author and avoids rerequesting reviews from people that already left a review. In a future commit, this can be expanded to also avoid requesting reviews from people not in the org --- .github/workflows/eval.yml | 22 +++++++++++++++++++--- 1 file changed, 19 insertions(+), 3 deletions(-) diff --git a/.github/workflows/eval.yml b/.github/workflows/eval.yml index ab62551ce4687..a813c72972881 100644 --- a/.github/workflows/eval.yml +++ b/.github/workflows/eval.yml @@ -219,7 +219,7 @@ jobs: tag: name: Tag runs-on: ubuntu-latest - needs: process + needs: [ attrs, process ] if: needs.process.outputs.baseRunId permissions: pull-requests: write @@ -239,6 +239,21 @@ jobs: name: comparison path: comparison + - name: Install Nix + uses: cachix/install-nix-action@08dcb3a5e62fa31e2da3d490afc4176ef55ecd72 # v30 + + # Important: This workflow job runs with extra permissions, + # so we need to make sure to not run untrusted code from PRs + - name: Check out Nixpkgs at the base commit + uses: actions/checkout@11bd71901bbe5b1630ceea73d27597364c9af683 # v4.2.2 + with: + ref: ${{ needs.attrs.outputs.baseSha }} + path: base + sparse-checkout: ci + + - name: Build the requestReviews derivation + run: nix-build base/ci -A requestReviews + - name: Tagging pull request run: | # Get all currently set rebuild labels @@ -271,8 +286,8 @@ jobs: # 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"; done \ - | jq -s '{ reviewers: map(.login) }' \ + | while read -r id; do gh api /user/"$id" --jq .login; done \ + | GH_TOKEN=${{ steps.app-token.outputs.token }} result/bin/process-reviewers.sh "$REPOSITORY" "$NUMBER" "$AUTHOR" \ > reviewers.json # Request reviewers from maintainers of changed output paths @@ -285,6 +300,7 @@ jobs: GH_TOKEN: ${{ github.token }} REPOSITORY: ${{ github.repository }} NUMBER: ${{ github.event.number }} + AUTHOR: ${{ github.event.pull_request.user.login }} - name: Add eval summary to commit statuses if: ${{ github.event_name == 'pull_request_target' }} From ab248be504bc38a3ea0f7409d31109b45e619224 Mon Sep 17 00:00:00 2001 From: Silvan Mosberger Date: Fri, 3 Jan 2025 03:09:19 +0100 Subject: [PATCH 3/4] workflows/eval: Minor cleanup The ${{ }} syntax is best avoided in scripts. While it wouldn't be a problem here, let's do this for consistency --- .github/workflows/eval.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/eval.yml b/.github/workflows/eval.yml index a813c72972881..2e99bb17b8bb8 100644 --- a/.github/workflows/eval.yml +++ b/.github/workflows/eval.yml @@ -293,7 +293,7 @@ jobs: # Request reviewers from maintainers of changed output paths GH_TOKEN=${{ steps.app-token.outputs.token }} gh api \ --method POST \ - /repos/${{ github.repository }}/pulls/${{ github.event.number }}/requested_reviewers \ + /repos/"$REPOSITORY"/pulls/"$NUMBER"/requested_reviewers \ --input reviewers.json env: From 077007a65827b7c8127458adf2c3661b91fc5545 Mon Sep 17 00:00:00 2001 From: Silvan Mosberger Date: Fri, 3 Jan 2025 03:24:37 +0100 Subject: [PATCH 4/4] ci/request-reviews: Don't request reviews from non-repo-collaborators Fixes this problem for maintainer-based reviews when the maintainer didn't yet accept or missed the automated invite: gh: Reviews may only be requested from collaborators. One or more of the users or teams you specified is not a collaborator of the NixOS/nixpkgs repository. (HTTP 422) --- ci/request-reviews/process-reviewers.sh | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/ci/request-reviews/process-reviewers.sh b/ci/request-reviews/process-reviewers.sh index ec929e25b9005..52c0dca0b5594 100755 --- a/ci/request-reviews/process-reviewers.sh +++ b/ci/request-reviews/process-reviewers.sh @@ -47,6 +47,16 @@ while read -r user; do fi done < "$tmp/already-reviewed-by" +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" + unset 'users[$user]' + fi +done + # Turn it into a JSON for the GitHub API call to request PR reviewers jq -n \ --arg users "${!users[*]}" \