From 1fbcad04345d37eb50aadac2d8eaf876a9a7fa15 Mon Sep 17 00:00:00 2001 From: Wolfgang Walther Date: Wed, 16 Jul 2025 12:28:29 +0200 Subject: [PATCH] ci/github-script/commits: block on errors Most of the checks we do for cherry-picks are dismissable warnings, with one exception: When a commit hash has been found, but this hash is not available in any of the pickable branches, we raise this with severity=error. This should also *block* the merge and not be dismissable. That's because this is a fixable issue in every case. --- .github/workflows/check.yml | 78 +---------------------------- ci/github-script/commits.js | 97 +++++++++++++++++++++++++++++++++---- 2 files changed, 89 insertions(+), 86 deletions(-) diff --git a/.github/workflows/check.yml b/.github/workflows/check.yml index 04f38641323c..2b8dffb9f163 100644 --- a/.github/workflows/check.yml +++ b/.github/workflows/check.yml @@ -55,7 +55,6 @@ jobs: - name: Check cherry-picks id: check - continue-on-error: true uses: actions/github-script@60a0d83039c74a4aee543508d2ffcb1c3799cdea # v7.0.1 with: script: | @@ -63,84 +62,9 @@ jobs: github, context, core, + dry: context.eventName == 'pull_request', }) - - name: Request changes - if: ${{ github.event_name == 'pull_request_target' && steps.check.outcome == 'failure' }} - uses: actions/github-script@60a0d83039c74a4aee543508d2ffcb1c3799cdea # v7.0.1 - with: - script: | - const { readFile } = require('node:fs/promises') - const body = await readFile('review.md', 'utf-8') - - const pendingReview = (await github.paginate(github.rest.pulls.listReviews, { - owner: context.repo.owner, - repo: context.repo.repo, - pull_number: context.payload.pull_request.number - })).find(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 == 'CHANGES_REQUESTED' || - // No need to post a new review, if an older one with the exact - // same content had already been dismissed. - review.body == body - ) - ) - - // Either of those two requests 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. - if (pendingReview) { - await github.rest.pulls.updateReview({ - owner: context.repo.owner, - repo: context.repo.repo, - pull_number: context.payload.pull_request.number, - review_id: pendingReview.id, - body - }) - } else { - await github.rest.pulls.createReview({ - owner: context.repo.owner, - repo: context.repo.repo, - pull_number: context.payload.pull_request.number, - event: 'REQUEST_CHANGES', - body - }) - } - - - name: Dismiss old reviews - if: ${{ github.event_name == 'pull_request_target' && steps.check.outcome == 'success' }} - uses: actions/github-script@60a0d83039c74a4aee543508d2ffcb1c3799cdea # v7.0.1 - with: - script: | - await Promise.all( - (await github.paginate(github.rest.pulls.listReviews, { - owner: context.repo.owner, - repo: context.repo.repo, - pull_number: context.payload.pull_request.number - })).filter(review => - review.user.login == 'github-actions[bot]' - ).map(async (review) => { - if (review.state == 'CHANGES_REQUESTED') { - await github.rest.pulls.dismissReview({ - owner: context.repo.owner, - repo: context.repo.repo, - pull_number: context.payload.pull_request.number, - review_id: review.id, - message: 'All cherry-picks are good now, thank you!' - }) - } - await github.graphql(`mutation($node_id:ID!) { - minimizeComment(input: { - classifier: RESOLVED, - subjectId: $node_id - }) - { clientMutationId } - }`, { node_id: review.node_id }) - }) - ) - - name: Log current API rate limits env: GH_TOKEN: ${{ github.token }} diff --git a/ci/github-script/commits.js b/ci/github-script/commits.js index 7ed5f94db003..5d0a904bdf55 100644 --- a/ci/github-script/commits.js +++ b/ci/github-script/commits.js @@ -1,6 +1,6 @@ -module.exports = async function ({ github, context, core }) { +module.exports = async function ({ github, context, core, dry }) { const { execFileSync } = require('node:child_process') - const { readFile, writeFile } = require('node:fs/promises') + const { readFile } = require('node:fs/promises') const { join } = require('node:path') const { classify } = require('../supportedBranches.js') const withRateLimit = require('./withRateLimit.js') @@ -8,6 +8,8 @@ module.exports = async function ({ github, context, core }) { await withRateLimit({ github, core }, async (stats) => { stats.prs = 1 + const pull_number = context.payload.pull_request.number + const job_url = context.runId && ( @@ -17,7 +19,7 @@ module.exports = async function ({ github, context, core }) { }) ).data.jobs[0].html_url + '?pr=' + - context.payload.pull_request.number + pull_number async function handle({ sha, commit }) { // Using the last line with "cherry" + hash, because a chained backport @@ -115,7 +117,7 @@ module.exports = async function ({ github, context, core }) { const commits = await github.paginate(github.rest.pulls.listCommits, { ...context.repo, - pull_number: context.payload.pull_request.number, + pull_number, }) const results = await Promise.all(commits.map(handle)) @@ -131,8 +133,46 @@ module.exports = async function ({ github, context, core }) { }) // Only create step summary below in case of warnings or errors. - if (results.every(({ severity }) => severity == 'info')) return - else process.exitCode = 1 + // Also clean up older reviews, when all checks are good now. + if (results.every(({ severity }) => severity == 'info')) { + if (!dry) { + await Promise.all( + ( + await github.paginate(github.rest.pulls.listReviews, { + ...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: 'All cherry-picks are good now, thank you!', + }) + } + await github.graphql( + `mutation($node_id:ID!) { + minimizeComment(input: { + classifier: RESOLVED, + subjectId: $node_id + }) + { clientMutationId } + }`, + { node_id: review.node_id }, + ) + }), + ) + } + return + } + + // In the case of "error" severity, we also fail the job. + // Those should be considered blocking and not be dismissable via review. + if (results.some(({ severity }) => severity == 'error')) + process.exitCode = 1 core.summary.addRaw( await readFile(join(__dirname, 'check-cherry-picks.md'), 'utf-8'), @@ -198,9 +238,48 @@ module.exports = async function ({ github, context, core }) { `\n\n_Hint: The full diffs are also available in the [runner logs](${job_url}) with slightly better highlighting._`, ) - // Write to disk temporarily for next step in GHA. - await writeFile('review.md', core.summary.stringify()) - + const body = core.summary.stringify() core.summary.write() + + const pendingReview = ( + await github.paginate(github.rest.pulls.listReviews, { + ...context.repo, + pull_number, + }) + ).find( + (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 == 'CHANGES_REQUESTED' || + // No need to post a new review, if an older one with the exact + // same content had already been dismissed. + review.body == body), + ) + + if (dry) { + if (pendingReview) + core.info('pending review found: ' + pendingReview.html_url) + else core.info('no pending review found') + } else { + // Either of those two requests 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. + if (pendingReview) { + await github.rest.pulls.updateReview({ + ...context.repo, + pull_number, + review_id: pendingReview.id, + body, + }) + } else { + await github.rest.pulls.createReview({ + ...context.repo, + pull_number, + event: 'REQUEST_CHANGES', + body, + }) + } + } }) }