From 83e86168b3dabffbdaabc6a7c12ddae762f56992 Mon Sep 17 00:00:00 2001 From: Kevin Zhu Date: Fri, 27 Jun 2025 12:02:40 -0700 Subject: [PATCH 1/7] Add detailed_owners to config --- codeowners.toml | 2 ++ internal/config/config.go | 2 ++ internal/config/config_test.go | 3 +++ 3 files changed, 7 insertions(+) diff --git a/codeowners.toml b/codeowners.toml index a859ab6..6914471 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_owners` (default false) means the codeowners will include a collapsible list of files and owners in its review comment +detailed_owners = false # `enforcement` allows you to specify how the codeowners check should be enforced [enforcement] diff --git a/internal/config/config.go b/internal/config/config.go index 85d0445..83e7e4a 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -15,6 +15,7 @@ type Config struct { Ignore []string `toml:"ignore"` Enforcement *Enforcement `toml:"enforcement"` HighPriorityLabels []string `toml:"high_priority_labels"` + DetailedOwners bool `toml:"detailed_owners"` } type Enforcement struct { @@ -34,6 +35,7 @@ func ReadConfig(path string) (*Config, error) { Ignore: []string{}, Enforcement: &Enforcement{Approval: false, FailCheck: true}, HighPriorityLabels: []string{}, + DetailedOwners: false, } fileName := path + "codeowners.toml" diff --git a/internal/config/config_test.go b/internal/config/config_test.go index d1622cf..5380a0f 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_owners = 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"}, + DetailedOwners: true, }, expectedErr: false, }, @@ -63,6 +65,7 @@ unskippable_reviewers = ["@user1"] Ignore: []string{}, Enforcement: &Enforcement{Approval: false, FailCheck: true}, HighPriorityLabels: []string{}, + DetailedOwners: false, }, expectedErr: false, }, From ffa6be5618e752faba3aca8c37faa1920c852787 Mon Sep 17 00:00:00 2001 From: Kevin Zhu Date: Tue, 1 Jul 2025 10:19:49 -0700 Subject: [PATCH 2/7] add logic --- internal/app/app.go | 23 ++++++++++++++++------- 1 file changed, 16 insertions(+), 7 deletions(-) diff --git a/internal/app/app.go b/internal/app/app.go index d80b765..59640f0 100644 --- a/internal/app/app.go +++ b/internal/app/app.go @@ -288,6 +288,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.DetailedOwners { + comment += "\n\n
Show detailed required file reviewers" + comment += a.getFileOwnersString(a.codeowners.FileRequired()) + comment += "
" + } + fiveDaysAgo := time.Now().AddDate(0, 0, -5) existingComment, existingFound, err := a.client.FindExistingComment(commentPrefix, &fiveDaysAgo) if err != nil { @@ -421,14 +427,17 @@ func (a *App) requestReviews() error { } func (a *App) printFileOwners(codeOwners codeowners.CodeOwners) { - fileRequired := 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.getFileOwnersString(codeOwners.FileRequired())) a.printDebug("File Optional:\n") - for file, reviewers := range fileOptional { - a.printDebug("- %s: %+v\n", file, reviewers.Flatten()) + a.printDebug(a.getFileOwnersString(codeOwners.FileOptional())) +} + +func (a *App) getFileOwnersString(fileReviewers map[string]codeowners.ReviewerGroups) string { + output := "" + for file, reviewers := range fileReviewers { + output += fmt.Sprintf("- %s: %+v\n", file, reviewers.Flatten()) } + return output } From 459df10818dee10c66df67a72486a8872a655a3a Mon Sep 17 00:00:00 2001 From: Kevin Zhu Date: Tue, 1 Jul 2025 10:50:44 -0700 Subject: [PATCH 3/7] Add tests --- internal/app/app.go | 10 +++---- internal/app/app_test.go | 64 ++++++++++++++++++++++++++++++++++++++++ 2 files changed, 69 insertions(+), 5 deletions(-) diff --git a/internal/app/app.go b/internal/app/app.go index 59640f0..a46d0a6 100644 --- a/internal/app/app.go +++ b/internal/app/app.go @@ -289,8 +289,8 @@ func (a *App) addReviewStatusComment(allRequiredOwners codeowners.ReviewerGroups } if a.Conf.DetailedOwners { - comment += "\n\n
Show detailed required file reviewers" - comment += a.getFileOwnersString(a.codeowners.FileRequired()) + comment += "\n\n
Show detailed file reviewers\n" + comment += a.getFileOwnersMapToString(a.codeowners.FileRequired()) comment += "
" } @@ -429,12 +429,12 @@ func (a *App) requestReviews() error { func (a *App) printFileOwners(codeOwners codeowners.CodeOwners) { codeOwners.FileRequired() a.printDebug("File Reviewers:\n") - a.printDebug(a.getFileOwnersString(codeOwners.FileRequired())) + a.printDebug(a.getFileOwnersMapToString(codeOwners.FileRequired())) a.printDebug("File Optional:\n") - a.printDebug(a.getFileOwnersString(codeOwners.FileOptional())) + a.printDebug(a.getFileOwnersMapToString(codeOwners.FileOptional())) } -func (a *App) getFileOwnersString(fileReviewers map[string]codeowners.ReviewerGroups) string { +func (a *App) getFileOwnersMapToString(fileReviewers map[string]codeowners.ReviewerGroups) string { output := "" for file, reviewers := range fileReviewers { output += fmt.Sprintf("- %s: %+v\n", file, reviewers.Flatten()) diff --git a/internal/app/app_test.go b/internal/app/app_test.go index ad3b6da..a0c02ff 100644 --- a/internal/app/app_test.go +++ b/internal/app/app_test.go @@ -1132,3 +1132,67 @@ func TestBuildOutputData(t *testing.T) { t.Errorf("expected StillRequired [@user1], got %v", output.StillRequired) } } + +func TestCommentDetailedOwners(t *testing.T) { + tt := []struct { + name string + detailedCodeowners bool + }{ + { + name: "exclude detailed owners when config off", + detailedCodeowners: false, + }, + { + name: "include detailed owners when config on", + detailedCodeowners: 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{}, + DetailedOwners: tc.detailedCodeowners, + }, + } + err := app.addReviewStatusComment(requiredOwners, false) + if err != nil { + t.Errorf("unexpected error: %v", err) + } + + detailedOwnersSnippet := "\n\n
Show detailed file reviewers\n" + + "- file1.go: [@user1 @user2]\n" + + "- file2.go: [@user1]\n" + + containsDetailedOwnersSnippet := strings.Contains(mockGH.AddCommentInput, detailedOwnersSnippet) + + if tc.detailedCodeowners != containsDetailedOwnersSnippet { + t.Errorf("expected comment to include detailed owners to be %t, got %t ", + tc.detailedCodeowners, containsDetailedOwnersSnippet) + } + + }) + } +} From 936232a92a4096384611f21b64dd09aa95ba6ce1 Mon Sep 17 00:00:00 2001 From: Kevin Zhu Date: Tue, 1 Jul 2025 10:55:24 -0700 Subject: [PATCH 4/7] rename to detailedReviewers --- codeowners.toml | 4 ++-- internal/app/app.go | 2 +- internal/app/app_test.go | 24 ++++++++++++------------ internal/config/config.go | 4 ++-- internal/config/config_test.go | 6 +++--- 5 files changed, 20 insertions(+), 20 deletions(-) diff --git a/codeowners.toml b/codeowners.toml index 6914471..7b82eb8 100644 --- a/codeowners.toml +++ b/codeowners.toml @@ -8,8 +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_owners` (default false) means the codeowners will include a collapsible list of files and owners in its review comment -detailed_owners = false +# `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 a46d0a6..97cd432 100644 --- a/internal/app/app.go +++ b/internal/app/app.go @@ -288,7 +288,7 @@ 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.DetailedOwners { + if a.Conf.DetailedReviewers { comment += "\n\n
Show detailed file reviewers\n" comment += a.getFileOwnersMapToString(a.codeowners.FileRequired()) comment += "
" diff --git a/internal/app/app_test.go b/internal/app/app_test.go index a0c02ff..5583ca2 100644 --- a/internal/app/app_test.go +++ b/internal/app/app_test.go @@ -1133,18 +1133,18 @@ func TestBuildOutputData(t *testing.T) { } } -func TestCommentDetailedOwners(t *testing.T) { +func TestCommentDetailedReviewers(t *testing.T) { tt := []struct { - name string - detailedCodeowners bool + name string + detailedReviewers bool }{ { - name: "exclude detailed owners when config off", - detailedCodeowners: false, + name: "exclude detailed owners when config off", + detailedReviewers: false, }, { - name: "include detailed owners when config on", - detailedCodeowners: true, + name: "include detailed owners when config on", + detailedReviewers: true, }, } @@ -1174,7 +1174,7 @@ func TestCommentDetailedOwners(t *testing.T) { }, Conf: &owners.Config{ HighPriorityLabels: []string{}, - DetailedOwners: tc.detailedCodeowners, + DetailedReviewers: tc.detailedReviewers, }, } err := app.addReviewStatusComment(requiredOwners, false) @@ -1182,15 +1182,15 @@ func TestCommentDetailedOwners(t *testing.T) { t.Errorf("unexpected error: %v", err) } - detailedOwnersSnippet := "\n\n
Show detailed file reviewers\n" + + DetailedReviewersSnippet := "\n\n
Show detailed file reviewers\n" + "- file1.go: [@user1 @user2]\n" + "- file2.go: [@user1]\n" - containsDetailedOwnersSnippet := strings.Contains(mockGH.AddCommentInput, detailedOwnersSnippet) + containsDetailedReviewersSnippet := strings.Contains(mockGH.AddCommentInput, DetailedReviewersSnippet) - if tc.detailedCodeowners != containsDetailedOwnersSnippet { + if tc.detailedReviewers != containsDetailedReviewersSnippet { t.Errorf("expected comment to include detailed owners to be %t, got %t ", - tc.detailedCodeowners, containsDetailedOwnersSnippet) + tc.detailedReviewers, containsDetailedReviewersSnippet) } }) diff --git a/internal/config/config.go b/internal/config/config.go index 83e7e4a..d14d940 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -15,7 +15,7 @@ type Config struct { Ignore []string `toml:"ignore"` Enforcement *Enforcement `toml:"enforcement"` HighPriorityLabels []string `toml:"high_priority_labels"` - DetailedOwners bool `toml:"detailed_owners"` + DetailedReviewers bool `toml:"detailed_reviewers"` } type Enforcement struct { @@ -35,7 +35,7 @@ func ReadConfig(path string) (*Config, error) { Ignore: []string{}, Enforcement: &Enforcement{Approval: false, FailCheck: true}, HighPriorityLabels: []string{}, - DetailedOwners: false, + DetailedReviewers: false, } fileName := path + "codeowners.toml" diff --git a/internal/config/config_test.go b/internal/config/config_test.go index 5380a0f..4c8038c 100644 --- a/internal/config/config_test.go +++ b/internal/config/config_test.go @@ -37,7 +37,7 @@ ignore = ["ignored/"] approval = true fail_check = false high_priority_labels = ["high-priority", "urgent"] -detailed_owners = true +detailed_reviewers = true `, path: "testdata/", expected: &Config{ @@ -47,7 +47,7 @@ detailed_owners = true Ignore: []string{"ignored/"}, Enforcement: &Enforcement{Approval: true, FailCheck: false}, HighPriorityLabels: []string{"high-priority", "urgent"}, - DetailedOwners: true, + DetailedReviewers: true, }, expectedErr: false, }, @@ -65,7 +65,7 @@ unskippable_reviewers = ["@user1"] Ignore: []string{}, Enforcement: &Enforcement{Approval: false, FailCheck: true}, HighPriorityLabels: []string{}, - DetailedOwners: false, + DetailedReviewers: false, }, expectedErr: false, }, From 2b1fb77a6b4925142aac79d28a44cad08e673644 Mon Sep 17 00:00:00 2001 From: Kevin Zhu Date: Wed, 2 Jul 2025 09:13:08 -0700 Subject: [PATCH 5/7] use string builder instead of concat --- internal/app/app.go | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/internal/app/app.go b/internal/app/app.go index 97cd432..538d126 100644 --- a/internal/app/app.go +++ b/internal/app/app.go @@ -427,7 +427,6 @@ func (a *App) requestReviews() error { } func (a *App) printFileOwners(codeOwners codeowners.CodeOwners) { - codeOwners.FileRequired() a.printDebug("File Reviewers:\n") a.printDebug(a.getFileOwnersMapToString(codeOwners.FileRequired())) a.printDebug("File Optional:\n") @@ -435,9 +434,10 @@ func (a *App) printFileOwners(codeOwners codeowners.CodeOwners) { } func (a *App) getFileOwnersMapToString(fileReviewers map[string]codeowners.ReviewerGroups) string { - output := "" + builder := strings.Builder{} for file, reviewers := range fileReviewers { - output += fmt.Sprintf("- %s: %+v\n", file, reviewers.Flatten()) + // builder.WriteString error return is always nil + _, _ = fmt.Fprintf(&builder, "- %s: %+v\n", file, reviewers.Flatten()) } - return output + return builder.String() } From aeffee35100833d58de2fa7fcd78e02e3197b08d Mon Sep 17 00:00:00 2001 From: Kevin Zhu Date: Wed, 2 Jul 2025 11:54:24 -0700 Subject: [PATCH 6/7] Update readme --- README.md | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/README.md b/README.md index e7cc53b..d83a83e 100644 --- a/README.md +++ b/README.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 From d0612ed9ca9b854580143ab3d106f27a17d27237 Mon Sep 17 00:00:00 2001 From: Kevin Zhu Date: Wed, 2 Jul 2025 11:57:12 -0700 Subject: [PATCH 7/7] update coverage badge --- README.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/README.md b/README.md index d83a83e..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)