diff --git a/ci/github-script/check-target-branch.js b/ci/github-script/check-target-branch.js index 2b671715d37d..47e36b9cb3f6 100644 --- a/ci/github-script/check-target-branch.js +++ b/ci/github-script/check-target-branch.js @@ -6,8 +6,9 @@ const { classify, split } = require('../supportedBranches.js') const { readFile } = require('node:fs/promises') -const { postReview } = require('./reviews.js') +const { postReview, dismissReviews } = require('./reviews.js') +const reviewKey = 'check-target-branch' /** * @param {{ * github: InstanceType, @@ -17,6 +18,19 @@ const { postReview } = require('./reviews.js') * }} CheckTargetBranchProps */ async function checkTargetBranch({ github, context, core, dry }) { + /** + * @type {{ + * attrdiff: { + * added: string[], + * changed: string[], + * removed: string[], + * }, + * labels: Record, + * rebuildCountByKernel: Record, + * rebuildsByKernel: Record, + * rebuildsByPlatform: Record, + * }} + */ const changed = JSON.parse( await readFile('comparison/changed-paths.json', 'utf-8'), ) @@ -43,6 +57,15 @@ async function checkTargetBranch({ github, context, core, dry }) { core.info( `Skipping checkTargetBranch: PR is from a development branch (${head})`, ) + + await dismissReviews({ + github, + context, + core, + dry, + reviewKey, + }) + return } // Don't run on PRs against staging branches, wip branches, haskell-updates, etc. @@ -50,6 +73,15 @@ async function checkTargetBranch({ github, context, core, dry }) { core.info( `Skipping checkTargetBranch: PR is against a non-primary base branch (${base})`, ) + + await dismissReviews({ + github, + context, + core, + dry, + reviewKey, + }) + return } @@ -57,12 +89,40 @@ async function checkTargetBranch({ github, context, core, dry }) { ...Object.values(changed.rebuildCountByKernel), ) const rebuildsAllTests = - changed.attrdiff.changed.includes('nixosTests.simple') ?? false + changed.attrdiff.changed.includes('nixosTests.simple') + + // https://github.com/NixOS/nixpkgs/pull/481205#issuecomment-3790123921 + // These should go to staging-nixos instead of master, + // but release-xx.xx (not staging-xx.xx) when backported + let isExemptKernelUpdate = false + if (prInfo.changed_files === 1 && base.startsWith('release-')) { + const changedFiles = ( + await github.rest.pulls.listFiles({ + ...context.repo, + pull_number, + }) + ).data + isExemptKernelUpdate = + changedFiles.length === 1 && + changedFiles[0].filename === + 'pkgs/os-specific/linux/kernel/kernels-org.json' + } + + // https://github.com/NixOS/nixpkgs/pull/483194#issuecomment-3793393218 + const isExemptHomeAssistantUpdate = + maxRebuildCount <= 1500 && head === 'wip-home-assistant' + core.info( - `checkTargetBranch: PR causes ${maxRebuildCount} rebuilds and ${rebuildsAllTests ? 'does' : 'does not'} rebuild all NixOS tests.`, + [ + `checkTargetBranch: this PR:`, + ` * causes ${maxRebuildCount} rebuilds`, + ` * ${rebuildsAllTests ? 'rebuilds' : 'does not rebuild'} all NixOS tests`, + ` * ${isExemptKernelUpdate ? 'is' : 'is not'} an exempt kernel update`, + ` * ${isExemptHomeAssistantUpdate ? 'is' : 'is not'} an exempt home-assistant update`, + ].join('\n'), ) - if (maxRebuildCount >= 1000) { + if (maxRebuildCount >= 1000 && !isExemptHomeAssistantUpdate) { const desiredBranch = base === 'master' ? 'staging' : `staging-${split(base).version}` const body = [ @@ -77,11 +137,12 @@ 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) { + } else if (rebuildsAllTests && !isExemptKernelUpdate) { let branchText if (base === 'master' && maxRebuildCount >= 500) { branchText = '(probably either `staging-nixos` or `staging`)' @@ -104,11 +165,16 @@ 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) { + } else if ( + maxRebuildCount >= 500 && + !isExemptKernelUpdate && + !isExemptHomeAssistantUpdate + ) { const stagingBranch = base === 'master' ? 'staging' : `staging-${split(base).version}` const body = [ @@ -125,10 +191,18 @@ async function checkTargetBranch({ github, context, core, dry }) { dry, body, event: 'COMMENT', + reviewKey, }) } else { - // Any existing reviews were dismissed by commits.js core.info('checkTargetBranch: this PR is against an appropriate branch.') + + await dismissReviews({ + github, + context, + core, + dry, + reviewKey, + }) } } diff --git a/ci/github-script/commits.js b/ci/github-script/commits.js index 91cfe00fa58c..56873c158af4 100644 --- a/ci/github-script/commits.js +++ b/ci/github-script/commits.js @@ -3,6 +3,7 @@ module.exports = async ({ github, context, core, dry, cherryPicks }) => { const { classify } = require('../supportedBranches.js') const withRateLimit = require('./withRateLimit.js') const { dismissReviews, postReview } = require('./reviews.js') + const reviewKey = 'check-commits' await withRateLimit({ github, core }, async (stats) => { stats.prs = 1 @@ -193,7 +194,7 @@ module.exports = async ({ github, context, core, dry, cherryPicks }) => { // An empty results array will always trigger this condition, which is helpful // to clean up reviews created by the prepare step when on the wrong branch. if (results.every(({ severity }) => severity === 'info')) { - await dismissReviews({ github, context, dry }) + await dismissReviews({ github, context, dry, reviewKey }) return } @@ -316,6 +317,6 @@ module.exports = async ({ github, context, core, dry, cherryPicks }) => { // Posting a review could fail for very long comments. This can only happen with // multiple commits all hitting the truncation limit for the diff. If you ever hit // this case, consider just splitting up those commits into multiple PRs. - await postReview({ github, context, core, dry, body }) + await postReview({ github, context, core, dry, body, reviewKey }) }) } diff --git a/ci/github-script/prepare.js b/ci/github-script/prepare.js index ce058edd2af7..005626c43ab5 100644 --- a/ci/github-script/prepare.js +++ b/ci/github-script/prepare.js @@ -1,5 +1,6 @@ const { classify } = require('../supportedBranches.js') -const { postReview } = require('./reviews.js') +const { postReview, dismissReviews } = require('./reviews.js') +const reviewKey = 'prepare' module.exports = async ({ github, context, core, dry }) => { const pull_number = context.payload.pull_request.number @@ -46,7 +47,7 @@ module.exports = async ({ github, context, core, dry }) => { `Please target \`${correctBranch}\` instead.`, ].join('\n') - await postReview({ github, context, core, dry, body }) + await postReview({ github, context, core, dry, body, reviewKey }) throw new Error('The PR targets a channel branch.') } @@ -170,12 +171,14 @@ module.exports = async ({ github, context, core, dry }) => { ' ```', ].join('\n') - await postReview({ github, context, core, dry, body }) + await postReview({ github, context, core, dry, body, reviewKey }) throw new Error(`The PR contains commits from a different base.`) } } + 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 e2880f3c5a01..0dacaa814e62 100644 --- a/ci/github-script/reviews.js +++ b/ci/github-script/reviews.js @@ -1,3 +1,5 @@ +// @ts-check + const eventToState = { COMMENT: 'COMMENTED', REQUEST_CHANGES: 'CHANGES_REQUESTED', @@ -6,49 +8,122 @@ const eventToState = { /** * @param {{ * github: InstanceType, - * context: import('@actions/github/lib/context').Context - * core: import('@actions/core') - * dry: boolean - * }} CheckTargetBranchProps + * context: import('@actions/github/lib/context').Context, + * core: import('@actions/core'), + * dry: boolean, + * reviewKey?: string, + * }} DismissReviewsProps */ -async function dismissReviews({ github, context, dry }) { - const pull_number = context.payload.pull_request.number +async function dismissReviews({ github, context, core, dry, reviewKey }) { + const pull_number = context.payload.pull_request?.number + if (!pull_number) { + core.warning('dismissReviews called outside of pull_request context') + return + } if (dry) { return } - await Promise.all( - ( - await github.paginate(github.rest.pulls.listReviews, { + 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 changesRequestedReviews = reviews.filter( + (review) => review.state === 'CHANGES_REQUESTED', + ) + + const commentRegex = new RegExp( + //, + ) + const reviewKeyRegex = new RegExp( + ``, + ) + const commentResolvedRegex = new RegExp( + //, + ) + + let reviewsToMinimize = reviews + let /** @type {typeof reviews} */ reviewsToDismiss = [] + let /** @type {typeof reviews} */ reviewsToResolve = [] + + if (reviewKey && reviews.every((review) => commentRegex.test(review.body))) { + reviewsToMinimize = reviews.filter((review) => + reviewKeyRegex.test(review.body), + ) + } + + // 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)), + ) + ) { + reviewsToDismiss = changesRequestedReviews + } else if (reviewsToMinimize.length) { + reviewsToResolve = reviewsToMinimize.filter( + (review) => + review.state === 'CHANGES_REQUESTED' && + !commentResolvedRegex.test(review.body), + ) + } + + await Promise.all([ + ...reviewsToMinimize.map(async (review) => + github.graphql( + `mutation($node_id:ID!) { + minimizeComment(input: { + classifier: OUTDATED, + subjectId: $node_id + }) + { clientMutationId } + }`, + { node_id: review.node_id }, + ), + ), + ...reviewsToDismiss.map(async (review) => + github.rest.pulls.dismissReview({ ...context.repo, pull_number, - }) - ) - .filter((review) => review.user?.login === 'github-actions[bot]') - .map(async (review) => { - if (review.state === 'CHANGES_REQUESTED') { - await github.rest.pulls.dismissReview({ - ...context.repo, - pull_number, - review_id: review.id, - message: 'Review dismissed automatically', - }) - } - await github.graphql( - `mutation($node_id:ID!) { - minimizeComment(input: { - classifier: OUTDATED, - subjectId: $node_id - }) - { clientMutationId } - }`, - { node_id: review.node_id }, - ) + review_id: review.id, + message: 'Review dismissed automatically', }), - ) + ), + ...reviewsToResolve.map(async (review) => + github.rest.pulls.updateReview({ + ...context.repo, + pull_number, + review_id: review.id, + body: review.body.replace( + reviewKeyRegex, + ``, + ), + }), + ), + ]) } +/** + * @param {{ + * github: InstanceType, + * context: import('@actions/github/lib/context').Context + * core: import('@actions/core'), + * dry: boolean, + * body: string, + * event: keyof eventToState, + * reviewKey: string, + * }} PostReviewProps + */ async function postReview({ github, context, @@ -56,22 +131,55 @@ async function postReview({ dry, body, event = 'REQUEST_CHANGES', + reviewKey, }) { - const pull_number = context.payload.pull_request.number + const pull_number = context.payload.pull_request?.number + if (!pull_number) { + core.warning('postReview called outside of pull_request context') + return + } - const pendingReview = ( + const reviewKeyRegex = new RegExp( + ``, + ) + const reviewKeyComment = `` + body = body + '\n\n' + reviewKeyComment + + const reviews = ( await github.paginate(github.rest.pulls.listReviews, { ...context.repo, pull_number, }) - ).find( + ).filter( (review) => review.user?.login === 'github-actions[bot]' && - // If a review is still pending, we can just update this instead - // of posting a new one. - review.state === eventToState[event], + review.state !== 'DISMISSED', ) + /** @type {null | typeof reviews[number]} */ + let pendingReview + const matchingReviews = reviews.filter((review) => + reviewKeyRegex.test(review.body), + ) + + if (matchingReviews.length === 0) { + pendingReview = null + } else if ( + matchingReviews.length === 1 && + matchingReviews[0].state === eventToState[event] + ) { + pendingReview = matchingReviews[0] + } else { + await dismissReviews({ + github, + context, + core, + dry, + reviewKey, + }) + pendingReview = null + } + if (dry) { if (pendingReview) core.info(`pending review found: ${pendingReview.html_url}`) @@ -79,12 +187,23 @@ async function postReview({ core.info(body) } else { if (pendingReview) { - await github.rest.pulls.updateReview({ - ...context.repo, - pull_number, - review_id: pendingReview.id, - body, - }) + await Promise.all([ + github.rest.pulls.updateReview({ + ...context.repo, + pull_number, + review_id: pendingReview.id, + body, + }), + github.graphql( + `mutation($node_id:ID!) { + unminimizeComment(input: { + subjectId: $node_id + }) + { clientMutationId } + }`, + { node_id: pendingReview.node_id }, + ), + ]) } else { await github.rest.pulls.createReview({ ...context.repo,