From f9e1305b299c895b15bad71d73a897ca761df08b Mon Sep 17 00:00:00 2001 From: BakerNet Date: Tue, 6 Jan 2026 14:47:33 -0800 Subject: [PATCH 1/4] Make codeowners checks case-insensitive --- internal/app/app.go | 11 +-- internal/github/approvals.go | 1 + internal/github/gh.go | 11 +-- internal/github/gh_test.go | 2 +- pkg/codeowners/codeowners.go | 20 +++-- pkg/codeowners/codeowners_test.go | 128 +++++++++++++++++++++++++++ pkg/codeowners/normalize.go | 24 +++++ pkg/codeowners/normalize_test.go | 142 ++++++++++++++++++++++++++++++ pkg/codeowners/reviewers.go | 26 +++++- pkg/codeowners/reviewers_test.go | 106 ++++++++++++++++++++++ 10 files changed, 452 insertions(+), 19 deletions(-) create mode 100644 pkg/codeowners/normalize.go create mode 100644 pkg/codeowners/normalize_test.go diff --git a/internal/app/app.go b/internal/app/app.go index 26234e9..e70dae5 100644 --- a/internal/app/app.go +++ b/internal/app/app.go @@ -177,9 +177,7 @@ func (a *App) processApprovalsAndReviewers() (bool, string, []string, error) { // Get optional reviewers allOptionalReviewerNames := a.codeowners.AllOptional().Flatten() - allOptionalReviewerNames = f.Filtered(allOptionalReviewerNames, func(name string) bool { - return !slices.Contains(allRequiredOwnerNames, name) - }) + allOptionalReviewerNames = codeowners.FilterOutNames(allOptionalReviewerNames, allRequiredOwnerNames) a.printDebug("All Optional Reviewers: %s\n", allOptionalReviewerNames) // Initialize user reviewer map @@ -229,7 +227,7 @@ func (a *App) processApprovalsAndReviewers() (bool, string, []string, error) { 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 { + if validApprovalCount >= *a.Conf.MaxReviews && !unapprovedOwners.ContainsAny(a.Conf.UnskippableReviewers) { maxReviewsMet = true } } @@ -431,7 +429,10 @@ func (a *App) processTokenOwnerApproval() (*gh.CurrentApproval, error) { } func (a *App) processApprovals(ghApprovals []*gh.CurrentApproval) (int, error) { - fileReviewers := f.MapMap(a.codeowners.FileRequired(), func(reviewers codeowners.ReviewerGroups) []string { return reviewers.Flatten() }) + // Create file reviewer map with normalized names for case-insensitive comparison + fileReviewers := f.MapMap(a.codeowners.FileRequired(), func(reviewers codeowners.ReviewerGroups) []string { + return codeowners.NormalizeUsernames(reviewers.Flatten()) + }) var approvers []string var approvalsToDismiss []*gh.CurrentApproval diff --git a/internal/github/approvals.go b/internal/github/approvals.go index 63762ab..f41891b 100644 --- a/internal/github/approvals.go +++ b/internal/github/approvals.go @@ -60,6 +60,7 @@ func checkStale( // for each file in the changes since approval // if the file is owned by the approval owner, mark stale // else, mark all overlapping owners as satisfied + // Note: both fileReviewerMap values and approval.inner.Reviewers contain normalized (lowercase) names stale := false for _, diffFile := range approval.Diff { fileOwners, ok := fileReviewerMap[diffFile.FileName] diff --git a/internal/github/gh.go b/internal/github/gh.go index f885e35..85f894f 100644 --- a/internal/github/gh.go +++ b/internal/github/gh.go @@ -11,6 +11,7 @@ import ( "github.com/google/go-github/v63/github" "github.com/multimediallc/codeowners-plus/internal/git" + "github.com/multimediallc/codeowners-plus/pkg/codeowners" f "github.com/multimediallc/codeowners-plus/pkg/functional" ) @@ -171,7 +172,7 @@ func (gh *GHClient) InitReviews() error { func (gh *GHClient) approvals() []*github.PullRequestReview { seen := make(map[string]bool, 0) approvals := f.Filtered(gh.reviews, func(approval *github.PullRequestReview) bool { - userName := approval.GetUser().GetLogin() + userName := codeowners.NormalizeUsername(approval.GetUser().GetLogin()) if _, ok := seen[userName]; ok { // we only care about the most recent reviews for each user return false @@ -206,7 +207,7 @@ func (gh *GHClient) ContainsValidBypassApproval(allowedUsers []string) (bool, er // Check if user is in allowed users list for _, allowedUser := range allowedUsers { - if username == allowedUser { + if codeowners.NormalizeUsername(username) == codeowners.NormalizeUsername(allowedUser) { return true, nil } } @@ -253,7 +254,7 @@ func (gh *GHClient) FindUserApproval(ghUser string) (*CurrentApproval, error) { } } review, found := f.Find(gh.approvals(), func(review *github.PullRequestReview) bool { - return review.GetUser().GetLogin() == ghUser + return codeowners.NormalizeUsername(review.GetUser().GetLogin()) == codeowners.NormalizeUsername(ghUser) }) if !found { @@ -604,7 +605,7 @@ func makeGHUserReviwerMap(reviewers []string, teamFetcher func(string, string) [ for _, reviewer := range reviewers { reviewerString := reviewer[1:] // trim the @ // Add the team or user to the map - insertReviewer(reviewerString, reviewer) + insertReviewer(codeowners.NormalizeUsername(reviewerString), reviewer) if !strings.Contains(reviewerString, "/") { // This is a user continue @@ -615,7 +616,7 @@ func makeGHUserReviwerMap(reviewers []string, teamFetcher func(string, string) [ users := teamFetcher(org, team) // Add the team members to the map for _, user := range users { - insertReviewer(user.GetLogin(), reviewer) + insertReviewer(codeowners.NormalizeUsername(user.GetLogin()), reviewer) } } return userReviewerMap diff --git a/internal/github/gh_test.go b/internal/github/gh_test.go index 018c20d..b1c7ae5 100644 --- a/internal/github/gh_test.go +++ b/internal/github/gh_test.go @@ -231,7 +231,7 @@ func TestMakeGHUserReviewerMap(t *testing.T) { "user3": []string{"@org/team1"}, "org/team1": []string{"@org/team1"}, "org/team2": []string{"@org/team2"}, - "other/teamX": []string{"@other/teamX"}, + "other/teamx": []string{"@other/teamX"}, // Key is normalized to lowercase } if !reflect.DeepEqual(userReviewerMap, expectedUserReviewerMap) { diff --git a/pkg/codeowners/codeowners.go b/pkg/codeowners/codeowners.go index 82f6c6c..3382c38 100644 --- a/pkg/codeowners/codeowners.go +++ b/pkg/codeowners/codeowners.go @@ -3,6 +3,7 @@ package codeowners import ( "errors" "io" + "slices" "strings" "github.com/multimediallc/codeowners-plus/pkg/functional" @@ -54,9 +55,12 @@ type ownersMap struct { } func (om *ownersMap) SetAuthor(author string) { - for _, reviewers := range om.nameReviewerMap[author] { - // remove author from the reviewers list - reviewers.Names = f.RemoveValue(reviewers.Names, author) + normalizedAuthor := NormalizeUsername(author) + for _, reviewers := range om.nameReviewerMap[normalizedAuthor] { + // remove author from the reviewers list (case-insensitive) + reviewers.Names = slices.DeleteFunc(reviewers.Names, func(name string) bool { + return NormalizeUsername(name) == normalizedAuthor + }) if len(reviewers.Names) == 0 { // mark the reviewer as approved if they are the author reviewers.Approved = true @@ -108,7 +112,8 @@ func (om *ownersMap) UnownedFiles() []string { // Apply approver satisfaction to the owners map, and return the approvals which should be invalidated func (om *ownersMap) ApplyApprovals(approvers []string) { applyApproved := func(user string) { - for _, reviewer := range om.nameReviewerMap[user] { + normalizedUser := NormalizeUsername(user) + for _, reviewer := range om.nameReviewerMap[normalizedUser] { reviewer.Approved = true } } @@ -259,10 +264,11 @@ func (otfm ownerTestFileMap) getOwners(fileNames []string) (*ownersMap, error) { for _, reviewer := range fileOwner.requiredReviewers { for _, name := range reviewer.Names { - if v, ok := nameReviewerMap[name]; ok { - nameReviewerMap[name] = append(v, reviewer) + normalizedName := NormalizeUsername(name) + if v, ok := nameReviewerMap[normalizedName]; ok { + nameReviewerMap[normalizedName] = append(v, reviewer) } else { - nameReviewerMap[name] = []*ReviewerGroup{reviewer} + nameReviewerMap[normalizedName] = []*ReviewerGroup{reviewer} } } } diff --git a/pkg/codeowners/codeowners_test.go b/pkg/codeowners/codeowners_test.go index 05921cf..d964930 100644 --- a/pkg/codeowners/codeowners_test.go +++ b/pkg/codeowners/codeowners_test.go @@ -238,3 +238,131 @@ func TestSetAuthor(t *testing.T) { } } } + +func TestSetAuthorCaseInsensitive(t *testing.T) { + owners, _, err := setupOwnersMap() + if err != nil { + t.Errorf("Error getting owners: %v", err) + } + + // Set author with different casing than what's in the .codeowners file + owners.SetAuthor("@B-OWNER") // .codeowners has @b-owner + + // Verify that @b-owner was removed from reviewers + allReviewers := owners.AllRequired().Flatten() + for _, reviewer := range allReviewers { + if NormalizeUsername(reviewer) == "@b-owner" { + t.Errorf("Expected @b-owner to be removed, but found %s", reviewer) + } + } + + // Verify with lowercase + owners2, _, err := setupOwnersMap() + if err != nil { + t.Errorf("Error getting owners: %v", err) + } + owners2.SetAuthor("@b-owner") + allReviewers2 := owners2.AllRequired().Flatten() + + // Both should have the same result + if len(allReviewers) != len(allReviewers2) { + t.Errorf("Case-insensitive SetAuthor produced different results") + } +} + +func TestApplyApprovalsCaseInsensitive(t *testing.T) { + owners, _, err := setupOwnersMap() + if err != nil { + t.Errorf("Error getting owners: %v", err) + } + + // Apply approvals with different casing + owners.ApplyApprovals([]string{"@BASE", "@B-OWNER"}) // .codeowners has @base, @b-owner + + // Verify approvals were applied regardless of case + allRequired := owners.AllRequired() + for _, rg := range allRequired { + normalizedNames := NormalizeUsernames(rg.Names) + for _, name := range normalizedNames { + if name == "@base" || name == "@b-owner" { + if !rg.Approved { + t.Errorf("Expected %v to be approved", rg.Names) + } + } + } + } + + // Test with lowercase + owners2, _, err := setupOwnersMap() + if err != nil { + t.Errorf("Error getting owners: %v", err) + } + owners2.ApplyApprovals([]string{"@base", "@b-owner"}) + + // Both should produce the same approval state + allRequired2 := owners2.AllRequired() + approvedCount1 := 0 + approvedCount2 := 0 + for _, rg := range allRequired { + if rg.Approved { + approvedCount1++ + } + } + for _, rg := range allRequired2 { + if rg.Approved { + approvedCount2++ + } + } + + if approvedCount1 != approvedCount2 { + t.Errorf("Case-insensitive ApplyApprovals produced different results: %d vs %d", approvedCount1, approvedCount2) + } +} + +func TestNameReviewerMapCaseInsensitive(t *testing.T) { + owners, _, err := setupOwnersMap() + if err != nil { + t.Errorf("Error getting owners: %v", err) + } + + // First, verify @frontend is in the required reviewers before approval + foundBefore := false + for _, fileOwner := range owners.fileToOwner { + for _, rg := range fileOwner.requiredReviewers { + for _, name := range rg.Names { + if NormalizeUsername(name) == "@frontend" { + foundBefore = true + t.Logf("Found @frontend before approval: %v, approved=%v", rg.Names, rg.Approved) + } + } + } + } + + if !foundBefore { + t.Errorf("@frontend not found in required reviewers - test setup issue") + return + } + + // Check that the nameReviewerMap uses normalized keys + // Apply an approval with uppercase + owners.ApplyApprovals([]string{"@FRONTEND"}) + + // Verify it matches @frontend by checking if it was approved + foundApproved := false + for _, fileOwner := range owners.fileToOwner { + for _, rg := range fileOwner.requiredReviewers { + for _, name := range rg.Names { + if NormalizeUsername(name) == "@frontend" { + t.Logf("After approval: %v, approved=%v", rg.Names, rg.Approved) + if rg.Approved { + foundApproved = true + } + } + } + } + } + + if !foundApproved { + t.Errorf("Expected @frontend to be approved when @FRONTEND was used") + } +} diff --git a/pkg/codeowners/normalize.go b/pkg/codeowners/normalize.go new file mode 100644 index 0000000..ba4c72b --- /dev/null +++ b/pkg/codeowners/normalize.go @@ -0,0 +1,24 @@ +package codeowners + +import "strings" + +// NormalizeUsername converts a username or team name to lowercase for case-insensitive comparison. +// This ensures that usernames like @User, @user, and @USER are treated as identical, +// matching GitHub's case-insensitive username behavior. +// The @ prefix is preserved and normalized as part of the string. +func NormalizeUsername(name string) string { + return strings.ToLower(name) +} + +// NormalizeUsernames normalizes a slice of usernames to lowercase for case-insensitive comparison. +// Returns a new slice with all usernames converted to lowercase. +func NormalizeUsernames(names []string) []string { + if names == nil { + return nil + } + normalized := make([]string, len(names)) + for i, name := range names { + normalized[i] = NormalizeUsername(name) + } + return normalized +} diff --git a/pkg/codeowners/normalize_test.go b/pkg/codeowners/normalize_test.go new file mode 100644 index 0000000..6c03049 --- /dev/null +++ b/pkg/codeowners/normalize_test.go @@ -0,0 +1,142 @@ +package codeowners + +import ( + "testing" +) + +func TestNormalizeUsername(t *testing.T) { + tt := []struct { + name string + input string + expected string + }{ + { + name: "lowercase username with @", + input: "@user", + expected: "@user", + }, + { + name: "uppercase username with @", + input: "@USER", + expected: "@user", + }, + { + name: "mixed case username with @", + input: "@JohnDoe", + expected: "@johndoe", + }, + { + name: "username without @", + input: "user", + expected: "user", + }, + { + name: "mixed case username without @", + input: "JohnDoe", + expected: "johndoe", + }, + { + name: "team name with slash", + input: "@org/TeamName", + expected: "@org/teamname", + }, + { + name: "uppercase team name", + input: "@ORG/TEAM", + expected: "@org/team", + }, + { + name: "empty string", + input: "", + expected: "", + }, + { + name: "unicode characters", + input: "@Üser", + expected: "@üser", + }, + { + name: "special characters preserved", + input: "@user-name_123", + expected: "@user-name_123", + }, + } + + for _, tc := range tt { + t.Run(tc.name, func(t *testing.T) { + result := NormalizeUsername(tc.input) + if result != tc.expected { + t.Errorf("NormalizeUsername(%q) = %q, expected %q", tc.input, result, tc.expected) + } + }) + } +} + +func TestNormalizeUsernames(t *testing.T) { + tt := []struct { + name string + input []string + expected []string + }{ + { + name: "mixed case usernames", + input: []string{"@User", "@ADMIN", "@john-doe"}, + expected: []string{"@user", "@admin", "@john-doe"}, + }, + { + name: "empty slice", + input: []string{}, + expected: []string{}, + }, + { + name: "nil slice", + input: nil, + expected: nil, + }, + { + name: "single username", + input: []string{"@JohnDoe"}, + expected: []string{"@johndoe"}, + }, + { + name: "team names", + input: []string{"@org/Team1", "@org/Team2"}, + expected: []string{"@org/team1", "@org/team2"}, + }, + { + name: "already lowercase", + input: []string{"@user1", "@user2"}, + expected: []string{"@user1", "@user2"}, + }, + { + name: "mixed teams and users", + input: []string{"@User", "@org/Team", "@ADMIN"}, + expected: []string{"@user", "@org/team", "@admin"}, + }, + } + + for _, tc := range tt { + t.Run(tc.name, func(t *testing.T) { + result := NormalizeUsernames(tc.input) + + // Handle nil case + if tc.expected == nil { + if result != nil { + t.Errorf("NormalizeUsernames(%v) = %v, expected nil", tc.input, result) + } + return + } + + if len(result) != len(tc.expected) { + t.Errorf("NormalizeUsernames(%v) returned %d items, expected %d", tc.input, len(result), len(tc.expected)) + return + } + + for i := range result { + if result[i] != tc.expected[i] { + t.Errorf("NormalizeUsernames(%v)[%d] = %q, expected %q", tc.input, i, result[i], tc.expected[i]) + } + } + }) + } +} diff --git a/pkg/codeowners/reviewers.go b/pkg/codeowners/reviewers.go index caf0d7f..9e0dade 100644 --- a/pkg/codeowners/reviewers.go +++ b/pkg/codeowners/reviewers.go @@ -53,11 +53,26 @@ func (rgs ReviewerGroups) Flatten() []string { return names } +// ContainsAny returns true if any of the provided names (case-insensitive) +// are present in any of the reviewer groups +func (rgs ReviewerGroups) ContainsAny(names []string) bool { + normalizedInput := NormalizeUsernames(names) + for _, rg := range rgs { + for _, name := range rg.Names { + if slices.Contains(normalizedInput, NormalizeUsername(name)) { + return true + } + } + } + return false +} + func (rgs ReviewerGroups) FilterOut(names ...string) ReviewerGroups { + normalizedNames := NormalizeUsernames(names) return f.Filtered(rgs, func(rg *ReviewerGroup) bool { found := false for _, name := range rg.Names { - if slices.Contains(names, name) { + if slices.Contains(normalizedNames, NormalizeUsername(name)) { found = true break } @@ -66,6 +81,15 @@ func (rgs ReviewerGroups) FilterOut(names ...string) ReviewerGroups { }) } +// FilterOutNames returns a new slice with names from 'names' that are NOT present +// in 'exclude' (case-insensitive comparison) +func FilterOutNames(names []string, exclude []string) []string { + normalizedExclude := NormalizeUsernames(exclude) + return f.Filtered(names, func(name string) bool { + return !slices.Contains(normalizedExclude, NormalizeUsername(name)) + }) +} + func (rgs ReviewerGroups) ToCommentString(includeCheckbox bool) string { ownersList := f.Map(rgs, func(s *ReviewerGroup) string { prefix := "- " diff --git a/pkg/codeowners/reviewers_test.go b/pkg/codeowners/reviewers_test.go index 15617b7..b888eea 100644 --- a/pkg/codeowners/reviewers_test.go +++ b/pkg/codeowners/reviewers_test.go @@ -165,3 +165,109 @@ func TestReviewerGroupsFilter(t *testing.T) { t.Error("Filter should work with multiple names") } } + +func TestReviewerGroupsContainsAny(t *testing.T) { + rgMan := NewReviewerGroupMemo() + rgs := ReviewerGroups{rgMan.ToReviewerGroup("@alice", "@bob"), rgMan.ToReviewerGroup("@charlie")} + + tt := []struct { + name string + input []string + expected bool + }{ + { + name: "contains exact match", + input: []string{"@alice"}, + expected: true, + }, + { + name: "contains case-insensitive match", + input: []string{"@ALICE"}, + expected: true, + }, + { + name: "contains mixed case match", + input: []string{"@BoB"}, + expected: true, + }, + { + name: "contains one of multiple", + input: []string{"@david", "@charlie"}, + expected: true, + }, + { + name: "does not contain", + input: []string{"@david"}, + expected: false, + }, + { + name: "empty input", + input: []string{}, + expected: false, + }, + } + + for _, tc := range tt { + t.Run(tc.name, func(t *testing.T) { + result := rgs.ContainsAny(tc.input) + if result != tc.expected { + t.Errorf("ContainsAny(%v) = %v, expected %v", tc.input, result, tc.expected) + } + }) + } +} + +func TestFilterOutNames(t *testing.T) { + tt := []struct { + name string + names []string + exclude []string + expected []string + }{ + { + name: "filter out exact match", + names: []string{"@alice", "@bob", "@charlie"}, + exclude: []string{"@bob"}, + expected: []string{"@alice", "@charlie"}, + }, + { + name: "filter out case-insensitive match", + names: []string{"@alice", "@bob", "@charlie"}, + exclude: []string{"@BOB"}, + expected: []string{"@alice", "@charlie"}, + }, + { + name: "filter out multiple", + names: []string{"@alice", "@bob", "@charlie"}, + exclude: []string{"@alice", "@CHARLIE"}, + expected: []string{"@bob"}, + }, + { + name: "no matches to filter", + names: []string{"@alice", "@bob"}, + exclude: []string{"@david"}, + expected: []string{"@alice", "@bob"}, + }, + { + name: "empty exclude list", + names: []string{"@alice", "@bob"}, + exclude: []string{}, + expected: []string{"@alice", "@bob"}, + }, + { + name: "empty names list", + names: []string{}, + exclude: []string{"@alice"}, + expected: []string{}, + }, + } + + for _, tc := range tt { + t.Run(tc.name, func(t *testing.T) { + result := FilterOutNames(tc.names, tc.exclude) + if !f.SlicesItemsMatch(result, tc.expected) { + t.Errorf("FilterOutNames(%v, %v) = %v, expected %v", tc.names, tc.exclude, result, tc.expected) + } + }) + } +} From ee3eef5d4643d8647adbd712d5beafa7d9445a3d Mon Sep 17 00:00:00 2001 From: BakerNet Date: Tue, 6 Jan 2026 15:19:37 -0800 Subject: [PATCH 2/4] Covbadge --- README.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/README.md b/README.md index 70ff2c3..6991132 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.8%25-brightgreen) +![Coverage](https://img.shields.io/badge/Coverage-82.4%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 378b923a9a8aba6c15bb222b5ce19f855411673b Mon Sep 17 00:00:00 2001 From: BakerNet Date: Tue, 6 Jan 2026 16:47:51 -0800 Subject: [PATCH 3/4] Replace string slugs with wrapped slugs to prevent future bugs --- internal/app/app.go | 44 ++-- internal/app/app_test.go | 196 +++++++-------- internal/github/approvals.go | 10 +- internal/github/approvals_test.go | 6 +- internal/github/gh.go | 69 +++--- internal/github/gh_test.go | 51 ++-- pkg/codeowners/codeowners.go | 22 +- pkg/codeowners/codeowners_test.go | 33 +-- pkg/codeowners/normalize.go | 24 -- pkg/codeowners/normalize_test.go | 142 ----------- pkg/codeowners/reader_test.go | 4 +- pkg/codeowners/reviewers.go | 70 +++--- pkg/codeowners/reviewers_test.go | 26 +- pkg/codeowners/slug.go | 101 ++++++++ pkg/codeowners/slug_test.go | 383 ++++++++++++++++++++++++++++++ tools/cli/main.go | 20 +- tools/cli/main_test.go | 44 ++-- 17 files changed, 798 insertions(+), 447 deletions(-) delete mode 100644 pkg/codeowners/normalize.go delete mode 100644 pkg/codeowners/normalize_test.go create mode 100644 pkg/codeowners/slug.go create mode 100644 pkg/codeowners/slug_test.go diff --git a/internal/app/app.go b/internal/app/app.go index e70dae5..f982c34 100644 --- a/internal/app/app.go +++ b/internal/app/app.go @@ -28,10 +28,10 @@ func NewOutputData(co codeowners.CodeOwners) *OutputData { fileOwners := make(map[string][]string) fileOptional := make(map[string][]string) for file, reviewers := range co.FileRequired() { - fileOwners[file] = reviewers.Flatten() + fileOwners[file] = codeowners.OriginalStrings(reviewers.Flatten()) } for file, reviewers := range co.FileOptional() { - fileOptional[file] = reviewers.Flatten() + fileOptional[file] = codeowners.OriginalStrings(reviewers.Flatten()) } return &OutputData{ FileOwners: fileOwners, @@ -173,15 +173,15 @@ func (a *App) processApprovalsAndReviewers() (bool, string, []string, error) { // Get all required owners before filtering allRequiredOwners := a.codeowners.AllRequired() allRequiredOwnerNames := allRequiredOwners.Flatten() - a.printDebug("All Required Owners: %s\n", allRequiredOwnerNames) + a.printDebug("All Required Owners: %s\n", codeowners.OriginalStrings(allRequiredOwnerNames)) // Get optional reviewers allOptionalReviewerNames := a.codeowners.AllOptional().Flatten() allOptionalReviewerNames = codeowners.FilterOutNames(allOptionalReviewerNames, allRequiredOwnerNames) - a.printDebug("All Optional Reviewers: %s\n", allOptionalReviewerNames) + a.printDebug("All Optional Reviewers: %s\n", codeowners.OriginalStrings(allOptionalReviewerNames)) // Initialize user reviewer map - if err := a.client.InitUserReviewerMap(allRequiredOwnerNames); err != nil { + if err := a.client.InitUserReviewerMap(codeowners.OriginalStrings(allRequiredOwnerNames)); err != nil { return false, message, nil, fmt.Errorf("InitUserReviewerMap Error: %v", err) } @@ -227,7 +227,8 @@ func (a *App) processApprovalsAndReviewers() (bool, string, []string, error) { unapprovedOwners := a.codeowners.AllRequired() maxReviewsMet := false if a.Conf.MaxReviews != nil && *a.Conf.MaxReviews > 0 { - if validApprovalCount >= *a.Conf.MaxReviews && !unapprovedOwners.ContainsAny(a.Conf.UnskippableReviewers) { + unskippableReviewerSlugs := codeowners.NewSlugs(a.Conf.UnskippableReviewers) + if validApprovalCount >= *a.Conf.MaxReviews && !unapprovedOwners.ContainsAny(unskippableReviewerSlugs) { maxReviewsMet = true } } @@ -237,7 +238,7 @@ func (a *App) processApprovalsAndReviewers() (bool, string, []string, error) { if err != nil { return false, message, nil, fmt.Errorf("failed to add review status comment: %w", err) } - err = a.addOptionalCcComment(allOptionalReviewerNames) + err = a.addOptionalCcComment(codeowners.OriginalStrings(allOptionalReviewerNames)) if err != nil { return false, message, nil, fmt.Errorf("failed to add optional CC comment: %w", err) } @@ -258,7 +259,8 @@ func (a *App) processApprovalsAndReviewers() (bool, string, []string, error) { } // Collect still required data - stillRequired := unapprovedOwners.Flatten() + stillRequiredSlugs := unapprovedOwners.Flatten() + stillRequired := codeowners.OriginalStrings(stillRequiredSlugs) // Exit if there are any unapproved codeowner teams if len(unapprovedOwners) > 0 && !maxReviewsMet { @@ -287,15 +289,15 @@ func (a *App) processApprovalsAndReviewers() (bool, string, []string, error) { } else { currentlyRequestedSet := make(map[string]struct{}, len(currentlyRequestedOwners)) for _, owner := range currentlyRequestedOwners { - currentlyRequestedSet[owner] = struct{}{} + currentlyRequestedSet[owner.Normalized()] = struct{}{} } - ownersToReRequest := f.Filtered(allRequiredOwnerNames, func(owner string) bool { - _, exists := currentlyRequestedSet[owner] + ownersToReRequest := f.Filtered(allRequiredOwnerNames, func(owner codeowners.Slug) bool { + _, exists := currentlyRequestedSet[owner.Normalized()] return !exists }) if len(ownersToReRequest) > 0 { - a.printDebug("Re-requesting Reviews from satisfied team(s) to meet min_reviews: %s\n", ownersToReRequest) - if err := a.client.RequestReviewers(ownersToReRequest); err != nil { + a.printDebug("Re-requesting Reviews from satisfied team(s) to meet min_reviews: %s\n", codeowners.OriginalStrings(ownersToReRequest)) + if err := a.client.RequestReviewers(codeowners.OriginalStrings(ownersToReRequest)); err != nil { a.printWarn("WARNING: Error re-requesting reviewers: %v\n", err) } } @@ -431,10 +433,10 @@ func (a *App) processTokenOwnerApproval() (*gh.CurrentApproval, error) { func (a *App) processApprovals(ghApprovals []*gh.CurrentApproval) (int, error) { // Create file reviewer map with normalized names for case-insensitive comparison fileReviewers := f.MapMap(a.codeowners.FileRequired(), func(reviewers codeowners.ReviewerGroups) []string { - return codeowners.NormalizeUsernames(reviewers.Flatten()) + return codeowners.NormalizedStrings(reviewers.Flatten()) }) - var approvers []string + var approvers []codeowners.Slug var approvalsToDismiss []*gh.CurrentApproval if a.Conf.DisableSmartDismissal { @@ -468,27 +470,27 @@ func (a *App) requestReviews() error { unapprovedOwners := a.codeowners.AllRequired() unapprovedOwnerNames := unapprovedOwners.Flatten() - a.printDebug("Remaining Required Owners: %s\n", unapprovedOwnerNames) + a.printDebug("Remaining Required Owners: %s\n", codeowners.OriginalStrings(unapprovedOwnerNames)) currentlyRequestedOwners, err := a.client.GetCurrentlyRequested() if err != nil { return fmt.Errorf("GetCurrentlyRequested Error: %v", err) } - a.printDebug("Currently Requested Owners: %s\n", currentlyRequestedOwners) + a.printDebug("Currently Requested Owners: %s\n", codeowners.OriginalStrings(currentlyRequestedOwners)) previousReviewers, err := a.client.GetAlreadyReviewed() if err != nil { return fmt.Errorf("GetAlreadyReviewed Error: %v", err) } - a.printDebug("Already Reviewed Owners: %s\n", previousReviewers) + a.printDebug("Already Reviewed Owners: %s\n", codeowners.OriginalStrings(previousReviewers)) filteredOwners := unapprovedOwners.FilterOut(currentlyRequestedOwners...) filteredOwners = filteredOwners.FilterOut(previousReviewers...) filteredOwnerNames := filteredOwners.Flatten() if len(filteredOwners) > 0 { - a.printDebug("Requesting Reviews from: %s\n", filteredOwnerNames) - if err := a.client.RequestReviewers(filteredOwnerNames); err != nil { + a.printDebug("Requesting Reviews from: %s\n", codeowners.OriginalStrings(filteredOwnerNames)) + if err := a.client.RequestReviewers(codeowners.OriginalStrings(filteredOwnerNames)); err != nil { return fmt.Errorf("RequestReviewers Error: %v", err) } } @@ -515,7 +517,7 @@ func (a *App) getFileOwnersMapToString(fileReviewers map[string]codeowners.Revie for _, file := range files { reviewers := fileReviewers[file] // builder.WriteString error return is always nil - _, _ = fmt.Fprintf(&builder, "- %s: %+v\n", file, reviewers.Flatten()) + _, _ = fmt.Fprintf(&builder, "- %s: %+v\n", file, codeowners.OriginalStrings(reviewers.Flatten())) } return builder.String() } diff --git a/internal/app/app_test.go b/internal/app/app_test.go index 4debce3..6f70ef0 100644 --- a/internal/app/app_test.go +++ b/internal/app/app_test.go @@ -52,7 +52,7 @@ type mockCodeOwners struct { optionalOwners codeowners.ReviewerGroups fileRequiredMap map[string]codeowners.ReviewerGroups fileOptionalMap map[string]codeowners.ReviewerGroups - appliedApprovals []string + appliedApprovals []codeowners.Slug author string unownedFiles []string } @@ -73,7 +73,7 @@ func (m *mockCodeOwners) FileOptional() map[string]codeowners.ReviewerGroups { return m.fileOptionalMap } -func (m *mockCodeOwners) ApplyApprovals(approvers []string) { +func (m *mockCodeOwners) ApplyApprovals(approvers []codeowners.Slug) { m.appliedApprovals = approvers } @@ -82,7 +82,7 @@ func (m *mockCodeOwners) SetAuthor(author string) { // Remove author from reviewers for _, reviewers := range m.requiredOwners { for i, name := range reviewers.Names { - if name == author { + if name.EqualsString(author) { reviewers.Names = append(reviewers.Names[:i], reviewers.Names[i+1:]...) if len(reviewers.Names) == 0 { reviewers.Approved = true @@ -93,7 +93,7 @@ func (m *mockCodeOwners) SetAuthor(author string) { } for _, reviewers := range m.optionalOwners { for i, name := range reviewers.Names { - if name == author { + if name.EqualsString(author) { reviewers.Names = append(reviewers.Names[:i], reviewers.Names[i+1:]...) if len(reviewers.Names) == 0 { reviewers.Approved = true @@ -115,9 +115,9 @@ type mockGitHubClient struct { currentApprovalsError error tokenUser string tokenUserError error - currentlyRequested []string + currentlyRequested []codeowners.Slug currentlyRequestedError error - alreadyReviewed []string + alreadyReviewed []codeowners.Slug alreadyReviewedError error dismissError error requestReviewersError error @@ -156,18 +156,18 @@ func (m *mockGitHubClient) GetTokenUser() (string, error) { func (m *mockGitHubClient) FindUserApproval(user string) (*gh.CurrentApproval, error) { for _, approval := range m.currentApprovals { - if approval.GHLogin == user { + if approval.GHLogin.EqualsString(user) { return approval, nil } } return nil, fmt.Errorf("not found") } -func (m *mockGitHubClient) GetCurrentlyRequested() ([]string, error) { +func (m *mockGitHubClient) GetCurrentlyRequested() ([]codeowners.Slug, error) { return m.currentlyRequested, m.currentlyRequestedError } -func (m *mockGitHubClient) GetAlreadyReviewed() ([]string, error) { +func (m *mockGitHubClient) GetAlreadyReviewed() ([]codeowners.Slug, error) { return m.alreadyReviewed, m.alreadyReviewedError } @@ -180,11 +180,11 @@ func (m *mockGitHubClient) RequestReviewers(reviewers []string) error { return m.requestReviewersError } -func (m *mockGitHubClient) CheckApprovals(fileReviewers map[string][]string, approvals []*gh.CurrentApproval, diff git.Diff) ([]string, []*gh.CurrentApproval) { +func (m *mockGitHubClient) CheckApprovals(fileReviewers map[string][]string, approvals []*gh.CurrentApproval, diff git.Diff) ([]codeowners.Slug, []*gh.CurrentApproval) { // Simple mock implementation - approve all reviewers - var approvers []string + var approvers []codeowners.Slug for _, reviewers := range fileReviewers { - approvers = append(approvers, reviewers...) + approvers = append(approvers, codeowners.NewSlugs(reviewers)...) } return approvers, nil } @@ -320,12 +320,12 @@ func (m *mockGitHubClient) IsRepositoryAdmin(username string) (bool, error) { func (m *mockGitHubClient) ContainsValidBypassApproval(allowedUsers []string) (bool, error) { // For testing, check if any approval is from an admin-user with review ID 999 for _, approval := range m.currentApprovals { - if approval.ReviewID == 999 && strings.Contains(approval.GHLogin, "admin") { + if approval.ReviewID == 999 && strings.Contains(approval.GHLogin.Original(), "admin") { return true, nil } // Also check if user is in allowed users list for _, allowedUser := range allowedUsers { - if approval.GHLogin == allowedUser { + if approval.GHLogin.EqualsString(allowedUser) { return true, nil } } @@ -513,7 +513,7 @@ func setupAppForTest(t *testing.T, quiet bool) (*App, *mockGitHubClient) { } mockOwners := &mockCodeOwners{ requiredOwners: codeowners.ReviewerGroups{ - &codeowners.ReviewerGroup{Names: []string{"@user1", "@user2"}}, + &codeowners.ReviewerGroup{Names: codeowners.NewSlugs([]string{"@user1", "@user2"})}, }, } @@ -541,7 +541,7 @@ func TestAddReviewStatusComment(t *testing.T) { { name: "no existing comment", requiredOwners: codeowners.ReviewerGroups{ - &codeowners.ReviewerGroup{Names: []string{"@user1"}}, + &codeowners.ReviewerGroup{Names: codeowners.NewSlugs([]string{"@user1"})}, }, expectAddComment: true, expectError: false, @@ -549,7 +549,7 @@ func TestAddReviewStatusComment(t *testing.T) { { name: "update existing comment", requiredOwners: codeowners.ReviewerGroups{ - &codeowners.ReviewerGroup{Names: []string{"@user1"}}, + &codeowners.ReviewerGroup{Names: codeowners.NewSlugs([]string{"@user1"})}, }, existingComments: []*github.IssueComment{ { @@ -563,7 +563,7 @@ func TestAddReviewStatusComment(t *testing.T) { { name: "quiet mode", requiredOwners: codeowners.ReviewerGroups{ - &codeowners.ReviewerGroup{Names: []string{"@user1"}}, + &codeowners.ReviewerGroup{Names: codeowners.NewSlugs([]string{"@user1"})}, }, expectAddComment: false, expectError: false, @@ -670,22 +670,22 @@ func TestRequestReviews(t *testing.T) { tt := []struct { name string quiet bool - mockCurrentlyRequested []string - mockAlreadyReviewed []string + mockCurrentlyRequested []codeowners.Slug + mockAlreadyReviewed []codeowners.Slug expectedShouldCall bool }{ { name: "short circuits in quiet mode", quiet: true, - mockCurrentlyRequested: []string{}, - mockAlreadyReviewed: []string{}, + mockCurrentlyRequested: []codeowners.Slug{}, + mockAlreadyReviewed: []codeowners.Slug{}, expectedShouldCall: false, }, { name: "sends requests when not in quiet mode", quiet: false, - mockCurrentlyRequested: []string{}, - mockAlreadyReviewed: []string{}, + mockCurrentlyRequested: []codeowners.Slug{}, + mockAlreadyReviewed: []codeowners.Slug{}, expectedShouldCall: true, }, } @@ -721,8 +721,8 @@ func TestProcessApprovalsAndReviewers(t *testing.T) { fileRequiredMap map[string]codeowners.ReviewerGroups fileOptionalMap map[string]codeowners.ReviewerGroups currentApprovals []*gh.CurrentApproval - currentlyRequested []string - alreadyReviewed []string + currentlyRequested []codeowners.Slug + alreadyReviewed []codeowners.Slug tokenUser string tokenUserError error userReviewerMapError error @@ -738,132 +738,132 @@ func TestProcessApprovalsAndReviewers(t *testing.T) { unownedFiles []string expectError bool expectSuccess bool - expectedApprovals []string + expectedApprovals []codeowners.Slug }{ { name: "successful approval process", requiredOwners: codeowners.ReviewerGroups{ - &codeowners.ReviewerGroup{Names: []string{"@user1", "@user2"}}, + &codeowners.ReviewerGroup{Names: codeowners.NewSlugs([]string{"@user1", "@user2"})}, }, optionalOwners: codeowners.ReviewerGroups{ - &codeowners.ReviewerGroup{Names: []string{"@user3"}}, + &codeowners.ReviewerGroup{Names: codeowners.NewSlugs([]string{"@user3"})}, }, fileRequiredMap: map[string]codeowners.ReviewerGroups{ "file1.go": { - &codeowners.ReviewerGroup{Names: []string{"@user1"}}, + &codeowners.ReviewerGroup{Names: codeowners.NewSlugs([]string{"@user1"})}, }, }, currentApprovals: []*gh.CurrentApproval{ - {GHLogin: "@user1"}, + {GHLogin: codeowners.NewSlug("@user1")}, }, - currentlyRequested: []string{"@user2"}, - alreadyReviewed: []string{"@user1"}, + currentlyRequested: codeowners.NewSlugs([]string{"@user2"}), + alreadyReviewed: codeowners.NewSlugs([]string{"@user1"}), expectError: false, expectSuccess: true, - expectedApprovals: []string{"@user1"}, + expectedApprovals: codeowners.NewSlugs([]string{"@user1"}), }, { name: "not enough approvals", requiredOwners: codeowners.ReviewerGroups{ - &codeowners.ReviewerGroup{Names: []string{"@user1"}}, - &codeowners.ReviewerGroup{Names: []string{"@user2"}}, - &codeowners.ReviewerGroup{Names: []string{"@user3"}}, + &codeowners.ReviewerGroup{Names: codeowners.NewSlugs([]string{"@user1"})}, + &codeowners.ReviewerGroup{Names: codeowners.NewSlugs([]string{"@user2"})}, + &codeowners.ReviewerGroup{Names: codeowners.NewSlugs([]string{"@user3"})}, }, currentApprovals: []*gh.CurrentApproval{ - {GHLogin: "@user1"}, - {GHLogin: "@user3"}, + {GHLogin: codeowners.NewSlug("@user1")}, + {GHLogin: codeowners.NewSlug("@user3")}, }, fileRequiredMap: map[string]codeowners.ReviewerGroups{ "file1.go": { - &codeowners.ReviewerGroup{Names: []string{"@user1"}}, + &codeowners.ReviewerGroup{Names: codeowners.NewSlugs([]string{"@user1"})}, }, "file3.go": { - &codeowners.ReviewerGroup{Names: []string{"@user3"}}, + &codeowners.ReviewerGroup{Names: codeowners.NewSlugs([]string{"@user3"})}, }, }, - currentlyRequested: []string{"@user2"}, + currentlyRequested: codeowners.NewSlugs([]string{"@user2"}), expectError: false, expectSuccess: false, - expectedApprovals: []string{"@user1", "@user3"}, + expectedApprovals: codeowners.NewSlugs([]string{"@user1", "@user3"}), }, { name: "max reviews bypass", requiredOwners: codeowners.ReviewerGroups{ - &codeowners.ReviewerGroup{Names: []string{"@user1"}}, - &codeowners.ReviewerGroup{Names: []string{"@user2"}}, - &codeowners.ReviewerGroup{Names: []string{"@user3"}}, + &codeowners.ReviewerGroup{Names: codeowners.NewSlugs([]string{"@user1"})}, + &codeowners.ReviewerGroup{Names: codeowners.NewSlugs([]string{"@user2"})}, + &codeowners.ReviewerGroup{Names: codeowners.NewSlugs([]string{"@user3"})}, }, currentApprovals: []*gh.CurrentApproval{ - {GHLogin: "@user1"}, - {GHLogin: "@user3"}, + {GHLogin: codeowners.NewSlug("@user1")}, + {GHLogin: codeowners.NewSlug("@user3")}, }, fileRequiredMap: map[string]codeowners.ReviewerGroups{ "file1.go": { - &codeowners.ReviewerGroup{Names: []string{"@user1"}}, + &codeowners.ReviewerGroup{Names: codeowners.NewSlugs([]string{"@user1"})}, }, "file3.go": { - &codeowners.ReviewerGroup{Names: []string{"@user3"}}, + &codeowners.ReviewerGroup{Names: codeowners.NewSlugs([]string{"@user3"})}, }, }, - currentlyRequested: []string{"@user2"}, + currentlyRequested: codeowners.NewSlugs([]string{"@user2"}), maxReviews: &maxReviews, expectError: false, expectSuccess: true, - expectedApprovals: []string{"@user1", "@user3"}, + expectedApprovals: codeowners.NewSlugs([]string{"@user1", "@user3"}), }, { name: "min reviews enforced", requiredOwners: codeowners.ReviewerGroups{ - &codeowners.ReviewerGroup{Names: []string{"@user1"}}, + &codeowners.ReviewerGroup{Names: codeowners.NewSlugs([]string{"@user1"})}, }, currentApprovals: []*gh.CurrentApproval{ - {GHLogin: "@user1"}, + {GHLogin: codeowners.NewSlug("@user1")}, }, fileRequiredMap: map[string]codeowners.ReviewerGroups{ "file1.go": { - &codeowners.ReviewerGroup{Names: []string{"@user1"}}, + &codeowners.ReviewerGroup{Names: codeowners.NewSlugs([]string{"@user1"})}, }, }, minReviews: &minReviews, expectError: false, expectSuccess: false, - expectedApprovals: []string{"@user1"}, + expectedApprovals: codeowners.NewSlugs([]string{"@user1"}), }, { name: "min reviews enforced - re-request from satisfied team", requiredOwners: codeowners.ReviewerGroups{ - &codeowners.ReviewerGroup{Names: []string{"@team/eng", "@user1", "@user2"}}, + &codeowners.ReviewerGroup{Names: codeowners.NewSlugs([]string{"@team/eng", "@user1", "@user2"})}, }, currentApprovals: []*gh.CurrentApproval{ - {GHLogin: "@user1", Reviewers: []string{"@team/eng"}}, + {GHLogin: codeowners.NewSlug("@user1"), Reviewers: codeowners.NewSlugs([]string{"@team/eng"})}, }, fileRequiredMap: map[string]codeowners.ReviewerGroups{ "file1.go": { - &codeowners.ReviewerGroup{Names: []string{"@team/eng"}}, + &codeowners.ReviewerGroup{Names: codeowners.NewSlugs([]string{"@team/eng"})}, }, }, - currentlyRequested: []string{}, - alreadyReviewed: []string{"@team/eng"}, + currentlyRequested: codeowners.NewSlugs([]string{}), + alreadyReviewed: codeowners.NewSlugs([]string{"@team/eng"}), minReviews: &minReviews, expectError: false, expectSuccess: false, - expectedApprovals: []string{"@team/eng"}, + expectedApprovals: codeowners.NewSlugs([]string{"@team/eng"}), }, { name: "token user is reviewer", requiredOwners: codeowners.ReviewerGroups{ - &codeowners.ReviewerGroup{Names: []string{"@user1", "@token-user"}}, + &codeowners.ReviewerGroup{Names: codeowners.NewSlugs([]string{"@user1", "@token-user"})}, }, tokenUser: "@token-user", currentApprovals: []*gh.CurrentApproval{}, - currentlyRequested: []string{"@user1"}, + currentlyRequested: codeowners.NewSlugs([]string{"@user1"}), expectError: false, expectSuccess: false, }, { name: "token user exists", requiredOwners: codeowners.ReviewerGroups{ - &codeowners.ReviewerGroup{Names: []string{"@user1"}}, + &codeowners.ReviewerGroup{Names: codeowners.NewSlugs([]string{"@user1"})}, }, expectError: false, enforcementApproval: true, @@ -872,7 +872,7 @@ func TestProcessApprovalsAndReviewers(t *testing.T) { { name: "error getting token user", requiredOwners: codeowners.ReviewerGroups{ - &codeowners.ReviewerGroup{Names: []string{"@user1"}}, + &codeowners.ReviewerGroup{Names: codeowners.NewSlugs([]string{"@user1"})}, }, tokenUserError: fmt.Errorf("failed to get token user"), expectError: false, @@ -882,7 +882,7 @@ func TestProcessApprovalsAndReviewers(t *testing.T) { { name: "error initializing reviewer map", requiredOwners: codeowners.ReviewerGroups{ - &codeowners.ReviewerGroup{Names: []string{"@user1"}}, + &codeowners.ReviewerGroup{Names: codeowners.NewSlugs([]string{"@user1"})}, }, userReviewerMapError: fmt.Errorf("failed to init reviewer map"), expectError: true, @@ -890,45 +890,45 @@ func TestProcessApprovalsAndReviewers(t *testing.T) { { name: "multiple file reviewers", requiredOwners: codeowners.ReviewerGroups{ - &codeowners.ReviewerGroup{Names: []string{"@user1", "@user2"}}, + &codeowners.ReviewerGroup{Names: codeowners.NewSlugs([]string{"@user1", "@user2"})}, }, fileRequiredMap: map[string]codeowners.ReviewerGroups{ "file1.go": { - &codeowners.ReviewerGroup{Names: []string{"@user1"}}, + &codeowners.ReviewerGroup{Names: codeowners.NewSlugs([]string{"@user1"})}, }, "file2.go": { - &codeowners.ReviewerGroup{Names: []string{"@user2"}}, + &codeowners.ReviewerGroup{Names: codeowners.NewSlugs([]string{"@user2"})}, }, }, currentApprovals: []*gh.CurrentApproval{ - {GHLogin: "@user1"}, - {GHLogin: "@user2"}, + {GHLogin: codeowners.NewSlug("@user1")}, + {GHLogin: codeowners.NewSlug("@user2")}, }, expectError: false, expectSuccess: true, - expectedApprovals: []string{"@user1", "@user2"}, + expectedApprovals: codeowners.NewSlugs([]string{"@user1", "@user2"}), }, { name: "optional reviewers only", optionalOwners: codeowners.ReviewerGroups{ - &codeowners.ReviewerGroup{Names: []string{"@user1", "@user2"}}, + &codeowners.ReviewerGroup{Names: codeowners.NewSlugs([]string{"@user1", "@user2"})}, }, fileOptionalMap: map[string]codeowners.ReviewerGroups{ "file1.go": { - &codeowners.ReviewerGroup{Names: []string{"@user1"}}, + &codeowners.ReviewerGroup{Names: codeowners.NewSlugs([]string{"@user1"})}, }, }, currentApprovals: []*gh.CurrentApproval{ - {GHLogin: "@user1"}, + {GHLogin: codeowners.NewSlug("@user1")}, }, expectError: false, expectSuccess: true, - expectedApprovals: []string{}, + expectedApprovals: codeowners.NewSlugs([]string{}), }, { name: "unowned files", requiredOwners: codeowners.ReviewerGroups{ - &codeowners.ReviewerGroup{Names: []string{"@user1"}}, + &codeowners.ReviewerGroup{Names: codeowners.NewSlugs([]string{"@user1"})}, }, unownedFiles: []string{"unowned.go"}, expectError: false, @@ -936,7 +936,7 @@ func TestProcessApprovalsAndReviewers(t *testing.T) { { name: "error getting approvals", requiredOwners: codeowners.ReviewerGroups{ - &codeowners.ReviewerGroup{Names: []string{"@user1"}}, + &codeowners.ReviewerGroup{Names: codeowners.NewSlugs([]string{"@user1"})}, }, approvalsError: fmt.Errorf("failed to get approvals"), expectError: true, @@ -944,7 +944,7 @@ func TestProcessApprovalsAndReviewers(t *testing.T) { { name: "error getting currently requested", requiredOwners: codeowners.ReviewerGroups{ - &codeowners.ReviewerGroup{Names: []string{"@user1"}}, + &codeowners.ReviewerGroup{Names: codeowners.NewSlugs([]string{"@user1"})}, }, currentApprovals: []*gh.CurrentApproval{}, requestedError: fmt.Errorf("failed to get requested reviewers"), @@ -953,23 +953,23 @@ func TestProcessApprovalsAndReviewers(t *testing.T) { { name: "admin bypass approval succeeds", requiredOwners: codeowners.ReviewerGroups{ - &codeowners.ReviewerGroup{Names: []string{"@user1"}}, - &codeowners.ReviewerGroup{Names: []string{"@user2"}}, + &codeowners.ReviewerGroup{Names: codeowners.NewSlugs([]string{"@user1"})}, + &codeowners.ReviewerGroup{Names: codeowners.NewSlugs([]string{"@user2"})}, }, fileRequiredMap: map[string]codeowners.ReviewerGroups{ "file1.go": { - &codeowners.ReviewerGroup{Names: []string{"@user1"}}, + &codeowners.ReviewerGroup{Names: codeowners.NewSlugs([]string{"@user1"})}, }, "file2.go": { - &codeowners.ReviewerGroup{Names: []string{"@user2"}}, + &codeowners.ReviewerGroup{Names: codeowners.NewSlugs([]string{"@user2"})}, }, }, currentApprovals: []*gh.CurrentApproval{ - {GHLogin: "admin-user", ReviewID: 999, Reviewers: []string{}}, + {GHLogin: codeowners.NewSlug("admin-user"), ReviewID: 999, Reviewers: codeowners.NewSlugs([]string{})}, }, expectError: false, expectSuccess: true, - expectedApprovals: []string{"@user1", "@user2"}, + expectedApprovals: codeowners.NewSlugs([]string{"@user1", "@user2"}), }, } @@ -1076,12 +1076,12 @@ func TestPrintFileOwners(t *testing.T) { verbose: true, fileRequired: map[string]codeowners.ReviewerGroups{ "file1.go": { - &codeowners.ReviewerGroup{Names: []string{"@user1", "@user2"}}, + &codeowners.ReviewerGroup{Names: codeowners.NewSlugs([]string{"@user1", "@user2"})}, }, }, fileOptional: map[string]codeowners.ReviewerGroups{ "file2.go": { - &codeowners.ReviewerGroup{Names: []string{"@user3"}}, + &codeowners.ReviewerGroup{Names: codeowners.NewSlugs([]string{"@user3"})}, }, }, expectedOutput: "File Reviewers:\n- file1.go: [@user1 @user2]\nFile Optional:\n- file2.go: [@user3]\n", @@ -1091,7 +1091,7 @@ func TestPrintFileOwners(t *testing.T) { verbose: true, fileRequired: map[string]codeowners.ReviewerGroups{ "file1.go": { - &codeowners.ReviewerGroup{Names: []string{"@user1"}}, + &codeowners.ReviewerGroup{Names: codeowners.NewSlugs([]string{"@user1"})}, }, }, fileOptional: map[string]codeowners.ReviewerGroups{}, @@ -1103,7 +1103,7 @@ func TestPrintFileOwners(t *testing.T) { fileRequired: map[string]codeowners.ReviewerGroups{}, fileOptional: map[string]codeowners.ReviewerGroups{ "file2.go": { - &codeowners.ReviewerGroup{Names: []string{"@user3"}}, + &codeowners.ReviewerGroup{Names: codeowners.NewSlugs([]string{"@user3"})}, }, }, expectedOutput: "File Reviewers:\nFile Optional:\n- file2.go: [@user3]\n", @@ -1113,12 +1113,12 @@ func TestPrintFileOwners(t *testing.T) { verbose: false, fileRequired: map[string]codeowners.ReviewerGroups{ "file1.go": { - &codeowners.ReviewerGroup{Names: []string{"@user1"}}, + &codeowners.ReviewerGroup{Names: codeowners.NewSlugs([]string{"@user1"})}, }, }, fileOptional: map[string]codeowners.ReviewerGroups{ "file2.go": { - &codeowners.ReviewerGroup{Names: []string{"@user3"}}, + &codeowners.ReviewerGroup{Names: codeowners.NewSlugs([]string{"@user3"})}, }, }, expectedOutput: "", @@ -1160,11 +1160,11 @@ func TestPrintFileOwners(t *testing.T) { func TestBuildOutputData(t *testing.T) { mockOwners := &mockCodeOwners{ fileRequiredMap: map[string]codeowners.ReviewerGroups{ - "file1.go": {&codeowners.ReviewerGroup{Names: []string{"@user1", "@user2"}}}, - "file2.go": {&codeowners.ReviewerGroup{Names: []string{"@user3"}}}, + "file1.go": {&codeowners.ReviewerGroup{Names: codeowners.NewSlugs([]string{"@user1", "@user2"})}}, + "file2.go": {&codeowners.ReviewerGroup{Names: codeowners.NewSlugs([]string{"@user3"})}}, }, fileOptionalMap: map[string]codeowners.ReviewerGroups{ - "file1.go": {&codeowners.ReviewerGroup{Names: []string{"@optional1"}}}, + "file1.go": {&codeowners.ReviewerGroup{Names: codeowners.NewSlugs([]string{"@optional1"})}}, }, unownedFiles: []string{"unowned.go"}, } @@ -1219,7 +1219,7 @@ func TestCommentDetailedReviewers(t *testing.T) { mockGH := &mockGitHubClient{} requiredOwners := codeowners.ReviewerGroups{ - &codeowners.ReviewerGroup{Names: []string{"@user1", "@user2"}}, + &codeowners.ReviewerGroup{Names: codeowners.NewSlugs([]string{"@user1", "@user2"})}, } app := &App{ config: &Config{ @@ -1230,10 +1230,10 @@ func TestCommentDetailedReviewers(t *testing.T) { codeowners: &mockCodeOwners{ fileRequiredMap: map[string]codeowners.ReviewerGroups{ "file1.go": { - &codeowners.ReviewerGroup{Names: []string{"@user1", "@user2"}}, + &codeowners.ReviewerGroup{Names: codeowners.NewSlugs([]string{"@user1", "@user2"})}, }, "file2.go": { - &codeowners.ReviewerGroup{Names: []string{"@user1"}}, + &codeowners.ReviewerGroup{Names: codeowners.NewSlugs([]string{"@user1"})}, }, }, requiredOwners: requiredOwners, diff --git a/internal/github/approvals.go b/internal/github/approvals.go index f41891b..0a4cc12 100644 --- a/internal/github/approvals.go +++ b/internal/github/approvals.go @@ -53,21 +53,23 @@ func getApprovalDiffs( func checkStale( fileReviewerMap map[string][]string, approvals []*approvalWithDiff, -) ([]string, []*CurrentApproval) { +) ([]codeowners.Slug, []*CurrentApproval) { staleApprovals := make([]*CurrentApproval, 0) - approvers := make([]string, 0) + approvers := make([]codeowners.Slug, 0) for _, approval := range approvals { // for each file in the changes since approval // if the file is owned by the approval owner, mark stale // else, mark all overlapping owners as satisfied - // Note: both fileReviewerMap values and approval.inner.Reviewers contain normalized (lowercase) names + // Note: fileReviewerMap values are normalized strings, approval.inner.Reviewers are Slugs stale := false for _, diffFile := range approval.Diff { fileOwners, ok := fileReviewerMap[diffFile.FileName] if !ok { continue } - if len(f.Intersection(fileOwners, approval.inner.Reviewers)) > 0 { + // Convert Reviewers to normalized strings for Intersection + reviewersNormalized := codeowners.NormalizedStrings(approval.inner.Reviewers) + if len(f.Intersection(fileOwners, reviewersNormalized)) > 0 { stale = true } } diff --git a/internal/github/approvals_test.go b/internal/github/approvals_test.go index d9bd071..9460449 100644 --- a/internal/github/approvals_test.go +++ b/internal/github/approvals_test.go @@ -28,9 +28,9 @@ func (gd *fakeDiff) Context() git.DiffContext { func TestGetApprovalDiffs(t *testing.T) { approvals := []*CurrentApproval{ - {Reviewers: []string{"@base", "@core"}, CommitID: "123"}, - {Reviewers: []string{"@b-owner"}, CommitID: "123"}, - {Reviewers: []string{"@frontend"}, CommitID: "bad"}, + {Reviewers: codeowners.NewSlugs([]string{"@base", "@core"}), CommitID: "123"}, + {Reviewers: codeowners.NewSlugs([]string{"@b-owner"}), CommitID: "123"}, + {Reviewers: codeowners.NewSlugs([]string{"@frontend"}), CommitID: "bad"}, } diff := &fakeDiff{} diff --git a/internal/github/gh.go b/internal/github/gh.go index 85f894f..bd14108 100644 --- a/internal/github/gh.go +++ b/internal/github/gh.go @@ -38,8 +38,8 @@ type Client interface { AllApprovals() ([]*CurrentApproval, error) FindUserApproval(ghUser string) (*CurrentApproval, error) GetCurrentReviewerApprovals() ([]*CurrentApproval, error) - GetAlreadyReviewed() ([]string, error) - GetCurrentlyRequested() ([]string, error) + GetAlreadyReviewed() ([]codeowners.Slug, error) + GetCurrentlyRequested() ([]codeowners.Slug, error) DismissStaleReviews(staleApprovals []*CurrentApproval) error RequestReviewers(reviewers []string) error ApprovePR() error @@ -49,7 +49,7 @@ type Client interface { UpdateComment(commentID int64, body string) error IsInComments(comment string, since *time.Time) (bool, error) IsSubstringInComments(substring string, since *time.Time) (bool, error) - CheckApprovals(fileReviewerMap map[string][]string, approvals []*CurrentApproval, originalDiff git.Diff) (approvers []string, staleApprovals []*CurrentApproval) + CheckApprovals(fileReviewerMap map[string][]string, approvals []*CurrentApproval, originalDiff git.Diff) (approvers []codeowners.Slug, staleApprovals []*CurrentApproval) IsInLabels(labels []string) (bool, error) IsRepositoryAdmin(username string) (bool, error) ContainsValidBypassApproval(allowedUsers []string) (bool, error) @@ -172,7 +172,7 @@ func (gh *GHClient) InitReviews() error { func (gh *GHClient) approvals() []*github.PullRequestReview { seen := make(map[string]bool, 0) approvals := f.Filtered(gh.reviews, func(approval *github.PullRequestReview) bool { - userName := codeowners.NormalizeUsername(approval.GetUser().GetLogin()) + userName := strings.ToLower(approval.GetUser().GetLogin()) if _, ok := seen[userName]; ok { // we only care about the most recent reviews for each user return false @@ -204,10 +204,11 @@ func (gh *GHClient) ContainsValidBypassApproval(allowedUsers []string) (bool, er } username := review.GetUser().GetLogin() + usernameSlug := codeowners.NewSlug(username) // Check if user is in allowed users list for _, allowedUser := range allowedUsers { - if codeowners.NormalizeUsername(username) == codeowners.NormalizeUsername(allowedUser) { + if usernameSlug.EqualsString(allowedUser) { return true, nil } } @@ -239,7 +240,7 @@ func (gh *GHClient) AllApprovals() ([]*CurrentApproval, error) { } } return f.Map(gh.approvals(), func(approval *github.PullRequestReview) *CurrentApproval { - return &CurrentApproval{approval.User.GetLogin(), approval.GetID(), nil, approval.GetCommitID()} + return &CurrentApproval{codeowners.NewSlug(approval.User.GetLogin()), approval.GetID(), nil, approval.GetCommitID()} }), nil } @@ -253,14 +254,15 @@ func (gh *GHClient) FindUserApproval(ghUser string) (*CurrentApproval, error) { return nil, err } } + ghUserSlug := codeowners.NewSlug(ghUser) review, found := f.Find(gh.approvals(), func(review *github.PullRequestReview) bool { - return codeowners.NormalizeUsername(review.GetUser().GetLogin()) == codeowners.NormalizeUsername(ghUser) + return ghUserSlug.EqualsString(review.GetUser().GetLogin()) }) if !found { return nil, nil } - return &CurrentApproval{review.User.GetLogin(), review.GetID(), nil, review.GetCommitID()}, nil + return &CurrentApproval{codeowners.NewSlug(review.User.GetLogin()), review.GetID(), nil, review.GetCommitID()}, nil } func (gh *GHClient) GetCurrentReviewerApprovals() ([]*CurrentApproval, error) { @@ -283,11 +285,12 @@ func currentReviewerApprovalsFromReviews(approvals []*github.PullRequestReview, filteredApprovals := make([]*CurrentApproval, 0, len(approvals)) for _, review := range approvals { reviewingUser := review.GetUser().GetLogin() - if reviewers, ok := userReviewerMap[reviewingUser]; ok { - newApproval := &CurrentApproval{reviewingUser, review.GetID(), reviewers, review.GetCommitID()} + reviewingUserSlug := codeowners.NewSlug(reviewingUser) + if reviewers, ok := userReviewerMap[strings.ToLower(reviewingUser)]; ok { + newApproval := &CurrentApproval{reviewingUserSlug, review.GetID(), reviewers, review.GetCommitID()} filteredApprovals = append(filteredApprovals, newApproval) } else { - newApproval := &CurrentApproval{reviewingUser, review.GetID(), []string{}, review.GetCommitID()} + newApproval := &CurrentApproval{reviewingUserSlug, review.GetID(), []codeowners.Slug{}, review.GetCommitID()} filteredApprovals = append(filteredApprovals, newApproval) } } @@ -295,7 +298,7 @@ func currentReviewerApprovalsFromReviews(approvals []*github.PullRequestReview, return filteredApprovals } -func (gh *GHClient) GetAlreadyReviewed() ([]string, error) { +func (gh *GHClient) GetAlreadyReviewed() ([]codeowners.Slug, error) { if gh.pr == nil { return nil, &NoPRError{} } @@ -305,21 +308,21 @@ func (gh *GHClient) GetAlreadyReviewed() ([]string, error) { return reviewerAlreadyReviewed(gh.reviews, gh.userReviewerMap), nil } -func reviewerAlreadyReviewed(reviews []*github.PullRequestReview, userReviewerMap ghUserReviewerMap) []string { - reviewsReviewers := make(map[string]bool, len(reviews)) +func reviewerAlreadyReviewed(reviews []*github.PullRequestReview, userReviewerMap ghUserReviewerMap) []codeowners.Slug { + reviewsReviewers := make(map[string]codeowners.Slug, len(reviews)) for _, review := range reviews { reviewingUser := review.GetUser().GetLogin() - if reviewers, ok := userReviewerMap[reviewingUser]; ok { + if reviewers, ok := userReviewerMap[strings.ToLower(reviewingUser)]; ok { for _, reviewer := range reviewers { - reviewsReviewers[reviewer] = true + reviewsReviewers[reviewer.Normalized()] = reviewer } } } - return slices.Collect(maps.Keys(reviewsReviewers)) + return slices.Collect(maps.Values(reviewsReviewers)) } -func (gh *GHClient) GetCurrentlyRequested() ([]string, error) { +func (gh *GHClient) GetCurrentlyRequested() ([]codeowners.Slug, error) { if gh.pr == nil { return nil, &NoPRError{} } @@ -329,7 +332,7 @@ func (gh *GHClient) GetCurrentlyRequested() ([]string, error) { return currentlyRequested(gh.pr, gh.owner, gh.userReviewerMap), nil } -func currentlyRequested(pr *github.PullRequest, owner string, userReviewerMap ghUserReviewerMap) []string { +func currentlyRequested(pr *github.PullRequest, owner string, userReviewerMap ghUserReviewerMap) []codeowners.Slug { requestedUsers := f.Map(pr.RequestedReviewers, func(user *github.User) string { return user.GetLogin() }) @@ -337,13 +340,18 @@ func currentlyRequested(pr *github.PullRequest, owner string, userReviewerMap gh return fmt.Sprintf("%s/%s", owner, team.GetSlug()) }) requested := slices.Concat(requestedUsers, requestedTeams) - reviewers := make([]string, 0, len(requested)) + reviewers := make([]codeowners.Slug, 0, len(requested)) for _, user := range requested { - if reviewer, ok := userReviewerMap[user]; ok { + if reviewer, ok := userReviewerMap[strings.ToLower(user)]; ok { reviewers = append(reviewers, reviewer...) } } - return f.RemoveDuplicates(reviewers) + // Deduplicate based on normalized form + seen := make(map[string]codeowners.Slug) + for _, rev := range reviewers { + seen[rev.Normalized()] = rev + } + return slices.Collect(maps.Values(seen)) } func (gh *GHClient) DismissStaleReviews(staleApprovals []*CurrentApproval) error { @@ -557,7 +565,7 @@ func (gh *GHClient) CheckApprovals( fileReviewerMap map[string][]string, approvals []*CurrentApproval, originalDiff git.Diff, -) (approvers []string, staleApprovals []*CurrentApproval) { +) (approvers []codeowners.Slug, staleApprovals []*CurrentApproval) { appovalsWithDiff, badApprovals := getApprovalDiffs(approvals, originalDiff, gh.warningBuffer, gh.infoBuffer) approvers, staleApprovals = checkStale(fileReviewerMap, appovalsWithDiff) return approvers, append(badApprovals, staleApprovals...) @@ -577,12 +585,12 @@ func (gh *GHClient) IsRepositoryAdmin(username string) (bool, error) { return permission.GetPermission() == "admin", nil } -type ghUserReviewerMap map[string][]string +type ghUserReviewerMap map[string][]codeowners.Slug type CurrentApproval struct { - GHLogin string + GHLogin codeowners.Slug ReviewID int64 - Reviewers []string + Reviewers []codeowners.Slug CommitID string } @@ -593,19 +601,20 @@ func (p *CurrentApproval) String() string { func makeGHUserReviwerMap(reviewers []string, teamFetcher func(string, string) []*github.User) ghUserReviewerMap { userReviewerMap := make(ghUserReviewerMap) - insertReviewer := func(userName string, reviewer string) { + insertReviewer := func(userName string, reviewer codeowners.Slug) { reviewers, found := userReviewerMap[userName] if found { userReviewerMap[userName] = append(reviewers, reviewer) } else { - userReviewerMap[userName] = []string{reviewer} + userReviewerMap[userName] = []codeowners.Slug{reviewer} } } for _, reviewer := range reviewers { + reviewerSlug := codeowners.NewSlug(reviewer) reviewerString := reviewer[1:] // trim the @ // Add the team or user to the map - insertReviewer(codeowners.NormalizeUsername(reviewerString), reviewer) + insertReviewer(strings.ToLower(reviewerString), reviewerSlug) if !strings.Contains(reviewerString, "/") { // This is a user continue @@ -616,7 +625,7 @@ func makeGHUserReviwerMap(reviewers []string, teamFetcher func(string, string) [ users := teamFetcher(org, team) // Add the team members to the map for _, user := range users { - insertReviewer(codeowners.NormalizeUsername(user.GetLogin()), reviewer) + insertReviewer(strings.ToLower(user.GetLogin()), reviewerSlug) } } return userReviewerMap diff --git a/internal/github/gh_test.go b/internal/github/gh_test.go index b1c7ae5..d928c1b 100644 --- a/internal/github/gh_test.go +++ b/internal/github/gh_test.go @@ -14,6 +14,7 @@ import ( "time" "github.com/google/go-github/v63/github" + "github.com/multimediallc/codeowners-plus/pkg/codeowners" f "github.com/multimediallc/codeowners-plus/pkg/functional" ) @@ -25,9 +26,9 @@ func setupReviews() *GHClient { {User: &github.User{Login: github.String("reviewer4")}, State: github.String("APPROVED"), ID: github.Int64(3), CommitID: github.String("commit3")}, } userReviewerMap := ghUserReviewerMap{ - "reviewer1": []string{"@a", "@b"}, - "reviewer2": []string{"@c"}, - "reviewer4": []string{"@e"}, + "reviewer1": codeowners.NewSlugs([]string{"@a", "@b"}), + "reviewer2": codeowners.NewSlugs([]string{"@c"}), + "reviewer4": codeowners.NewSlugs([]string{"@e"}), } gh := &GHClient{ reviews: reviews, @@ -72,9 +73,9 @@ func TestCurrentApprovalsFromReviews(t *testing.T) { t.Fatalf("Unexpected error: %v", err) } expectedApprovals := []*CurrentApproval{ - {CommitID: "commit1", Reviewers: []string{"@a", "@b"}}, - {CommitID: "commit3", Reviewers: []string{"@e"}}, - {CommitID: "commit3", Reviewers: []string{}}, + {CommitID: "commit1", Reviewers: codeowners.NewSlugs([]string{"@a", "@b"})}, + {CommitID: "commit3", Reviewers: codeowners.NewSlugs([]string{"@e"})}, + {CommitID: "commit3", Reviewers: []codeowners.Slug{}}, } if len(currentApprovals) != len(expectedApprovals) { @@ -104,9 +105,9 @@ func TestAllApprovals(t *testing.T) { } expectedApprovals := []*CurrentApproval{ - {CommitID: "commit1", Reviewers: []string{"@a", "@b"}}, - {CommitID: "commit1", Reviewers: []string{"@a", "@d"}}, // reviewer3 - {CommitID: "commit3", Reviewers: []string{"@e"}}, + {CommitID: "commit1", Reviewers: codeowners.NewSlugs([]string{"@a", "@b"})}, + {CommitID: "commit1", Reviewers: codeowners.NewSlugs([]string{"@a", "@d"})}, // reviewer3 + {CommitID: "commit3", Reviewers: codeowners.NewSlugs([]string{"@e"})}, } if len(currentApprovals) != len(expectedApprovals) { @@ -149,7 +150,7 @@ func TestGetAlreadyReviewed(t *testing.T) { if err != nil { t.Fatalf("Unexpected error: %v", err) } - expected := []string{"@a", "@b", "@c", "@e"} + expected := codeowners.NewSlugs([]string{"@a", "@b", "@c", "@e"}) if len(alreadyReviewed) != len(expected) { t.Errorf("Expected %d reviewers, got %d", len(expected), len(alreadyReviewed)) } @@ -160,11 +161,11 @@ func TestGetAlreadyReviewed(t *testing.T) { func TestCurrentlyRequested(t *testing.T) { userReviewerMap := ghUserReviewerMap{ - "user1": []string{"@a", "@b"}, - "user2": []string{"@c"}, - "user3": []string{"@d"}, - "org/team1": []string{"@e", "@a"}, - "org/team2": []string{"@f"}, + "user1": codeowners.NewSlugs([]string{"@a", "@b"}), + "user2": codeowners.NewSlugs([]string{"@c"}), + "user3": codeowners.NewSlugs([]string{"@d"}), + "org/team1": codeowners.NewSlugs([]string{"@e", "@a"}), + "org/team2": codeowners.NewSlugs([]string{"@f"}), } pr := &github.PullRequest{ RequestedReviewers: []*github.User{ @@ -188,7 +189,7 @@ func TestCurrentlyRequested(t *testing.T) { t.Fatalf("Unexpected error: %v", err) } - expected := []string{"@a", "@b", "@c", "@e", "@f"} + expected := codeowners.NewSlugs([]string{"@a", "@b", "@c", "@e", "@f"}) if len(requested) != len(expected) { t.Errorf("Expected %d requested reviewers, got %d", len(expected), len(requested)) @@ -226,12 +227,12 @@ func TestMakeGHUserReviewerMap(t *testing.T) { } userReviewerMap := makeGHUserReviwerMap([]string{"@user1", "@user2", "@org/team1", "@org/team2", "@other/teamX"}, teamFetcher) expectedUserReviewerMap := ghUserReviewerMap{ - "user1": []string{"@user1", "@org/team1", "@org/team2"}, - "user2": []string{"@user2"}, - "user3": []string{"@org/team1"}, - "org/team1": []string{"@org/team1"}, - "org/team2": []string{"@org/team2"}, - "other/teamx": []string{"@other/teamX"}, // Key is normalized to lowercase + "user1": codeowners.NewSlugs([]string{"@user1", "@org/team1", "@org/team2"}), + "user2": codeowners.NewSlugs([]string{"@user2"}), + "user3": codeowners.NewSlugs([]string{"@org/team1"}), + "org/team1": codeowners.NewSlugs([]string{"@org/team1"}), + "org/team2": codeowners.NewSlugs([]string{"@org/team2"}), + "other/teamx": codeowners.NewSlugs([]string{"@other/teamX"}), // Key is normalized to lowercase } if !reflect.DeepEqual(userReviewerMap, expectedUserReviewerMap) { @@ -1017,9 +1018,9 @@ func TestInitUserReviewerMap(t *testing.T) { // Validate the userReviewerMap expectedMap := ghUserReviewerMap{ - "user1": []string{"@user1"}, - "team_member1": []string{"@org1/team1", "@org2/team2"}, - "team_member2": []string{"@org1/team1"}, + "user1": codeowners.NewSlugs([]string{"@user1"}), + "team_member1": codeowners.NewSlugs([]string{"@org1/team1", "@org2/team2"}), + "team_member2": codeowners.NewSlugs([]string{"@org1/team1"}), } for user := range expectedMap { diff --git a/pkg/codeowners/codeowners.go b/pkg/codeowners/codeowners.go index 3382c38..2e34a01 100644 --- a/pkg/codeowners/codeowners.go +++ b/pkg/codeowners/codeowners.go @@ -30,7 +30,7 @@ type CodeOwners interface { UnownedFiles() []string // ApplyApprovals marks the given approvers as satisfied - ApplyApprovals(approvers []string) + ApplyApprovals(approvers []Slug) } // New creates a new CodeOwners object from a root path and a list of diff files @@ -55,11 +55,11 @@ type ownersMap struct { } func (om *ownersMap) SetAuthor(author string) { - normalizedAuthor := NormalizeUsername(author) - for _, reviewers := range om.nameReviewerMap[normalizedAuthor] { + authorSlug := NewSlug(author) + for _, reviewers := range om.nameReviewerMap[authorSlug.Normalized()] { // remove author from the reviewers list (case-insensitive) - reviewers.Names = slices.DeleteFunc(reviewers.Names, func(name string) bool { - return NormalizeUsername(name) == normalizedAuthor + reviewers.Names = slices.DeleteFunc(reviewers.Names, func(name Slug) bool { + return name.Equals(authorSlug) }) if len(reviewers.Names) == 0 { // mark the reviewer as approved if they are the author @@ -110,16 +110,12 @@ func (om *ownersMap) UnownedFiles() []string { } // Apply approver satisfaction to the owners map, and return the approvals which should be invalidated -func (om *ownersMap) ApplyApprovals(approvers []string) { - applyApproved := func(user string) { - normalizedUser := NormalizeUsername(user) - for _, reviewer := range om.nameReviewerMap[normalizedUser] { +func (om *ownersMap) ApplyApprovals(approvers []Slug) { + for _, user := range approvers { + for _, reviewer := range om.nameReviewerMap[user.Normalized()] { reviewer.Approved = true } } - for _, user := range approvers { - applyApproved(user) - } } type ownerTreeNode struct { @@ -264,7 +260,7 @@ func (otfm ownerTestFileMap) getOwners(fileNames []string) (*ownersMap, error) { for _, reviewer := range fileOwner.requiredReviewers { for _, name := range reviewer.Names { - normalizedName := NormalizeUsername(name) + normalizedName := name.Normalized() if v, ok := nameReviewerMap[normalizedName]; ok { nameReviewerMap[normalizedName] = append(v, reviewer) } else { diff --git a/pkg/codeowners/codeowners_test.go b/pkg/codeowners/codeowners_test.go index d964930..35e8b0f 100644 --- a/pkg/codeowners/codeowners_test.go +++ b/pkg/codeowners/codeowners_test.go @@ -16,7 +16,7 @@ func TestInitOwnerTree(t *testing.T) { t.Errorf("Expected name to be ../test_project, got %s", tree.name) } - if (*tree.fallback).Names[0] != "@base" { + if (*tree.fallback).Names[0].Original() != "@base" { t.Errorf("Expected fallback to be @base, got %+v", tree.fallback) } @@ -194,8 +194,9 @@ func TestAllReviewers(t *testing.T) { } for _, owner := range allReviewers { - if _, ok := expectedOwners[owner]; ok { - expectedOwners[owner] = true + ownerStr := owner.Original() + if _, ok := expectedOwners[ownerStr]; ok { + expectedOwners[ownerStr] = true } else { t.Errorf("Unexpected owner %s", owner) } @@ -251,8 +252,8 @@ func TestSetAuthorCaseInsensitive(t *testing.T) { // Verify that @b-owner was removed from reviewers allReviewers := owners.AllRequired().Flatten() for _, reviewer := range allReviewers { - if NormalizeUsername(reviewer) == "@b-owner" { - t.Errorf("Expected @b-owner to be removed, but found %s", reviewer) + if reviewer.Normalized() == "@b-owner" { + t.Errorf("Expected @b-owner to be removed, but found %s", reviewer.Original()) } } @@ -277,16 +278,16 @@ func TestApplyApprovalsCaseInsensitive(t *testing.T) { } // Apply approvals with different casing - owners.ApplyApprovals([]string{"@BASE", "@B-OWNER"}) // .codeowners has @base, @b-owner + owners.ApplyApprovals(NewSlugs([]string{"@BASE", "@B-OWNER"})) // .codeowners has @base, @b-owner // Verify approvals were applied regardless of case allRequired := owners.AllRequired() for _, rg := range allRequired { - normalizedNames := NormalizeUsernames(rg.Names) - for _, name := range normalizedNames { - if name == "@base" || name == "@b-owner" { + for _, name := range rg.Names { + normalized := name.Normalized() + if normalized == "@base" || normalized == "@b-owner" { if !rg.Approved { - t.Errorf("Expected %v to be approved", rg.Names) + t.Errorf("Expected %v to be approved", OriginalStrings(rg.Names)) } } } @@ -297,7 +298,7 @@ func TestApplyApprovalsCaseInsensitive(t *testing.T) { if err != nil { t.Errorf("Error getting owners: %v", err) } - owners2.ApplyApprovals([]string{"@base", "@b-owner"}) + owners2.ApplyApprovals(NewSlugs([]string{"@base", "@b-owner"})) // Both should produce the same approval state allRequired2 := owners2.AllRequired() @@ -330,9 +331,9 @@ func TestNameReviewerMapCaseInsensitive(t *testing.T) { for _, fileOwner := range owners.fileToOwner { for _, rg := range fileOwner.requiredReviewers { for _, name := range rg.Names { - if NormalizeUsername(name) == "@frontend" { + if name.Normalized() == "@frontend" { foundBefore = true - t.Logf("Found @frontend before approval: %v, approved=%v", rg.Names, rg.Approved) + t.Logf("Found @frontend before approval: %v, approved=%v", OriginalStrings(rg.Names), rg.Approved) } } } @@ -345,15 +346,15 @@ func TestNameReviewerMapCaseInsensitive(t *testing.T) { // Check that the nameReviewerMap uses normalized keys // Apply an approval with uppercase - owners.ApplyApprovals([]string{"@FRONTEND"}) + owners.ApplyApprovals(NewSlugs([]string{"@FRONTEND"})) // Verify it matches @frontend by checking if it was approved foundApproved := false for _, fileOwner := range owners.fileToOwner { for _, rg := range fileOwner.requiredReviewers { for _, name := range rg.Names { - if NormalizeUsername(name) == "@frontend" { - t.Logf("After approval: %v, approved=%v", rg.Names, rg.Approved) + if name.Normalized() == "@frontend" { + t.Logf("After approval: %v, approved=%v", OriginalStrings(rg.Names), rg.Approved) if rg.Approved { foundApproved = true } diff --git a/pkg/codeowners/normalize.go b/pkg/codeowners/normalize.go deleted file mode 100644 index ba4c72b..0000000 --- a/pkg/codeowners/normalize.go +++ /dev/null @@ -1,24 +0,0 @@ -package codeowners - -import "strings" - -// NormalizeUsername converts a username or team name to lowercase for case-insensitive comparison. -// This ensures that usernames like @User, @user, and @USER are treated as identical, -// matching GitHub's case-insensitive username behavior. -// The @ prefix is preserved and normalized as part of the string. -func NormalizeUsername(name string) string { - return strings.ToLower(name) -} - -// NormalizeUsernames normalizes a slice of usernames to lowercase for case-insensitive comparison. -// Returns a new slice with all usernames converted to lowercase. -func NormalizeUsernames(names []string) []string { - if names == nil { - return nil - } - normalized := make([]string, len(names)) - for i, name := range names { - normalized[i] = NormalizeUsername(name) - } - return normalized -} diff --git a/pkg/codeowners/normalize_test.go b/pkg/codeowners/normalize_test.go deleted file mode 100644 index 6c03049..0000000 --- a/pkg/codeowners/normalize_test.go +++ /dev/null @@ -1,142 +0,0 @@ -package codeowners - -import ( - "testing" -) - -func TestNormalizeUsername(t *testing.T) { - tt := []struct { - name string - input string - expected string - }{ - { - name: "lowercase username with @", - input: "@user", - expected: "@user", - }, - { - name: "uppercase username with @", - input: "@USER", - expected: "@user", - }, - { - name: "mixed case username with @", - input: "@JohnDoe", - expected: "@johndoe", - }, - { - name: "username without @", - input: "user", - expected: "user", - }, - { - name: "mixed case username without @", - input: "JohnDoe", - expected: "johndoe", - }, - { - name: "team name with slash", - input: "@org/TeamName", - expected: "@org/teamname", - }, - { - name: "uppercase team name", - input: "@ORG/TEAM", - expected: "@org/team", - }, - { - name: "empty string", - input: "", - expected: "", - }, - { - name: "unicode characters", - input: "@Üser", - expected: "@üser", - }, - { - name: "special characters preserved", - input: "@user-name_123", - expected: "@user-name_123", - }, - } - - for _, tc := range tt { - t.Run(tc.name, func(t *testing.T) { - result := NormalizeUsername(tc.input) - if result != tc.expected { - t.Errorf("NormalizeUsername(%q) = %q, expected %q", tc.input, result, tc.expected) - } - }) - } -} - -func TestNormalizeUsernames(t *testing.T) { - tt := []struct { - name string - input []string - expected []string - }{ - { - name: "mixed case usernames", - input: []string{"@User", "@ADMIN", "@john-doe"}, - expected: []string{"@user", "@admin", "@john-doe"}, - }, - { - name: "empty slice", - input: []string{}, - expected: []string{}, - }, - { - name: "nil slice", - input: nil, - expected: nil, - }, - { - name: "single username", - input: []string{"@JohnDoe"}, - expected: []string{"@johndoe"}, - }, - { - name: "team names", - input: []string{"@org/Team1", "@org/Team2"}, - expected: []string{"@org/team1", "@org/team2"}, - }, - { - name: "already lowercase", - input: []string{"@user1", "@user2"}, - expected: []string{"@user1", "@user2"}, - }, - { - name: "mixed teams and users", - input: []string{"@User", "@org/Team", "@ADMIN"}, - expected: []string{"@user", "@org/team", "@admin"}, - }, - } - - for _, tc := range tt { - t.Run(tc.name, func(t *testing.T) { - result := NormalizeUsernames(tc.input) - - // Handle nil case - if tc.expected == nil { - if result != nil { - t.Errorf("NormalizeUsernames(%v) = %v, expected nil", tc.input, result) - } - return - } - - if len(result) != len(tc.expected) { - t.Errorf("NormalizeUsernames(%v) returned %d items, expected %d", tc.input, len(result), len(tc.expected)) - return - } - - for i := range result { - if result[i] != tc.expected[i] { - t.Errorf("NormalizeUsernames(%v)[%d] = %q, expected %q", tc.input, i, result[i], tc.expected[i]) - } - } - }) - } -} diff --git a/pkg/codeowners/reader_test.go b/pkg/codeowners/reader_test.go index 04fe47b..5989965 100644 --- a/pkg/codeowners/reader_test.go +++ b/pkg/codeowners/reader_test.go @@ -60,8 +60,8 @@ func TestRead(t *testing.T) { t.Errorf("Expected fallback to be nil, got %+v", rules.Fallback) } - if tc.fallback && rules.Fallback.Names[0] != tc.fallbackName { - t.Errorf("Expected fallback to be %s, got %s", tc.fallbackName, rules.Fallback.Names[0]) + if tc.fallback && rules.Fallback.Names[0].Original() != tc.fallbackName { + t.Errorf("Expected fallback to be %s, got %s", tc.fallbackName, rules.Fallback.Names[0].Original()) } if len(rules.OwnerTests) != tc.owners { diff --git a/pkg/codeowners/reviewers.go b/pkg/codeowners/reviewers.go index 9e0dade..9ba8804 100644 --- a/pkg/codeowners/reviewers.go +++ b/pkg/codeowners/reviewers.go @@ -26,67 +26,83 @@ func (rgm ReviewerGroupMemo) ToReviewerGroup(names ...string) *ReviewerGroup { if item, found := rgm[key]; found { return item } - newReviewers := &ReviewerGroup{names, false} + newReviewers := &ReviewerGroup{NewSlugs(names), false} rgm[key] = newReviewers return newReviewers } // Represents a group of ReviewerGroup, with a list of names and an approved status type ReviewerGroup struct { - Names []string + Names []Slug Approved bool } type ReviewerGroups []*ReviewerGroup func (rg *ReviewerGroup) ToCommentString() string { - return strings.Join(rg.Names, " or ") + return strings.Join(OriginalStrings(rg.Names), " or ") } -func (rgs ReviewerGroups) Flatten() []string { - names := make([]string, 0) +func (rgs ReviewerGroups) Flatten() []Slug { + names := make([]Slug, 0) for _, rg := range rgs { names = append(names, rg.Names...) } - names = f.RemoveDuplicates(names) - slices.Sort(names) - return names + // Use a map for deduplication based on normalized form + seen := make(map[string]Slug) + for _, name := range names { + seen[name.Normalized()] = name + } + // Extract unique slugs + unique := make([]Slug, 0, len(seen)) + for _, slug := range seen { + unique = append(unique, slug) + } + // Sort by normalized form + slices.SortFunc(unique, func(a, b Slug) int { + return strings.Compare(a.Normalized(), b.Normalized()) + }) + return unique } // ContainsAny returns true if any of the provided names (case-insensitive) // are present in any of the reviewer groups -func (rgs ReviewerGroups) ContainsAny(names []string) bool { - normalizedInput := NormalizeUsernames(names) +func (rgs ReviewerGroups) ContainsAny(names []Slug) bool { for _, rg := range rgs { - for _, name := range rg.Names { - if slices.Contains(normalizedInput, NormalizeUsername(name)) { - return true + for _, rgName := range rg.Names { + for _, name := range names { + if rgName.Equals(name) { + return true + } } } } return false } -func (rgs ReviewerGroups) FilterOut(names ...string) ReviewerGroups { - normalizedNames := NormalizeUsernames(names) +func (rgs ReviewerGroups) FilterOut(names ...Slug) ReviewerGroups { return f.Filtered(rgs, func(rg *ReviewerGroup) bool { - found := false - for _, name := range rg.Names { - if slices.Contains(normalizedNames, NormalizeUsername(name)) { - found = true - break + for _, rgName := range rg.Names { + for _, name := range names { + if rgName.Equals(name) { + return false + } } } - return !found + return true }) } // FilterOutNames returns a new slice with names from 'names' that are NOT present // in 'exclude' (case-insensitive comparison) -func FilterOutNames(names []string, exclude []string) []string { - normalizedExclude := NormalizeUsernames(exclude) - return f.Filtered(names, func(name string) bool { - return !slices.Contains(normalizedExclude, NormalizeUsername(name)) +func FilterOutNames(names []Slug, exclude []Slug) []Slug { + return f.Filtered(names, func(name Slug) bool { + for _, ex := range exclude { + if name.Equals(ex) { + return false + } + } + return true }) } @@ -136,12 +152,12 @@ func (fo *fileOwners) OptionalReviewers() ReviewerGroups { // Returns the names of the required reviewers, excluding those who have already approved func (fo *fileOwners) RequiredNames() []string { - return fo.RequiredReviewers().Flatten() + return OriginalStrings(fo.RequiredReviewers().Flatten()) } // Returns the names of the opitonal reviewers func (fo *fileOwners) OptionalNames() []string { - return fo.OptionalReviewers().Flatten() + return OriginalStrings(fo.OptionalReviewers().Flatten()) } type reviewerTest struct { diff --git a/pkg/codeowners/reviewers_test.go b/pkg/codeowners/reviewers_test.go index b888eea..fff3ee9 100644 --- a/pkg/codeowners/reviewers_test.go +++ b/pkg/codeowners/reviewers_test.go @@ -13,31 +13,31 @@ func TestToReviewers(t *testing.T) { tt := []struct { name string input []string - expected []string + expected []Slug checkExisting bool }{ { name: "empty input", input: []string{}, - expected: []string{}, + expected: []Slug{}, checkExisting: false, }, { name: "single input", input: []string{"@a"}, - expected: []string{"@a"}, + expected: NewSlugs([]string{"@a"}), checkExisting: true, }, { name: "multiple inputs", input: []string{"@a", "@b"}, - expected: []string{"@a", "@b"}, + expected: NewSlugs([]string{"@a", "@b"}), checkExisting: false, }, { name: "maintain order", input: []string{"@b", "@a"}, - expected: []string{"@b", "@a"}, + expected: NewSlugs([]string{"@b", "@a"}), checkExisting: false, }, } @@ -143,7 +143,7 @@ func TestToCommentString(t *testing.T) { func TestReviewerGroupsFlatten(t *testing.T) { rgMan := NewReviewerGroupMemo() rgs := ReviewerGroups{rgMan.ToReviewerGroup("@a", "@c"), rgMan.ToReviewerGroup("@b"), rgMan.ToReviewerGroup("@b", "@c")} - if !f.SlicesItemsMatch(rgs.Flatten(), []string{"@a", "@b", "@c"}) { + if !f.SlicesItemsMatch(rgs.Flatten(), NewSlugs([]string{"@a", "@b", "@c"})) { t.Error("Flatten should return a list of sorted reviewer names with duplicates removed") } } @@ -151,17 +151,17 @@ func TestReviewerGroupsFlatten(t *testing.T) { func TestReviewerGroupsFilter(t *testing.T) { rgMan := NewReviewerGroupMemo() rgs := ReviewerGroups{rgMan.ToReviewerGroup("@a", "@c"), rgMan.ToReviewerGroup("@b")} - rgs = rgs.FilterOut("@a") + rgs = rgs.FilterOut(NewSlug("@a")) // Filtering "@a" should remove the whole ReviewerGroup from the list - if !f.SlicesItemsMatch(rgs.Flatten(), []string{"@b"}) { + if !f.SlicesItemsMatch(rgs.Flatten(), NewSlugs([]string{"@b"})) { t.Error("Filter should remove ReviewerGroup[s] with names in the filter list") } rgMan = NewReviewerGroupMemo() rgs = ReviewerGroups{rgMan.ToReviewerGroup("@a", "@c"), rgMan.ToReviewerGroup("@b"), rgMan.ToReviewerGroup("@c", "@d")} - rgs = rgs.FilterOut("@a", "@b") + rgs = rgs.FilterOut(NewSlug("@a"), NewSlug("@b")) // Filtering "@a" should remove the whole ReviewerGroup from the list - if !f.SlicesItemsMatch(rgs.Flatten(), []string{"@c", "@d"}) { + if !f.SlicesItemsMatch(rgs.Flatten(), NewSlugs([]string{"@c", "@d"})) { t.Error("Filter should work with multiple names") } } @@ -209,7 +209,7 @@ func TestReviewerGroupsContainsAny(t *testing.T) { for _, tc := range tt { t.Run(tc.name, func(t *testing.T) { - result := rgs.ContainsAny(tc.input) + result := rgs.ContainsAny(NewSlugs(tc.input)) if result != tc.expected { t.Errorf("ContainsAny(%v) = %v, expected %v", tc.input, result, tc.expected) } @@ -264,8 +264,8 @@ func TestFilterOutNames(t *testing.T) { for _, tc := range tt { t.Run(tc.name, func(t *testing.T) { - result := FilterOutNames(tc.names, tc.exclude) - if !f.SlicesItemsMatch(result, tc.expected) { + result := FilterOutNames(NewSlugs(tc.names), NewSlugs(tc.exclude)) + if !f.SlicesItemsMatch(result, NewSlugs(tc.expected)) { t.Errorf("FilterOutNames(%v, %v) = %v, expected %v", tc.names, tc.exclude, result, tc.expected) } }) diff --git a/pkg/codeowners/slug.go b/pkg/codeowners/slug.go new file mode 100644 index 0000000..e55b5b5 --- /dev/null +++ b/pkg/codeowners/slug.go @@ -0,0 +1,101 @@ +package codeowners + +import ( + "encoding/json" + "strings" +) + +// Slug represents a GitHub username or team name (handle) with case-insensitive semantics. +// It stores both the original representation (for display/API calls) and normalized +// form (for comparisons). The zero value is not valid - use NewSlug to construct. +// +// GitHub uses "slug" as the canonical term for handles that work for both users and teams. +type Slug struct { + original string + normalized string +} + +// NewSlug creates a Slug from a string, normalizing it for comparison. +// The original casing is preserved for display purposes. +func NewSlug(name string) Slug { + return Slug{ + original: name, + normalized: strings.ToLower(name), + } +} + +// Equals performs case-insensitive comparison with another Slug. +func (s Slug) Equals(other Slug) bool { + return s.normalized == other.normalized +} + +// EqualsString performs case-insensitive comparison with a string. +// Provided for convenience during migration and at boundaries. +func (s Slug) EqualsString(str string) bool { + return s.normalized == strings.ToLower(str) +} + +// Original returns the original string representation (preserves case). +// Use this for display, comments, API calls to GitHub. +func (s Slug) Original() string { + return s.original +} + +// Normalized returns the lowercase normalized form. +// Use this for map keys and comparisons with legacy code. +func (s Slug) Normalized() string { + return s.normalized +} + +// String returns the original representation for fmt.Printf, logging, etc. +func (s Slug) String() string { + return s.original +} + +// MarshalJSON implements json.Marshaler for GitHub API compatibility. +// Serializes as the original string to preserve case. +func (s Slug) MarshalJSON() ([]byte, error) { + return json.Marshal(s.original) +} + +// NewSlugs converts a slice of strings to Slugs. +func NewSlugs(names []string) []Slug { + if names == nil { + return nil + } + slugs := make([]Slug, len(names)) + for i, name := range names { + slugs[i] = NewSlug(name) + } + return slugs +} + +// OriginalStrings extracts original strings from Slug slice. +// Use at boundaries: API calls, comments, display. +func OriginalStrings(slugs []Slug) []string { + result := make([]string, len(slugs)) + for i, s := range slugs { + result[i] = s.Original() + } + return result +} + +// NormalizedStrings extracts normalized strings from Slug slice. +// Use for comparisons with legacy code, generic functions (Intersection, Filtered). +func NormalizedStrings(slugs []Slug) []string { + result := make([]string, len(slugs)) + for i, s := range slugs { + result[i] = s.Normalized() + } + return result +} + +// ContainsSlug checks if a Slug is in a slice (case-insensitive). +func ContainsSlug(slugs []Slug, target Slug) bool { + for _, s := range slugs { + if s.Equals(target) { + return true + } + } + return false +} diff --git a/pkg/codeowners/slug_test.go b/pkg/codeowners/slug_test.go new file mode 100644 index 0000000..c872060 --- /dev/null +++ b/pkg/codeowners/slug_test.go @@ -0,0 +1,383 @@ +package codeowners + +import ( + "encoding/json" + "testing" +) + +func TestNewSlug(t *testing.T) { + tt := []struct { + name string + input string + wantOrig string + wantNorm string + }{ + { + name: "lowercase username", + input: "@user", + wantOrig: "@user", + wantNorm: "@user", + }, + { + name: "uppercase username", + input: "@USER", + wantOrig: "@USER", + wantNorm: "@user", + }, + { + name: "mixed case username", + input: "@UsEr", + wantOrig: "@UsEr", + wantNorm: "@user", + }, + { + name: "team name", + input: "@org/team", + wantOrig: "@org/team", + wantNorm: "@org/team", + }, + { + name: "mixed case team", + input: "@Org/Team", + wantOrig: "@Org/Team", + wantNorm: "@org/team", + }, + { + name: "without @ prefix", + input: "user", + wantOrig: "user", + wantNorm: "user", + }, + } + + for _, tc := range tt { + t.Run(tc.name, func(t *testing.T) { + slug := NewSlug(tc.input) + if slug.Original() != tc.wantOrig { + t.Errorf("Original() = %q, want %q", slug.Original(), tc.wantOrig) + } + if slug.Normalized() != tc.wantNorm { + t.Errorf("Normalized() = %q, want %q", slug.Normalized(), tc.wantNorm) + } + }) + } +} + +func TestSlugEquals(t *testing.T) { + tt := []struct { + name string + slug1 string + slug2 string + want bool + }{ + { + name: "exact match", + slug1: "@user", + slug2: "@user", + want: true, + }, + { + name: "case insensitive match", + slug1: "@user", + slug2: "@USER", + want: true, + }, + { + name: "mixed case match", + slug1: "@UsEr", + slug2: "@uSeR", + want: true, + }, + { + name: "different users", + slug1: "@user1", + slug2: "@user2", + want: false, + }, + { + name: "team case insensitive", + slug1: "@org/team", + slug2: "@Org/Team", + want: true, + }, + } + + for _, tc := range tt { + t.Run(tc.name, func(t *testing.T) { + s1 := NewSlug(tc.slug1) + s2 := NewSlug(tc.slug2) + if got := s1.Equals(s2); got != tc.want { + t.Errorf("Equals() = %v, want %v", got, tc.want) + } + }) + } +} + +func TestSlugEqualsString(t *testing.T) { + tt := []struct { + name string + slug string + str string + want bool + }{ + { + name: "exact match", + slug: "@user", + str: "@user", + want: true, + }, + { + name: "case insensitive match", + slug: "@user", + str: "@USER", + want: true, + }, + { + name: "mixed case match", + slug: "@UsEr", + str: "@uSeR", + want: true, + }, + { + name: "different users", + slug: "@user1", + str: "@user2", + want: false, + }, + } + + for _, tc := range tt { + t.Run(tc.name, func(t *testing.T) { + s := NewSlug(tc.slug) + if got := s.EqualsString(tc.str); got != tc.want { + t.Errorf("EqualsString() = %v, want %v", got, tc.want) + } + }) + } +} + +func TestSlugString(t *testing.T) { + tt := []struct { + name string + input string + want string + }{ + { + name: "preserves lowercase", + input: "@user", + want: "@user", + }, + { + name: "preserves uppercase", + input: "@USER", + want: "@USER", + }, + { + name: "preserves mixed case", + input: "@UsEr", + want: "@UsEr", + }, + } + + for _, tc := range tt { + t.Run(tc.name, func(t *testing.T) { + slug := NewSlug(tc.input) + if got := slug.String(); got != tc.want { + t.Errorf("String() = %q, want %q", got, tc.want) + } + }) + } +} + +func TestSlugMarshalJSON(t *testing.T) { + tt := []struct { + name string + input string + want string + }{ + { + name: "lowercase", + input: "@user", + want: `"@user"`, + }, + { + name: "uppercase", + input: "@USER", + want: `"@USER"`, + }, + { + name: "mixed case", + input: "@UsEr", + want: `"@UsEr"`, + }, + } + + for _, tc := range tt { + t.Run(tc.name, func(t *testing.T) { + slug := NewSlug(tc.input) + data, err := json.Marshal(slug) + if err != nil { + t.Fatalf("Marshal() error = %v", err) + } + if string(data) != tc.want { + t.Errorf("Marshal() = %s, want %s", data, tc.want) + } + }) + } +} + +func TestNewSlugs(t *testing.T) { + tt := []struct { + name string + input []string + want []string // original strings + }{ + { + name: "mixed case", + input: []string{"@user1", "@USER2", "@UsEr3"}, + want: []string{"@user1", "@USER2", "@UsEr3"}, + }, + { + name: "empty slice", + input: []string{}, + want: []string{}, + }, + { + name: "nil slice", + input: nil, + want: nil, + }, + } + + for _, tc := range tt { + t.Run(tc.name, func(t *testing.T) { + slugs := NewSlugs(tc.input) + if tc.want == nil { + if slugs != nil { + t.Errorf("NewSlugs() = %v, want nil", slugs) + } + return + } + if len(slugs) != len(tc.want) { + t.Fatalf("NewSlugs() length = %d, want %d", len(slugs), len(tc.want)) + } + for i, slug := range slugs { + if slug.Original() != tc.want[i] { + t.Errorf("NewSlugs()[%d].Original() = %q, want %q", i, slug.Original(), tc.want[i]) + } + } + }) + } +} + +func TestOriginalStrings(t *testing.T) { + tt := []struct { + name string + input []string + want []string + }{ + { + name: "preserves case", + input: []string{"@user1", "@USER2", "@UsEr3"}, + want: []string{"@user1", "@USER2", "@UsEr3"}, + }, + { + name: "empty slice", + input: []string{}, + want: []string{}, + }, + } + + for _, tc := range tt { + t.Run(tc.name, func(t *testing.T) { + slugs := NewSlugs(tc.input) + got := OriginalStrings(slugs) + if len(got) != len(tc.want) { + t.Fatalf("OriginalStrings() length = %d, want %d", len(got), len(tc.want)) + } + for i, s := range got { + if s != tc.want[i] { + t.Errorf("OriginalStrings()[%d] = %q, want %q", i, s, tc.want[i]) + } + } + }) + } +} + +func TestNormalizedStrings(t *testing.T) { + tt := []struct { + name string + input []string + want []string + }{ + { + name: "normalizes to lowercase", + input: []string{"@user1", "@USER2", "@UsEr3"}, + want: []string{"@user1", "@user2", "@user3"}, + }, + { + name: "empty slice", + input: []string{}, + want: []string{}, + }, + } + + for _, tc := range tt { + t.Run(tc.name, func(t *testing.T) { + slugs := NewSlugs(tc.input) + got := NormalizedStrings(slugs) + if len(got) != len(tc.want) { + t.Fatalf("NormalizedStrings() length = %d, want %d", len(got), len(tc.want)) + } + for i, s := range got { + if s != tc.want[i] { + t.Errorf("NormalizedStrings()[%d] = %q, want %q", i, s, tc.want[i]) + } + } + }) + } +} + +func TestContainsSlug(t *testing.T) { + tt := []struct { + name string + slugs []string + target string + want bool + }{ + { + name: "contains exact match", + slugs: []string{"@user1", "@user2", "@user3"}, + target: "@user2", + want: true, + }, + { + name: "contains case insensitive", + slugs: []string{"@user1", "@user2", "@user3"}, + target: "@USER2", + want: true, + }, + { + name: "not contains", + slugs: []string{"@user1", "@user2", "@user3"}, + target: "@user4", + want: false, + }, + { + name: "empty slice", + slugs: []string{}, + target: "@user1", + want: false, + }, + } + + for _, tc := range tt { + t.Run(tc.name, func(t *testing.T) { + slugs := NewSlugs(tc.slugs) + target := NewSlug(tc.target) + if got := ContainsSlug(slugs, target); got != tc.want { + t.Errorf("ContainsSlug() = %v, want %v", got, tc.want) + } + }) + } +} diff --git a/tools/cli/main.go b/tools/cli/main.go index 2b1f790..d7b4fc8 100644 --- a/tools/cli/main.go +++ b/tools/cli/main.go @@ -508,8 +508,9 @@ func mapFilesToOwners(ownersMap codeowners.CodeOwners) map[string][]string { for file, reviewerGroups := range allFileOwners { // Flatten, de-duplicate, and sort the owners. - owners := f.RemoveDuplicates(reviewerGroups.Flatten()) - if len(owners) > 0 { + ownerSlugs := f.RemoveDuplicates(reviewerGroups.Flatten()) + if len(ownerSlugs) > 0 { + owners := codeowners.OriginalStrings(ownerSlugs) slices.Sort(owners) fileToOwners[file] = owners } @@ -524,7 +525,8 @@ func mapOwnersToFiles(ownersMap codeowners.CodeOwners) map[string][]string { ownerToFilesSet := make(map[string]map[string]struct{}) for file, reviewerGroups := range allFileOwners { - for _, owner := range reviewerGroups.Flatten() { + for _, ownerSlug := range reviewerGroups.Flatten() { + owner := ownerSlug.Original() if _, ok := ownerToFilesSet[owner]; !ok { ownerToFilesSet[owner] = make(map[string]struct{}) } @@ -564,28 +566,32 @@ func validateCodeowners(repo string, target string) error { codeowners := codeowners.Read(target, rgm, &codeowners.FilesystemReader{}, io.Discard) if codeowners.Fallback != nil { - for _, name := range codeowners.Fallback.Names { + for _, nameSlug := range codeowners.Fallback.Names { + name := nameSlug.Original() if !strings.HasPrefix(name, "@") { _, _ = fmt.Fprintln(warningBuffer, "Fallback owner doesn't start with @: "+name) } } } for _, test := range codeowners.OwnerTests { - for _, name := range test.Reviewer.Names { + for _, nameSlug := range test.Reviewer.Names { + name := nameSlug.Original() if !strings.HasPrefix(name, "@") { _, _ = fmt.Fprintf(warningBuffer, "Owner test (%s) name doesn't start with @: %s\n", test.Match, name) } } } for _, test := range codeowners.AdditionalReviewerTests { - for _, name := range test.Reviewer.Names { + for _, nameSlug := range test.Reviewer.Names { + name := nameSlug.Original() if !strings.HasPrefix(name, "@") { _, _ = fmt.Fprintf(warningBuffer, "Additional reviewer test (%s) name doesn't start with @: %s\n", test.Match, name) } } } for _, test := range codeowners.OptionalReviewerTests { - for _, name := range test.Reviewer.Names { + for _, nameSlug := range test.Reviewer.Names { + name := nameSlug.Original() if !strings.HasPrefix(name, "@") { _, _ = fmt.Fprintf(warningBuffer, "Optional reviewer test (%s) name doesn't start with @: %s\n", test.Match, name) } diff --git a/tools/cli/main_test.go b/tools/cli/main_test.go index 30706ce..8751b24 100644 --- a/tools/cli/main_test.go +++ b/tools/cli/main_test.go @@ -416,20 +416,20 @@ func (f *fakeCodeOwners) FileRequired() map[string]codeowners.ReviewerGroups { func (f *fakeCodeOwners) FileOptional() map[string]codeowners.ReviewerGroups { return f.optional } -func (f *fakeCodeOwners) SetAuthor(author string) {} -func (f *fakeCodeOwners) AllRequired() codeowners.ReviewerGroups { return nil } -func (f *fakeCodeOwners) AllOptional() codeowners.ReviewerGroups { return nil } -func (f *fakeCodeOwners) UnownedFiles() []string { return nil } -func (f *fakeCodeOwners) ApplyApprovals(approvers []string) {} +func (f *fakeCodeOwners) SetAuthor(author string) {} +func (f *fakeCodeOwners) AllRequired() codeowners.ReviewerGroups { return nil } +func (f *fakeCodeOwners) AllOptional() codeowners.ReviewerGroups { return nil } +func (f *fakeCodeOwners) UnownedFiles() []string { return nil } +func (f *fakeCodeOwners) ApplyApprovals(approvers []codeowners.Slug) {} func TestJsonTargets(t *testing.T) { owners := &fakeCodeOwners{ required: map[string]codeowners.ReviewerGroups{ - "file1.txt": {&codeowners.ReviewerGroup{Names: []string{"@alice"}}}, - "file2.txt": {&codeowners.ReviewerGroup{Names: []string{"@bob"}}}, + "file1.txt": {&codeowners.ReviewerGroup{Names: codeowners.NewSlugs([]string{"@alice"})}}, + "file2.txt": {&codeowners.ReviewerGroup{Names: codeowners.NewSlugs([]string{"@bob"})}}, }, optional: map[string]codeowners.ReviewerGroups{ - "file1.txt": {&codeowners.ReviewerGroup{Names: []string{"@carol"}}}, + "file1.txt": {&codeowners.ReviewerGroup{Names: codeowners.NewSlugs([]string{"@carol"})}}, "file2.txt": {}, }, } @@ -484,11 +484,11 @@ func TestJsonTargets(t *testing.T) { func TestPrintTargets(t *testing.T) { owners := &fakeCodeOwners{ required: map[string]codeowners.ReviewerGroups{ - "file1.txt": {&codeowners.ReviewerGroup{Names: []string{"@alice"}}}, - "file2.txt": {&codeowners.ReviewerGroup{Names: []string{"@bob"}}}, + "file1.txt": {&codeowners.ReviewerGroup{Names: codeowners.NewSlugs([]string{"@alice"})}}, + "file2.txt": {&codeowners.ReviewerGroup{Names: codeowners.NewSlugs([]string{"@bob"})}}, }, optional: map[string]codeowners.ReviewerGroups{ - "file1.txt": {&codeowners.ReviewerGroup{Names: []string{"@carol"}}}, + "file1.txt": {&codeowners.ReviewerGroup{Names: codeowners.NewSlugs([]string{"@carol"})}}, "file2.txt": {}, }, } @@ -752,8 +752,8 @@ func TestMapOwnersToFiles(t *testing.T) { name: "simple case", input: &fakeCodeOwners{ required: map[string]codeowners.ReviewerGroups{ - "a.txt": {&codeowners.ReviewerGroup{Names: []string{"@owner1"}}}, - "b.txt": {&codeowners.ReviewerGroup{Names: []string{"@owner2"}}}, + "a.txt": {&codeowners.ReviewerGroup{Names: codeowners.NewSlugs([]string{"@owner1"})}}, + "b.txt": {&codeowners.ReviewerGroup{Names: codeowners.NewSlugs([]string{"@owner2"})}}, }, }, expected: map[string][]string{ @@ -765,8 +765,8 @@ func TestMapOwnersToFiles(t *testing.T) { name: "owner with multiple files", input: &fakeCodeOwners{ required: map[string]codeowners.ReviewerGroups{ - "a.txt": {&codeowners.ReviewerGroup{Names: []string{"@owner1"}}}, - "b.txt": {&codeowners.ReviewerGroup{Names: []string{"@owner1"}}}, + "a.txt": {&codeowners.ReviewerGroup{Names: codeowners.NewSlugs([]string{"@owner1"})}}, + "b.txt": {&codeowners.ReviewerGroup{Names: codeowners.NewSlugs([]string{"@owner1"})}}, }, }, expected: map[string][]string{ @@ -777,7 +777,7 @@ func TestMapOwnersToFiles(t *testing.T) { name: "file with multiple owners", input: &fakeCodeOwners{ required: map[string]codeowners.ReviewerGroups{ - "a.txt": {&codeowners.ReviewerGroup{Names: []string{"@owner1", "@owner2"}}}, + "a.txt": {&codeowners.ReviewerGroup{Names: codeowners.NewSlugs([]string{"@owner1", "@owner2"})}}, }, }, expected: map[string][]string{ @@ -789,12 +789,12 @@ func TestMapOwnersToFiles(t *testing.T) { name: "complex case with optional and duplicates", input: &fakeCodeOwners{ required: map[string]codeowners.ReviewerGroups{ - "a.txt": {&codeowners.ReviewerGroup{Names: []string{"@owner1"}}}, - "b.txt": {&codeowners.ReviewerGroup{Names: []string{"@owner2"}}}, + "a.txt": {&codeowners.ReviewerGroup{Names: codeowners.NewSlugs([]string{"@owner1"})}}, + "b.txt": {&codeowners.ReviewerGroup{Names: codeowners.NewSlugs([]string{"@owner2"})}}, }, optional: map[string]codeowners.ReviewerGroups{ - "a.txt": {&codeowners.ReviewerGroup{Names: []string{"@owner2"}}}, - "c.txt": {&codeowners.ReviewerGroup{Names: []string{"@owner1"}}}, + "a.txt": {&codeowners.ReviewerGroup{Names: codeowners.NewSlugs([]string{"@owner2"})}}, + "c.txt": {&codeowners.ReviewerGroup{Names: codeowners.NewSlugs([]string{"@owner1"})}}, }, }, expected: map[string][]string{ @@ -806,8 +806,8 @@ func TestMapOwnersToFiles(t *testing.T) { name: "files are sorted for an owner", input: &fakeCodeOwners{ required: map[string]codeowners.ReviewerGroups{ - "z.txt": {&codeowners.ReviewerGroup{Names: []string{"@owner1"}}}, - "a.txt": {&codeowners.ReviewerGroup{Names: []string{"@owner1"}}}, + "z.txt": {&codeowners.ReviewerGroup{Names: codeowners.NewSlugs([]string{"@owner1"})}}, + "a.txt": {&codeowners.ReviewerGroup{Names: codeowners.NewSlugs([]string{"@owner1"})}}, }, }, expected: map[string][]string{ From 54149c733781ef74146aa4655ab65d6f0c0a0535 Mon Sep 17 00:00:00 2001 From: BakerNet Date: Tue, 6 Jan 2026 16:49:01 -0800 Subject: [PATCH 4/4] covbadge --- README.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/README.md b/README.md index 6991132..43aeda2 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.4%25-brightgreen) +![Coverage](https://img.shields.io/badge/Coverage-82.8%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)