diff --git a/.github/workflows/codeowners.yml b/.github/workflows/codeowners.yml index d9f9904..4097e1e 100644 --- a/.github/workflows/codeowners.yml +++ b/.github/workflows/codeowners.yml @@ -13,7 +13,6 @@ jobs: codeowners: name: 'Run Codeowners Plus' runs-on: ubuntu-latest - if: ${{ !github.event.pull_request.draft }} steps: - name: 'Checkout Code Repository' uses: actions/checkout@v4 @@ -24,10 +23,11 @@ jobs: run: | sed -i "s/image: .*/image: 'Dockerfile'/" action.yml cat action.yml - + - name: 'Codeowners Plus' uses: ./ with: github-token: '${{ secrets.GITHUB_TOKEN }}' pr: '${{ github.event.pull_request.number }}' verbose: true + quiet: ${{ github.event.pull_request.draft }} diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index a1cd9b3..2f56018 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -18,7 +18,7 @@ Thank you for considering contributing to Codeowners Plus! We welcome contributi > Running locally still requires real Github PRs to exist that you are testing against. ```bash -go run main.go -token -dir ../chaturbate -pr -repo multimediallc/chaturbate -v true +go run main.go -token -dir ../chaturbate -pr -repo multimediallc/chaturbate -v true -quiet=true ``` #### Running the CLI tool Locally diff --git a/README.md b/README.md index f2ff66b..bf7ac87 100644 --- a/README.md +++ b/README.md @@ -4,7 +4,7 @@ Code Ownership & Review Assignment Tool - GitHub CODEOWNERS but better [![Go Report Card](https://goreportcard.com/badge/github.com/multimediallc/codeowners-plus)](https://goreportcard.com/report/github.com/multimediallc/codeowners-plus?kill_cache=1) [![Tests](https://github.com/multimediallc/codeowners-plus/actions/workflows/go.yml/badge.svg)](https://github.com/multimediallc/codeowners-plus/actions/workflows/go.yml) -![Coverage](https://img.shields.io/badge/Coverage-83.6%25-brightgreen) +![Coverage](https://img.shields.io/badge/Coverage-83.3%25-brightgreen) [![License](https://img.shields.io/badge/License-BSD%203--Clause-blue.svg)](https://opensource.org/licenses/BSD-3-Clause) [![Contributor Covenant](https://img.shields.io/badge/Contributor%20Covenant-2.1-4baaaa.svg)](CODE_OF_CONDUCT.md) @@ -70,9 +70,9 @@ jobs: uses: actions/checkout@v4 with: fetch-depth: 0 - + - name: 'Codeowners Plus' - uses: multimediallc/codeowners-plus@v0.1.0 + uses: multimediallc/codeowners-plus@v0.1.4 with: github-token: '${{ secrets.GITHUB_TOKEN }}' pr: '${{ github.event.pull_request.number }}' @@ -121,7 +121,7 @@ By default, each file will resolve to a single reviewer. See [priority section] To instead require an owner as an additional reviewer (`AND` rule), put an `&` at the start of the line: ``` -# this rule will add `@task-auditor` as a required review in addition to the file owner's required review +# this rule will add `@task-auditor` as a required review in addition to the file owner's required review & **/task.go @task-auditor ``` @@ -219,7 +219,7 @@ If you want to allow PR authors to bypass some reviews when there are a large nu `codeowners.toml` ```toml -# +# # `max_reviews` (default nil) allows you to skip some reviewers if the number of reviewers is greater than the max_reviewers max_reviews = 2 ``` @@ -254,13 +254,44 @@ high_priority_labels = ["high-priority", "urgent"] When a PR has any of these labels, the comment will look like this: ``` -❗High Prio❗ +❗High Prio❗ Codeowners approval required for this PR: - @user1 - @user2 ``` +## Quiet Mode + +You can run Codeowners Plus in a "quiet" mode using the `quiet` input in the GitHub Action. + +### When Quiet Mode is Enabled + +* **No Comments:** The action will **not** post the review status comment (listing required/unapproved reviewers) or the optional reviewer "cc" comment to the Pull Request. +* **No Review Requests:** The action will **not** automatically request reviews from required owners who have not yet approved via the GitHub API. + +### Behavior: + +Even in quiet mode, the tool still performs all its internal calculations: determining required/optional owners based on file changes, checking existing approvals, and determining if the ownership rules are satisfied. The primary outcome is still the success or failure status of the associated status check (unless you've configured `enforcement.fail_check = false`). + +### Use Cases: + +* **Draft Pull Requests:** This is a common use case. You might want the Codeowners Plus logic to run and report a status (e.g., pending or failed) on draft PRs, but without notifying reviewers prematurely by adding comments or requesting reviews until the PR is marked "Ready for review". +* **Custom Notification Workflows:** You might prefer to handle notifications or review requests through a different mechanism and only use Codeowners Plus for the status check enforcement. + +### Activation: + +* **GitHub Action:** Set the `quiet` input to `'true'`. + ```yaml + - name: 'Codeowners Plus (Quiet)' + uses: multimediallc/codeowners-plus@v0.1.0 + with: + # ... other inputs ... + quiet: 'true' + ``` + +**Default:** Quiet mode is **disabled** (`false`) by default. + ## CLI Tool A CLI tool is available which provides some utilities for working with `.codeowners` files. @@ -276,7 +307,10 @@ Available commands are: * `owner` to check who owns a specific file * `verify` to check for typos in a `.codeowners` file +## Contributing + +See [CONTRIBUTING.md](https://github.com/multimediallc/codeowners-plus/blob/main/CONTRIBUTING.md) + ## Future Features * Inline ownership comments for having owners for specific functions, classes, etc. - diff --git a/main.go b/main.go index f9a8822..ec7daae 100644 --- a/main.go +++ b/main.go @@ -26,6 +26,7 @@ type AppConfig struct { PR int Repo string Verbose bool + Quiet bool } // App represents the application with its dependencies @@ -44,6 +45,7 @@ type Flags struct { PR *int Repo *string Verbose *bool + Quiet *bool } var ( @@ -53,6 +55,7 @@ var ( PR: flag.Int("pr", ignoreError(strconv.Atoi(getEnv("INPUT_PR", ""))), "Pull Request number"), Repo: flag.String("repo", getEnv("INPUT_REPOSITORY", ""), "GitHub repo name"), Verbose: flag.Bool("v", ignoreError(strconv.ParseBool(getEnv("INPUT_VERBOSE", "0"))), "Verbose output"), + Quiet: flag.Bool("quiet", ignoreError(strconv.ParseBool(getEnv("INPUT_QUIET", "0"))), "Prevents addition of comments to PR and requesting reviews from unapproved owners"), } WarningBuffer = bytes.NewBuffer([]byte{}) InfoBuffer = bytes.NewBuffer([]byte{}) @@ -199,11 +202,12 @@ func (a *App) processApprovalsAndReviewers() (bool, string, error) { } // Request reviews from required owners - unapprovedOwners, err := a.requestReviews() + err = a.requestReviews() if err != nil { return false, message, err } + unapprovedOwners := a.codeowners.AllRequired() maxReviewsMet := false if a.conf.MaxReviews != nil && *a.conf.MaxReviews > 0 { if validApprovalCount >= *a.conf.MaxReviews && len(f.Intersection(unapprovedOwners.Flatten(), a.conf.UnskippableReviewers)) == 0 { @@ -212,51 +216,13 @@ func (a *App) processApprovalsAndReviewers() (bool, string, error) { } // Add comments to the PR if necessary - if len(unapprovedOwners) > 0 { - // Comment on the PR with the codeowner teams that have not approved the PR - comment := allRequiredOwners.ToCommentString() - hasHighPriority, err := a.client.IsInLabels(a.conf.HighPriorityLabels) - if err != nil { - fmt.Fprintf(WarningBuffer, "WARNING: Error checking high priority labels: %v\n", err) - } else if hasHighPriority { - comment = "❗High Prio❗\n\n" + comment - } - if maxReviewsMet { - comment += "\n\n" - comment += "The PR has received the max number of required reviews. No further action is required." - } - fiveDaysAgo := time.Now().AddDate(0, 0, -5) - found, err := a.client.IsInComments(comment, &fiveDaysAgo) - if err != nil { - return false, message, fmt.Errorf("IsInComments Error: %v\n", err) - } - if !found { - err = a.client.AddComment(comment) - if err != nil { - return false, message, fmt.Errorf("AddComment Error: %v\n", err) - } - } + err = a.addReviewStatusComment(allRequiredOwners, unapprovedOwners, maxReviewsMet) + if err != nil { + return false, message, fmt.Errorf("failed to add review status comment: %w", err) } - if len(allOptionalReviewerNames) > 0 { - var isInCommentsError error = nil - // Add CC comment to the PR with the optional reviewers that have not already been mentioned in the PR comments - viewersToPing := f.Filtered(allOptionalReviewerNames, func(name string) bool { - found, err := a.client.IsSubstringInComments(name, nil) - if err != nil { - isInCommentsError = err - } - return !found - }) - if isInCommentsError != nil { - return false, message, fmt.Errorf("IsInComments Error: %v\n", err) - } - if len(viewersToPing) > 0 { - comment := fmt.Sprintf("cc %s", strings.Join(viewersToPing, " ")) - err = a.client.AddComment(comment) - if err != nil { - return false, message, fmt.Errorf("AddComment Error: %v\n", err) - } - } + err = a.addOptionalCcComment(allOptionalReviewerNames) + if err != nil { + return false, message, fmt.Errorf("failed to add optional CC comment: %w", err) } // Exit if there are any unapproved codeowner teams @@ -294,6 +260,87 @@ func (a *App) processApprovalsAndReviewers() (bool, string, error) { return true, message, nil } +func (a *App) addReviewStatusComment(allRequiredOwners, unapprovedOwners codeowners.ReviewerGroups, maxReviewsMet bool) error { + // Comment on the PR with the codeowner teams that have not approved the PR + + if a.config.Quiet || len(unapprovedOwners) == 0 { + printDebug("Skipping review status comment (disabled or no unapproved owners).\n") + return nil + } + + comment := allRequiredOwners.ToCommentString() + hasHighPriority, err := a.client.IsInLabels(a.conf.HighPriorityLabels) + if err != nil { + printWarning("WARNING: Error checking high priority labels: %v\n", err) + } else if hasHighPriority { + comment = "❗High Prio❗\n\n" + comment + } + + if maxReviewsMet { + comment += "\n\nThe PR has received the max number of required reviews. No further action is required." + } + + fiveDaysAgo := time.Now().AddDate(0, 0, -5) + found, err := a.client.IsInComments(comment, &fiveDaysAgo) + if err != nil { + return fmt.Errorf("IsInComments Error: %v\n", err) + } + + // Add the comment if it wasn't found recently + if !found { + printDebug("Adding review status comment: %q\n", comment) + err = a.client.AddComment(comment) + if err != nil { + return fmt.Errorf("AddComment Error: %v\n", err) + } + } else { + printDebug("Similar review status comment already exists.\n") + } + + return nil +} + +func (a *App) addOptionalCcComment(allOptionalReviewerNames []string) error { + // Add CC comment to the PR with the optional reviewers that have not already been mentioned in the PR comments + + if a.config.Quiet || len(allOptionalReviewerNames) == 0 { + printDebug("Skipping optional CC comment (disabled or no optional reviewers).\n") + return nil + } + + var isInCommentsError error + viewersToPing := f.Filtered(allOptionalReviewerNames, func(name string) bool { + if isInCommentsError != nil { + return false + } + found, err := a.client.IsSubstringInComments(name, nil) + if err != nil { + printWarning("WARNING: Error checking comments for substring '%s': %v\n", name, err) + isInCommentsError = err + return false + } + return !found + }) + + if isInCommentsError != nil { + return fmt.Errorf("IsInComments Error: %v\n", isInCommentsError) + } + + // Add the CC comment if there are any viewers to ping + if len(viewersToPing) > 0 { + comment := fmt.Sprintf("cc %s", strings.Join(viewersToPing, " ")) + printDebug("Adding CC comment: %q\n", comment) + err := a.client.AddComment(comment) + if err != nil { + return fmt.Errorf("AddComment Error: %v\n", err) + } + } else { + printDebug("No new optional reviewers to CC.\n") + } + + return nil +} + func (a *App) processTokenOwnerApproval() (*gh.CurrentApproval, error) { tokenOwner, err := a.client.GetTokenUser() if err != nil { @@ -324,20 +371,25 @@ func (a *App) processApprovals(ghApprovals []*gh.CurrentApproval) (int, error) { return len(ghApprovals) - len(approvalsToDismiss), nil } -func (a *App) requestReviews() (codeowners.ReviewerGroups, error) { +func (a *App) requestReviews() error { + if a.config.Quiet { + printDebug("Skipping review requests (disabled in quiet mode).\n") + return nil + } + unapprovedOwners := a.codeowners.AllRequired() unapprovedOwnerNames := unapprovedOwners.Flatten() printDebug("Remaining Required Owners: %s\n", unapprovedOwnerNames) currentlyRequestedOwners, err := a.client.GetCurrentlyRequested() if err != nil { - return nil, fmt.Errorf("GetCurrentlyRequested Error: %v", err) + return fmt.Errorf("GetCurrentlyRequested Error: %v", err) } printDebug("Currently Requested Owners: %s\n", currentlyRequestedOwners) previousReviewers, err := a.client.GetAlreadyReviewed() if err != nil { - return nil, fmt.Errorf("GetAlreadyReviewed Error: %v", err) + return fmt.Errorf("GetAlreadyReviewed Error: %v", err) } printDebug("Already Reviewed Owners: %s\n", previousReviewers) @@ -348,11 +400,11 @@ func (a *App) requestReviews() (codeowners.ReviewerGroups, error) { if len(filteredOwners) > 0 { printDebug("Requesting Reviews from: %s\n", filteredOwnerNames) if err := a.client.RequestReviewers(filteredOwnerNames); err != nil { - return nil, fmt.Errorf("RequestReviewers Error: %v", err) + return fmt.Errorf("RequestReviewers Error: %v", err) } } - return unapprovedOwners, nil + return nil } func printFileOwners(codeOwners codeowners.CodeOwners) { @@ -425,6 +477,7 @@ func main() { PR: *flags.PR, Repo: *flags.Repo, Verbose: *flags.Verbose, + Quiet: *flags.Quiet, } app, err := NewApp(cfg) diff --git a/main_test.go b/main_test.go index f4b5a78..88d7fbd 100644 --- a/main_test.go +++ b/main_test.go @@ -129,6 +129,9 @@ type mockGitHubClient struct { initCommentsError error addCommentError error approvePRError error + AddCommentCalled bool + AddCommentInput string + RequestReviewersCalled bool } func (m *mockGitHubClient) PR() *github.PullRequest { @@ -169,6 +172,7 @@ func (m *mockGitHubClient) DismissStaleReviews(approvals []*gh.CurrentApproval) } func (m *mockGitHubClient) RequestReviewers(reviewers []string) error { + m.RequestReviewersCalled = true return m.requestReviewersError } @@ -218,6 +222,8 @@ func (m *mockGitHubClient) InitComments() error { } func (m *mockGitHubClient) AddComment(comment string) error { + m.AddCommentCalled = true + m.AddCommentInput = comment if m.addCommentError != nil { return m.addCommentError } @@ -247,6 +253,12 @@ func (m *mockGitHubClient) IsInComments(comment string, since *time.Time) (bool, return false, nil } +func (m *mockGitHubClient) ResetGHClientTracking() { + m.AddCommentCalled = false + m.AddCommentInput = "" + m.RequestReviewersCalled = false +} + func (m *mockGitHubClient) IsSubstringInComments(substring string, since *time.Time) (bool, error) { if m.comments == nil { return false, nil @@ -381,6 +393,7 @@ func TestNewApp(t *testing.T) { PR: 123, Repo: "owner/repo", Verbose: true, + Quiet: true, }, expectError: false, }, @@ -432,6 +445,9 @@ func TestNewApp(t *testing.T) { if app.config.Verbose != tc.config.Verbose { t.Errorf("expected verbose %v, got %v", tc.config.Verbose, app.config.Verbose) } + if app.config.Quiet != tc.config.Quiet { + t.Errorf("expected Quiet %v, got %v", tc.config.Quiet, app.config.Quiet) + } }) } } @@ -513,7 +529,7 @@ func TestPrintWarning(t *testing.T) { } } -func TestErrorAndExit(t *testing.T) { +func TestOuputAndExit(t *testing.T) { // Note: This test can't actually verify the exit behavior // It only verifies that the buffers are written correctly tt := []struct { @@ -628,6 +644,181 @@ func TestInitFlags(t *testing.T) { } } +func setupAppForTest(t *testing.T, quiet bool) (*App, *mockGitHubClient) { + t.Helper() + + mockGH := &mockGitHubClient{} + mockGH.ResetGHClientTracking() + + cfg := AppConfig{ + Quiet: quiet, + } + + conf := &owners.Config{ + HighPriorityLabels: []string{"high-prio"}, + } + mockOwners := &mockCodeOwners{ + requiredOwners: codeowners.ReviewerGroups{ + &codeowners.ReviewerGroup{Names: []string{"@user1", "@user2"}}, + }, + } + + app := &App{ + config: cfg, + client: mockGH, + conf: conf, + codeowners: mockOwners, + gitDiff: mockGitDiff{}, + } + + return app, mockGH +} + +func TestAddReviewStatusComment(t *testing.T) { + user1Group := codeowners.ReviewerGroups{ + &codeowners.ReviewerGroup{Names: []string{"@user1"}}, + } + pendingReviewerGroup := codeowners.ReviewerGroups{ + &codeowners.ReviewerGroup{Names: []string{"@pending-reviewer"}}, + } + + tt := []struct { + name string + quiet bool + unapproved codeowners.ReviewerGroups + allRequired codeowners.ReviewerGroups + expectedShouldCall bool + expectedComment string + }{ + { + name: "short circuits in quiet mode", + quiet: true, + unapproved: pendingReviewerGroup, + allRequired: pendingReviewerGroup, + expectedShouldCall: false, + expectedComment: "", + }, + { + name: "adds comment when not in quiet mode and a required reviewer has not approved", + quiet: false, + unapproved: user1Group, + allRequired: user1Group, + expectedShouldCall: true, + expectedComment: user1Group.ToCommentString(), + }, + } + + for _, tc := range tt { + t.Run(tc.name, func(t *testing.T) { + app, mockClient := setupAppForTest(t, tc.quiet) + err := app.addReviewStatusComment(tc.allRequired, tc.unapproved, false) + if err != nil { + t.Errorf("Unexpected error when adding comment: %v", err) + } + if mockClient.AddCommentCalled != tc.expectedShouldCall { + t.Errorf("Expected mockClient.AddCommentCalled to be %t, but got %t", tc.expectedShouldCall, mockClient.AddCommentCalled) + } + if mockClient.AddCommentInput != tc.expectedComment { + t.Errorf("Expected comment body %q, got %q", tc.expectedComment, mockClient.AddCommentInput) + } + }) + } +} + +func TestAddOptionalCcComment(t *testing.T) { + optionalSingle := []string{"@optional-cc"} + optionalMultiple := []string{"@cc-user1", "@cc-user2"} + + tt := []struct { + name string + quiet bool + optionalReviewers []string + expectedShouldCall bool + expectedComment string + }{ + { + name: "short circuits in quiet mode", + quiet: true, + optionalReviewers: optionalSingle, + expectedShouldCall: false, + expectedComment: "", + }, + { + name: "adds comment when not in quiet mode", + quiet: false, + optionalReviewers: optionalMultiple, + expectedShouldCall: true, + expectedComment: fmt.Sprintf("cc %s", strings.Join(optionalMultiple, " ")), + }, + } + + for _, tc := range tt { + t.Run(tc.name, func(t *testing.T) { + app, mockClient := setupAppForTest(t, tc.quiet) + mockClient.ResetGHClientTracking() + + err := app.addOptionalCcComment(tc.optionalReviewers) + if err != nil { + t.Errorf("Unexpected error when adding optional cc comment: %v", err) + } + if mockClient.AddCommentCalled != tc.expectedShouldCall { + t.Errorf("Expected mockClient.AddCommentCalled to be %t, but got %t", tc.expectedShouldCall, mockClient.AddCommentCalled) + } + if tc.expectedShouldCall && mockClient.AddCommentInput != tc.expectedComment { + t.Errorf("Expected comment body %q, got %q", tc.expectedComment, mockClient.AddCommentInput) + } + if !tc.expectedShouldCall && mockClient.AddCommentInput != "" { + t.Errorf("Expected empty comment body when AddCommentCalled is false, but got %q", mockClient.AddCommentInput) + } + }) + } +} + +func TestRequestReviews(t *testing.T) { + tt := []struct { + name string + quiet bool + mockCurrentlyRequested []string + mockAlreadyReviewed []string + expectedShouldCall bool + }{ + { + name: "short circuits in quiet mode", + quiet: true, + mockCurrentlyRequested: []string{}, + mockAlreadyReviewed: []string{}, + expectedShouldCall: false, + }, + { + name: "sends requests when not in quiet mode", + quiet: false, + mockCurrentlyRequested: []string{}, + mockAlreadyReviewed: []string{}, + expectedShouldCall: true, + }, + } + + for _, tc := range tt { + t.Run(tc.name, func(t *testing.T) { + app, mockClient := setupAppForTest(t, tc.quiet) + mockClient.ResetGHClientTracking() + + mockClient.currentlyRequested = tc.mockCurrentlyRequested + mockClient.alreadyReviewed = tc.mockAlreadyReviewed + + err := app.requestReviews() + + if err != nil { + t.Errorf("Unexpected error during requestReviews: %v", err) + } + + if mockClient.RequestReviewersCalled != tc.expectedShouldCall { + t.Errorf("Expected mockClient.RequestReviewersCalled to be %t, but got %t", tc.expectedShouldCall, mockClient.RequestReviewersCalled) + } + }) + } +} + func TestProcessApprovalsAndReviewers(t *testing.T) { maxReviews := 2 minReviews := 2 @@ -875,7 +1066,9 @@ func TestProcessApprovalsAndReviewers(t *testing.T) { } app := &App{ - config: AppConfig{}, + config: AppConfig{ + Quiet: false, + }, client: mockGH, codeowners: mockOwners, gitDiff: mockGitDiff{