From 22bad80939c8f42d125907075d294513b86de946 Mon Sep 17 00:00:00 2001 From: Mike McQuaid Date: Tue, 19 Mar 2024 08:56:12 +0000 Subject: [PATCH] Remove BrewTestBot critical approval process. We seem to have enough maintainers across enough timezones that this is no longer necessary any more (and it presents a bit of a security risk). --- .github/workflows/triage.yml | 84 -------------------------- docs/Homebrew-brew-Maintainer-Guide.md | 7 +-- 2 files changed, 2 insertions(+), 89 deletions(-) delete mode 100644 .github/workflows/triage.yml diff --git a/.github/workflows/triage.yml b/.github/workflows/triage.yml deleted file mode 100644 index c66b6b653e023..0000000000000 --- a/.github/workflows/triage.yml +++ /dev/null @@ -1,84 +0,0 @@ -name: Triage - -on: - pull_request_target: - types: - - opened - - synchronize - - reopened - - labeled - - unlabeled - -permissions: {} - -concurrency: triage-${{ github.head_ref }} - -jobs: - review: - runs-on: ubuntu-22.04 - if: startsWith(github.repository, 'Homebrew/') - steps: - - name: Review pull request - if: > - (github.event_name == 'pull_request' || github.event_name == 'pull_request_target') && - github.event.pull_request.state != 'closed' - uses: actions/github-script@v7 - with: - github-token: ${{ secrets.HOMEBREW_BREW_TRIAGE_PULL_REQUESTS_TOKEN }} - script: | - async function approvePullRequest(pullRequestNumber) { - const reviews = await approvalsByAuthenticatedUser(pullRequestNumber) - - if (reviews.length > 0) { - return - } - - await github.rest.pulls.createReview({ - ...context.repo, - pull_number: pullRequestNumber, - event: 'APPROVE', - }) - } - - async function approvalsByAuthenticatedUser(pullRequestNumber) { - const { data: user } = await github.rest.users.getAuthenticated() - - const { data: reviews } = await github.rest.pulls.listReviews({ - ...context.repo, - pull_number: pullRequestNumber, - }) - - const approvals = reviews.filter(review => review.state == 'APPROVED') - return approvals.filter(review => review.user.login == user.login) - } - - async function reviewPullRequest(pullRequestNumber) { - const { data: pullRequest } = await github.rest.pulls.get({ - owner: context.repo.owner, - repo: context.repo.repo, - pull_number: pullRequestNumber, - }) - - const { data: user } = await github.rest.users.getAuthenticated() - if (pullRequest.user.login == user.login) { - core.warning('Pull request author is a bot.') - return - } - - if (pullRequest.author_association != 'MEMBER') { - core.warning('Pull request author is not a member.') - return - } - - const criticalLabel = 'critical' - const labels = pullRequest.labels.map(label => label.name) - const hasCriticalLabel = labels.includes(criticalLabel) - - if (hasCriticalLabel) { - const message = `Review granted due to \`${criticalLabel}\` label.` - core.info(message) - await approvePullRequest(pullRequestNumber) - } - } - - await reviewPullRequest(context.issue.number) diff --git a/docs/Homebrew-brew-Maintainer-Guide.md b/docs/Homebrew-brew-Maintainer-Guide.md index 18a139fe27478..845bfc7d9bd64 100644 --- a/docs/Homebrew-brew-Maintainer-Guide.md +++ b/docs/Homebrew-brew-Maintainer-Guide.md @@ -18,16 +18,14 @@ Merging is done using the standard "Merge" button in the `Homebrew/brew` reposit PRs must meet the following conditions to be merged: -- Have at least one maintainer (or [@BrewTestBot](https://github.com/BrewTestBot)) approval. See the [Automatic approvals](#automatic-approvals) section below for more details about how [@BrewTestBot](https://github.com/BrewTestBot) approves PRs. +- Have at least one maintainer approval. - Have passing CI (continuous integration). This is a _mandatory_ step. PRs with failing CI should _never_ be merged. See the [CI](#ci) section below for more information about `Homebrew/brew` CI. If possible, PRs should also have GPG-signed commits (see the private `ops` repository for instructions on setting this up). ### Automatic approvals -To ensure that non-urgent PRs have the opportunity to be seen and reviewed by any other maintainers who wish to take a look, all PRs require an approval before they can be merged. However, some PRs are urgent enough that they need to be merged without an approval by another maintainer. - -As a compromise between always needing a review and allowing maintainers to merge PRs they deem critical, the `Triage` CI job will ensure that if a PR is labelled `critical`, [@BrewTestBot](https://github.com/BrewTestBot) approves the PR, allowing it to be merged. +To ensure that non-urgent PRs have the opportunity to be seen and reviewed by any other maintainers who wish to take a look, all PRs require an approval before they can be merged. ## CI @@ -35,7 +33,6 @@ Every PR in `Homebrew/brew` runs a series of CI tests to try to prevent bugs fro There are many checks that run on every PR. The following is a quick list of the various checks and what they represent: -- `Triage / review`: This verifies that the PR has been open for long enough. See the [Automatic approvals](#automatic-approvals) section above for more information about automatic approvals. - `Vendor Gems / vendor-gems`: This is skipped except for dependabot PRs. It updates the RBI files to match any new/changed dependencies. See [Type Checking With Sorbet](Typechecking.md) for more information about RBI files and typechecking. - `Codecov / codecov/patch` and `codecov/project`: These show the Codecov report for the PR. See the [`brew tests` and Codecov](#brew-tests-and-codecov) section below for more info about Codecov. - `CI / vendored gems`: This checks whether there was a change to the vendored gems on Linux that needs to be committed to the PR branch.