From 77eec283cc423ed583059fb19fe665d10e941350 Mon Sep 17 00:00:00 2001 From: zbedforrest Date: Tue, 25 Mar 2025 14:10:00 -0700 Subject: [PATCH 01/17] test with const var --- main.go | 83 ++++++++++++++++++++++++++++++--------------------------- 1 file changed, 43 insertions(+), 40 deletions(-) diff --git a/main.go b/main.go index 0043dac..0d4d0ef 100644 --- a/main.go +++ b/main.go @@ -53,8 +53,9 @@ var ( Repo: flag.String("repo", getEnv("INPUT_REPOSITORY", ""), "GitHub repo name"), Verbose: flag.Bool("v", ignoreError(strconv.ParseBool(getEnv("INPUT_VERBOSE", "0"))), "Verbose output"), } - WarningBuffer = bytes.NewBuffer([]byte{}) - InfoBuffer = bytes.NewBuffer([]byte{}) + WarningBuffer = bytes.NewBuffer([]byte{}) + InfoBuffer = bytes.NewBuffer([]byte{}) + QUIET_MODE_TEST = true ) // initFlags initializes and parses command line flags @@ -211,49 +212,51 @@ 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 !QUIET_MODE_TEST { + 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 { - return false, message, fmt.Errorf("AddComment Error: %v\n", err) + fmt.Fprintf(WarningBuffer, "WARNING: Error checking high priority labels: %v\n", err) + } else if hasHighPriority { + comment = "❗High Prio❗\n\n" + comment } - } - } - 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 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 { - isInCommentsError = err + 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) + } } - 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) + 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) + } } } } From b2ff62f910e3a4b04a5e6ddb9bddad3e70055cfa Mon Sep 17 00:00:00 2001 From: zbedforrest Date: Tue, 25 Mar 2025 14:58:36 -0700 Subject: [PATCH 02/17] add new flag --- main.go | 122 ++++++++++++++++++++++++++++---------------------------- 1 file changed, 62 insertions(+), 60 deletions(-) diff --git a/main.go b/main.go index 0d4d0ef..4ba352c 100644 --- a/main.go +++ b/main.go @@ -20,11 +20,12 @@ import ( // AppConfig holds the application configuration type AppConfig struct { - Token string - RepoDir string - PR int - Repo string - Verbose bool + Token string + RepoDir string + PR int + Repo string + Verbose bool + AddComments bool } // App represents the application with its dependencies @@ -38,20 +39,22 @@ type App struct { // Flags holds the command line flags type Flags struct { - Token *string - RepoDir *string - PR *int - Repo *string - Verbose *bool + Token *string + RepoDir *string + PR *int + Repo *string + Verbose *bool + AddComments *bool } var ( flags = &Flags{ - Token: flag.String("token", getEnv("INPUT_GITHUB-TOKEN", ""), "GitHub authentication token"), - RepoDir: flag.String("dir", getEnv("GITHUB_WORKSPACE", "/"), "Path to local Git repo"), - 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"), + Token: flag.String("token", getEnv("INPUT_GITHUB-TOKEN", ""), "GitHub authentication token"), + RepoDir: flag.String("dir", getEnv("GITHUB_WORKSPACE", "/"), "Path to local Git repo"), + 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"), + AddComments: flag.Bool("comments", ignoreError(strconv.ParseBool(getEnv("INPUT_ADD_COMMENTS", "1"))), "Add comments to Pull Request"), } WarningBuffer = bytes.NewBuffer([]byte{}) InfoBuffer = bytes.NewBuffer([]byte{}) @@ -212,51 +215,49 @@ func (a *App) processApprovalsAndReviewers() (bool, string, error) { } // Add comments to the PR if necessary - if !QUIET_MODE_TEST { - 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 a.config.AddComments && 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 { - 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." + return false, message, fmt.Errorf("AddComment Error: %v\n", err) } - fiveDaysAgo := time.Now().AddDate(0, 0, -5) - found, err := a.client.IsInComments(comment, &fiveDaysAgo) + } + } + if a.config.AddComments && 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 { - 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) - } + isInCommentsError = err } + return !found + }) + if isInCommentsError != nil { + return false, message, fmt.Errorf("IsInComments Error: %v\n", 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) - } + 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) } } } @@ -422,11 +423,12 @@ func main() { } cfg := AppConfig{ - Token: *flags.Token, - RepoDir: *flags.RepoDir, - PR: *flags.PR, - Repo: *flags.Repo, - Verbose: *flags.Verbose, + Token: *flags.Token, + RepoDir: *flags.RepoDir, + PR: *flags.PR, + Repo: *flags.Repo, + Verbose: *flags.Verbose, + AddComments: *flags.AddComments, } app, err := NewApp(cfg) From 31f8e05493eec365baab17fa72c2f52766a90851 Mon Sep 17 00:00:00 2001 From: zbedforrest Date: Tue, 25 Mar 2025 15:32:06 -0700 Subject: [PATCH 03/17] refactor the commenting code --- main.go | 131 +++++++++++++++++++++++++++++++++++++------------------- 1 file changed, 87 insertions(+), 44 deletions(-) diff --git a/main.go b/main.go index 4ba352c..d2d014b 100644 --- a/main.go +++ b/main.go @@ -215,51 +215,13 @@ func (a *App) processApprovalsAndReviewers() (bool, string, error) { } // Add comments to the PR if necessary - if a.config.AddComments && 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 a.config.AddComments && 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 @@ -297,6 +259,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.AddComments || 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.AddComments || 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 { From 389739df8432f296c645ea003ab2b5991662bcef Mon Sep 17 00:00:00 2001 From: zbedforrest Date: Tue, 25 Mar 2025 16:05:13 -0700 Subject: [PATCH 04/17] add unit tests --- main_test.go | 127 +++++++++++++++++++++++++++++++++++++++++++++++++-- 1 file changed, 122 insertions(+), 5 deletions(-) diff --git a/main_test.go b/main_test.go index 1900ad7..4218c67 100644 --- a/main_test.go +++ b/main_test.go @@ -129,6 +129,8 @@ type mockGitHubClient struct { initCommentsError error addCommentError error approvePRError error + AddCommentCalled bool + AddCommentInput string } func (m *mockGitHubClient) PR() *github.PullRequest { @@ -218,6 +220,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 +251,11 @@ func (m *mockGitHubClient) IsInComments(comment string, since *time.Time) (bool, return false, nil } +func (m *mockGitHubClient) ResetCommentCallTracking() { + m.AddCommentCalled = false + m.AddCommentInput = "" +} + func (m *mockGitHubClient) IsSubstringInComments(substring string, since *time.Time) (bool, error) { if m.comments == nil { return false, nil @@ -376,11 +385,12 @@ func TestNewApp(t *testing.T) { { name: "valid config", config: AppConfig{ - Token: "test-token", - RepoDir: "/test/dir", - PR: 123, - Repo: "owner/repo", - Verbose: true, + Token: "test-token", + RepoDir: "/test/dir", + PR: 123, + Repo: "owner/repo", + Verbose: true, + AddComments: false, }, expectError: false, }, @@ -432,6 +442,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.AddComments != tc.config.AddComments { + t.Errorf("expected AddComments %v, got %v", tc.config.AddComments, app.config.AddComments) + } }) } } @@ -628,6 +641,110 @@ func TestInitFlags(t *testing.T) { } } +func setupAppForCommentTest(t *testing.T, addComments bool) (*App, *mockGitHubClient) { + t.Helper() + + mockGH := &mockGitHubClient{} + mockGH.ResetCommentCallTracking() + + cfg := AppConfig{ + AddComments: addComments, + } + + conf := &owners.Config{ + HighPriorityLabels: []string{"high-prio"}, + } + + app := &App{ + config: cfg, + client: mockGH, + conf: conf, + codeowners: &mockCodeOwners{}, + gitDiff: mockGitDiff{}, + } + + return app, mockGH +} + +func TestAddReviewStatusComment_ShortCircuit(t *testing.T) { + app, mockClient := setupAppForCommentTest(t, false) // AddComments = false + + // Prepare some data that *would* trigger a comment if AddComments were true + unapproved := codeowners.ReviewerGroups{ + &codeowners.ReviewerGroup{Names: []string{"@pending-reviewer"}}, + } + allRequired := codeowners.ReviewerGroups{ + &codeowners.ReviewerGroup{Names: []string{"@pending-reviewer"}}, + } + + err := app.addReviewStatusComment(allRequired, unapproved, false) + if err != nil { + t.Errorf("Expected no error when AddComments is false, but got: %v", err) + } + + if mockClient.AddCommentCalled { + t.Error("Expected AddComment not to be called when AddComments is false") + } +} + +func TestAddOptionalCcComment_ShortCircuit(t *testing.T) { + app, mockClient := setupAppForCommentTest(t, false) // AddComments = false + + // Prepare some data that *would* trigger a comment if AddComments were true + optionalReviewers := []string{"@optional-cc"} + + err := app.addOptionalCcComment(optionalReviewers) + if err != nil { + t.Errorf("Expected no error when AddComments is false, but got: %v", err) + } + + if mockClient.AddCommentCalled { + t.Error("Expected AddComment not to be called when AddComments is false") + } +} + +func TestAddReviewStatusComment_AddsComment(t *testing.T) { + app, mockClient := setupAppForCommentTest(t, true) // AddComments = true + + unapproved := codeowners.ReviewerGroups{ + &codeowners.ReviewerGroup{Names: []string{"@user1"}}, + } + allRequired := codeowners.ReviewerGroups{ + &codeowners.ReviewerGroup{Names: []string{"@user1"}}, + } + expectedComment := allRequired.ToCommentString() + + err := app.addReviewStatusComment(allRequired, unapproved, false) + + if err != nil { + t.Errorf("Unexpected error when adding comment: %v", err) + } + if !mockClient.AddCommentCalled { + t.Error("Expected AddComment to be called when AddComments is true and unapproved exist") + } + if mockClient.AddCommentInput != expectedComment { + t.Errorf("Expected comment body %q, got %q", expectedComment, mockClient.AddCommentInput) + } +} + +func TestAddOptionalCcComment_AddsComment(t *testing.T) { + app, mockClient := setupAppForCommentTest(t, true) // AddComments = true + + optionalReviewers := []string{"@cc-user1", "@cc-user2"} + expectedComment := "cc @cc-user1 @cc-user2" + + err := app.addOptionalCcComment(optionalReviewers) + if err != nil { + t.Errorf("Unexpected error when adding comment: %v", err) + } + if !mockClient.AddCommentCalled { + t.Error("Expected AddComment to be called when AddComments is true and viewers need pinging") + } + if mockClient.AddCommentInput != expectedComment { + t.Errorf("Expected comment body %q, got %q", expectedComment, mockClient.AddCommentInput) + } +} + func TestProcessApprovalsAndReviewers(t *testing.T) { maxReviews := 2 minReviews := 2 From 3732b4deac8172b2e4ccfdbe82171aa37532f13c Mon Sep 17 00:00:00 2001 From: zbedforrest Date: Wed, 26 Mar 2025 11:15:45 -0700 Subject: [PATCH 05/17] requestReviews should not return easily accesible and unmutated list of unapprovedReviewers --- main.go | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-) diff --git a/main.go b/main.go index d2d014b..5a8c70c 100644 --- a/main.go +++ b/main.go @@ -202,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 { @@ -370,20 +371,20 @@ 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 { 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) @@ -394,11 +395,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) { From aa3f5d21358150dbe9481ea7739e506d6fa90259 Mon Sep 17 00:00:00 2001 From: zbedforrest Date: Wed, 26 Mar 2025 11:36:49 -0700 Subject: [PATCH 06/17] short circuit review requests when in quiet mode --- main.go | 5 +++++ main_test.go | 48 +++++++++++++++++++++++++++++++++++++++--------- 2 files changed, 44 insertions(+), 9 deletions(-) diff --git a/main.go b/main.go index 5a8c70c..39e56a9 100644 --- a/main.go +++ b/main.go @@ -372,6 +372,11 @@ func (a *App) processApprovals(ghApprovals []*gh.CurrentApproval) (int, error) { } func (a *App) requestReviews() error { + if !a.config.AddComments { + 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) diff --git a/main_test.go b/main_test.go index 4218c67..66d67ed 100644 --- a/main_test.go +++ b/main_test.go @@ -131,6 +131,7 @@ type mockGitHubClient struct { approvePRError error AddCommentCalled bool AddCommentInput string + RequestReviewersCalled bool } func (m *mockGitHubClient) PR() *github.PullRequest { @@ -171,6 +172,7 @@ func (m *mockGitHubClient) DismissStaleReviews(approvals []*gh.CurrentApproval) } func (m *mockGitHubClient) RequestReviewers(reviewers []string) error { + m.RequestReviewersCalled = true return m.requestReviewersError } @@ -251,9 +253,10 @@ func (m *mockGitHubClient) IsInComments(comment string, since *time.Time) (bool, return false, nil } -func (m *mockGitHubClient) ResetCommentCallTracking() { +func (m *mockGitHubClient) ResetGHClientTracking() { m.AddCommentCalled = false m.AddCommentInput = "" + m.RequestReviewersCalled = false } func (m *mockGitHubClient) IsSubstringInComments(substring string, since *time.Time) (bool, error) { @@ -641,11 +644,11 @@ func TestInitFlags(t *testing.T) { } } -func setupAppForCommentTest(t *testing.T, addComments bool) (*App, *mockGitHubClient) { +func setupAppForTest(t *testing.T, addComments bool) (*App, *mockGitHubClient) { t.Helper() mockGH := &mockGitHubClient{} - mockGH.ResetCommentCallTracking() + mockGH.ResetGHClientTracking() cfg := AppConfig{ AddComments: addComments, @@ -654,12 +657,17 @@ func setupAppForCommentTest(t *testing.T, addComments bool) (*App, *mockGitHubCl 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: &mockCodeOwners{}, + codeowners: mockOwners, gitDiff: mockGitDiff{}, } @@ -667,7 +675,7 @@ func setupAppForCommentTest(t *testing.T, addComments bool) (*App, *mockGitHubCl } func TestAddReviewStatusComment_ShortCircuit(t *testing.T) { - app, mockClient := setupAppForCommentTest(t, false) // AddComments = false + app, mockClient := setupAppForTest(t, false) // AddComments = false // Prepare some data that *would* trigger a comment if AddComments were true unapproved := codeowners.ReviewerGroups{ @@ -688,7 +696,7 @@ func TestAddReviewStatusComment_ShortCircuit(t *testing.T) { } func TestAddOptionalCcComment_ShortCircuit(t *testing.T) { - app, mockClient := setupAppForCommentTest(t, false) // AddComments = false + app, mockClient := setupAppForTest(t, false) // AddComments = false // Prepare some data that *would* trigger a comment if AddComments were true optionalReviewers := []string{"@optional-cc"} @@ -704,7 +712,7 @@ func TestAddOptionalCcComment_ShortCircuit(t *testing.T) { } func TestAddReviewStatusComment_AddsComment(t *testing.T) { - app, mockClient := setupAppForCommentTest(t, true) // AddComments = true + app, mockClient := setupAppForTest(t, true) // AddComments = true unapproved := codeowners.ReviewerGroups{ &codeowners.ReviewerGroup{Names: []string{"@user1"}}, @@ -728,7 +736,7 @@ func TestAddReviewStatusComment_AddsComment(t *testing.T) { } func TestAddOptionalCcComment_AddsComment(t *testing.T) { - app, mockClient := setupAppForCommentTest(t, true) // AddComments = true + app, mockClient := setupAppForTest(t, true) // AddComments = true optionalReviewers := []string{"@cc-user1", "@cc-user2"} expectedComment := "cc @cc-user1 @cc-user2" @@ -745,6 +753,22 @@ func TestAddOptionalCcComment_AddsComment(t *testing.T) { } } +func TestRequestReviews_ShortCircuit(t *testing.T) { + app, mockClient := setupAppForTest(t, false) // AddComments = false + + mockClient.currentlyRequested = []string{} + mockClient.alreadyReviewed = []string{} + + err := app.requestReviews() + if err != nil { + t.Errorf("Expected no error when AddComments is false, but got: %v", err) + } + + if mockClient.RequestReviewersCalled { + t.Error("Expected client.RequestReviewers not to be called when AddComments is false") + } +} + func TestProcessApprovalsAndReviewers(t *testing.T) { maxReviews := 2 minReviews := 2 @@ -992,7 +1016,9 @@ func TestProcessApprovalsAndReviewers(t *testing.T) { } app := &App{ - config: AppConfig{}, + config: AppConfig{ + AddComments: true, + }, client: mockGH, codeowners: mockOwners, gitDiff: mockGitDiff{ @@ -1013,6 +1039,10 @@ func TestProcessApprovalsAndReviewers(t *testing.T) { }, } + var err error + if tc.name == "error getting currently requested" { + err = nil + } success, _, err := app.processApprovalsAndReviewers() if tc.expectError { if err == nil { From 2e2e131a71d449b27df64ca683641f1c29c809d0 Mon Sep 17 00:00:00 2001 From: zbedforrest Date: Wed, 26 Mar 2025 11:44:09 -0700 Subject: [PATCH 07/17] rename flag to quiet --- main.go | 61 ++++++++++++++++++++++++++-------------------------- main_test.go | 52 ++++++++++++++++++++++---------------------- 2 files changed, 56 insertions(+), 57 deletions(-) diff --git a/main.go b/main.go index 39e56a9..493d9df 100644 --- a/main.go +++ b/main.go @@ -20,12 +20,12 @@ import ( // AppConfig holds the application configuration type AppConfig struct { - Token string - RepoDir string - PR int - Repo string - Verbose bool - AddComments bool + Token string + RepoDir string + PR int + Repo string + Verbose bool + Quiet bool } // App represents the application with its dependencies @@ -39,26 +39,25 @@ type App struct { // Flags holds the command line flags type Flags struct { - Token *string - RepoDir *string - PR *int - Repo *string - Verbose *bool - AddComments *bool + Token *string + RepoDir *string + PR *int + Repo *string + Verbose *bool + Quiet *bool } var ( flags = &Flags{ - Token: flag.String("token", getEnv("INPUT_GITHUB-TOKEN", ""), "GitHub authentication token"), - RepoDir: flag.String("dir", getEnv("GITHUB_WORKSPACE", "/"), "Path to local Git repo"), - 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"), - AddComments: flag.Bool("comments", ignoreError(strconv.ParseBool(getEnv("INPUT_ADD_COMMENTS", "1"))), "Add comments to Pull Request"), - } - WarningBuffer = bytes.NewBuffer([]byte{}) - InfoBuffer = bytes.NewBuffer([]byte{}) - QUIET_MODE_TEST = true + Token: flag.String("token", getEnv("INPUT_GITHUB-TOKEN", ""), "GitHub authentication token"), + RepoDir: flag.String("dir", getEnv("GITHUB_WORKSPACE", "/"), "Path to local Git repo"), + 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{}) ) // initFlags initializes and parses command line flags @@ -263,7 +262,7 @@ func (a *App) processApprovalsAndReviewers() (bool, string, error) { 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.AddComments || len(unapprovedOwners) == 0 { + if a.config.Quiet || len(unapprovedOwners) == 0 { printDebug("Skipping review status comment (disabled or no unapproved owners).\n") return nil } @@ -303,7 +302,7 @@ func (a *App) addReviewStatusComment(allRequiredOwners, unapprovedOwners codeown 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.AddComments || len(allOptionalReviewerNames) == 0 { + if a.config.Quiet || len(allOptionalReviewerNames) == 0 { printDebug("Skipping optional CC comment (disabled or no optional reviewers).\n") return nil } @@ -372,7 +371,7 @@ func (a *App) processApprovals(ghApprovals []*gh.CurrentApproval) (int, error) { } func (a *App) requestReviews() error { - if !a.config.AddComments { + if a.config.Quiet { printDebug("Skipping review requests (disabled in quiet mode).\n") return nil } @@ -472,12 +471,12 @@ func main() { } cfg := AppConfig{ - Token: *flags.Token, - RepoDir: *flags.RepoDir, - PR: *flags.PR, - Repo: *flags.Repo, - Verbose: *flags.Verbose, - AddComments: *flags.AddComments, + Token: *flags.Token, + RepoDir: *flags.RepoDir, + 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 66d67ed..8c773f9 100644 --- a/main_test.go +++ b/main_test.go @@ -388,12 +388,12 @@ func TestNewApp(t *testing.T) { { name: "valid config", config: AppConfig{ - Token: "test-token", - RepoDir: "/test/dir", - PR: 123, - Repo: "owner/repo", - Verbose: true, - AddComments: false, + Token: "test-token", + RepoDir: "/test/dir", + PR: 123, + Repo: "owner/repo", + Verbose: true, + Quiet: true, }, expectError: false, }, @@ -445,8 +445,8 @@ 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.AddComments != tc.config.AddComments { - t.Errorf("expected AddComments %v, got %v", tc.config.AddComments, app.config.AddComments) + if app.config.Quiet != tc.config.Quiet { + t.Errorf("expected Quiet %v, got %v", tc.config.Quiet, app.config.Quiet) } }) } @@ -644,14 +644,14 @@ func TestInitFlags(t *testing.T) { } } -func setupAppForTest(t *testing.T, addComments bool) (*App, *mockGitHubClient) { +func setupAppForTest(t *testing.T, quiet bool) (*App, *mockGitHubClient) { t.Helper() mockGH := &mockGitHubClient{} mockGH.ResetGHClientTracking() cfg := AppConfig{ - AddComments: addComments, + Quiet: quiet, } conf := &owners.Config{ @@ -675,9 +675,9 @@ func setupAppForTest(t *testing.T, addComments bool) (*App, *mockGitHubClient) { } func TestAddReviewStatusComment_ShortCircuit(t *testing.T) { - app, mockClient := setupAppForTest(t, false) // AddComments = false + app, mockClient := setupAppForTest(t, true) // Quiet = true - // Prepare some data that *would* trigger a comment if AddComments were true + // Prepare some data that *would* trigger a comment if Quiet were false unapproved := codeowners.ReviewerGroups{ &codeowners.ReviewerGroup{Names: []string{"@pending-reviewer"}}, } @@ -687,32 +687,32 @@ func TestAddReviewStatusComment_ShortCircuit(t *testing.T) { err := app.addReviewStatusComment(allRequired, unapproved, false) if err != nil { - t.Errorf("Expected no error when AddComments is false, but got: %v", err) + t.Errorf("Expected no error when Quiet is true, but got: %v", err) } if mockClient.AddCommentCalled { - t.Error("Expected AddComment not to be called when AddComments is false") + t.Error("Expected AddComment not to be called when Quiet is true") } } func TestAddOptionalCcComment_ShortCircuit(t *testing.T) { - app, mockClient := setupAppForTest(t, false) // AddComments = false + app, mockClient := setupAppForTest(t, true) // Quiet = true - // Prepare some data that *would* trigger a comment if AddComments were true + // Prepare some data that *would* trigger a comment if Quiet were false optionalReviewers := []string{"@optional-cc"} err := app.addOptionalCcComment(optionalReviewers) if err != nil { - t.Errorf("Expected no error when AddComments is false, but got: %v", err) + t.Errorf("Expected no error when Quiet is true, but got: %v", err) } if mockClient.AddCommentCalled { - t.Error("Expected AddComment not to be called when AddComments is false") + t.Error("Expected AddComment not to be called when Quiet is true") } } func TestAddReviewStatusComment_AddsComment(t *testing.T) { - app, mockClient := setupAppForTest(t, true) // AddComments = true + app, mockClient := setupAppForTest(t, false) // Quiet = false unapproved := codeowners.ReviewerGroups{ &codeowners.ReviewerGroup{Names: []string{"@user1"}}, @@ -728,7 +728,7 @@ func TestAddReviewStatusComment_AddsComment(t *testing.T) { t.Errorf("Unexpected error when adding comment: %v", err) } if !mockClient.AddCommentCalled { - t.Error("Expected AddComment to be called when AddComments is true and unapproved exist") + t.Error("Expected AddComment to be called when Quiet is false and unapproved exist") } if mockClient.AddCommentInput != expectedComment { t.Errorf("Expected comment body %q, got %q", expectedComment, mockClient.AddCommentInput) @@ -736,7 +736,7 @@ func TestAddReviewStatusComment_AddsComment(t *testing.T) { } func TestAddOptionalCcComment_AddsComment(t *testing.T) { - app, mockClient := setupAppForTest(t, true) // AddComments = true + app, mockClient := setupAppForTest(t, false) // Quiet = false optionalReviewers := []string{"@cc-user1", "@cc-user2"} expectedComment := "cc @cc-user1 @cc-user2" @@ -746,7 +746,7 @@ func TestAddOptionalCcComment_AddsComment(t *testing.T) { t.Errorf("Unexpected error when adding comment: %v", err) } if !mockClient.AddCommentCalled { - t.Error("Expected AddComment to be called when AddComments is true and viewers need pinging") + t.Error("Expected AddComment to be called when Quiet is false and viewers need to be pinged") } if mockClient.AddCommentInput != expectedComment { t.Errorf("Expected comment body %q, got %q", expectedComment, mockClient.AddCommentInput) @@ -754,18 +754,18 @@ func TestAddOptionalCcComment_AddsComment(t *testing.T) { } func TestRequestReviews_ShortCircuit(t *testing.T) { - app, mockClient := setupAppForTest(t, false) // AddComments = false + app, mockClient := setupAppForTest(t, true) // Quiet = true mockClient.currentlyRequested = []string{} mockClient.alreadyReviewed = []string{} err := app.requestReviews() if err != nil { - t.Errorf("Expected no error when AddComments is false, but got: %v", err) + t.Errorf("Expected no error when Quiet is true, but got: %v", err) } if mockClient.RequestReviewersCalled { - t.Error("Expected client.RequestReviewers not to be called when AddComments is false") + t.Error("Expected client.RequestReviewers not to be called when Quiet is true") } } @@ -1017,7 +1017,7 @@ func TestProcessApprovalsAndReviewers(t *testing.T) { app := &App{ config: AppConfig{ - AddComments: true, + Quiet: false, }, client: mockGH, codeowners: mockOwners, From eafd96eae42336805d6e1f28416271fd883f1dd2 Mon Sep 17 00:00:00 2001 From: zbedforrest Date: Wed, 26 Mar 2025 12:31:09 -0700 Subject: [PATCH 08/17] lint test module and update readme --- README.md | 41 ++++++++++++++++++++++++++++++++++++----- main_test.go | 4 ---- 2 files changed, 36 insertions(+), 9 deletions(-) diff --git a/README.md b/README.md index 328cd1c..cc55a3d 100644 --- a/README.md +++ b/README.md @@ -70,7 +70,7 @@ jobs: uses: actions/checkout@v4 with: fetch-depth: 0 - + - name: 'Codeowners Plus' uses: multimediallc/codeowners-plus@v0.1.0 with: @@ -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,45 @@ 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 or the `-quiet=true` flag in the CLI. + +**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 # Or your desired version + with: + # ... other inputs ... + quiet: 'true' + ``` +* **CLI:** Use the flag `-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. @@ -279,4 +311,3 @@ Available commands are: ## Future Features * Inline ownership comments for having owners for specific functions, classes, etc. - diff --git a/main_test.go b/main_test.go index 8c773f9..f380616 100644 --- a/main_test.go +++ b/main_test.go @@ -1039,10 +1039,6 @@ func TestProcessApprovalsAndReviewers(t *testing.T) { }, } - var err error - if tc.name == "error getting currently requested" { - err = nil - } success, _, err := app.processApprovalsAndReviewers() if tc.expectError { if err == nil { From 3115a3cfaa31565b2abfa89485e5bfcfce365d6d Mon Sep 17 00:00:00 2001 From: zbedforrest Date: Wed, 26 Mar 2025 12:34:59 -0700 Subject: [PATCH 09/17] tweak readme --- README.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/README.md b/README.md index cc55a3d..354a3ce 100644 --- a/README.md +++ b/README.md @@ -284,7 +284,7 @@ Even in quiet mode, the tool still performs all its internal calculations: deter * **GitHub Action:** Set the `quiet` input to `'true'`. ```yaml - name: 'Codeowners Plus (Quiet)' - uses: multimediallc/codeowners-plus@v0.1.0 # Or your desired version + uses: multimediallc/codeowners-plus@v0.1.0 with: # ... other inputs ... quiet: 'true' From c8f65ce2bf200d20520c089ec74a52499d605d4a Mon Sep 17 00:00:00 2001 From: zbedforrest Date: Wed, 26 Mar 2025 12:36:52 -0700 Subject: [PATCH 10/17] specify quiet as false in codeowners workflow for this repo --- .github/workflows/codeowners.yml | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/.github/workflows/codeowners.yml b/.github/workflows/codeowners.yml index d9f9904..3aea78c 100644 --- a/.github/workflows/codeowners.yml +++ b/.github/workflows/codeowners.yml @@ -24,10 +24,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: false From 28f4de3bacfa829b45f3b7741c9e25190b5d3935 Mon Sep 17 00:00:00 2001 From: zbedforrest Date: Wed, 26 Mar 2025 12:50:23 -0700 Subject: [PATCH 11/17] update coverage badge --- README.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/README.md b/README.md index 354a3ce..c10c2c2 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.1%25-brightgreen) +![Coverage](https://img.shields.io/badge/Coverage-82.7%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) From 3d8bf6a43a63323e9d45d42a8b9b73e911292451 Mon Sep 17 00:00:00 2001 From: zbedforrest Date: Wed, 26 Mar 2025 13:04:05 -0700 Subject: [PATCH 12/17] test commit, no real change --- README.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/README.md b/README.md index c10c2c2..7ca9bc7 100644 --- a/README.md +++ b/README.md @@ -72,7 +72,7 @@ jobs: fetch-depth: 0 - name: 'Codeowners Plus' - uses: multimediallc/codeowners-plus@v0.1.0 + uses: multimediallc/codeowners-plus@v0.1.2 with: github-token: '${{ secrets.GITHUB_TOKEN }}' pr: '${{ github.event.pull_request.number }}' From c3ee6eae6686f916ba0ee3a0fa3003054c40057e Mon Sep 17 00:00:00 2001 From: zbedforrest Date: Wed, 26 Mar 2025 13:14:36 -0700 Subject: [PATCH 13/17] update coverage --- README.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/README.md b/README.md index 7ca9bc7..257f20f 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-82.7%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) From e10388b35b257342d9d74db0ef73843c6ff42ac7 Mon Sep 17 00:00:00 2001 From: zbedforrest Date: Wed, 26 Mar 2025 13:19:30 -0700 Subject: [PATCH 14/17] add link to contibuting page --- README.md | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/README.md b/README.md index 257f20f..abfeac6 100644 --- a/README.md +++ b/README.md @@ -308,6 +308,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. From f7d57058911ec376a77e0897628308aa7cb79d73 Mon Sep 17 00:00:00 2001 From: zbedforrest Date: Wed, 26 Mar 2025 15:56:05 -0700 Subject: [PATCH 15/17] update readme's and enable quiet mode for this repo based on draft status --- .github/workflows/codeowners.yml | 3 +-- CONTRIBUTING.md | 2 +- README.md | 13 ++++++------- 3 files changed, 8 insertions(+), 10 deletions(-) diff --git a/.github/workflows/codeowners.yml b/.github/workflows/codeowners.yml index 3aea78c..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 @@ -31,4 +30,4 @@ jobs: github-token: '${{ secrets.GITHUB_TOKEN }}' pr: '${{ github.event.pull_request.number }}' verbose: true - quiet: false + 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 abfeac6..1747ae6 100644 --- a/README.md +++ b/README.md @@ -261,25 +261,25 @@ Codeowners approval required for this PR: - @user2 ``` -### Quiet Mode +## Quiet Mode -You can run Codeowners Plus in a "quiet" mode using the `quiet` input in the GitHub Action or the `-quiet=true` flag in the CLI. +You can run Codeowners Plus in a "quiet" mode using the `quiet` input in the GitHub Action. -**When Quiet Mode is Enabled:** +### 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:** +### 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:** +### 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:** +### Activation: * **GitHub Action:** Set the `quiet` input to `'true'`. ```yaml @@ -289,7 +289,6 @@ Even in quiet mode, the tool still performs all its internal calculations: deter # ... other inputs ... quiet: 'true' ``` -* **CLI:** Use the flag `-quiet=true`. **Default:** Quiet mode is **disabled** (`false`) by default. From d44bc64d9527e0699409fd1ef0e442327ea66271 Mon Sep 17 00:00:00 2001 From: zbedforrest Date: Wed, 26 Mar 2025 16:40:24 -0700 Subject: [PATCH 16/17] use tabular tests --- main_test.go | 194 ++++++++++++++++++++++++++++++++------------------- 1 file changed, 122 insertions(+), 72 deletions(-) diff --git a/main_test.go b/main_test.go index bebbc5f..88d7fbd 100644 --- a/main_test.go +++ b/main_test.go @@ -529,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 { @@ -674,98 +674,148 @@ func setupAppForTest(t *testing.T, quiet bool) (*App, *mockGitHubClient) { return app, mockGH } -func TestAddReviewStatusComment_ShortCircuit(t *testing.T) { - app, mockClient := setupAppForTest(t, true) // Quiet = true - - // Prepare some data that *would* trigger a comment if Quiet were false - unapproved := codeowners.ReviewerGroups{ - &codeowners.ReviewerGroup{Names: []string{"@pending-reviewer"}}, +func TestAddReviewStatusComment(t *testing.T) { + user1Group := codeowners.ReviewerGroups{ + &codeowners.ReviewerGroup{Names: []string{"@user1"}}, } - allRequired := codeowners.ReviewerGroups{ + pendingReviewerGroup := codeowners.ReviewerGroups{ &codeowners.ReviewerGroup{Names: []string{"@pending-reviewer"}}, } - err := app.addReviewStatusComment(allRequired, unapproved, false) - if err != nil { - t.Errorf("Expected no error when Quiet is true, but got: %v", err) - } - - if mockClient.AddCommentCalled { - t.Error("Expected AddComment not to be called when Quiet is true") - } -} - -func TestAddOptionalCcComment_ShortCircuit(t *testing.T) { - app, mockClient := setupAppForTest(t, true) // Quiet = true - - // Prepare some data that *would* trigger a comment if Quiet were false - optionalReviewers := []string{"@optional-cc"} - - err := app.addOptionalCcComment(optionalReviewers) - if err != nil { - t.Errorf("Expected no error when Quiet is true, but got: %v", err) + 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(), + }, } - if mockClient.AddCommentCalled { - t.Error("Expected AddComment not to be called when Quiet is true") + 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 TestAddReviewStatusComment_AddsComment(t *testing.T) { - app, mockClient := setupAppForTest(t, false) // Quiet = false +func TestAddOptionalCcComment(t *testing.T) { + optionalSingle := []string{"@optional-cc"} + optionalMultiple := []string{"@cc-user1", "@cc-user2"} - unapproved := codeowners.ReviewerGroups{ - &codeowners.ReviewerGroup{Names: []string{"@user1"}}, - } - allRequired := codeowners.ReviewerGroups{ - &codeowners.ReviewerGroup{Names: []string{"@user1"}}, + 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, " ")), + }, } - expectedComment := allRequired.ToCommentString() - err := app.addReviewStatusComment(allRequired, unapproved, false) + for _, tc := range tt { + t.Run(tc.name, func(t *testing.T) { + app, mockClient := setupAppForTest(t, tc.quiet) + mockClient.ResetGHClientTracking() - if err != nil { - t.Errorf("Unexpected error when adding comment: %v", err) - } - if !mockClient.AddCommentCalled { - t.Error("Expected AddComment to be called when Quiet is false and unapproved exist") - } - if mockClient.AddCommentInput != expectedComment { - t.Errorf("Expected comment body %q, got %q", expectedComment, mockClient.AddCommentInput) + 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 TestAddOptionalCcComment_AddsComment(t *testing.T) { - app, mockClient := setupAppForTest(t, false) // Quiet = false - - optionalReviewers := []string{"@cc-user1", "@cc-user2"} - expectedComment := "cc @cc-user1 @cc-user2" - - err := app.addOptionalCcComment(optionalReviewers) - if err != nil { - t.Errorf("Unexpected error when adding comment: %v", err) - } - if !mockClient.AddCommentCalled { - t.Error("Expected AddComment to be called when Quiet is false and viewers need to be pinged") - } - if mockClient.AddCommentInput != expectedComment { - t.Errorf("Expected comment body %q, got %q", expectedComment, 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, + }, } -} -func TestRequestReviews_ShortCircuit(t *testing.T) { - app, mockClient := setupAppForTest(t, true) // Quiet = true + for _, tc := range tt { + t.Run(tc.name, func(t *testing.T) { + app, mockClient := setupAppForTest(t, tc.quiet) + mockClient.ResetGHClientTracking() - mockClient.currentlyRequested = []string{} - mockClient.alreadyReviewed = []string{} + mockClient.currentlyRequested = tc.mockCurrentlyRequested + mockClient.alreadyReviewed = tc.mockAlreadyReviewed - err := app.requestReviews() - if err != nil { - t.Errorf("Expected no error when Quiet is true, but got: %v", err) - } + err := app.requestReviews() - if mockClient.RequestReviewersCalled { - t.Error("Expected client.RequestReviewers not to be called when Quiet is true") + 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) + } + }) } } From 15f02ca62d47755ba1123fe04aaadd33db7513b2 Mon Sep 17 00:00:00 2001 From: zach bednarke <112985143+zbedforrest@users.noreply.github.com> Date: Wed, 26 Mar 2025 16:59:33 -0700 Subject: [PATCH 17/17] Update README.md Co-authored-by: Hans Baker --- README.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/README.md b/README.md index 1747ae6..bf7ac87 100644 --- a/README.md +++ b/README.md @@ -72,7 +72,7 @@ jobs: fetch-depth: 0 - name: 'Codeowners Plus' - uses: multimediallc/codeowners-plus@v0.1.2 + uses: multimediallc/codeowners-plus@v0.1.4 with: github-token: '${{ secrets.GITHUB_TOKEN }}' pr: '${{ github.event.pull_request.number }}'