diff --git a/.github/workflows/add_docs_review_label.yml b/.github/workflows/add_docs_review_label.yml index fcf77af53ce72..6fec2bcb1af46 100644 --- a/.github/workflows/add_docs_review_label.yml +++ b/.github/workflows/add_docs_review_label.yml @@ -1,12 +1,21 @@ -# Adds a label to PRs that touch documentation paths until a maintainer approves. +# Docs review hold — workflow 1/3 +# +# Purpose: avoid pulling the docs team into a PR review before the code is +# directionally approved by a maintainer. This workflow adds +# "docs review on hold" to any PR touching documentation paths, and the label is +# cleared automatically once a MEMBER or OWNER approves (see check_docs_review.yml). +# +# Design is intentionally best-effort. Edge cases such as an approval being later +# dismissed without a new commit are accepted — the label corrects itself on the +# next push. add and check/remove use separate concurrency groups (pr-labels-add-* +# vs pr-labels-check-*) so a new push cannot cancel a pending approval-check run. +# Each workflow independently checks state before acting, so concurrent runs +# across groups are safe. +# # Currently dedicated to the docs review flow. If a second use case appears, promote # this into a reusable workflow (`workflow_call`) with the label as an input. name: Add Docs Review Label -concurrency: - group: pr-labels-${{ github.event.pull_request.number }} - cancel-in-progress: false - permissions: pull-requests: write @@ -21,6 +30,10 @@ on: - "website/content/**" - "website/cue/reference/**" +concurrency: + group: pr-labels-add-${{ github.event.pull_request.number }} + cancel-in-progress: false + env: LABEL_NAME: "docs review on hold" @@ -29,34 +42,21 @@ jobs: runs-on: ubuntu-24.04 timeout-minutes: 5 steps: - - name: Check member review state + - name: Check member approval id: check uses: actions/github-script@3a2844b7e9c422d3c10d287c895573f7108da1b3 # v9.0.0 with: script: | - const { data: reviews } = await github.rest.pulls.listReviews({ + const reviews = await github.paginate(github.rest.pulls.listReviews, { owner: context.repo.owner, repo: context.repo.repo, pull_number: context.payload.pull_request.number, per_page: 100, }); - const allowed = ['MEMBER', 'OWNER']; - const latestByUser = new Map(); - for (const r of reviews) { - if (!allowed.includes(r.author_association)) continue; - if (r.state === 'COMMENTED') continue; - latestByUser.set(r.user.id, r.state); - } - - const states = [...latestByUser.values()]; - const hasApproved = states.includes('APPROVED'); - const hasChangesRequested = states.includes('CHANGES_REQUESTED'); - const skip = hasApproved && !hasChangesRequested; - - core.info(`Latest member review states: ${states.join(', ') || '(none)'}`); - core.info(`skip=${skip}`); - core.setOutput('skip', skip); + const approved = reviews.some(r => allowed.includes(r.author_association) && r.state === 'APPROVED'); + core.info(`Member approval found: ${approved}`); + core.setOutput('skip', String(approved)); - if: steps.check.outputs.skip != 'true' uses: actions-ecosystem/action-add-labels@18f1af5e3544586314bbe15c0273249c770b2daf # v1.1.3 with: diff --git a/.github/workflows/check_docs_review.yml b/.github/workflows/check_docs_review.yml index ead1e438894db..99a6f193bc724 100644 --- a/.github/workflows/check_docs_review.yml +++ b/.github/workflows/check_docs_review.yml @@ -1,40 +1,59 @@ +# Docs review hold — workflow 2/3 +# +# Fires on every review submission or dismissal. If at least one MEMBER/OWNER +# has an APPROVED review and the hold label is present, writes the PR number to +# an artifact for remove_docs_review_label.yml (3/3) to act on. +# +# The workflow_run indirection exists for a security reason: pull_request_review +# events from fork PRs run with a read-only token, so label writes must happen in +# a separate workflow that runs in the base-repo context with a write token. +# +# Review state check is intentionally simple: any APPROVED review from a +# MEMBER/OWNER is sufficient, regardless of later CHANGES_REQUESTED from the +# same reviewer. Best-effort is good enough here. name: Check Docs Review on: pull_request_review: - types: [submitted] + types: [submitted, dismissed] permissions: contents: read pull-requests: read +concurrency: + group: pr-labels-check-${{ github.event.pull_request.number }} + cancel-in-progress: false + env: LABEL_NAME: "docs review on hold" jobs: check: - if: github.event.review.state == 'approved' runs-on: ubuntu-24.04 timeout-minutes: 5 steps: - - name: Check association and label + - name: Check approval and label uses: actions/github-script@3a2844b7e9c422d3c10d287c895573f7108da1b3 # v9.0.0 with: script: | - const association = context.payload.review.author_association; - core.info(`Author association: ${association}`); - const allowed = ['MEMBER', 'OWNER']; - if (!allowed.includes(association)) { - core.info(`Association "${association}" not in allowed list, skipping`); - return; - } + const reviews = await github.paginate(github.rest.pulls.listReviews, { + owner: context.repo.owner, + repo: context.repo.repo, + pull_number: context.payload.pull_request.number, + per_page: 100, + }); + const approved = reviews.some(r => allowed.includes(r.author_association) && r.state === 'APPROVED'); + core.info(`Member approval found: ${approved}`); + if (!approved) return; const labelName = process.env.LABEL_NAME; - const { data: labels } = await github.rest.issues.listLabelsOnIssue({ + const labels = await github.paginate(github.rest.issues.listLabelsOnIssue, { owner: context.repo.owner, repo: context.repo.repo, issue_number: context.payload.pull_request.number, + per_page: 100, }); if (!labels.some(l => l.name === labelName)) { core.info(`Label "${labelName}" not found, skipping`); diff --git a/.github/workflows/remove_docs_review_label.yml b/.github/workflows/remove_docs_review_label.yml index f49acc25646b4..87b8d3c0c3d09 100644 --- a/.github/workflows/remove_docs_review_label.yml +++ b/.github/workflows/remove_docs_review_label.yml @@ -1,3 +1,10 @@ +# Docs review hold — workflow 3/3 +# +# Triggered by check_docs_review.yml (2/3) completing successfully. Runs in the +# base-repo context so it has a write token, which is required to modify labels +# on fork PRs. Re-checks review state immediately before removal to reduce (but +# not eliminate) the chance of removing the label while a CHANGES_REQUESTED was +# submitted in the small window between check and removal. Best-effort is fine. name: Remove Docs Review Label on: @@ -13,14 +20,14 @@ env: LABEL_NAME: "docs review on hold" jobs: - remove_label: + download: if: github.event.workflow_run.conclusion == 'success' runs-on: ubuntu-24.04 timeout-minutes: 5 permissions: actions: read - issues: write - pull-requests: write + outputs: + pr_number: ${{ steps.read.outputs.pr_number }} steps: - name: Download PR number id: download @@ -31,23 +38,64 @@ jobs: run-id: ${{ github.event.workflow_run.id }} github-token: ${{ github.token }} - - name: Remove label + - name: Read PR number + id: read if: steps.download.outcome == 'success' + run: | + pr_number=$(cat pr-number | tr -d '[:space:]') + echo "pr_number=${pr_number}" >> $GITHUB_OUTPUT + + remove_label: + needs: download + if: needs.download.outputs.pr_number != '' + runs-on: ubuntu-24.04 + timeout-minutes: 5 + concurrency: + group: pr-labels-${{ needs.download.outputs.pr_number }} + cancel-in-progress: false + permissions: + issues: write + pull-requests: write + steps: + - name: Remove label uses: actions/github-script@3a2844b7e9c422d3c10d287c895573f7108da1b3 # v9.0.0 + env: + PR_NUMBER: ${{ needs.download.outputs.pr_number }} with: script: | - const fs = require('fs'); - const prNumber = parseInt(fs.readFileSync('pr-number', 'utf8').trim(), 10); + const prNumber = parseInt(process.env.PR_NUMBER, 10); if (isNaN(prNumber)) { core.setFailed('Invalid PR number'); return; } - const labelName = process.env.LABEL_NAME; - await github.rest.issues.removeLabel({ + const allowed = ['MEMBER', 'OWNER']; + const reviews = await github.paginate(github.rest.pulls.listReviews, { owner: context.repo.owner, repo: context.repo.repo, - issue_number: prNumber, - name: labelName, + pull_number: prNumber, + per_page: 100, }); - core.info(`Removed "${labelName}" label from PR #${prNumber}`); + const approved = reviews.some(r => allowed.includes(r.author_association) && r.state === 'APPROVED'); + core.info(`Member approval found: ${approved}`); + if (!approved) { + core.info('No member approval, skipping removal'); + return; + } + + const labelName = process.env.LABEL_NAME; + try { + await github.rest.issues.removeLabel({ + owner: context.repo.owner, + repo: context.repo.repo, + issue_number: prNumber, + name: labelName, + }); + core.info(`Removed "${labelName}" label from PR #${prNumber}`); + } catch (err) { + if (err.status === 404) { + core.info(`Label "${labelName}" already absent from PR #${prNumber}, skipping`); + } else { + throw err; + } + }