From 515b174c42ca9f5fa645688d249a00db90c578a0 Mon Sep 17 00:00:00 2001 From: Wolfgang Walther Date: Wed, 28 May 2025 21:16:01 +0200 Subject: [PATCH] workflows/check-cherry-picks: post review comments Instead of failing the job, the workflow will now post review comments as "Request Changes". This makes the feedback more readily visible and avoids having to merge despite a failing CI job. It is also a pre-requisite to enable required status checks / required workflows in the future. Committers are asked to confirm the differences by explicitly dismissing the generated review. After dismissal, the related review comment will automatically be marked as "resolved". The comments only report warnings and errors. Reviews are automatically dismissed when they have been addressed by the author and no problems remain. If problems remain, existing, still pending, review comments will be updated. If the same problems had already been dismissed earlier, no new review comment will be created either. --- .github/workflows/check-cherry-picks.yml | 102 ++++++++++++++++++++++- .github/workflows/dismissed-review.yml | 30 +++++++ ci/check-cherry-picks.md | 7 ++ ci/check-cherry-picks.sh | 37 ++++++-- 4 files changed, 165 insertions(+), 11 deletions(-) create mode 100644 .github/workflows/dismissed-review.yml create mode 100644 ci/check-cherry-picks.md diff --git a/.github/workflows/check-cherry-picks.yml b/.github/workflows/check-cherry-picks.yml index 3c6ef1d18920..b44027e326d2 100644 --- a/.github/workflows/check-cherry-picks.yml +++ b/.github/workflows/check-cherry-picks.yml @@ -10,7 +10,8 @@ on: - 'staging-**' - '!staging-next' -permissions: {} +permissions: + pull-requests: write jobs: check: @@ -24,8 +25,105 @@ jobs: path: trusted - name: Check cherry-picks + id: check + continue-on-error: true env: BASE_SHA: ${{ github.event.pull_request.base.sha }} HEAD_SHA: ${{ github.event.pull_request.head.sha }} run: | - ./trusted/ci/check-cherry-picks.sh "$BASE_SHA" "$HEAD_SHA" + ./trusted/ci/check-cherry-picks.sh "$BASE_SHA" "$HEAD_SHA" checked-cherry-picks.md + + - name: Prepare review + if: steps.check.outcome == 'failure' + uses: actions/github-script@60a0d83039c74a4aee543508d2ffcb1c3799cdea # v7.0.1 + with: + script: | + const { readFile, writeFile } = require('node:fs/promises') + + const job_url = (await github.rest.actions.listJobsForWorkflowRun({ + owner: context.repo.owner, + repo: context.repo.repo, + run_id: context.runId + })).data.jobs[0].html_url + '?pr=' + context.payload.pull_request.number + + const header = await readFile('trusted/ci/check-cherry-picks.md') + const body = await readFile('checked-cherry-picks.md') + const footer = + `\n_Hint: The diffs are also available in the [runner logs](${job_url}) with slightly better highlighting._` + + const review = header + body + footer + await writeFile('review.md', review) + core.summary.addRaw(review) + core.summary.write() + + - 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 + ) + ) + + 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]' && + review.state == 'CHANGES_REQUESTED' + ).map(async (review) => { + 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 }) + }) + ) diff --git a/.github/workflows/dismissed-review.yml b/.github/workflows/dismissed-review.yml new file mode 100644 index 000000000000..9886dad2b74e --- /dev/null +++ b/.github/workflows/dismissed-review.yml @@ -0,0 +1,30 @@ +name: Dismissed Review + +on: + pull_request_review: + types: [dismissed] + +permissions: + pull-requests: write + +jobs: + # The check-cherry-picks workflow creates review comments, + # that should sometimes be manually dismissed. + # When a CI-generated review is dismissed, this job automatically + # minimizes it, to prevent it from cluttering the PR. + minimize: + name: Minimize as resolved + if: github.event.review.user.login == 'github-actions[bot]' + runs-on: ubuntu-24.04-arm + steps: + - uses: actions/github-script@60a0d83039c74a4aee543508d2ffcb1c3799cdea # v7.0.1 + with: + script: | + await github.graphql(`mutation($node_id:ID!) { + minimizeComment(input: { + classifier: RESOLVED, + subjectId: $node_id + }) + { clientMutationId } + }`, { node_id: context.payload.review.node_id }) + diff --git a/ci/check-cherry-picks.md b/ci/check-cherry-picks.md new file mode 100644 index 000000000000..5af41aadac5e --- /dev/null +++ b/ci/check-cherry-picks.md @@ -0,0 +1,7 @@ +This report is automatically generated by the `check-cherry-picks` CI workflow. + +Some of the commits in this PR have not been cherry-picked exactly and require the author's and reviewer's attention. + +Please make sure to follow the [backporting guidelines](https://github.com/NixOS/nixpkgs/blob/master/CONTRIBUTING.md#how-to-backport-pull-requests) and cherry-pick with the `-x` flag. This requires changes to go to the unstable branches (`master` / `staging`) first, before backporting them. + +Occasionally, it is not possible to cherry-pick exactly the same patch. This most frequently happens when resolving merge conflicts while cherry-picking or when updating minor versions of packages which have already advanced to the next major on unstable. If you need to merge this PR despite the warnings, please [dismiss](https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/reviewing-changes-in-pull-requests/dismissing-a-pull-request-review) this review. diff --git a/ci/check-cherry-picks.sh b/ci/check-cherry-picks.sh index f00011915dcd..e0b278207875 100755 --- a/ci/check-cherry-picks.sh +++ b/ci/check-cherry-picks.sh @@ -3,11 +3,14 @@ set -euo pipefail -if [ $# != "2" ] ; then - echo "usage: check-cherry-picks.sh base_rev head_rev" +if [[ $# != "2" && $# != "3" ]] ; then + echo "usage: check-cherry-picks.sh base_rev head_rev [markdown_file]" exit 2 fi +markdown_file="$(realpath ${3:-/dev/null})" +[ -v 3 ] && rm -f "$markdown_file" + # Make sure we are inside the nixpkgs repo, even when called from outside cd "$(dirname "${BASH_SOURCE[0]}")" @@ -34,6 +37,18 @@ log() { fi echo "${prefix[$type]}$@" + + # Only logging errors and warnings, which allows comparing the markdown file + # between pushes to the PR. Even if a new, proper cherry-pick, commit is added + # it won't change the markdown file's content and thus not trigger another comment. + if [ "$type" != "success" ]; then + local -A alert + alert[warning]="WARNING" + alert[error]="CAUTION" + echo >> $markdown_file + echo "> [!${alert[$type]}]" >> $markdown_file + echo "> $@" >> $markdown_file + fi } endgroup() { @@ -58,9 +73,7 @@ while read -r new_commit_sha ; do ) if [ -z "$original_commit_sha" ] ; then endgroup - log error "Couldn't locate original commit hash in message" - echo "Note this should not necessarily be treated as a hard fail, but a reviewer's attention should" \ - "be drawn to it and github actions have no way of doing that but to raise a 'failure'" + log warning "Couldn't locate original commit hash in message of $new_commit_sha." problem=1 continue fi @@ -90,13 +103,19 @@ while read -r new_commit_sha ; do if $range_diff_common --no-color 2> /dev/null | grep -E '^ {4}[+-]{2}' > /dev/null ; then log success "$original_commit_sha present in branch $picked_branch" endgroup - log warning "Difference between $new_commit_sha and original $original_commit_sha may warrant inspection:" + log warning "Difference between $new_commit_sha and original $original_commit_sha may warrant inspection." # First line contains commit SHAs, which we already printed. $range_diff_common --color | tail -n +2 - echo "Note this should not necessarily be treated as a hard fail, but a reviewer's attention should" \ - "be drawn to it and github actions have no way of doing that but to raise a 'failure'" + echo -e ">
Show diff\n>" >> $markdown_file + echo '> ```diff' >> $markdown_file + # The output of `git range-diff` is indented with 4 spaces, which we need to match with the + # code blocks indent to get proper syntax highlighting on GitHub. + $range_diff_common | tail -n +2 | sed -Ee 's/^ {4}/> /g' >> $markdown_file + echo '> ```' >> $markdown_file + echo ">
" >> $markdown_file + problem=1 else log success "$original_commit_sha present in branch $picked_branch" @@ -112,7 +131,7 @@ while read -r new_commit_sha ; do done endgroup - log error "$original_commit_sha not found in any pickable branch" + log error "$original_commit_sha given in $new_commit_sha not found in any pickable branch." problem=1 done <<< "$commits"