diff --git a/.github/workflows/check.yml b/.github/workflows/check.yml index 99e83c2c151f..fa16aec65997 100644 --- a/.github/workflows/check.yml +++ b/.github/workflows/check.yml @@ -16,14 +16,6 @@ on: required: true type: string secrets: - # Can be provided in pull requests because the job it is used in does - # not evaluate untrusted code. - NIXPKGS_COMMIT_CHECK_APP_PRIVATE_KEY: - required: false - # Can be provided in pull requests because the job it is used in does - # not evaluate untrusted code. - NIXPKGS_MANUAL_EDIT_CHECK_APP_PRIVATE_KEY: - required: false # Should only be provided in the merge queue, not in pull requests, # where we're evaluating untrusted code. CACHIX_AUTH_TOKEN_GHA: @@ -53,19 +45,9 @@ jobs: - name: Install dependencies run: npm install bottleneck@2.19.5 - # It's fine to reuse this app in the 'pull-request-target / prepare' job, - # because that job has to run before this one. - - uses: actions/create-github-app-token@1b10c78c7865c340bc4f6099eb2f838309f1e8c3 # v3.1.1 - if: github.event_name != 'pull_request' && vars.NIXPKGS_COMMIT_CHECK_CLIENT_ID - id: app-token - with: - client-id: ${{ vars.NIXPKGS_COMMIT_CHECK_CLIENT_ID }} - private-key: ${{ secrets.NIXPKGS_COMMIT_CHECK_APP_PRIVATE_KEY }} - permission-pull-requests: write - - name: Log current API rate limits env: - GH_TOKEN: ${{ steps.app-token.outputs.token || github.token }} + GH_TOKEN: ${{ github.token }} run: gh api /rate_limit | jq - name: Check commits @@ -74,7 +56,6 @@ jobs: env: TARGETS_STABLE: ${{ fromJSON(inputs.baseBranch).stable && !contains(fromJSON(inputs.headBranch).type, 'development') }} with: - github-token: ${{ steps.app-token.outputs.token || github.token }} script: | const targetsStable = JSON.parse(process.env.TARGETS_STABLE) require('./trusted/ci/github-script/commits.js')({ @@ -87,7 +68,7 @@ jobs: - name: Log current API rate limits env: - GH_TOKEN: ${{ steps.app-token.outputs.token || github.token }} + GH_TOKEN: ${{ github.token }} run: gh api /rate_limit | jq manual-file-edits: @@ -104,35 +85,25 @@ jobs: sparse-checkout: | ci/github-script - - uses: actions/create-github-app-token@1b10c78c7865c340bc4f6099eb2f838309f1e8c3 # v3.1.1 - if: github.event_name != 'pull_request' && vars.NIXPKGS_MANUAL_EDIT_CHECK_CLIENT_ID - id: app-token - with: - client-id: ${{ vars.NIXPKGS_MANUAL_EDIT_CHECK_CLIENT_ID }} - private-key: ${{ secrets.NIXPKGS_MANUAL_EDIT_CHECK_APP_PRIVATE_KEY }} - permission-pull-requests: write - - name: Log current API rate limits env: - GH_TOKEN: ${{ steps.app-token.outputs.token || github.token }} + GH_TOKEN: ${{ github.token }} run: gh api /rate_limit | jq - name: Discourage manual edits to certain files uses: actions/github-script@3a2844b7e9c422d3c10d287c895573f7108da1b3 # v9.0.0 with: - github-token: ${{ steps.app-token.outputs.token || github.token }} script: | require('./trusted/ci/github-script/manual-file-edits.js')({ github, context, core, - dry: context.eventName == 'pull_request', repoPath: 'trusted', }) - name: Log current API rate limits env: - GH_TOKEN: ${{ steps.app-token.outputs.token || github.token }} + GH_TOKEN: ${{ github.token }} run: gh api /rate_limit | jq owners: diff --git a/.github/workflows/eval.yml b/.github/workflows/eval.yml index a8dcaf29456f..4939fa5ec329 100644 --- a/.github/workflows/eval.yml +++ b/.github/workflows/eval.yml @@ -23,10 +23,6 @@ on: default: false type: boolean secrets: - # Can be provided in pull requests because the job it is used in does - # not evaluate untrusted code. - NIXPKGS_BRANCH_CHECK_APP_PRIVATE_KEY: - required: false # Should only be provided in the merge queue, not in pull requests, # where we're evaluating untrusted code. CACHIX_AUTH_TOKEN_GHA: @@ -353,20 +349,10 @@ jobs: description, target_url }) - - - uses: actions/create-github-app-token@1b10c78c7865c340bc4f6099eb2f838309f1e8c3 # v3.1.1 - if: github.event_name == 'pull_request_target' && vars.NIXPKGS_BRANCH_CHECK_CLIENT_ID - id: app-token - with: - client-id: ${{ vars.NIXPKGS_BRANCH_CHECK_CLIENT_ID }} - private-key: ${{ secrets.NIXPKGS_BRANCH_CHECK_APP_PRIVATE_KEY }} - permission-pull-requests: write - - name: Request changes if PR is against an inappropriate branch if: ${{ github.event_name == 'pull_request_target' }} uses: actions/github-script@3a2844b7e9c422d3c10d287c895573f7108da1b3 # v9.0.0 with: - github-token: ${{ steps.app-token.outputs.token || github.token }} script: | require('./nixpkgs/trusted/ci/github-script/check-target-branch.js')({ github, diff --git a/.github/workflows/pull-request-target.yml b/.github/workflows/pull-request-target.yml index 02d07344eee8..190ce2510a3e 100644 --- a/.github/workflows/pull-request-target.yml +++ b/.github/workflows/pull-request-target.yml @@ -10,12 +10,6 @@ on: secrets: NIXPKGS_CI_APP_PRIVATE_KEY: required: true - NIXPKGS_BRANCH_CHECK_APP_PRIVATE_KEY: - required: false - NIXPKGS_COMMIT_CHECK_APP_PRIVATE_KEY: - required: false - NIXPKGS_MANUAL_EDIT_CHECK_APP_PRIVATE_KEY: - required: false concurrency: group: pr-${{ github.workflow }}-${{ github.event_name }}-${{ github.event.pull_request.number || github.run_id }} @@ -42,17 +36,6 @@ jobs: sparse-checkout-cone-mode: true # default, for clarity sparse-checkout: | ci/github-script - - # It's fine to reuse this app in the 'check / commits' job, - # because this job has to run before that one. - - uses: actions/create-github-app-token@1b10c78c7865c340bc4f6099eb2f838309f1e8c3 # v3.1.1 - if: vars.NIXPKGS_COMMIT_CHECK_CLIENT_ID - id: app-token - with: - client-id: ${{ vars.NIXPKGS_COMMIT_CHECK_CLIENT_ID }} - private-key: ${{ secrets.NIXPKGS_COMMIT_CHECK_APP_PRIVATE_KEY }} - permission-pull-requests: write - - id: prepare uses: actions/github-script@3a2844b7e9c422d3c10d287c895573f7108da1b3 # v9.0.0 with: @@ -77,9 +60,6 @@ jobs: permissions: # cherry-picks pull-requests: write - secrets: - NIXPKGS_COMMIT_CHECK_APP_PRIVATE_KEY: ${{ secrets.NIXPKGS_COMMIT_CHECK_APP_PRIVATE_KEY }} - NIXPKGS_MANUAL_EDIT_CHECK_APP_PRIVATE_KEY: ${{ secrets.NIXPKGS_MANUAL_EDIT_CHECK_APP_PRIVATE_KEY }} with: baseBranch: ${{ needs.prepare.outputs.baseBranch }} headBranch: ${{ needs.prepare.outputs.headBranch }} @@ -102,8 +82,6 @@ jobs: # compare pull-requests: write statuses: write - secrets: - NIXPKGS_BRANCH_CHECK_APP_PRIVATE_KEY: ${{ secrets.NIXPKGS_BRANCH_CHECK_APP_PRIVATE_KEY }} with: artifact-prefix: ${{ inputs.artifact-prefix }} mergedSha: ${{ needs.prepare.outputs.mergedSha }} diff --git a/ci/github-script/check-target-branch.js b/ci/github-script/check-target-branch.js index a31520c02fc6..9b47a946b889 100644 --- a/ci/github-script/check-target-branch.js +++ b/ci/github-script/check-target-branch.js @@ -151,9 +151,11 @@ async function checkTargetBranch({ github, context, core, dry }) { core, dry, body, - event: 'REQUEST_CHANGES', + event: 'COMMENT', reviewKey, }) + + throw new Error('This PR is against the wrong branch.') } else if (rebuildsAllTests && !isExemptKernelUpdate) { let branchText if (base === 'master' && maxRebuildCount >= 500) { @@ -177,9 +179,11 @@ async function checkTargetBranch({ github, context, core, dry }) { core, dry, body, - event: 'REQUEST_CHANGES', + event: 'COMMENT', reviewKey, }) + + throw new Error('This PR is against the wrong branch.') } else if ( maxRebuildCount >= 500 && !isExemptKernelUpdate && @@ -200,7 +204,7 @@ async function checkTargetBranch({ github, context, core, dry }) { core, dry, body, - event: 'REQUEST_CHANGES', + event: 'COMMENT', reviewKey, }) } else { diff --git a/ci/github-script/manual-file-edits.js b/ci/github-script/manual-file-edits.js index 84235d44752c..e40d6decbb7d 100644 --- a/ci/github-script/manual-file-edits.js +++ b/ci/github-script/manual-file-edits.js @@ -7,13 +7,9 @@ const { getCommitDetailsForPR } = require('./get-pr-commit-details') * context: import('@actions/github/lib/context').Context, * core: import('@actions/core'), * repoPath?: string, - * dry: boolean, * }} CheckManualFileEditsProps */ -async function checkManualFileEdits({ github, context, core, repoPath, dry }) { - const { dismissReviews, postReview } = require('./reviews.js') - const reviewKey = 'manual-file-edits' - +async function checkManualFileEdits({ github, context, core, repoPath }) { const pull_number = context.payload.pull_request?.number if (!pull_number) { core.info('This is not a pull request. Skipping checks.') @@ -39,13 +35,8 @@ async function checkManualFileEdits({ github, context, core, repoPath, dry }) { changedPaths.includes('maintainers/github-teams.json'), ) ) { - postReview({ - github, - context, - core, - dry, - event: 'REQUEST_CHANGES', - body: [ + core.setFailed( + [ 'maintainers/github-teams.json is supposed to accurately reflect the state of the teams in GitHub.\n', 'Therefore, it should not be edited manually.\n', 'All changes to teams listed in maintainers/github-teams.json should be performed in GitHub by a team maintainer.\n', @@ -57,16 +48,7 @@ async function checkManualFileEdits({ github, context, core, repoPath, dry }) { (prev, curr) => prev + (!prev || prev.endsWith('\n') ? '' : ' ') + curr, '', ), - reviewKey, - }) - } else { - dismissReviews({ - github, - context, - core, - dry, - reviewKey, - }) + ) } } diff --git a/ci/github-script/prepare.js b/ci/github-script/prepare.js index d4d69eb71692..dfe2d93f3d69 100644 --- a/ci/github-script/prepare.js +++ b/ci/github-script/prepare.js @@ -172,20 +172,14 @@ module.exports = async ({ github, context, core, dry }) => { ' ```', ].join('\n') - await postReview({ - github, - context, - core, - dry, - body, - event: 'REQUEST_CHANGES', - reviewKey, - }) + await postReview({ github, context, core, dry, body, reviewKey }) + + throw new Error(`The PR contains commits from a different base.`) } - } else { - await dismissReviews({ github, context, core, dry, reviewKey }) } + await dismissReviews({ github, context, core, dry, reviewKey }) + let mergedSha, targetSha if (prInfo.mergeable) { diff --git a/ci/github-script/reviews.js b/ci/github-script/reviews.js index 80e250cfa7c3..7041fdae1f10 100644 --- a/ci/github-script/reviews.js +++ b/ci/github-script/reviews.js @@ -5,28 +5,10 @@ const eventToState = { REQUEST_CHANGES: 'CHANGES_REQUESTED', } -// Use substring checks in order to allow testing in forks -// Usernames must also end in "[bot]" -const reviewUsers = [ - 'github-actions', - 'nixpkgs-ci', - 'branch-check', - 'commit-check', - 'manual-edit', -] - -/** - * @typedef {InstanceType} GitHub - * @typedef {typeof import('@actions/github').context} Context - * - * @typedef {Awaited>['data'][number]} Review - * @typedef {Review & { user: NonNullable }} ReviewWithNonNullUser - */ - /** * @param {{ - * github: GitHub, - * context: Context, + * github: InstanceType, + * context: import('@actions/github/lib/context').Context, * core: import('@actions/core'), * dry: boolean, * reviewKey?: string, @@ -43,32 +25,18 @@ async function dismissReviews({ github, context, core, dry, reviewKey }) { return } - const allReviews = await github.paginate(github.rest.pulls.listReviews, { - ...context.repo, - pull_number, - }) - - const reviews = /** @type {ReviewWithNonNullUser[]} */ ( - allReviews.filter( - (review) => - review.user && - review.state !== 'DISMISSED' && - review.user.login.endsWith('[bot]') && - reviewUsers.some((substr) => review.user?.login.includes(substr)), - ) + const reviews = ( + await github.paginate(github.rest.pulls.listReviews, { + ...context.repo, + pull_number, + }) + ).filter( + (review) => + review.user?.login === 'github-actions[bot]' && + review.state !== 'DISMISSED', ) - - const reviewsByUser = reviews.reduce( - (prev, curr) => { - if (!(curr.user.login in prev)) { - prev[curr.user.login] = [] - } - - prev[curr.user.login].push(curr) - - return prev - }, - /** @type {Record } */ ({}), + const changesRequestedReviews = reviews.filter( + (review) => review.state === 'CHANGES_REQUESTED', ) const commentRegex = new RegExp( @@ -82,8 +50,8 @@ async function dismissReviews({ github, context, core, dry, reviewKey }) { ) let reviewsToMinimize = reviews - const /** @type {ReviewWithNonNullUser[]} */ reviewsToDismiss = [] - const /** @type {ReviewWithNonNullUser[]} */ reviewsToResolve = [] + let /** @type {typeof reviews} */ reviewsToDismiss = [] + let /** @type {typeof reviews} */ reviewsToResolve = [] if (reviewKey && reviews.every((review) => commentRegex.test(review.body))) { reviewsToMinimize = reviews.filter((review) => @@ -91,39 +59,29 @@ async function dismissReviews({ github, context, core, dry, reviewKey }) { ) } - for (const reviewsForUser of Object.values(reviewsByUser)) { - // Make sure that we don't dismiss all reviews by a user if they - // have any reviews we don't want to dismiss. - if ( - reviewsForUser.every( - (review) => - commentResolvedRegex.test(review.body) || - (reviewKey && reviewKeyRegex.test(review.body)) || - // If we are called by check-commits and the review body is clearly - // from `commits.js`, then we can safely dismiss the review. - // This helps with pre-existing reviews (before the comments were added). - (reviewKey && - reviewKey === 'check-commits' && - review.body.includes('PR / Check / cherry-pick')), - ) - ) { - reviewsToDismiss.push( - ...reviewsForUser.filter( - (review) => review.state === 'CHANGES_REQUESTED', - ), - ) - } else { - reviewsToResolve.push( - ...reviewsForUser.filter( - (review) => - review.state === 'CHANGES_REQUESTED' && - !commentResolvedRegex.test(review.body) && - reviewsToMinimize.some( - (toMinimize) => toMinimize.node_id === review.node_id, - ), - ), - ) - } + // If we want to dismiss all reviews with the key reviewKey, + // but there are other requested changes from CI, we can't dismiss, + // because then the other requested changes will be dismissed too. + if ( + changesRequestedReviews.every( + (review) => + commentResolvedRegex.test(review.body) || + (reviewKey && reviewKeyRegex.test(review.body)) || + // If we are called by check-commits and the review body is clearly + // from `commits.js`, then we can safely dismiss the review. + // This helps with pre-existing reviews (before the comments were added). + (reviewKey && + reviewKey === 'check-commits' && + review.body.includes('PR / Check / cherry-pick')), + ) + ) { + reviewsToDismiss = changesRequestedReviews + } else if (reviewsToMinimize.length) { + reviewsToResolve = reviewsToMinimize.filter( + (review) => + review.state === 'CHANGES_REQUESTED' && + !commentResolvedRegex.test(review.body), + ) } await Promise.all([ @@ -163,8 +121,8 @@ async function dismissReviews({ github, context, core, dry, reviewKey }) { /** * @param {{ - * github: GitHub, - * context: Context, + * github: InstanceType, + * context: import('@actions/github/lib/context').Context * core: import('@actions/core'), * dry: boolean, * body: string, @@ -200,13 +158,11 @@ async function postReview({ }) ).filter( (review) => - review.user && - review.state !== 'DISMISSED' && - review.user.login.endsWith('[bot]') && - reviewUsers.some((substr) => review.user?.login.includes(substr)), + review.user?.login === 'github-actions[bot]' && + review.state !== 'DISMISSED', ) - /** @type {null | Review} */ + /** @type {null | typeof reviews[number]} */ let pendingReview const matchingReviews = reviews.filter((review) => reviewKeyRegex.test(review.body),