diff --git a/.github/workflows/pull-request-target.yml b/.github/workflows/pull-request-target.yml index 76a77c7942e5..a3774366bc9b 100644 --- a/.github/workflows/pull-request-target.yml +++ b/.github/workflows/pull-request-target.yml @@ -112,18 +112,6 @@ jobs: with: headBranch: ${{ needs.prepare.outputs.headBranch }} - reviewers: - name: Reviewers - needs: [prepare, eval] - if: | - needs.prepare.outputs.targetSha && - !contains(fromJSON(needs.prepare.outputs.headBranch).type, 'development') - uses: ./.github/workflows/reviewers.yml - secrets: - OWNER_APP_PRIVATE_KEY: ${{ secrets.OWNER_APP_PRIVATE_KEY }} - with: - artifact-prefix: ${{ inputs.artifact-prefix }} - build: name: Build needs: [prepare] diff --git a/.github/workflows/reviewers.yml b/.github/workflows/reviewers.yml deleted file mode 100644 index 060d82d84201..000000000000 --- a/.github/workflows/reviewers.yml +++ /dev/null @@ -1,157 +0,0 @@ -# This workflow will request reviews from the maintainers of each package -# listed in the PR's most recent eval comparison artifact. - -name: Reviewers - -on: - pull_request_target: - types: [ready_for_review] - workflow_call: - inputs: - artifact-prefix: - required: true - type: string - secrets: - OWNER_APP_PRIVATE_KEY: - required: true - -concurrency: - group: reviewers-${{ github.workflow }}-${{ github.event_name }}-${{ github.event.pull_request.number || github.run_id }} - cancel-in-progress: true - -permissions: {} - -defaults: - run: - shell: bash - -jobs: - request: - runs-on: ubuntu-24.04-arm - timeout-minutes: 20 - steps: - - name: Check out the PR at the base commit - uses: actions/checkout@08c6903cd8c0fde910a37f88322edcfb5dd907a8 # v5.0.0 - with: - persist-credentials: false - path: trusted - sparse-checkout: ci - - - name: Install Nix - uses: cachix/install-nix-action@456688f15bc354bef6d396e4a35f4f89d40bf2b7 # v31 - - - name: Build the requestReviews derivation - run: nix-build trusted/ci -A requestReviews - - # For requesting reviewers, this job depends on a GitHub App with the following permissions: - # - Permissions: - # - Repository > Administration: read-only - # - Organization > Members: read-only - # - Repository > Pull Requests: read-write - # - Install App on this repository, setting these variables: - # - OWNER_APP_ID (variable) - # - OWNER_APP_PRIVATE_KEY (secret) - # - # Can't use the token received from permissions above, because it can't get enough permissions. - - uses: actions/create-github-app-token@67018539274d69449ef7c02e8e71183d1719ab42 # v2.1.4 - if: github.event_name == 'pull_request_target' && vars.OWNER_APP_ID - id: app-token - with: - app-id: ${{ vars.OWNER_APP_ID }} - private-key: ${{ secrets.OWNER_APP_PRIVATE_KEY }} - permission-administration: read - permission-members: read - permission-pull-requests: write - - - name: Log current API rate limits (github.token) - env: - GH_TOKEN: ${{ github.token }} - run: gh api /rate_limit | jq - - # In the regular case, this workflow is called via workflow_call from the eval workflow directly. - # In the more special case, when a PR is undrafted an eval run will have started already. - - name: Wait for comparison to be done - uses: actions/github-script@ed597411d8f924073f98dfc5c65a23a2325f34cd # v8.0.0 - id: eval - env: - ARTIFACT: ${{ inputs.artifact-prefix }}comparison - with: - script: | - const run_id = (await github.rest.actions.listWorkflowRuns({ - owner: context.repo.owner, - repo: context.repo.repo, - workflow_id: context.eventName === 'pull_request' ? 'test.yml' : 'pull-request-target.yml', - event: context.eventName, - head_sha: context.payload.pull_request.head.sha - })).data.workflow_runs[0].id - - core.setOutput('run-id', run_id) - - // Waiting 120 * 5 sec = 10 min. max. - // The extreme case is an Eval run that just started when the PR is undrafted. - // Eval takes max 5-6 minutes, normally. - for (let i = 0; i < 120; i++) { - const result = await github.rest.actions.listWorkflowRunArtifacts({ - owner: context.repo.owner, - repo: context.repo.repo, - run_id, - name: process.env.ARTIFACT, - }) - if (result.data.total_count > 0) return - await new Promise(resolve => setTimeout(resolve, 5000)) - } - throw new Error("No comparison artifact found.") - - - name: Log current API rate limits (github.token) - env: - GH_TOKEN: ${{ github.token }} - run: gh api /rate_limit | jq - - - name: Download the comparison results - uses: actions/download-artifact@018cc2cf5baa6db3ef3c5f8a56943fffe632ef53 # v6.0.0 - with: - run-id: ${{ steps.eval.outputs.run-id }} - github-token: ${{ github.token }} - pattern: ${{ inputs.artifact-prefix }}comparison - path: comparison - merge-multiple: true - - - name: Log current API rate limits (app-token) - if: ${{ steps.app-token.outputs.token }} - env: - GH_TOKEN: ${{ steps.app-token.outputs.token }} - run: gh api /rate_limit | jq - - - name: Log current API rate limits (github.token) - env: - GH_TOKEN: ${{ github.token }} - run: gh api /rate_limit | jq - - - name: Requesting reviews - if: ${{ steps.app-token.outputs.token }} - env: - GH_TOKEN: ${{ github.token }} - APP_GH_TOKEN: ${{ steps.app-token.outputs.token }} - REPOSITORY: ${{ github.repository }} - 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 \ - | cat comparison/owners.txt - \ - | 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 }} - env: - GH_TOKEN: ${{ steps.app-token.outputs.token }} - run: gh api /rate_limit | jq - - - name: Log current API rate limits (github.token) - env: - GH_TOKEN: ${{ github.token }} - run: gh api /rate_limit | jq diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml index b9369de2af3d..727a9f965845 100644 --- a/.github/workflows/test.yml +++ b/.github/workflows/test.yml @@ -69,7 +69,6 @@ jobs: '.github/workflows/eval.yml', '.github/workflows/lint.yml', '.github/workflows/pull-request-target.yml', - '.github/workflows/reviewers.yml', '.github/workflows/test.yml', 'ci/github-script/bot.js', 'ci/github-script/merge.js', diff --git a/ci/default.nix b/ci/default.nix index 5f22dc47ed60..8712a44e0584 100644 --- a/ci/default.nix +++ b/ci/default.nix @@ -156,7 +156,6 @@ let in rec { inherit pkgs fmt; - requestReviews = pkgs.callPackage ./request-reviews { }; codeownersValidator = pkgs.callPackage ./codeowners-validator { }; # FIXME(lf-): it might be useful to test other Nix implementations diff --git a/ci/github-script/bot.js b/ci/github-script/bot.js index 654ca84d2367..1b807b30ec7d 100644 --- a/ci/github-script/bot.js +++ b/ci/github-script/bot.js @@ -5,6 +5,7 @@ module.exports = async ({ github, context, core, dry }) => { const withRateLimit = require('./withRateLimit.js') const { classify } = require('../supportedBranches.js') const { handleMerge } = require('./merge.js') + const { handleReviewers } = require('./reviewers.js') const artifactClient = new DefaultArtifactClient() @@ -209,10 +210,16 @@ module.exports = async ({ github, context, core, dry }) => { } } - const reviews = await github.paginate(github.rest.pulls.listReviews, { - ...context.repo, - pull_number, - }) + // Check for any human reviews other than GitHub actions and other GitHub apps. + // Accounts could be deleted as well, so don't count them. + const reviews = ( + await github.paginate(github.rest.pulls.listReviews, { + ...context.repo, + pull_number, + }) + ).filter( + (r) => r.user && !r.user.login.endsWith('[bot]') && r.user.type !== 'Bot', + ) const approvals = new Set( reviews @@ -282,13 +289,6 @@ module.exports = async ({ github, context, core, dry }) => { log('Last eval run', run_id ?? '') if (conclusion === 'success') { - // Check for any human reviews other than GitHub actions and other GitHub apps. - // Accounts could be deleted as well, so don't count them. - const humanReviews = reviews.filter( - (r) => - r.user && !r.user.login.endsWith('[bot]') && r.user.type !== 'Bot', - ) - Object.assign(prLabels, { // We only set this label if the latest eval run was successful, because if it was not, it // *could* have requested reviewers. We will let the PR author fix CI first, before "escalating" @@ -301,7 +301,7 @@ module.exports = async ({ github, context, core, dry }) => { '9.needs: reviewer': !pull_request.draft && pull_request.requested_reviewers.length === 0 && - humanReviews.length === 0, + reviews.length === 0, }) } @@ -373,6 +373,33 @@ module.exports = async ({ github, context, core, dry }) => { maintainers[pkg]?.some((m) => approvals.has(m)), ), }) + + if (!pull_request.draft) { + // We set this label earlier already, but the current PR state can be very different + // after handleReviewers has requested reviews, so update it in this case to prevent + // this label from flip-flopping. + prLabels['9.needs: reviewer'] = await handleReviewers({ + github, + context, + core, + log, + dry, + pull_request, + reviews, + // TODO: Use maintainer map instead of the artifact. + maintainers: Object.keys( + JSON.parse( + await readFile(`${pull_number}/maintainers.json`, 'utf-8'), + ), + ).map((id) => parseInt(id)), + // TODO: Create owner map similar to maintainer map. + owners: (await readFile(`${pull_number}/owners.txt`, 'utf-8')).split( + '\n', + ), + getTeamMembers, + getUser, + }) + } } return prLabels @@ -521,7 +548,7 @@ module.exports = async ({ github, context, core, dry }) => { const hasChanges = Object.keys(after).some( (name) => (before[name] ?? false) !== after[name], ) - if (log('Has changes', hasChanges, !hasChanges)) return + if (log('Has label changes', hasChanges, !hasChanges)) return // Skipping labeling on a pull_request event, because we have no privileges. const labels = Object.entries(after) diff --git a/ci/github-script/reviewers.js b/ci/github-script/reviewers.js new file mode 100644 index 000000000000..81ad151264a1 --- /dev/null +++ b/ci/github-script/reviewers.js @@ -0,0 +1,128 @@ +async function handleReviewers({ + github, + context, + core, + log, + dry, + pull_request, + reviews, + maintainers, + owners, + getTeamMembers, + getUser, +}) { + const pull_number = pull_request.number + + const users = new Set([ + ...(await Promise.all( + maintainers.map(async (id) => (await getUser(id)).login), + )), + ...owners.filter((handle) => handle && !handle.includes('/')), + ]) + log('reviewers - users', Array.from(users).join(', ')) + + const teams = new Set( + owners + .map((handle) => handle.split('/')) + .filter(([org, slug]) => org === context.repo.owner && slug) + .map(([, slug]) => slug), + ) + log('reviewers - teams', Array.from(teams).join(', ')) + + const team_members = new Set( + (await Promise.all(Array.from(teams, getTeamMembers))) + .flat(1) + .map(({ login }) => login), + ) + log('reviewers - team_members', Array.from(team_members).join(', ')) + + const new_reviewers = users + .union(team_members) + // We can't request a review from the author. + .difference(new Set([pull_request.user?.login])) + log('reviewers - new_reviewers', Array.from(new_reviewers).join(', ')) + + // Filter users to repository collaborators. If they're not, they can't be requested + // for review. In that case, they probably missed their invite to the maintainers team. + const reviewers = ( + await Promise.all( + Array.from(new_reviewers, async (username) => { + try { + await github.rest.repos.checkCollaborator({ + ...context.repo, + username, + }) + return username + } catch (e) { + if (e.status !== 404) throw e + core.warn( + `PR #${pull_number}: User ${username} cannot be requested for review because they don't exist or are not a repository collaborator, ignoring. They probably missed the automated invite to the maintainers team (see ).`, + ) + } + }), + ) + ).filter(Boolean) + log('reviewers - reviewers', reviewers.join(', ')) + + if (reviewers.length > 15) { + log( + `Too many reviewers (${reviewers.join(', ')}), skipping review requests.`, + ) + // false indicates, that we do have reviewers and don't need the "needs: reviewers" label. + return false + } + + const requested_reviewers = new Set( + pull_request.requested_reviewers.map(({ login }) => login), + ) + log( + 'reviewers - requested_reviewers', + Array.from(requested_reviewers).join(', '), + ) + + const existing_reviewers = new Set( + reviews.map(({ user }) => user?.login).filter(Boolean), + ) + log( + 'reviewers - existing_reviewers', + Array.from(existing_reviewers).join(', '), + ) + + const non_requested_reviewers = new Set(reviewers) + .difference(requested_reviewers) + // We don't want to rerequest reviews from people who already reviewed. + .difference(existing_reviewers) + log( + 'reviewers - non_requested_reviewers', + Array.from(non_requested_reviewers).join(', '), + ) + + if (non_requested_reviewers.size === 0) { + log('Has reviewer changes', 'false (skipped)') + } else if (dry) { + core.info( + `Requesting reviewers for #${pull_number}: ${Array.from(non_requested_reviewers).join(', ')} (dry)`, + ) + } else { + // We had tried the "request all reviewers at once" thing in the past, but it didn't work out: + // https://github.com/NixOS/nixpkgs/commit/034613f860fcd339bd2c20c8f6bc259a2f9dc034 + // If we're hitting API errors here again, we'll need to investigate - and possibly reverse + // course. + await github.rest.pulls.requestReviewers({ + ...context.repo, + pull_number, + reviewers, + }) + } + + // Return a boolean on whether the "needs: reviewers" label should be set. + return ( + new_reviewers.size === 0 && + existing_reviewers.size === 0 && + requested_reviewers.size === 0 + ) +} + +module.exports = { + handleReviewers, +} diff --git a/ci/request-reviews/default.nix b/ci/request-reviews/default.nix deleted file mode 100644 index 55a1889fe89f..000000000000 --- a/ci/request-reviews/default.nix +++ /dev/null @@ -1,33 +0,0 @@ -{ - lib, - stdenvNoCC, - makeWrapper, - coreutils, - jq, - github-cli, -}: -stdenvNoCC.mkDerivation { - name = "request-reviews"; - src = lib.fileset.toSource { - root = ./.; - fileset = lib.fileset.unions [ - ./request-reviewers.sh - ]; - }; - nativeBuildInputs = [ makeWrapper ]; - dontBuild = true; - installPhase = '' - mkdir -p $out/bin - for bin in *.sh; do - mv "$bin" "$out/bin" - wrapProgram "$out/bin/$bin" \ - --set PATH ${ - lib.makeBinPath [ - coreutils - jq - github-cli - ] - } - done - ''; -} diff --git a/ci/request-reviews/request-reviewers.sh b/ci/request-reviews/request-reviewers.sh deleted file mode 100755 index a7cde16eb97b..000000000000 --- a/ci/request-reviews/request-reviewers.sh +++ /dev/null @@ -1,120 +0,0 @@ -#!/usr/bin/env bash - -# 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 - -set -euo pipefail - -tmp=$(mktemp -d) -trap 'rm -rf "$tmp"' exit - -log() { - echo "$@" >&2 -} - -effect() { - if [[ -n "${DRY_MODE:-}" ]]; then - log "Skipping in dry mode:" "${@@Q}" - else - "$@" - fi -} - -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 - -# 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 users=() -while read -r handle && [[ -n "$handle" ]]; do - if [[ "$handle" =~ (.*)/(.*) ]]; 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 $handle has these members: ${members[*]}" - - for user in "${members[@]}"; do - users[${user,,}]= - done - else - # Everything else is a user - users[${handle,,}]= - fi -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 potential reviewer, but has already left a review, ignoring" - unset 'users[${user,,}]' - 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 - -if [[ "${#users[@]}" -gt 15 ]]; then - log "Too many reviewers (${!users[*]}), skipping review requests" - exit 0 -fi - -for user in "${!users[@]}"; do - log "Requesting review from: $user" - - 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" \ - --input -); then - log "Failed to request review from $user: $response" - fi -done