diff --git a/README.md b/README.md index e7cc53b..fec013c 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-81.6%25-brightgreen) +![Coverage](https://img.shields.io/badge/Coverage-81.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) @@ -225,6 +225,10 @@ ignore = ["test_project"] # when one of these PR labels is present high_priority_labels = ["high-priority", "urgent"] +# `detailed_reviewers` (default false) means the codeowners will include a collapsible list +# of files and owners in its review comment +detailed_reviewers = true + # `enforcement` allows you to specify how the Codeowners Plus check should be enforced [enforcement] # see "Enforcement Options" below for more details diff --git a/codeowners.toml b/codeowners.toml index a859ab6..7b82eb8 100644 --- a/codeowners.toml +++ b/codeowners.toml @@ -8,6 +8,8 @@ unskippable_reviewers = ["@BakerNet"] ignore = ["test_project"] # `high_priority_lables` allows you to specify labels that should be considered high priority high_priority_labels = ["P0"] +# `detailed_reviewers` (default false) means the codeowners will include a collapsible list of files and owners in its review comment +detailed_reviewers = false # `enforcement` allows you to specify how the codeowners check should be enforced [enforcement] diff --git a/internal/app/app.go b/internal/app/app.go index 58031a1..76871e8 100644 --- a/internal/app/app.go +++ b/internal/app/app.go @@ -314,6 +314,12 @@ func (a *App) addReviewStatusComment(allRequiredOwners codeowners.ReviewerGroups comment += "\n\nThe PR has received the max number of required reviews. No further action is required." } + if a.Conf.DetailedReviewers { + comment += "\n\n
Show detailed file reviewers\n" + comment += a.getFileOwnersMapToString(a.codeowners.FileRequired()) + comment += "
" + } + fiveDaysAgo := time.Now().AddDate(0, 0, -5) existingComment, existingFound, err := a.client.FindExistingComment(commentPrefix, &fiveDaysAgo) if err != nil { @@ -447,14 +453,17 @@ func (a *App) requestReviews() error { } func (a *App) printFileOwners(codeOwners codeowners.CodeOwners) { - fileRequired := codeOwners.FileRequired() a.printDebug("File Reviewers:\n") - for file, reviewers := range fileRequired { - a.printDebug("- %s: %+v\n", file, reviewers.Flatten()) - } - fileOptional := codeOwners.FileOptional() + a.printDebug(a.getFileOwnersMapToString(codeOwners.FileRequired())) a.printDebug("File Optional:\n") - for file, reviewers := range fileOptional { - a.printDebug("- %s: %+v\n", file, reviewers.Flatten()) + a.printDebug(a.getFileOwnersMapToString(codeOwners.FileOptional())) +} + +func (a *App) getFileOwnersMapToString(fileReviewers map[string]codeowners.ReviewerGroups) string { + builder := strings.Builder{} + for file, reviewers := range fileReviewers { + // builder.WriteString error return is always nil + _, _ = fmt.Fprintf(&builder, "- %s: %+v\n", file, reviewers.Flatten()) } + return builder.String() } diff --git a/internal/app/app_test.go b/internal/app/app_test.go index b775921..ee312d9 100644 --- a/internal/app/app_test.go +++ b/internal/app/app_test.go @@ -1178,3 +1178,67 @@ func TestBuildOutputData(t *testing.T) { t.Errorf("expected StillRequired [@user1], got %v", output.StillRequired) } } + +func TestCommentDetailedReviewers(t *testing.T) { + tt := []struct { + name string + detailedReviewers bool + }{ + { + name: "exclude detailed owners when config off", + detailedReviewers: false, + }, + { + name: "include detailed owners when config on", + detailedReviewers: true, + }, + } + + for _, tc := range tt { + t.Run(tc.name, func(t *testing.T) { + mockGH := &mockGitHubClient{} + + requiredOwners := codeowners.ReviewerGroups{ + &codeowners.ReviewerGroup{Names: []string{"@user1", "@user2"}}, + } + app := &App{ + config: &Config{ + InfoBuffer: io.Discard, + WarningBuffer: io.Discard, + }, + client: mockGH, + codeowners: &mockCodeOwners{ + fileRequiredMap: map[string]codeowners.ReviewerGroups{ + "file1.go": { + &codeowners.ReviewerGroup{Names: []string{"@user1", "@user2"}}, + }, + "file2.go": { + &codeowners.ReviewerGroup{Names: []string{"@user1"}}, + }, + }, + requiredOwners: requiredOwners, + }, + Conf: &owners.Config{ + HighPriorityLabels: []string{}, + DetailedReviewers: tc.detailedReviewers, + }, + } + err := app.addReviewStatusComment(requiredOwners, false) + if err != nil { + t.Errorf("unexpected error: %v", err) + } + + DetailedReviewersSnippet := "\n\n
Show detailed file reviewers\n" + + "- file1.go: [@user1 @user2]\n" + + "- file2.go: [@user1]\n" + + containsDetailedReviewersSnippet := strings.Contains(mockGH.AddCommentInput, DetailedReviewersSnippet) + + if tc.detailedReviewers != containsDetailedReviewersSnippet { + t.Errorf("expected comment to include detailed owners to be %t, got %t ", + tc.detailedReviewers, containsDetailedReviewersSnippet) + } + + }) + } +} diff --git a/internal/config/config.go b/internal/config/config.go index 7090e09..80190cc 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -16,6 +16,7 @@ type Config struct { Enforcement *Enforcement `toml:"enforcement"` HighPriorityLabels []string `toml:"high_priority_labels"` AdminBypass *AdminBypass `toml:"admin_bypass"` + DetailedReviewers bool `toml:"detailed_reviewers"` } type Enforcement struct { @@ -41,6 +42,7 @@ func ReadConfig(path string) (*Config, error) { Enforcement: &Enforcement{Approval: false, FailCheck: true}, HighPriorityLabels: []string{}, AdminBypass: &AdminBypass{Enabled: false, AllowedUsers: []string{}}, + DetailedReviewers: false, } fileName := path + "codeowners.toml" diff --git a/internal/config/config_test.go b/internal/config/config_test.go index d1622cf..4c8038c 100644 --- a/internal/config/config_test.go +++ b/internal/config/config_test.go @@ -37,6 +37,7 @@ ignore = ["ignored/"] approval = true fail_check = false high_priority_labels = ["high-priority", "urgent"] +detailed_reviewers = true `, path: "testdata/", expected: &Config{ @@ -46,6 +47,7 @@ high_priority_labels = ["high-priority", "urgent"] Ignore: []string{"ignored/"}, Enforcement: &Enforcement{Approval: true, FailCheck: false}, HighPriorityLabels: []string{"high-priority", "urgent"}, + DetailedReviewers: true, }, expectedErr: false, }, @@ -63,6 +65,7 @@ unskippable_reviewers = ["@user1"] Ignore: []string{}, Enforcement: &Enforcement{Approval: false, FailCheck: true}, HighPriorityLabels: []string{}, + DetailedReviewers: false, }, expectedErr: false, },