From b7759aafd28e745fef95ae66f1c59e85250c20b8 Mon Sep 17 00:00:00 2001 From: Wolfgang Walther Date: Sun, 8 Jun 2025 18:51:00 +0200 Subject: [PATCH 1/7] workflows/review-{dismissed,minimize}: init This brings back the "minimize CI reviews after dismissal" job that was previously removed. The first time around, we had a single job triggered by the `pull_request_review` event. This lacks permission to do meaningful stuff, though. This time, we trigger an empty no-op job on `pull_request_review` and then run a second workflow on `workflow_run`. This can run with the proper permissions. (cherry picked from commit a34a22d8b9529df4e92e1e8c3670ac8da2542ab1) --- .github/workflows/dismissed-review.yml | 65 ++++++++++++++++++++++++++ .github/workflows/review-dismissed.yml | 17 +++++++ 2 files changed, 82 insertions(+) create mode 100644 .github/workflows/dismissed-review.yml create mode 100644 .github/workflows/review-dismissed.yml diff --git a/.github/workflows/dismissed-review.yml b/.github/workflows/dismissed-review.yml new file mode 100644 index 0000000000000..256627af66167 --- /dev/null +++ b/.github/workflows/dismissed-review.yml @@ -0,0 +1,65 @@ +name: Dismissed review + +on: + workflow_run: + workflows: + - Review dismissed + types: [completed] + +concurrency: + group: ${{ github.workflow }}-${{ github.event_name }}-${{ github.head_ref || github.run_id }} + cancel-in-progress: true + +permissions: + pull-requests: write + +defaults: + run: + shell: bash + +jobs: + # The `check-cherry-picks` workflow creates review comments which reviewers + # are encouraged to manually dismiss if they're not relevant. + # When a CI-generated review is dismissed, this job automatically minimizes + # it, preventing it from cluttering the PR. + minimize: + name: Minimize as resolved + runs-on: ubuntu-24.04-arm + steps: + - uses: actions/github-script@60a0d83039c74a4aee543508d2ffcb1c3799cdea # v7.0.1 + with: + script: | + // PRs from forks don't have any PRs associated by default. + // Thus, we request the PR number with an API call *to* the fork's repo. + // Multiple pull requests can be open from the same head commit, either via + // different base branches or head branches. + const { head_repository, head_sha, repository } = context.payload.workflow_run + await Promise.all( + (await github.paginate(github.rest.repos.listPullRequestsAssociatedWithCommit, { + owner: head_repository.owner.login, + repo: head_repository.name, + commit_sha: head_sha + })) + .filter(pull_request => pull_request.base.repo.id == repository.id) + .map(async (pull_request) => + Promise.all( + (await github.paginate(github.rest.pulls.listReviews, { + owner: context.repo.owner, + repo: context.repo.repo, + pull_number: pull_request.number + })).filter(review => + review.user.login == 'github-actions[bot]' && + review.state == 'DISMISSED' + ).map(review => github.graphql(` + mutation($node_id:ID!) { + minimizeComment(input: { + classifier: RESOLVED, + subjectId: $node_id + }) + { clientMutationId } + }`, + { node_id: review.node_id } + )) + ) + ) + ) diff --git a/.github/workflows/review-dismissed.yml b/.github/workflows/review-dismissed.yml new file mode 100644 index 0000000000000..988b4a47df14b --- /dev/null +++ b/.github/workflows/review-dismissed.yml @@ -0,0 +1,17 @@ +name: Review dismissed + +on: + pull_request_review: + types: [dismissed] + +permissions: {} + +defaults: + run: + shell: bash + +jobs: + trigger: + runs-on: ubuntu-24.04-arm + steps: + - run: echo This is a no-op only used as a trigger for workflow_run. From 1a19fa145471e74ae614fd60a122bbc86b50c206 Mon Sep 17 00:00:00 2001 From: Wolfgang Walther Date: Sun, 8 Jun 2025 20:06:00 +0200 Subject: [PATCH 2/7] workflows/labels: move labels logic from eval workflow This moves the actual labeling from the eval workflow to the labels workflow. At this stage, this only has a disadvantage: Adding the topic-labels to the pull request will now only happen after eval has finished, instead of instantly. We will only benefit from this later, when we manage approval related events. With this change, we will have the comparison results and thus the package maintainer info available. (cherry picked from commit 2d0bcd7165516e90df43582667ec6c1918ba48f1) --- .github/workflows/eval.yml | 50 ++++++------------------------ .github/workflows/labels.yml | 60 ++++++++++++++++++++++++++++++++++-- 2 files changed, 67 insertions(+), 43 deletions(-) diff --git a/.github/workflows/eval.yml b/.github/workflows/eval.yml index 711932b6a02bc..edbaf78d961f9 100644 --- a/.github/workflows/eval.yml +++ b/.github/workflows/eval.yml @@ -217,46 +217,6 @@ jobs: name: comparison path: comparison/* - - name: Labelling pull request - if: ${{ github.event_name == 'pull_request_target' }} - uses: actions/github-script@60a0d83039c74a4aee543508d2ffcb1c3799cdea # v7.0.1 - with: - script: | - const { readFile } = require('node:fs/promises') - - const pr = { - owner: context.repo.owner, - repo: context.repo.repo, - issue_number: context.payload.pull_request.number - } - - // Get all currently set labels that we manage - const before = - (await github.paginate(github.rest.issues.listLabelsOnIssue, pr)) - .map(({ name }) => name) - .filter(name => name.startsWith('10.rebuild') || name == '11.by: package-maintainer') - - // And the labels that should be there - const after = JSON.parse(await readFile('comparison/changed-paths.json', 'utf-8')).labels - - // Remove the ones not needed anymore - await Promise.all( - before.filter(name => !after.includes(name)) - .map(name => github.rest.issues.removeLabel({ - ...pr, - name - })) - ) - - // And add the ones that aren't set already - const added = after.filter(name => !before.includes(name)) - if (added.length > 0) { - await github.rest.issues.addLabels({ - ...pr, - labels: added - }) - } - - name: Add eval summary to commit statuses if: ${{ github.event_name == 'pull_request_target' }} uses: actions/github-script@60a0d83039c74a4aee543508d2ffcb1c3799cdea # v7.0.1 @@ -289,6 +249,16 @@ jobs: target_url }) + labels: + name: Labels + needs: [ compare ] + uses: ./.github/workflows/labels.yml + permissions: + issues: write + pull-requests: write + with: + caller: ${{ github.workflow }} + reviewers: name: Reviewers # No dependency on "compare", so that it can start at the same time. diff --git a/.github/workflows/labels.yml b/.github/workflows/labels.yml index 1e485a6df89be..b66573e1d968a 100644 --- a/.github/workflows/labels.yml +++ b/.github/workflows/labels.yml @@ -6,14 +6,18 @@ name: "Label PR" on: - pull_request_target: + workflow_call: + inputs: + caller: + description: Name of the calling workflow. + required: true + type: string concurrency: - group: ${{ github.workflow }}-${{ github.event_name }}-${{ github.head_ref || github.run_id }} + group: ${{ inputs.caller }}-${{ github.workflow }}-${{ github.event_name }}-${{ github.head_ref || github.run_id }} cancel-in-progress: true permissions: - contents: read issues: write # needed to create *new* labels pull-requests: write @@ -27,7 +31,55 @@ jobs: runs-on: ubuntu-24.04-arm if: "!contains(github.event.pull_request.title, '[skip treewide]')" steps: + - name: Download the comparison results + uses: actions/download-artifact@fa0a91b85d4f404e444e00e005971372dc801d16 # v4.1.8 + with: + pattern: comparison + path: comparison + merge-multiple: true + + - name: Labels from eval + if: ${{ github.event_name != 'pull_request' }} + uses: actions/github-script@60a0d83039c74a4aee543508d2ffcb1c3799cdea # v7.0.1 + with: + script: | + const { readFile } = require('node:fs/promises') + + const pr = { + owner: context.repo.owner, + repo: context.repo.repo, + issue_number: context.payload.pull_request.number + } + + // Get all currently set labels that we manage + const before = + (await github.paginate(github.rest.issues.listLabelsOnIssue, pr)) + .map(({ name }) => name) + .filter(name => name.startsWith('10.rebuild') || name == '11.by: package-maintainer') + + // And the labels that should be there + const after = JSON.parse(await readFile('comparison/changed-paths.json', 'utf-8')).labels + + // Remove the ones not needed anymore + await Promise.all( + before.filter(name => !after.includes(name)) + .map(name => github.rest.issues.removeLabel({ + ...pr, + name + })) + ) + + // And add the ones that aren't set already + const added = after.filter(name => !before.includes(name)) + if (added.length > 0) { + await github.rest.issues.addLabels({ + ...pr, + labels: added + }) + } + - uses: actions/labeler@8558fd74291d67161a8a78ce36a881fa63b766a9 # v5.0.0 + name: Labels from touched files if: | github.event.pull_request.head.repo.owner.login != 'NixOS' || !( github.head_ref == 'haskell-updates' || @@ -40,6 +92,7 @@ jobs: configuration-path: .github/labeler.yml # default sync-labels: true - uses: actions/labeler@8558fd74291d67161a8a78ce36a881fa63b766a9 # v5.0.0 + name: Labels from touched files (no sync) if: | github.event.pull_request.head.repo.owner.login != 'NixOS' || !( github.head_ref == 'haskell-updates' || @@ -52,6 +105,7 @@ jobs: configuration-path: .github/labeler-no-sync.yml sync-labels: false - uses: actions/labeler@8558fd74291d67161a8a78ce36a881fa63b766a9 # v5.0.0 + name: Labels from touched files (development branches) # Development branches like staging-next, haskell-updates and python-updates get special labels. # This is to avoid the mass of labels there, which is mostly useless - and really annoying for # the backport labels. From cf25ce07e8005927ff40c01756a8dc096645d3bd Mon Sep 17 00:00:00 2001 From: Wolfgang Walther Date: Sun, 8 Jun 2025 21:54:48 +0200 Subject: [PATCH 3/7] workflows/labels: manage approval labels The category 12 labels for number of approvals and approved by package maintainer are now automatically managed by the labels workflow. The logic is slightly different from the "by: package-maintainer" label. For approval, it's enough if *any one* maintainer approves the PR to have the label added, even if the PR touches multiple packages. The workflow only counts approved reviews, no matter whether there had been a push in the meantime or not. To achieve the currently used logic of "expiring approvals after push", we will have to set up a branch protection rule, which actually dismissed those reviews automatically. (cherry picked from commit 5f09e16f001a875e6fe7060dd49062bfe2312d76) --- .github/workflows/labels.yml | 120 +++++++++++++++++++------ .github/workflows/review-submitted.yml | 17 ++++ 2 files changed, 110 insertions(+), 27 deletions(-) create mode 100644 .github/workflows/review-submitted.yml diff --git a/.github/workflows/labels.yml b/.github/workflows/labels.yml index b66573e1d968a..950b5f07f7758 100644 --- a/.github/workflows/labels.yml +++ b/.github/workflows/labels.yml @@ -12,6 +12,11 @@ on: description: Name of the calling workflow. required: true type: string + workflow_run: + workflows: + - Review dismissed + - Review submitted + types: [completed] concurrency: group: ${{ inputs.caller }}-${{ github.workflow }}-${{ github.event_name }}-${{ github.head_ref || github.run_id }} @@ -31,56 +36,113 @@ jobs: runs-on: ubuntu-24.04-arm if: "!contains(github.event.pull_request.title, '[skip treewide]')" steps: + - uses: actions/github-script@60a0d83039c74a4aee543508d2ffcb1c3799cdea # v7.0.1 + id: eval + with: + script: | + const run_id = (await github.rest.actions.listWorkflowRuns({ + owner: context.repo.owner, + repo: context.repo.repo, + workflow_id: 'eval.yml', + event: 'pull_request_target', + head_sha: context.payload.pull_request?.head.sha ?? context.payload.workflow_run.head_sha + })).data.workflow_runs[0]?.id + core.setOutput('run-id', run_id) + - name: Download the comparison results + if: steps.eval.outputs.run-id uses: actions/download-artifact@fa0a91b85d4f404e444e00e005971372dc801d16 # v4.1.8 with: + run-id: ${{ steps.eval.outputs.run-id }} + github-token: ${{ github.token }} pattern: comparison path: comparison merge-multiple: true - name: Labels from eval - if: ${{ github.event_name != 'pull_request' }} + if: steps.eval.outputs.run-id && github.event_name != 'pull_request' uses: actions/github-script@60a0d83039c74a4aee543508d2ffcb1c3799cdea # v7.0.1 with: script: | const { readFile } = require('node:fs/promises') - const pr = { - owner: context.repo.owner, - repo: context.repo.repo, - issue_number: context.payload.pull_request.number + let pull_requests + if (context.payload.workflow_run) { + // PRs from forks don't have any PRs associated by default. + // Thus, we request the PR number with an API call *to* the fork's repo. + // Multiple pull requests can be open from the same head commit, either via + // different base branches or head branches. + const { head_repository, head_sha, repository } = context.payload.workflow_run + pull_requests = (await github.paginate(github.rest.repos.listPullRequestsAssociatedWithCommit, { + owner: head_repository.owner.login, + repo: head_repository.name, + commit_sha: head_sha + })).filter(pull_request => pull_request.base.repo.id == repository.id) + } else { + pull_requests = [ context.payload.pull_request ] } - // Get all currently set labels that we manage - const before = - (await github.paginate(github.rest.issues.listLabelsOnIssue, pr)) - .map(({ name }) => name) - .filter(name => name.startsWith('10.rebuild') || name == '11.by: package-maintainer') + await Promise.all( + pull_requests.map(async (pull_request) => { + const pr = { + owner: context.repo.owner, + repo: context.repo.repo, + issue_number: pull_request.number + } - // And the labels that should be there - const after = JSON.parse(await readFile('comparison/changed-paths.json', 'utf-8')).labels + // Get all currently set labels that we manage + const before = + (await github.paginate(github.rest.issues.listLabelsOnIssue, pr)) + .map(({ name }) => name) + .filter(name => + name.startsWith('10.rebuild') || + name == '11.by: package-maintainer' || + name.startsWith('12.approvals:') || + name == '12.approved-by: package-maintainer' + ) - // Remove the ones not needed anymore - await Promise.all( - before.filter(name => !after.includes(name)) - .map(name => github.rest.issues.removeLabel({ - ...pr, - name - })) - ) + const approvals = + (await github.paginate(github.rest.pulls.listReviews, { + owner: context.repo.owner, + repo: context.repo.repo, + pull_number: pull_request.number + })) + .filter(review => review.state == 'APPROVED') + .map(review => review.user.id) + + const maintainers = Object.keys( + JSON.parse(await readFile('comparison/maintainers.json', 'utf-8')) + ) + + // And the labels that should be there + const after = JSON.parse(await readFile('comparison/changed-paths.json', 'utf-8')).labels + if (approvals.length > 0) after.push(`12.approvals: ${approvals.length > 2 ? '3+' : approvals.length}`) + if (maintainers.some(id => approvals.includes(id))) after.push('12.approved-by: package-maintainer') + + // Remove the ones not needed anymore + await Promise.all( + before.filter(name => !after.includes(name)) + .map(name => github.rest.issues.removeLabel({ + ...pr, + name + })) + ) - // And add the ones that aren't set already - const added = after.filter(name => !before.includes(name)) - if (added.length > 0) { - await github.rest.issues.addLabels({ - ...pr, - labels: added + // And add the ones that aren't set already + const added = after.filter(name => !before.includes(name)) + if (added.length > 0) { + await github.rest.issues.addLabels({ + ...pr, + labels: added + }) + } }) - } + ) - uses: actions/labeler@8558fd74291d67161a8a78ce36a881fa63b766a9 # v5.0.0 name: Labels from touched files if: | + github.event_name != 'workflow_run' && github.event.pull_request.head.repo.owner.login != 'NixOS' || !( github.head_ref == 'haskell-updates' || github.head_ref == 'python-updates' || @@ -91,9 +153,11 @@ jobs: repo-token: ${{ secrets.GITHUB_TOKEN }} configuration-path: .github/labeler.yml # default sync-labels: true + - uses: actions/labeler@8558fd74291d67161a8a78ce36a881fa63b766a9 # v5.0.0 name: Labels from touched files (no sync) if: | + github.event_name != 'workflow_run' && github.event.pull_request.head.repo.owner.login != 'NixOS' || !( github.head_ref == 'haskell-updates' || github.head_ref == 'python-updates' || @@ -104,12 +168,14 @@ jobs: repo-token: ${{ secrets.GITHUB_TOKEN }} configuration-path: .github/labeler-no-sync.yml sync-labels: false + - uses: actions/labeler@8558fd74291d67161a8a78ce36a881fa63b766a9 # v5.0.0 name: Labels from touched files (development branches) # Development branches like staging-next, haskell-updates and python-updates get special labels. # This is to avoid the mass of labels there, which is mostly useless - and really annoying for # the backport labels. if: | + github.event_name != 'workflow_run' && github.event.pull_request.head.repo.owner.login == 'NixOS' && ( github.head_ref == 'haskell-updates' || github.head_ref == 'python-updates' || diff --git a/.github/workflows/review-submitted.yml b/.github/workflows/review-submitted.yml new file mode 100644 index 0000000000000..69663054f15b0 --- /dev/null +++ b/.github/workflows/review-submitted.yml @@ -0,0 +1,17 @@ +name: Review submitted + +on: + pull_request_review: + types: [submitted] + +permissions: {} + +defaults: + run: + shell: bash + +jobs: + trigger: + runs-on: ubuntu-24.04-arm + steps: + - run: echo This is a no-op only used as a trigger for workflow_run. From 93ecbedbaed9f1592d6f384c7d95b8f1e0b1e010 Mon Sep 17 00:00:00 2001 From: Wolfgang Walther Date: Fri, 13 Jun 2025 16:59:36 +0200 Subject: [PATCH 4/7] workflows: prevent accidental cancelling of other PRs This can happen when two PRs run at the same time, which come from different forks, but have the same head branch name. github.head_ref is suggested by GitHub's docs, but.. that's not really useful for cases with forks. (cherry picked from commit 7ba7720b28cc03d44d5ad1f5931ad4a88e068470) --- .github/workflows/backport.yml | 2 +- .github/workflows/build.yml | 2 +- .github/workflows/check.yml | 2 +- .github/workflows/codeowners-v2.yml | 2 +- .github/workflows/dismissed-review.yml | 2 +- .github/workflows/edited.yml | 2 +- .github/workflows/eval-aliases.yml | 2 +- .github/workflows/eval.yml | 2 +- .github/workflows/labels.yml | 2 +- .github/workflows/lint.yml | 2 +- .github/workflows/reviewers.yml | 2 +- 11 files changed, 11 insertions(+), 11 deletions(-) diff --git a/.github/workflows/backport.yml b/.github/workflows/backport.yml index 9cd96cd4e73fc..c5ba0126aeb2d 100644 --- a/.github/workflows/backport.yml +++ b/.github/workflows/backport.yml @@ -10,7 +10,7 @@ on: types: [closed, labeled] concurrency: - group: ${{ github.workflow }}-${{ github.event_name }}-${{ github.head_ref || github.run_id }} + group: ${{ github.workflow }}-${{ github.event_name }}-${{ github.event.pull_request.number || github.run_id }} cancel-in-progress: true permissions: diff --git a/.github/workflows/build.yml b/.github/workflows/build.yml index d92f73eb0d3aa..5013c7da1524c 100644 --- a/.github/workflows/build.yml +++ b/.github/workflows/build.yml @@ -7,7 +7,7 @@ on: pull_request_target: concurrency: - group: ${{ github.workflow }}-${{ github.event_name }}-${{ github.head_ref || github.run_id }} + group: ${{ github.workflow }}-${{ github.event_name }}-${{ github.event.pull_request.number || github.run_id }} cancel-in-progress: true permissions: {} diff --git a/.github/workflows/check.yml b/.github/workflows/check.yml index b348bef5bfc0d..3bcac620c7ed0 100644 --- a/.github/workflows/check.yml +++ b/.github/workflows/check.yml @@ -7,7 +7,7 @@ on: pull_request_target: concurrency: - group: ${{ github.workflow }}-${{ github.event_name }}-${{ github.head_ref || github.run_id }} + group: ${{ github.workflow }}-${{ github.event_name }}-${{ github.event.pull_request.number || github.run_id }} cancel-in-progress: true permissions: {} diff --git a/.github/workflows/codeowners-v2.yml b/.github/workflows/codeowners-v2.yml index 84bbbc9806332..f8f909aff6d77 100644 --- a/.github/workflows/codeowners-v2.yml +++ b/.github/workflows/codeowners-v2.yml @@ -30,7 +30,7 @@ on: types: [opened, ready_for_review, synchronize, reopened] concurrency: - group: ${{ github.workflow }}-${{ github.event_name }}-${{ github.head_ref || github.run_id }} + group: ${{ github.workflow }}-${{ github.event_name }}-${{ github.event.pull_request.number || github.run_id }} cancel-in-progress: true permissions: {} diff --git a/.github/workflows/dismissed-review.yml b/.github/workflows/dismissed-review.yml index 256627af66167..82cfe75dc3fc3 100644 --- a/.github/workflows/dismissed-review.yml +++ b/.github/workflows/dismissed-review.yml @@ -7,7 +7,7 @@ on: types: [completed] concurrency: - group: ${{ github.workflow }}-${{ github.event_name }}-${{ github.head_ref || github.run_id }} + group: ${{ github.workflow }}-${{ github.event_name }}-${{ github.event.pull_request.number || github.run_id }} cancel-in-progress: true permissions: diff --git a/.github/workflows/edited.yml b/.github/workflows/edited.yml index 186bd9cb8a0c6..23e7afb624a14 100644 --- a/.github/workflows/edited.yml +++ b/.github/workflows/edited.yml @@ -17,7 +17,7 @@ on: types: [edited] concurrency: - group: ${{ github.workflow }}-${{ github.event_name }}-${{ github.head_ref || github.run_id }} + group: ${{ github.workflow }}-${{ github.event_name }}-${{ github.event.pull_request.number || github.run_id }} cancel-in-progress: true permissions: {} diff --git a/.github/workflows/eval-aliases.yml b/.github/workflows/eval-aliases.yml index 913341d6c8148..91930ea3d6e3c 100644 --- a/.github/workflows/eval-aliases.yml +++ b/.github/workflows/eval-aliases.yml @@ -7,7 +7,7 @@ on: pull_request_target: concurrency: - group: ${{ github.workflow }}-${{ github.event_name }}-${{ github.head_ref || github.run_id }} + group: ${{ github.workflow }}-${{ github.event_name }}-${{ github.event.pull_request.number || github.run_id }} cancel-in-progress: true permissions: {} diff --git a/.github/workflows/eval.yml b/.github/workflows/eval.yml index edbaf78d961f9..447561dca73c0 100644 --- a/.github/workflows/eval.yml +++ b/.github/workflows/eval.yml @@ -17,7 +17,7 @@ on: - python-updates concurrency: - group: ${{ github.workflow }}-${{ github.event_name }}-${{ github.head_ref || github.run_id }} + group: ${{ github.workflow }}-${{ github.event_name }}-${{ github.event.pull_request.number || github.run_id }} cancel-in-progress: true permissions: {} diff --git a/.github/workflows/labels.yml b/.github/workflows/labels.yml index 950b5f07f7758..225c913c08b22 100644 --- a/.github/workflows/labels.yml +++ b/.github/workflows/labels.yml @@ -19,7 +19,7 @@ on: types: [completed] concurrency: - group: ${{ inputs.caller }}-${{ github.workflow }}-${{ github.event_name }}-${{ github.head_ref || github.run_id }} + group: ${{ inputs.caller }}-${{ github.workflow }}-${{ github.event_name }}-${{ github.event.pull_request.number || github.run_id }} cancel-in-progress: true permissions: diff --git a/.github/workflows/lint.yml b/.github/workflows/lint.yml index c26160ba8f07d..b5ed0c4323c4f 100644 --- a/.github/workflows/lint.yml +++ b/.github/workflows/lint.yml @@ -7,7 +7,7 @@ on: pull_request_target: concurrency: - group: ${{ github.workflow }}-${{ github.event_name }}-${{ github.head_ref || github.run_id }} + group: ${{ github.workflow }}-${{ github.event_name }}-${{ github.event.pull_request.number || github.run_id }} cancel-in-progress: true permissions: {} diff --git a/.github/workflows/reviewers.yml b/.github/workflows/reviewers.yml index 71d0bcb273823..1a47e38d7b0f9 100644 --- a/.github/workflows/reviewers.yml +++ b/.github/workflows/reviewers.yml @@ -17,7 +17,7 @@ on: type: string concurrency: - group: ${{ inputs.caller }}-${{ github.workflow }}-${{ github.event_name }}-${{ github.head_ref || github.run_id }} + group: ${{ inputs.caller }}-${{ github.workflow }}-${{ github.event_name }}-${{ github.event.pull_request.number || github.run_id }} cancel-in-progress: true permissions: {} From 4aa51a994c09f697b100cbc8286bf1dbd68fa63e Mon Sep 17 00:00:00 2001 From: Wolfgang Walther Date: Fri, 13 Jun 2025 15:17:33 +0200 Subject: [PATCH 5/7] workflows/{labels,reviewers}: fix concurrency groups for nested workflows This didn't work as intended. When a workflow is run with `workflow_call`, it will have `github.workflow` set to the *parent* workflow. So the `caller` input that we passed, resulted in this concurrency key: ``` Eval-Eval-... ``` But that's bad, because the labels and reviewers workflows will cancel each other! What we actually want is this: - Label and Reviewers workflow should have different groups. - Reviewers called via Eval and called directly via undraft should have *different* groups. We can't use the default condition we use everywhere else, because `github.workflow` is the same for Label and Reviewers. Thus, we hardcode the workflow's name as well. This essentially means we have this as a key: ``` --- ``` This should do what we want. Since workflows can be made reusable workflows later on, we add those hardcoded names to *all* concurrency groups. This avoids copy&paste errors later on. (cherry picked from commit 6793e238faeffcb58f1f6e1241c7390649fb8197) --- .github/workflows/README.md | 22 ++++++++++++++++++++++ .github/workflows/backport.yml | 2 +- .github/workflows/build.yml | 2 +- .github/workflows/check.yml | 2 +- .github/workflows/codeowners-v2.yml | 2 +- .github/workflows/dismissed-review.yml | 2 +- .github/workflows/edited.yml | 2 +- .github/workflows/eval-aliases.yml | 2 +- .github/workflows/eval.yml | 6 +----- .github/workflows/labels.yml | 7 +------ .github/workflows/lint.yml | 2 +- .github/workflows/reviewers.yml | 7 +------ 12 files changed, 33 insertions(+), 25 deletions(-) diff --git a/.github/workflows/README.md b/.github/workflows/README.md index 7089501d5e405..78303c2c64b86 100644 --- a/.github/workflows/README.md +++ b/.github/workflows/README.md @@ -18,3 +18,25 @@ Some architectural notes about key decisions and concepts in our workflows: - **head commit**: The HEAD commit in the pull request's branch. Same as `github.event.pull_request.head.sha`. - **merge commit**: The temporary "test merge commit" that GitHub Actions creates and updates for the pull request. Same as `refs/pull/${{ github.event.pull_request.number }}/merge`. - **target commit**: The base branch's parent of the "test merge commit" to compare against. + +## Concurrency Groups + +We use [GitHub's Concurrency Groups](https://docs.github.com/en/actions/writing-workflows/choosing-what-your-workflow-does/control-the-concurrency-of-workflows-and-jobs) to cancel older jobs on pushes to Pull Requests. +When two workflows are in the same group, a newer workflow cancels an older workflow. +Thus, it is important how to construct the group keys: + +- Because we want to run jobs for different events at same time, we add `github.event_name` to the key. This is the case for the `pull_request` which runs on changes to the workflow files to test the new files and the same workflow from the base branch run via `pull_request_event`. + +- We don't want workflows of different Pull Requests to cancel each other, so we include `github.event.pull_request.number`. The [GitHub docs](https://docs.github.com/en/actions/writing-workflows/choosing-what-your-workflow-does/control-the-concurrency-of-workflows-and-jobs#example-using-a-fallback-value) show using `github.head_ref` for this purpose, but this doesn't work well with forks: Different users could have the same head branch name in their forks and run CI for their PRs at the same time. + +- Sometimes, there is no `pull_request.number`. That's the case for `push` or `workflow_run` events. To ensure non-PR runs are never cancelled, we add a fallback of `github.run_id`. This is a unique value for each workflow run. + +- Of course, we run multiple workflows at the same time, so we add `github.workflow` to the key. Otherwise workflows would cancel each other. + +- There is a special case for reusable workflows called via `workflow_call` - they will have `github.workflow` set to their parent workflow's name. Thus, they would cancel each other. That's why we additionally hardcode the name of the workflow as well. + +This results in a key with the following semantics: + +``` +--- +``` diff --git a/.github/workflows/backport.yml b/.github/workflows/backport.yml index c5ba0126aeb2d..72b8482ef439d 100644 --- a/.github/workflows/backport.yml +++ b/.github/workflows/backport.yml @@ -10,7 +10,7 @@ on: types: [closed, labeled] concurrency: - group: ${{ github.workflow }}-${{ github.event_name }}-${{ github.event.pull_request.number || github.run_id }} + group: backport-${{ github.workflow }}-${{ github.event_name }}-${{ github.event.pull_request.number || github.run_id }} cancel-in-progress: true permissions: diff --git a/.github/workflows/build.yml b/.github/workflows/build.yml index 5013c7da1524c..a46a382ff42db 100644 --- a/.github/workflows/build.yml +++ b/.github/workflows/build.yml @@ -7,7 +7,7 @@ on: pull_request_target: concurrency: - group: ${{ github.workflow }}-${{ github.event_name }}-${{ github.event.pull_request.number || github.run_id }} + group: build-${{ github.workflow }}-${{ github.event_name }}-${{ github.event.pull_request.number || github.run_id }} cancel-in-progress: true permissions: {} diff --git a/.github/workflows/check.yml b/.github/workflows/check.yml index 3bcac620c7ed0..dfe5999df23ff 100644 --- a/.github/workflows/check.yml +++ b/.github/workflows/check.yml @@ -7,7 +7,7 @@ on: pull_request_target: concurrency: - group: ${{ github.workflow }}-${{ github.event_name }}-${{ github.event.pull_request.number || github.run_id }} + group: check-${{ github.workflow }}-${{ github.event_name }}-${{ github.event.pull_request.number || github.run_id }} cancel-in-progress: true permissions: {} diff --git a/.github/workflows/codeowners-v2.yml b/.github/workflows/codeowners-v2.yml index f8f909aff6d77..947f441f2e60e 100644 --- a/.github/workflows/codeowners-v2.yml +++ b/.github/workflows/codeowners-v2.yml @@ -30,7 +30,7 @@ on: types: [opened, ready_for_review, synchronize, reopened] concurrency: - group: ${{ github.workflow }}-${{ github.event_name }}-${{ github.event.pull_request.number || github.run_id }} + group: codeowners-${{ github.workflow }}-${{ github.event_name }}-${{ github.event.pull_request.number || github.run_id }} cancel-in-progress: true permissions: {} diff --git a/.github/workflows/dismissed-review.yml b/.github/workflows/dismissed-review.yml index 82cfe75dc3fc3..e8ab48bda0755 100644 --- a/.github/workflows/dismissed-review.yml +++ b/.github/workflows/dismissed-review.yml @@ -7,7 +7,7 @@ on: types: [completed] concurrency: - group: ${{ github.workflow }}-${{ github.event_name }}-${{ github.event.pull_request.number || github.run_id }} + group: dismissed-review-${{ github.workflow }}-${{ github.event_name }}-${{ github.event.pull_request.number || github.run_id }} cancel-in-progress: true permissions: diff --git a/.github/workflows/edited.yml b/.github/workflows/edited.yml index 23e7afb624a14..49fccb5f48ba0 100644 --- a/.github/workflows/edited.yml +++ b/.github/workflows/edited.yml @@ -17,7 +17,7 @@ on: types: [edited] concurrency: - group: ${{ github.workflow }}-${{ github.event_name }}-${{ github.event.pull_request.number || github.run_id }} + group: edited-${{ github.workflow }}-${{ github.event_name }}-${{ github.event.pull_request.number || github.run_id }} cancel-in-progress: true permissions: {} diff --git a/.github/workflows/eval-aliases.yml b/.github/workflows/eval-aliases.yml index 91930ea3d6e3c..599d82a93e51d 100644 --- a/.github/workflows/eval-aliases.yml +++ b/.github/workflows/eval-aliases.yml @@ -7,7 +7,7 @@ on: pull_request_target: concurrency: - group: ${{ github.workflow }}-${{ github.event_name }}-${{ github.event.pull_request.number || github.run_id }} + group: eval-aliases-${{ github.workflow }}-${{ github.event_name }}-${{ github.event.pull_request.number || github.run_id }} cancel-in-progress: true permissions: {} diff --git a/.github/workflows/eval.yml b/.github/workflows/eval.yml index 447561dca73c0..dd7c433815dc9 100644 --- a/.github/workflows/eval.yml +++ b/.github/workflows/eval.yml @@ -17,7 +17,7 @@ on: - python-updates concurrency: - group: ${{ github.workflow }}-${{ github.event_name }}-${{ github.event.pull_request.number || github.run_id }} + group: eval-${{ github.workflow }}-${{ github.event_name }}-${{ github.event.pull_request.number || github.run_id }} cancel-in-progress: true permissions: {} @@ -256,8 +256,6 @@ jobs: permissions: issues: write pull-requests: write - with: - caller: ${{ github.workflow }} reviewers: name: Reviewers @@ -268,5 +266,3 @@ jobs: if: needs.prepare.outputs.targetSha uses: ./.github/workflows/reviewers.yml secrets: inherit - with: - caller: ${{ github.workflow }} diff --git a/.github/workflows/labels.yml b/.github/workflows/labels.yml index 225c913c08b22..1a85953529507 100644 --- a/.github/workflows/labels.yml +++ b/.github/workflows/labels.yml @@ -7,11 +7,6 @@ name: "Label PR" on: workflow_call: - inputs: - caller: - description: Name of the calling workflow. - required: true - type: string workflow_run: workflows: - Review dismissed @@ -19,7 +14,7 @@ on: types: [completed] concurrency: - group: ${{ inputs.caller }}-${{ github.workflow }}-${{ github.event_name }}-${{ github.event.pull_request.number || github.run_id }} + group: labels-${{ github.workflow }}-${{ github.event_name }}-${{ github.event.pull_request.number || github.run_id }} cancel-in-progress: true permissions: diff --git a/.github/workflows/lint.yml b/.github/workflows/lint.yml index b5ed0c4323c4f..3b212a18e0e0c 100644 --- a/.github/workflows/lint.yml +++ b/.github/workflows/lint.yml @@ -7,7 +7,7 @@ on: pull_request_target: concurrency: - group: ${{ github.workflow }}-${{ github.event_name }}-${{ github.event.pull_request.number || github.run_id }} + group: lint-${{ github.workflow }}-${{ github.event_name }}-${{ github.event.pull_request.number || github.run_id }} cancel-in-progress: true permissions: {} diff --git a/.github/workflows/reviewers.yml b/.github/workflows/reviewers.yml index 1a47e38d7b0f9..894da05c32e11 100644 --- a/.github/workflows/reviewers.yml +++ b/.github/workflows/reviewers.yml @@ -10,14 +10,9 @@ on: pull_request_target: types: [ready_for_review] workflow_call: - inputs: - caller: - description: Name of the calling workflow. - required: true - type: string concurrency: - group: ${{ inputs.caller }}-${{ github.workflow }}-${{ github.event_name }}-${{ github.event.pull_request.number || github.run_id }} + group: reviewers-${{ github.workflow }}-${{ github.event_name }}-${{ github.event.pull_request.number || github.run_id }} cancel-in-progress: true permissions: {} From c6336fdae2a0c22abbdb8572b86e5862c437b7ab Mon Sep 17 00:00:00 2001 From: Wolfgang Walther Date: Sat, 14 Jun 2025 13:53:01 +0200 Subject: [PATCH 6/7] workflows/labels: count approving reviewers, not reviews A single reviewer approving a Pull Request multiple times should only count once. (cherry picked from commit 2e033512487f35ea755c8a42116c420568b8e14c) --- .github/workflows/labels.yml | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/.github/workflows/labels.yml b/.github/workflows/labels.yml index 1a85953529507..d29df9002b449 100644 --- a/.github/workflows/labels.yml +++ b/.github/workflows/labels.yml @@ -96,7 +96,7 @@ jobs: name == '12.approved-by: package-maintainer' ) - const approvals = + const approvals = new Set( (await github.paginate(github.rest.pulls.listReviews, { owner: context.repo.owner, repo: context.repo.repo, @@ -104,15 +104,16 @@ jobs: })) .filter(review => review.state == 'APPROVED') .map(review => review.user.id) + ) - const maintainers = Object.keys( + const maintainers = new Set(Object.keys( JSON.parse(await readFile('comparison/maintainers.json', 'utf-8')) - ) + )) // And the labels that should be there const after = JSON.parse(await readFile('comparison/changed-paths.json', 'utf-8')).labels - if (approvals.length > 0) after.push(`12.approvals: ${approvals.length > 2 ? '3+' : approvals.length}`) - if (maintainers.some(id => approvals.includes(id))) after.push('12.approved-by: package-maintainer') + if (approvals.size > 0) after.push(`12.approvals: ${approvals.size > 2 ? '3+' : approvals.size}`) + if (Array.from(maintainers).some(approvals.has)) after.push('12.approved-by: package-maintainer') // Remove the ones not needed anymore await Promise.all( From 4f90e240fe56c652be91ed0784ff5c930af6c68c Mon Sep 17 00:00:00 2001 From: Wolfgang Walther Date: Sat, 14 Jun 2025 18:47:45 +0200 Subject: [PATCH 7/7] workflows/labels: fix approval label with maintainers This currently fails with: ``` Method Set.prototype.has called on incompatible receiver undefined ``` Seems like my syntax test previously only hit the case without maintainers, in which case it doesn't throw :/. (cherry picked from commit 4b9fb45060225c9590fc4f07e2863af16db07ac4) --- .github/workflows/labels.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/labels.yml b/.github/workflows/labels.yml index d29df9002b449..a7e34775d6eaa 100644 --- a/.github/workflows/labels.yml +++ b/.github/workflows/labels.yml @@ -113,7 +113,7 @@ jobs: // And the labels that should be there const after = JSON.parse(await readFile('comparison/changed-paths.json', 'utf-8')).labels if (approvals.size > 0) after.push(`12.approvals: ${approvals.size > 2 ? '3+' : approvals.size}`) - if (Array.from(maintainers).some(approvals.has)) after.push('12.approved-by: package-maintainer') + if (Array.from(maintainers).some(m => approvals.has(m))) after.push('12.approved-by: package-maintainer') // Remove the ones not needed anymore await Promise.all(