diff --git a/dev/pr-auditor/BUILD.bazel b/dev/pr-auditor/BUILD.bazel deleted file mode 100644 index bc485afe7d8..00000000000 --- a/dev/pr-auditor/BUILD.bazel +++ /dev/null @@ -1,43 +0,0 @@ -load("//dev:go_defs.bzl", "go_test") -load("@io_bazel_rules_go//go:def.bzl", "go_binary", "go_library") - -go_library( - name = "pr-auditor_lib", - srcs = [ - "check.go", - "issue.go", - "main.go", - "webhook.go", - ], - importpath = "github.com/sourcegraph/sourcegraph/dev/pr-auditor", - visibility = ["//visibility:private"], - deps = [ - "//lib/errors", - "@com_github_google_go_github_v55//github", - "@com_github_grafana_regexp//:regexp", - "@org_golang_x_exp//slices", - "@org_golang_x_oauth2//:oauth2", - ], -) - -go_binary( - name = "pr-auditor", - embed = [":pr-auditor_lib"], - visibility = ["//visibility:public"], -) - -go_test( - name = "pr-auditor_test", - timeout = "short", - srcs = [ - "check_test.go", - "issue_test.go", - ], - data = glob(["testdata/**"]), - embed = [":pr-auditor_lib"], - deps = [ - "@com_github_google_go_cmp//cmp", - "@com_github_stretchr_testify//assert", - "@com_github_stretchr_testify//require", - ], -) diff --git a/dev/pr-auditor/README.md b/dev/pr-auditor/README.md index d8672169785..f2c4d477e17 100644 --- a/dev/pr-auditor/README.md +++ b/dev/pr-auditor/README.md @@ -1,30 +1,3 @@ -# pr-auditor [![pr-auditor](https://github.com/sourcegraph/sourcegraph/actions/workflows/pr-auditor.yml/badge.svg)](https://github.com/sourcegraph/sourcegraph/actions/workflows/pr-auditor.yml) +# pr-auditor -`pr-auditor` is a tool designed to operate on some [GitHub Actions pull request events](https://docs.github.com/en/actions/using-workflows/events-that-trigger-workflows#pull_request) in order to check for SOC2 compliance. -Owned by the [DevInfra team](https://handbook.sourcegraph.com/departments/engineering/teams/devinfra/). - -Learn more: [Testing principles and guidelines](https://docs.sourcegraph.com/dev/background-information/testing_principles) - -## Usage - -This action is primarily designed to run on GitHub Actions, and leverages the [pull request event payloads](https://docs.github.com/en/actions/using-workflows/events-that-trigger-workflows#pull_request) extensively. - -The optional `-protected-branch` flag defines a base branch that always opens a PR audit issue to track all pull requests made to it. - -```sh -GITHUB_EVENT_PATH="/path/to/json/payload.json" -GITHUB_TOKEN="personal-access-token" - -# run directly -go run ./dev/pr-auditor/ check \ - -github.payload-path="$GITHUB_EVENT_PATH" \ - -github.token="$GITHUB_TOKEN" \ - -protected-branch="release" - -# run using wrapper script -./dev/buildchecker/check-pr.sh -``` - -## Deployment - -`pr-auditor` can be deployed to repositories using the available [batch changes](./batch-changes/README.md). +This action has been moved to https://github.com/sourcegraph/pr-auditor diff --git a/dev/pr-auditor/batch-changes/README.md b/dev/pr-auditor/batch-changes/README.md deleted file mode 100644 index abbfb3a54a1..00000000000 --- a/dev/pr-auditor/batch-changes/README.md +++ /dev/null @@ -1,16 +0,0 @@ -# pr-auditor batch changes - -## Rollouts - -[`pr-auditor-rollout.yml`](./pr-auditor-rollout.yml) describes a batch change that can be used to roll out `pr-auditor` to repositories that do not have it set up yet. - -To use it: - -1. [Create a batch change on `/batch-changes/create`](https://k8s.sgdev.org/batch-changes/create) -2. Paste [`pr-auditor-rollout.yml`](./pr-auditor-rollout.yml) into the spec input and make the appropriate changes to the template (for example, adjust the `on:` parameters to target the desired repositories) -3. Preview and apply the changes -4. Selectively publish the appropriate changesets for review and merge - -## Updates - -The [`pr-auditor-patch.yml`](./pr-auditor-patch.yml) spec describes a batch change that can be used to roll out `pr-auditor` workflow updates to repositories that already have it set up by synchronizing them with the source-of-truth script in the batch change spec. diff --git a/dev/pr-auditor/batch-changes/pr-auditor-patch.yml b/dev/pr-auditor/batch-changes/pr-auditor-patch.yml deleted file mode 100644 index 4f48aee3b5b..00000000000 --- a/dev/pr-auditor/batch-changes/pr-auditor-patch.yml +++ /dev/null @@ -1,51 +0,0 @@ -# Describes a batch change for rolling out pr-auditor workflow updates - -name: pr-auditor-patch -description: Roll out pr-auditor workflow patch. - -on: - - repositoriesMatchingQuery: | - repo:github.com/sourcegraph/.* - repohasfile:.github/workflows/pr-auditor.yml - count:all - -changesetTemplate: - title: 'workflows: update pr-auditor workflow' - body: | - This pull request updates the pr-auditor workflow. - - Test plan: We have tested the code itself in https://github.com/sourcegraph/sourcegraph/tree/main/dev/pr-auditor, - and the pr-auditor action should continue working as expected on this pull request. - branch: update-pr-auditor - commit: - message: 'workflows: update pr-auditor workflow' - -steps: - # Overwrite the GitHub action - - run: | - read -r -d '' TEMPLATE << 'EOF' - # See https://docs.sourcegraph.com/dev/background-information/ci#pr-auditor - name: pr-auditor - on: - pull_request_target: - types: [ closed, edited, opened, synchronize, ready_for_review ] - - jobs: - check-pr: - runs-on: ubuntu-latest - steps: - - uses: actions/checkout@v2 - with: { repository: 'sourcegraph/sourcegraph' } - - uses: actions/setup-go@v2 - with: { go-version: '1.18' } - - - run: ./dev/pr-auditor/check-pr.sh - env: - GITHUB_EVENT_PATH: ${{ "${{ env.GITHUB_EVENT_PATH }}" }} - GITHUB_TOKEN: ${{ "${{ secrets.CODENOTIFY_GITHUB_TOKEN }}" }} - GITHUB_RUN_URL: https://github.com/${{ "${{ github.repository }}" }}/actions/runs/${{ "${{ github.run_id }}" }} - EOF - - rm -rf .github/workflows/pr-auditor.yml - echo "$TEMPLATE" >> .github/workflows/pr-auditor.yml - container: alpine:3 diff --git a/dev/pr-auditor/batch-changes/pr-auditor-rollout.yml b/dev/pr-auditor/batch-changes/pr-auditor-rollout.yml deleted file mode 100644 index 8cbec6026f5..00000000000 --- a/dev/pr-auditor/batch-changes/pr-auditor-rollout.yml +++ /dev/null @@ -1,73 +0,0 @@ -# Describes a batch change for rolling out pr-auditor workflows - -name: pr-auditor-rollout -description: Roll out pr-auditor workflow to more repositories. - -on: - - repositoriesMatchingQuery: | - -repohasfile:.github/workflows/pr-auditor.yml count:all - ( - repo:^github\.com/sourcegraph/infrastructure$ - OR repo:^github\.com/sourcegraph/src-cli$ - OR repo:^github\.com/sourcegraph/deploy-sourcegraph$ - OR repo:^github\.com/sourcegraph/deploy-sourcegraph-(docker|cloud|dogfood-k8s|managed|helm)$ - OR repo:^github\.com/sourcegraph/lsif-.* - OR repo:^github\.com/sourcegraph/sbt-sourcegraph$ - OR repo:^github\.com/sourcegraph/terraform-.*-executors - OR repo:^github\.com/sourcegraph/sourcegraph-.* - ) - -changesetTemplate: - title: 'workflows: add pr-auditor and test plans to PR templates' - body: | - This pull request adds test plans to all PR templates, and introduces a pr-auditor workflow to ensure test plans are provided. - For more details see https://docs.sourcegraph.com/dev/background-information/testing_principles - - Test plan: created action should correctly set a status on this pull request. - branch: add-pr-auditor-and-test-plan - commit: - message: 'workflows: add pr-auditor and test plans to PR templates' - -steps: - # Add test plans to PR templates - - run: | - read -r -d '' TEMPLATE << 'EOF' - ## Test plan - - - - EOF - - mkdir -p .github - echo "\n\n$TEMPLATE" >> .github/PULL_REQUEST_TEMPLATE.md - echo "\n\n$TEMPLATE" | tee -a $(find -name .github/PULL_REQUEST_TEMPLATE/*.md) - container: alpine:3 - # Create a GitHub action - - run: | - read -r -d '' TEMPLATE << 'EOF' - # See https://docs.sourcegraph.com/dev/background-information/ci#pr-auditor - name: pr-auditor - on: - pull_request_target: - types: [ closed, edited, opened, synchronize, ready_for_review ] - - jobs: - check-pr: - runs-on: ubuntu-latest - steps: - - uses: actions/checkout@v2 - with: { repository: 'sourcegraph/sourcegraph' } - - uses: actions/setup-go@v2 - with: { go-version: '1.18' } - - - run: ./dev/pr-auditor/check-pr.sh - env: - GITHUB_EVENT_PATH: ${{ "${{ env.GITHUB_EVENT_PATH }}" }} - GITHUB_TOKEN: ${{ "${{ secrets.CODENOTIFY_GITHUB_TOKEN }}" }} - GITHUB_RUN_URL: https://github.com/${{ "${{ github.repository }}" }}/actions/runs/${{ "${{ github.run_id }}" }} - EOF - - mkdir -p .github/workflows - rm -rf .github/workflows/pr-auditor.yml - echo "$TEMPLATE" >> .github/workflows/pr-auditor.yml - container: alpine:3 diff --git a/dev/pr-auditor/check-pr.sh b/dev/pr-auditor/check-pr.sh deleted file mode 100755 index ed8f4bd16f9..00000000000 --- a/dev/pr-auditor/check-pr.sh +++ /dev/null @@ -1,14 +0,0 @@ -#! /usr/bin/env bash - -# Make this script independent of where it's called -cd "$(dirname "${BASH_SOURCE[0]}")"/../.. - -set -eu - -echo "--- Running 'pr-auditor'" -go run ./dev/pr-auditor/ \ - -github.payload-path="$GITHUB_EVENT_PATH" \ - -github.token="$GITHUB_TOKEN" \ - -github.run-url="$GITHUB_RUN_URL" \ - -skip-check-test-plan="${SKIP_CHECK_TEST_PLAN:-False}" \ - -skip-check-reviews="${SKIP_CHECK_REVIEWS:-False}" diff --git a/dev/pr-auditor/check.go b/dev/pr-auditor/check.go deleted file mode 100644 index 85b7b213153..00000000000 --- a/dev/pr-auditor/check.go +++ /dev/null @@ -1,117 +0,0 @@ -package main - -import ( - "context" - "strings" - - "github.com/google/go-github/v55/github" - "github.com/grafana/regexp" - "golang.org/x/exp/slices" -) - -type checkResult struct { - // ReviewSatisfied indicates that *any* review has been made on the PR. It is also set to - // true if the test plan indicates that this PR does not need to be review. - ReviewSatisfied bool - // CanSkipTestPlan indicates that the test plan is not required for audit. - CanSkipTestPlan bool - // TestPlan is the content provided after the acceptance checklist checkbox. - TestPlan string - // ProtectedBranch indicates that the base branch for this PR is protected and merges - // are considered to be exceptional and should always be justified. - ProtectedBranch bool - // Error indicating any issue that might have occured during the check. - Error error -} - -func (r checkResult) IsSatisfied() bool { - return r.IsTestPlanSatisfied() && r.ReviewSatisfied && !r.ProtectedBranch -} - -func (r checkResult) IsTestPlanSatisfied() bool { - return r.CanSkipTestPlan || r.TestPlan != "" -} - -var ( - testPlanDividerRegexp = regexp.MustCompile("(?m)(#+ Test [pP]lan)|(Test [pP]lan:)") - noReviewNeededDividerRegexp = regexp.MustCompile("(?m)([nN]o [rR]eview [rR]equired:)") - - markdownCommentRegexp = regexp.MustCompile("(\n)*") - - noReviewNeedLabels = []string{"no-review-required", "automerge"} -) - -type checkOpts struct { - SkipReviews bool - SkipTestPlan bool - ProtectedBranch string -} - -func isProtectedBranch(payload *EventPayload, protectedBranch string) bool { - return protectedBranch != "" && payload.PullRequest.Base.Ref == protectedBranch -} - -func checkPR(ctx context.Context, ghc *github.Client, payload *EventPayload, opts checkOpts) checkResult { - pr := payload.PullRequest - - // Whether or not this PR was reviewed can be inferred from payload, but an approval - // might not have any comments so we need to double-check through the GitHub API - var err error - reviewed := pr.ReviewComments > 0 - if !reviewed && !opts.SkipReviews { - owner, repo := payload.Repository.GetOwnerAndName() - var reviews []*github.PullRequestReview - // Continue, but return err later - reviews, _, err = ghc.PullRequests.ListReviews(ctx, owner, repo, payload.PullRequest.Number, &github.ListOptions{}) - reviewed = len(reviews) > 0 - } - - // Parse test plan data from body - sections := testPlanDividerRegexp.Split(pr.Body, 2) - if len(sections) < 2 { - return checkResult{ - ReviewSatisfied: reviewed, - CanSkipTestPlan: opts.SkipTestPlan, - Error: err, - } - } - - testPlan := cleanMarkdown(sections[1]) - - // Look for no review required explanation in the test plan - if sections := noReviewNeededDividerRegexp.Split(testPlan, 2); len(sections) > 1 { - noReviewRequiredExplanation := cleanMarkdown(sections[1]) - if len(noReviewRequiredExplanation) > 0 { - reviewed = true - } - } - - if testPlan != "" { - for _, label := range pr.Labels { - if slices.Contains(noReviewNeedLabels, label.Name) { - reviewed = true - break - } - } - } - - mergeAgainstProtected := isProtectedBranch(payload, opts.ProtectedBranch) - - return checkResult{ - ReviewSatisfied: reviewed, - CanSkipTestPlan: opts.SkipTestPlan, - TestPlan: testPlan, - ProtectedBranch: mergeAgainstProtected, - Error: err, - } -} - -func cleanMarkdown(s string) string { - content := s - // Remove comments - content = markdownCommentRegexp.ReplaceAllString(content, "") - // Remove whitespace - content = strings.TrimSpace(content) - - return content -} diff --git a/dev/pr-auditor/check_test.go b/dev/pr-auditor/check_test.go deleted file mode 100644 index e8991936f6c..00000000000 --- a/dev/pr-auditor/check_test.go +++ /dev/null @@ -1,199 +0,0 @@ -package main - -import ( - "context" - "os" - "strings" - "testing" - - "github.com/google/go-cmp/cmp" - "github.com/stretchr/testify/assert" - "github.com/stretchr/testify/require" -) - -func TestCheckTestPlan(t *testing.T) { - tests := []struct { - name string - bodyFile string - labels []string - baseBranch string - protectedBranch string - canSkipTestPlan bool - want checkResult - }{ - { - name: "has test plan", - bodyFile: "testdata/pull_request_body/has-plan.md", - want: checkResult{ - ReviewSatisfied: false, - TestPlan: "I have a plan!", - }, - }, - { - name: "protected branch", - bodyFile: "testdata/pull_request_body/has-plan.md", - baseBranch: "release", - protectedBranch: "release", - want: checkResult{ - ReviewSatisfied: false, - TestPlan: "I have a plan!", - ProtectedBranch: true, - }, - }, - { - name: "non protected branch", - bodyFile: "testdata/pull_request_body/has-plan.md", - baseBranch: "preprod", - protectedBranch: "release", - want: checkResult{ - ReviewSatisfied: false, - TestPlan: "I have a plan!", - ProtectedBranch: false, - }, - }, - { - name: "no test plan", - bodyFile: "testdata/pull_request_body/no-plan.md", - want: checkResult{ - ReviewSatisfied: false, - }, - }, - { - name: "complicated test plan", - bodyFile: "testdata/pull_request_body/has-plan-fancy.md", - want: checkResult{ - ReviewSatisfied: false, - TestPlan: `This is a plan! -Quite lengthy - -And a little complicated; there's also the following reasons: - -1. A -2. B -3. C`, - }, - }, - { - name: "inline test plan", - bodyFile: "testdata/pull_request_body/inline-plan.md", - want: checkResult{ - ReviewSatisfied: false, - TestPlan: `This is a plan! -Quite lengthy - -And a little complicated; there's also the following reasons: - -1. A -2. B -3. C`, - }, - }, - { - name: "no review required", - bodyFile: "testdata/pull_request_body/no-review-required.md", - want: checkResult{ - ReviewSatisfied: true, - TestPlan: "I have a plan! No review required: this is a bot PR", - }, - }, - { - name: "bad markdown still passes", - bodyFile: "testdata/pull_request_body/bad-markdown.md", - want: checkResult{ - ReviewSatisfied: true, - TestPlan: "This is still a plan! No review required: just trust me", - }, - }, - { - name: "no review required via automerge label", - bodyFile: "testdata/pull_request_body/has-plan.md", - labels: []string{"automerge"}, - want: checkResult{ - ReviewSatisfied: true, - TestPlan: "I have a plan!", - }, - }, - { - name: "no review required via no-review-required label", - bodyFile: "testdata/pull_request_body/has-plan.md", - labels: []string{"no-review-required"}, - want: checkResult{ - ReviewSatisfied: true, - TestPlan: "I have a plan!", - }, - }, - { - name: "no review required via automerge label but no plan", - bodyFile: "testdata/pull_request_body/no-plan.md", - labels: []string{"automerge"}, - want: checkResult{ - ReviewSatisfied: false, - }, - }, - { - name: "no review required via no-review-required label but no plan", - bodyFile: "testdata/pull_request_body/no-plan.md", - labels: []string{"no-review-required"}, - want: checkResult{ - ReviewSatisfied: false, - }, - }, - { - name: "no review required but with the wrong label", - bodyFile: "testdata/pull_request_body/has-plan.md", - labels: []string{"random-label"}, - want: checkResult{ - ReviewSatisfied: false, - TestPlan: "I have a plan!", - }, - }, - { - name: "no test plan but skip-test-plans enabled", - bodyFile: "testdata/pull_request_body/no-plan.md", - canSkipTestPlan: true, - want: checkResult{ - ReviewSatisfied: false, - CanSkipTestPlan: true, - }, - }, - } - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - body, err := os.ReadFile(tt.bodyFile) - require.NoError(t, err) - - payload := &EventPayload{ - PullRequest: PullRequestPayload{ - Body: string(body), - }, - } - if tt.labels != nil { - payload.PullRequest.Labels = make([]Label, len(tt.labels)) - for i, label := range tt.labels { - payload.PullRequest.Labels[i] = Label{Name: label} - } - } - checkOpts := checkOpts{ - SkipReviews: true, - SkipTestPlan: tt.canSkipTestPlan, - } - - if tt.baseBranch != "" && tt.protectedBranch != "" { - payload.PullRequest.Base = RefPayload{Ref: tt.baseBranch} - checkOpts.ProtectedBranch = tt.protectedBranch - } - - got := checkPR(context.Background(), nil, payload, checkOpts) - assert.Equal(t, tt.want.IsTestPlanSatisfied(), got.IsTestPlanSatisfied()) - t.Log("got.TestPlan: ", got.TestPlan) - if tt.want.TestPlan == "" { - assert.Empty(t, got.TestPlan) - } else { - assert.True(t, strings.Contains(got.TestPlan, tt.want.TestPlan), - cmp.Diff(got.TestPlan, tt.want.TestPlan)) - } - assert.Equal(t, tt.want.ProtectedBranch, got.ProtectedBranch) - assert.Equal(t, tt.want.ReviewSatisfied, got.ReviewSatisfied) - }) - } -} diff --git a/dev/pr-auditor/issue.go b/dev/pr-auditor/issue.go deleted file mode 100644 index 429effa4102..00000000000 --- a/dev/pr-auditor/issue.go +++ /dev/null @@ -1,76 +0,0 @@ -package main - -import ( - "fmt" - - "github.com/google/go-github/v55/github" -) - -const ( - testPlanDocs = "https://docs.sourcegraph.com/dev/background-information/testing_principles#test-plans" -) - -func generateExceptionIssue(payload *EventPayload, result *checkResult, additionalContext string) *github.IssueRequest { - // 🚨 SECURITY: Do not reference other potentially sensitive fields of pull requests - prTitle := payload.PullRequest.Title - if payload.Repository.Private { - prTitle = "" - } - - var ( - issueTitle = fmt.Sprintf("%s#%d: %q", payload.Repository.FullName, payload.PullRequest.Number, prTitle) - issueBody string - exceptionLabels = []string{} - issueAssignees = []string{} - ) - - if !result.ReviewSatisfied { - exceptionLabels = append(exceptionLabels, "exception/review") - } - - if !result.IsTestPlanSatisfied() { - exceptionLabels = append(exceptionLabels, "exception/test-plan") - } - if result.ProtectedBranch { - exceptionLabels = append(exceptionLabels, "exception/protected-branch") - } - - if !result.ReviewSatisfied { - if result.IsTestPlanSatisfied() { - issueBody = fmt.Sprintf("%s %q **has a test plan (or does not require one)** but **was not reviewed**.", payload.PullRequest.URL, prTitle) - } else { - issueBody = fmt.Sprintf("%s %q **has no test plan** and **was not reviewed**.", payload.PullRequest.URL, prTitle) - } - } else if !result.IsTestPlanSatisfied() { - issueBody = fmt.Sprintf("%s %q **has no test plan**.", payload.PullRequest.URL, prTitle) - } - - if !result.IsTestPlanSatisfied() { - issueBody += fmt.Sprintf("\n\nLearn more about test plans in our [testing guidelines](%s).", testPlanDocs) - } - - if result.ProtectedBranch { - issueBody += fmt.Sprintf("\n\nThe base branch %q is protected and should not have direct pull requests to it.", payload.PullRequest.Base.Ref) - } - - if additionalContext != "" { - issueBody += fmt.Sprintf("\n\n%s", additionalContext) - } - - user := payload.PullRequest.MergedBy.Login - issueAssignees = append(issueAssignees, user) - issueBody += fmt.Sprintf("\n\n@%s please comment in this issue with an explanation for this exception and close this issue.", user) - - if result.Error != nil { - // Log the error in the issue - issueBody += fmt.Sprintf("\n\nEncountered error when checking PR: %s", result.Error) - } - - labels := append(exceptionLabels, payload.Repository.FullName) - return &github.IssueRequest{ - Title: github.String(issueTitle), - Body: github.String(issueBody), - Assignees: &issueAssignees, - Labels: &labels, - } -} diff --git a/dev/pr-auditor/issue_test.go b/dev/pr-auditor/issue_test.go deleted file mode 100644 index fe7880ea5df..00000000000 --- a/dev/pr-auditor/issue_test.go +++ /dev/null @@ -1,114 +0,0 @@ -package main - -import ( - "testing" - - "github.com/stretchr/testify/assert" -) - -func TestGenerateExceptionIssue(t *testing.T) { - payload := EventPayload{ - Repository: RepositoryPayload{FullName: "bobheadxi/robert"}, - PullRequest: PullRequestPayload{ - Title: "some pull request", - URL: "https://bobheadxi.dev", - MergedBy: UserPayload{Login: "robert"}, - }, - } - privatePayload := payload - privatePayload.Repository.Private = true - - protectedPayload := payload - protectedPayload.PullRequest.Base = RefPayload{Ref: "release"} - - tests := []struct { - name string - payload EventPayload - result checkResult - additionalContext string - - wantAssignees []string - wantLabels []string - wantBodyContains []string - wantBodyExcludes []string - }{{ - name: "not reviewed, not planned", - payload: payload, - result: checkResult{ - ReviewSatisfied: false, - }, - wantAssignees: []string{"robert"}, - wantLabels: []string{"exception/review", "exception/test-plan", "bobheadxi/robert"}, - wantBodyContains: []string{"some pull request", "has no test plan", "was not reviewed"}, - wantBodyExcludes: []string{"protected"}, - }, { - name: "not reviewed, planned", - payload: payload, - result: checkResult{ - ReviewSatisfied: false, - TestPlan: "A plan!", - }, - wantAssignees: []string{"robert"}, - wantLabels: []string{"exception/review", "bobheadxi/robert"}, - wantBodyContains: []string{"some pull request", "has a test plan", "was not reviewed"}, - wantBodyExcludes: []string{"protected"}, - }, { - name: "not reviewed, not planned, but plan skipping is disabled", - payload: payload, - result: checkResult{ - ReviewSatisfied: false, - CanSkipTestPlan: true, - }, - wantAssignees: []string{"robert"}, - wantLabels: []string{"exception/review", "bobheadxi/robert"}, - wantBodyContains: []string{"some pull request", "has a test plan", "was not reviewed"}, - wantBodyExcludes: []string{"protected"}, - }, { - name: "not planned, reviewed", - payload: payload, - result: checkResult{ - ReviewSatisfied: true, - }, - wantAssignees: []string{"robert"}, - wantLabels: []string{"exception/test-plan", "bobheadxi/robert"}, - wantBodyContains: []string{"some pull request", "has no test plan"}, - wantBodyExcludes: []string{"protected"}, - }, { - name: "private repo, not planned, reviewed", - payload: privatePayload, - result: checkResult{}, - wantAssignees: []string{"robert"}, - wantLabels: []string{"exception/review", "exception/test-plan", "bobheadxi/robert"}, - wantBodyExcludes: []string{"some pull request", "protected"}, - }, { - name: "reviewed, planned but protected", - payload: protectedPayload, - result: checkResult{ProtectedBranch: true}, - wantAssignees: []string{"robert"}, - wantLabels: []string{"exception/review", "exception/test-plan", "exception/protected-branch", "bobheadxi/robert"}, - wantBodyContains: []string{"\"release\" is protected"}, - }, { - name: "reviewed, planned but protected", - payload: protectedPayload, - additionalContext: "please use preprod branch", - result: checkResult{ProtectedBranch: true}, - wantAssignees: []string{"robert"}, - wantLabels: []string{"exception/review", "exception/test-plan", "exception/protected-branch", "bobheadxi/robert"}, - wantBodyContains: []string{"please use preprod branch"}, - }, - } - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - got := generateExceptionIssue(&tt.payload, &tt.result, tt.additionalContext) - t.Log(got.GetTitle(), "\n", got.GetBody()) - assert.Equal(t, tt.wantAssignees, got.GetAssignees()) - assert.Equal(t, tt.wantLabels, got.GetLabels()) - for _, content := range tt.wantBodyContains { - assert.Contains(t, *got.Body, content, "body does not contain expected strings") - } - for _, content := range tt.wantBodyExcludes { - assert.NotContains(t, *got.Body, content, "body contains unexpected strings") - } - }) - } -} diff --git a/dev/pr-auditor/main.go b/dev/pr-auditor/main.go deleted file mode 100644 index 2dc0a2b753c..00000000000 --- a/dev/pr-auditor/main.go +++ /dev/null @@ -1,219 +0,0 @@ -package main - -import ( - "context" - "encoding/json" - "flag" - "fmt" - "log" - "os" - - "github.com/sourcegraph/sourcegraph/lib/errors" - - "github.com/google/go-github/v55/github" - "golang.org/x/oauth2" -) - -type Flags struct { - GitHubPayloadPath string - GitHubToken string - GitHubRunURL string - - IssuesRepoOwner string - IssuesRepoName string - - // ProtectedBranch designates a branch name that should always record an exception when a PR is opened - // against it. It's primary use case is to discourage PRs againt the release branch on sourcegraph/deploy-sourcegraph-cloud. - ProtectedBranch string - - // AdditionalContext contains a paragraph that will be appended at the end of the created exception. It enables - // repositories to further explain why an exception has been recorded. - AdditionalContext string - - // SkipStatus if true will skip updating commit status on GitHub and just record exceptions. Useful when crawling through failed runs caused by infrastructure issues. - SkipStatus bool - - SkipCheckTestPlan bool - SkipCheckReview bool -} - -func (f *Flags) Parse() { - flag.StringVar(&f.GitHubPayloadPath, "github.payload-path", "", "path to JSON file with GitHub event payload") - flag.StringVar(&f.GitHubToken, "github.token", "", "GitHub token") - flag.StringVar(&f.GitHubRunURL, "github.run-url", "", "URL to GitHub actions run") - flag.StringVar(&f.IssuesRepoOwner, "issues.repo-owner", "sourcegraph", "owner of repo to create issues in") - flag.StringVar(&f.IssuesRepoName, "issues.repo-name", "sec-pr-audit-trail", "name of repo to create issues in") - flag.StringVar(&f.ProtectedBranch, "protected-branch", "", "name of branch that if set as the base branch in a PR, will always open an exception") - flag.StringVar(&f.AdditionalContext, "additional-context", "", "additional information that will be appended to the recorded exception, if any.") - flag.BoolVar(&f.SkipStatus, "skip-status", false, "skip updating commit status on GitHub and just record exceptions") - flag.BoolVar(&f.SkipCheckTestPlan, "skip-check-test-plan", false, "Allows PRs without a test plan to pass audit") - flag.BoolVar(&f.SkipCheckReview, "skip-check-review", false, "Allows PRs without any approving reviews to be merged") - flag.Parse() -} - -func main() { - flags := &Flags{} - flags.Parse() - - ctx := context.Background() - ghc := github.NewClient(oauth2.NewClient(ctx, oauth2.StaticTokenSource( - &oauth2.Token{AccessToken: flags.GitHubToken}, - ))) - - payloadData, err := os.ReadFile(flags.GitHubPayloadPath) - if err != nil { - log.Fatal("ReadFile: ", err) - } - var payload *EventPayload - if err := json.Unmarshal(payloadData, &payload); err != nil { - log.Fatal("Unmarshal: ", err) - } - log.Printf("handling event for pull request %s, payload: %+v\n", payload.PullRequest.URL, payload.Dump()) - - // Discard unwanted events - switch ref := payload.PullRequest.Base.Ref; ref { - // This is purely an API call usage optimization, so we don't need to be so specific - // as to require usage to provide the default branch - we can just rely on a simple - // allowlist of commonly used default branches. - case "main", "master", "release": - log.Printf("performing checks against allow-listed pull request base %q", ref) - case flags.ProtectedBranch: - if flags.ProtectedBranch == "" { - log.Printf("unknown pull request base %q - discarding\n", ref) - return - } - - log.Printf("performing checks against protected pull request base %q", ref) - default: - log.Printf("unknown pull request base %q - discarding\n", ref) - return - } - if payload.PullRequest.Draft { - log.Println("skipping event on draft PR") - return - } - if payload.Action == "closed" && !payload.PullRequest.Merged { - log.Println("ignoring closure of un-merged pull request") - return - } - if payload.Action == "edited" && payload.PullRequest.Merged { - log.Println("ignoring edit of already-merged pull request") - return - } - - // Do checks - if payload.PullRequest.Merged { - if err := postMergeAudit(ctx, ghc, payload, flags); err != nil { - log.Fatalf("postMergeAudit: %s", err) - } - } else { - if err := preMergeAudit(ctx, ghc, payload, flags); err != nil { - log.Fatalf("preMergeAudit: %s", err) - } - } -} - -const ( - commitStatusPostMerge = "pr-auditor / post-merge" - commitStatusPreMerge = "pr-auditor / pre-merge" -) - -func postMergeAudit(ctx context.Context, ghc *github.Client, payload *EventPayload, flags *Flags) error { - result := checkPR(ctx, ghc, payload, checkOpts{ - SkipReviews: flags.SkipCheckReview, - SkipTestPlan: flags.SkipCheckTestPlan, - ProtectedBranch: flags.ProtectedBranch, - }) - log.Printf("checkPR: %+v\n", result) - - if result.IsSatisfied() { - log.Println("Acceptance checked and PR reviewed, done") - // Don't create status that likely nobody will check anyway - return nil - } - - owner, repo := payload.Repository.GetOwnerAndName() - if result.Error != nil && !flags.SkipStatus { - _, _, statusErr := ghc.Repositories.CreateStatus(ctx, owner, repo, payload.PullRequest.Head.SHA, &github.RepoStatus{ - Context: github.String(commitStatusPostMerge), - State: github.String("error"), - Description: github.String(fmt.Sprintf("checkPR: %s", result.Error.Error())), - TargetURL: github.String(flags.GitHubRunURL), - }) - if statusErr != nil { - return errors.Newf("result.Error != nil (%w), statusErr: %w", result.Error, statusErr) - } - return nil - } - - issue := generateExceptionIssue(payload, &result, flags.AdditionalContext) - - log.Printf("Ensuring label for repository %q\n", payload.Repository.FullName) - _, _, err := ghc.Issues.CreateLabel(ctx, flags.IssuesRepoOwner, flags.IssuesRepoName, &github.Label{ - Name: github.String(payload.Repository.FullName), - }) - if err != nil { - log.Printf("Ignoring error on CreateLabel: %s\n", err) - } - - log.Printf("Creating issue for exception: %+v\n", issue) - created, _, err := ghc.Issues.Create(ctx, flags.IssuesRepoOwner, flags.IssuesRepoName, issue) - if err != nil { - // Let run fail, don't include special status - return errors.Newf("Issues.Create: %w", err) - } - - log.Println("Created issue: ", created.GetHTMLURL()) - - // Let run succeed, create separate status indicating an exception was created - _, _, err = ghc.Repositories.CreateStatus(ctx, owner, repo, payload.PullRequest.Head.SHA, &github.RepoStatus{ - Context: github.String(commitStatusPostMerge), - State: github.String("failure"), - Description: github.String("Exception detected and audit trail issue created"), - TargetURL: github.String(created.GetHTMLURL()), - }) - if err != nil { - return errors.Newf("CreateStatus: %w", err) - } - - return nil -} - -func preMergeAudit(ctx context.Context, ghc *github.Client, payload *EventPayload, flags *Flags) error { - result := checkPR(ctx, ghc, payload, checkOpts{ - SkipReviews: true, // only validate reviews on post-merge - SkipTestPlan: flags.SkipCheckTestPlan, - ProtectedBranch: flags.ProtectedBranch, - }) - log.Printf("checkPR: %+v\n", result) - - var prState, stateDescription string - stateURL := flags.GitHubRunURL - switch { - case result.Error != nil: - prState = "error" - stateDescription = fmt.Sprintf("checkPR: %s", result.Error.Error()) - case !result.IsTestPlanSatisfied(): - prState = "failure" - stateDescription = "No test plan detected - please provide one!" - stateURL = "https://docs.sourcegraph.com/dev/background-information/testing_principles#test-plans" - case result.ProtectedBranch: - prState = "success" - stateDescription = "No action needed, but an exception will be opened post-merge." - default: - prState = "success" - stateDescription = "No action needed, nice!" - } - - owner, repo := payload.Repository.GetOwnerAndName() - _, _, err := ghc.Repositories.CreateStatus(ctx, owner, repo, payload.PullRequest.Head.SHA, &github.RepoStatus{ - Context: github.String(commitStatusPreMerge), - State: github.String(prState), - Description: github.String(stateDescription), - TargetURL: github.String(stateURL), - }) - if err != nil { - return errors.Newf("CreateStatus: %w", err) - } - return nil -} diff --git a/dev/pr-auditor/testdata/pull_request_body/bad-markdown.md b/dev/pr-auditor/testdata/pull_request_body/bad-markdown.md deleted file mode 100644 index d35015e5f25..00000000000 --- a/dev/pr-auditor/testdata/pull_request_body/bad-markdown.md +++ /dev/null @@ -1,3 +0,0 @@ -Some poorly formatted markdown test plan - ## Test plan - This is still a plan! No review required: just trust me diff --git a/dev/pr-auditor/testdata/pull_request_body/has-plan-fancy.md b/dev/pr-auditor/testdata/pull_request_body/has-plan-fancy.md deleted file mode 100644 index 486bba3be52..00000000000 --- a/dev/pr-auditor/testdata/pull_request_body/has-plan-fancy.md +++ /dev/null @@ -1,18 +0,0 @@ - - ---- - -### Test plan - - - -This is a plan! -Quite lengthy - -And a little complicated; there's also the following reasons: - - - -1. A -2. B -3. C diff --git a/dev/pr-auditor/testdata/pull_request_body/has-plan.md b/dev/pr-auditor/testdata/pull_request_body/has-plan.md deleted file mode 100644 index b106d306fcb..00000000000 --- a/dev/pr-auditor/testdata/pull_request_body/has-plan.md +++ /dev/null @@ -1,12 +0,0 @@ - - ---- - -### Test Plan - - - -I have a plan! - - - \ No newline at end of file diff --git a/dev/pr-auditor/testdata/pull_request_body/inline-plan.md b/dev/pr-auditor/testdata/pull_request_body/inline-plan.md deleted file mode 100644 index a48fdab06a6..00000000000 --- a/dev/pr-auditor/testdata/pull_request_body/inline-plan.md +++ /dev/null @@ -1,12 +0,0 @@ -Some description - -Test plan: This is a plan! -Quite lengthy - - - -And a little complicated; there's also the following reasons: - -1. A -2. B -3. C diff --git a/dev/pr-auditor/testdata/pull_request_body/no-plan.md b/dev/pr-auditor/testdata/pull_request_body/no-plan.md deleted file mode 100644 index 3fef8510dd8..00000000000 --- a/dev/pr-auditor/testdata/pull_request_body/no-plan.md +++ /dev/null @@ -1,13 +0,0 @@ - - ---- - -### Acceptance checklist - - - - - - diff --git a/dev/pr-auditor/testdata/pull_request_body/no-review-required.md b/dev/pr-auditor/testdata/pull_request_body/no-review-required.md deleted file mode 100644 index 13467d274f2..00000000000 --- a/dev/pr-auditor/testdata/pull_request_body/no-review-required.md +++ /dev/null @@ -1 +0,0 @@ -Test plan: I have a plan! No review required: this is a bot PR diff --git a/dev/pr-auditor/webhook.go b/dev/pr-auditor/webhook.go deleted file mode 100644 index 94d1fcc88a3..00000000000 --- a/dev/pr-auditor/webhook.go +++ /dev/null @@ -1,64 +0,0 @@ -package main - -import ( - "fmt" - "strings" -) - -// EventPayload describes the payload of the pull_request event we subscribe to: -// https://docs.github.com/en/developers/webhooks-and-events/webhooks/webhook-events-and-payloads#pull_request -type EventPayload struct { - Action string `json:"action"` - PullRequest PullRequestPayload `json:"pull_request"` - Repository RepositoryPayload `json:"repository"` -} - -func (p EventPayload) Dump() string { - return fmt.Sprintf(`Action: %s, PullRequest: { Merged: %v, MergedBy: %+v, Head: %+v }, Repository: %s`, - p.Action, p.PullRequest.Merged, p.PullRequest.MergedBy, p.PullRequest.Head, p.Repository.FullName) -} - -type PullRequestPayload struct { - Number int `json:"number"` - Title string `json:"title"` - Body string `json:"body"` - Draft bool `json:"draft"` - - Labels []Label `json:"labels"` - - ReviewComments int `json:"review_comments"` - - Merged bool `json:"merged"` - MergedBy UserPayload `json:"merged_by"` - - URL string `json:"html_url"` - - Base RefPayload `json:"base"` - Head RefPayload `json:"head"` -} - -type Label struct { - Name string `json:"name"` -} - -type UserPayload struct { - Login string `json:"login"` - URL string `json:"html_url"` -} - -type RepositoryPayload struct { - FullName string `json:"full_name"` - URL string `json:"html_url"` - Private bool `json:"private"` -} - -func (r *RepositoryPayload) GetOwnerAndName() (string, string) { - repoParts := strings.Split(r.FullName, "/") - return repoParts[0], repoParts[1] -} - -type RefPayload struct { - // e.g. 'main' - Ref string `json:"ref"` - SHA string `json:"sha"` -}