From cbfe07ae0539c752794efd1de3ab10e189e7a6da Mon Sep 17 00:00:00 2001 From: BakerNet Date: Fri, 21 Mar 2025 13:25:55 -0700 Subject: [PATCH 1/6] Implement high priority indicator via PR labels --- README.md | 17 +++++ internal/config/config.go | 2 + internal/config/config_test.go | 3 + internal/github/approvals.go | 2 +- internal/github/gh.go | 21 ++++++- internal/github/gh_test.go | 100 +++++++++++++++++++++++++++++- main.go | 12 +++- main_test.go | 21 ++++++- pkg/codeowners/codeowners_test.go | 2 +- pkg/codeowners/reviewers.go | 2 +- pkg/codeowners/reviewers_test.go | 2 +- tools/cli/main.go | 2 +- 12 files changed, 174 insertions(+), 12 deletions(-) diff --git a/README.md b/README.md index b18862a..12846cd 100644 --- a/README.md +++ b/README.md @@ -244,6 +244,23 @@ To prevent ownership rules from being checked or applied for certain directories ignore = ["test_project"] ``` +### High Priority Labels + +You can configure labels that indicate a high priority PR. When a PR has any of these labels, the comment will include a high priority indicator: + +```toml +high_priority_labels = ["high-priority", "urgent"] +``` + +When a PR has any of these labels, the comment will look like this: +``` +Codeowners approval required for this PR: +- @user1 +- @user2 + +❗High Prio❗ +``` + ## CLI Tool A CLI tool is available which provides some utilities for working with `.codeowners` files. diff --git a/internal/config/config.go b/internal/config/config.go index 22e9bcd..85d0445 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -14,6 +14,7 @@ type Config struct { UnskippableReviewers []string `toml:"unskippable_reviewers"` Ignore []string `toml:"ignore"` Enforcement *Enforcement `toml:"enforcement"` + HighPriorityLabels []string `toml:"high_priority_labels"` } type Enforcement struct { @@ -32,6 +33,7 @@ func ReadConfig(path string) (*Config, error) { UnskippableReviewers: []string{}, Ignore: []string{}, Enforcement: &Enforcement{Approval: false, FailCheck: true}, + HighPriorityLabels: []string{}, } fileName := path + "codeowners.toml" diff --git a/internal/config/config_test.go b/internal/config/config_test.go index 478bc60..d1622cf 100644 --- a/internal/config/config_test.go +++ b/internal/config/config_test.go @@ -36,6 +36,7 @@ ignore = ["ignored/"] [enforcement] approval = true fail_check = false +high_priority_labels = ["high-priority", "urgent"] `, path: "testdata/", expected: &Config{ @@ -44,6 +45,7 @@ fail_check = false UnskippableReviewers: []string{"@user1", "@user2"}, Ignore: []string{"ignored/"}, Enforcement: &Enforcement{Approval: true, FailCheck: false}, + HighPriorityLabels: []string{"high-priority", "urgent"}, }, expectedErr: false, }, @@ -60,6 +62,7 @@ unskippable_reviewers = ["@user1"] UnskippableReviewers: []string{"@user1"}, Ignore: []string{}, Enforcement: &Enforcement{Approval: false, FailCheck: true}, + HighPriorityLabels: []string{}, }, expectedErr: false, }, diff --git a/internal/github/approvals.go b/internal/github/approvals.go index a9c230f..3f7791f 100644 --- a/internal/github/approvals.go +++ b/internal/github/approvals.go @@ -7,7 +7,7 @@ import ( "github.com/multimediallc/codeowners-plus/internal/git" "github.com/multimediallc/codeowners-plus/pkg/codeowners" - "github.com/multimediallc/codeowners-plus/pkg/functional" + f "github.com/multimediallc/codeowners-plus/pkg/functional" ) type approvalWithDiff struct { diff --git a/internal/github/gh.go b/internal/github/gh.go index 12b6c15..ef77ec2 100644 --- a/internal/github/gh.go +++ b/internal/github/gh.go @@ -11,7 +11,7 @@ import ( "github.com/google/go-github/v63/github" "github.com/multimediallc/codeowners-plus/internal/git" - "github.com/multimediallc/codeowners-plus/pkg/functional" + f "github.com/multimediallc/codeowners-plus/pkg/functional" ) type NoPRError struct{} @@ -47,6 +47,7 @@ type Client interface { 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) + IsInLabels(labels []string) (bool, error) } type GHClient struct { @@ -414,6 +415,24 @@ func (gh *GHClient) IsSubstringInComments(substring string, since *time.Time) (b return false, nil } +// IsInLabels checks if the PR has any of the given labels +func (gh *GHClient) IsInLabels(labels []string) (bool, error) { + if gh.pr == nil { + return false, &NoPRError{} + } + if len(labels) == 0 { + return false, nil + } + for _, label := range gh.pr.Labels { + for _, targetLabel := range labels { + if label.GetName() == targetLabel { + return true, nil + } + } + } + return false, nil +} + // Apply approver satisfaction to the owners map, and return the approvals which should be invalidated func (gh *GHClient) CheckApprovals( fileReviewerMap map[string][]string, diff --git a/internal/github/gh_test.go b/internal/github/gh_test.go index d1e4831..c52f60f 100644 --- a/internal/github/gh_test.go +++ b/internal/github/gh_test.go @@ -12,7 +12,7 @@ import ( "time" "github.com/google/go-github/v63/github" - "github.com/multimediallc/codeowners-plus/pkg/functional" + f "github.com/multimediallc/codeowners-plus/pkg/functional" ) func setupReviews() *GHClient { @@ -424,6 +424,13 @@ func TestNilPRErr(t *testing.T) { return err }, }, + { + name: "IsInLabels", + testFn: func() error { + _, err := gh.IsInLabels([]string{"label"}) + return err + }, + }, } for _, tc := range tt { t.Run(tc.name, func(t *testing.T) { @@ -1019,3 +1026,94 @@ func TestInitUserReviewerMap(t *testing.T) { } } } + +func TestIsInLabels(t *testing.T) { + tt := []struct { + name string + pr *github.PullRequest + labels []string + expected bool + expectError bool + failMessage string + }{ + { + name: "has matching label", + pr: &github.PullRequest{ + Labels: []*github.Label{ + {Name: github.String("high-priority")}, + }, + }, + labels: []string{"high-priority"}, + expected: true, + expectError: false, + failMessage: "Should detect matching label", + }, + { + name: "has multiple labels but no match", + pr: &github.PullRequest{ + Labels: []*github.Label{ + {Name: github.String("bug")}, + {Name: github.String("enhancement")}, + }, + }, + labels: []string{"high-priority"}, + expected: false, + expectError: false, + failMessage: "Should not detect label when not present", + }, + { + name: "empty labels list", + pr: &github.PullRequest{ + Labels: []*github.Label{ + {Name: github.String("high-priority")}, + }, + }, + labels: []string{}, + expected: false, + expectError: false, + failMessage: "Should return false for empty labels list", + }, + { + name: "multiple target labels", + pr: &github.PullRequest{ + Labels: []*github.Label{ + {Name: github.String("urgent")}, + }, + }, + labels: []string{"high-priority", "urgent"}, + expected: true, + expectError: false, + failMessage: "Should detect any of the target labels", + }, + { + name: "nil PR", + pr: nil, + labels: []string{"high-priority"}, + expected: false, + expectError: true, + failMessage: "Should return error for nil PR", + }, + } + + for _, tc := range tt { + t.Run(tc.name, func(t *testing.T) { + client := &GHClient{pr: tc.pr} + hasLabel, err := client.IsInLabels(tc.labels) + if tc.expectError { + if err == nil { + t.Error("Expected error but got none") + } + if _, ok := err.(*NoPRError); !ok { + t.Errorf("Expected NoPRError, got %T", err) + } + return + } + if err != nil { + t.Errorf("Unexpected error: %v", err) + } + if hasLabel != tc.expected { + t.Error(tc.failMessage) + } + }) + } +} diff --git a/main.go b/main.go index 9db5a7d..3c0aa7a 100644 --- a/main.go +++ b/main.go @@ -11,11 +11,11 @@ import ( "testing" "time" - "github.com/multimediallc/codeowners-plus/internal/config" + owners "github.com/multimediallc/codeowners-plus/internal/config" "github.com/multimediallc/codeowners-plus/internal/git" - "github.com/multimediallc/codeowners-plus/internal/github" + gh "github.com/multimediallc/codeowners-plus/internal/github" "github.com/multimediallc/codeowners-plus/pkg/codeowners" - "github.com/multimediallc/codeowners-plus/pkg/functional" + f "github.com/multimediallc/codeowners-plus/pkg/functional" ) // AppConfig holds the application configuration @@ -214,6 +214,12 @@ func (a *App) processApprovalsAndReviewers() (bool, string, error) { if len(unapprovedOwners) > 0 { // Comment on the PR with the codeowner teams that have not approved the PR comment := allRequiredOwners.ToCommentString() + hasHighPriority, err := a.client.IsInLabels(a.conf.HighPriorityLabels) + if err != nil { + fmt.Fprintf(WarningBuffer, "WARNING: Error checking high priority labels: %v\n", err) + } else if hasHighPriority { + comment += "\n❗High Prio❗" + } if maxReviewsMet { comment += "\n\n" comment += "The PR has received the max number of required reviews. No further action is required." diff --git a/main_test.go b/main_test.go index b713d64..1900ad7 100644 --- a/main_test.go +++ b/main_test.go @@ -9,9 +9,9 @@ import ( "time" "github.com/google/go-github/v63/github" - "github.com/multimediallc/codeowners-plus/internal/config" + owners "github.com/multimediallc/codeowners-plus/internal/config" "github.com/multimediallc/codeowners-plus/internal/git" - "github.com/multimediallc/codeowners-plus/internal/github" + gh "github.com/multimediallc/codeowners-plus/internal/github" "github.com/multimediallc/codeowners-plus/pkg/codeowners" f "github.com/multimediallc/codeowners-plus/pkg/functional" ) @@ -262,6 +262,23 @@ func (m *mockGitHubClient) IsSubstringInComments(substring string, since *time.T return false, nil } +func (m *mockGitHubClient) IsInLabels(labels []string) (bool, error) { + if m.pr == nil { + return false, &gh.NoPRError{} + } + if len(labels) == 0 { + return false, nil + } + for _, label := range m.pr.Labels { + for _, targetLabel := range labels { + if label.GetName() == targetLabel { + return true, nil + } + } + } + return false, nil +} + func init() { // Initialize test flags with default values flags = &Flags{ diff --git a/pkg/codeowners/codeowners_test.go b/pkg/codeowners/codeowners_test.go index 845b32a..3b38998 100644 --- a/pkg/codeowners/codeowners_test.go +++ b/pkg/codeowners/codeowners_test.go @@ -5,7 +5,7 @@ import ( "reflect" "testing" - "github.com/multimediallc/codeowners-plus/pkg/functional" + f "github.com/multimediallc/codeowners-plus/pkg/functional" ) func TestInitOwnerTree(t *testing.T) { diff --git a/pkg/codeowners/reviewers.go b/pkg/codeowners/reviewers.go index c229ecd..200b5e8 100644 --- a/pkg/codeowners/reviewers.go +++ b/pkg/codeowners/reviewers.go @@ -7,7 +7,7 @@ import ( "strings" "github.com/bmatcuk/doublestar/v4" - "github.com/multimediallc/codeowners-plus/pkg/functional" + f "github.com/multimediallc/codeowners-plus/pkg/functional" ) var commentPrefix = "Codeowners approval required for this PR:\n" diff --git a/pkg/codeowners/reviewers_test.go b/pkg/codeowners/reviewers_test.go index 909417d..3662269 100644 --- a/pkg/codeowners/reviewers_test.go +++ b/pkg/codeowners/reviewers_test.go @@ -5,7 +5,7 @@ import ( "sort" "testing" - "github.com/multimediallc/codeowners-plus/pkg/functional" + f "github.com/multimediallc/codeowners-plus/pkg/functional" ) func TestToReviewers(t *testing.T) { diff --git a/tools/cli/main.go b/tools/cli/main.go index 15b05af..c130904 100644 --- a/tools/cli/main.go +++ b/tools/cli/main.go @@ -11,7 +11,7 @@ import ( "github.com/boyter/gocodewalker" "github.com/multimediallc/codeowners-plus/pkg/codeowners" - "github.com/multimediallc/codeowners-plus/pkg/functional" + f "github.com/multimediallc/codeowners-plus/pkg/functional" "github.com/urfave/cli/v2" ) From d2785009e4bde4c6d39560067b4452ae4c42cc4f Mon Sep 17 00:00:00 2001 From: BakerNet Date: Fri, 21 Mar 2025 13:31:56 -0700 Subject: [PATCH 2/6] Minor --- README.md | 1 - 1 file changed, 1 deletion(-) diff --git a/README.md b/README.md index 12846cd..78762cd 100644 --- a/README.md +++ b/README.md @@ -257,7 +257,6 @@ When a PR has any of these labels, the comment will look like this: Codeowners approval required for this PR: - @user1 - @user2 - ❗High Prio❗ ``` From 0ede1c18e87e43a4ff0f9d4ce10b5bf663d97251 Mon Sep 17 00:00:00 2001 From: BakerNet Date: Fri, 21 Mar 2025 13:34:47 -0700 Subject: [PATCH 3/6] Rearrange high prio comment text --- README.md | 3 ++- main.go | 2 +- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/README.md b/README.md index 78762cd..8eae158 100644 --- a/README.md +++ b/README.md @@ -254,10 +254,11 @@ high_priority_labels = ["high-priority", "urgent"] When a PR has any of these labels, the comment will look like this: ``` +❗High Prio❗ + Codeowners approval required for this PR: - @user1 - @user2 -❗High Prio❗ ``` ## CLI Tool diff --git a/main.go b/main.go index 3c0aa7a..0043dac 100644 --- a/main.go +++ b/main.go @@ -218,7 +218,7 @@ func (a *App) processApprovalsAndReviewers() (bool, string, error) { if err != nil { fmt.Fprintf(WarningBuffer, "WARNING: Error checking high priority labels: %v\n", err) } else if hasHighPriority { - comment += "\n❗High Prio❗" + comment = "❗High Prio❗\n\n" + comment } if maxReviewsMet { comment += "\n\n" From 7bc2eb8208da3a65ca42aa54aa1972cce5e48c51 Mon Sep 17 00:00:00 2001 From: BakerNet Date: Fri, 21 Mar 2025 13:49:38 -0700 Subject: [PATCH 4/6] Add high_prio_lables --- .github/workflows/codeowners.yml | 2 +- README.md | 2 +- codeowners.toml | 2 ++ 3 files changed, 4 insertions(+), 2 deletions(-) diff --git a/.github/workflows/codeowners.yml b/.github/workflows/codeowners.yml index 88ff1cd..d9f9904 100644 --- a/.github/workflows/codeowners.yml +++ b/.github/workflows/codeowners.yml @@ -7,7 +7,7 @@ concurrency: on: pull_request: branches: [main] - types: [opened, reopened, synchronize, ready_for_review] + types: [opened, reopened, synchronize, ready_for_review, labeled, unlabeled] jobs: codeowners: diff --git a/README.md b/README.md index 8eae158..b80807b 100644 --- a/README.md +++ b/README.md @@ -58,7 +58,7 @@ concurrency: on: pull_request: - types: [opened, reopened, synchronize, ready_for_review] + types: [opened, reopened, synchronize, ready_for_review, labeled, unlabeled] jobs: codeowners: diff --git a/codeowners.toml b/codeowners.toml index 8ba8516..a516046 100644 --- a/codeowners.toml +++ b/codeowners.toml @@ -6,6 +6,8 @@ max_reviews = 2 unskippable_reviewers = ["@BakerNet"] # `ignore` allows you to specify directories that should be ignored by the codeowners check ignore = ["test_project"] +# `high_priority_lables` allows you to specify labels that should be considered high priority +high_priority_lables = ["P0"] # `enforcement` allows you to specify how the codeowners check should be enforced [enforcement] From 55b9ee49714bd412e604754f7c7720d28494c360 Mon Sep 17 00:00:00 2001 From: BakerNet Date: Fri, 21 Mar 2025 13:52:57 -0700 Subject: [PATCH 5/6] Add other owner for testing --- .codeowners | 1 + 1 file changed, 1 insertion(+) diff --git a/.codeowners b/.codeowners index 6679fd2..75c73be 100644 --- a/.codeowners +++ b/.codeowners @@ -1,2 +1,3 @@ # For now, Hans reviews everything * @BakerNet +codeowners.toml @zbedforrest From 7af2a433aafe2fc867d6650f97ea5f0ce66f7399 Mon Sep 17 00:00:00 2001 From: BakerNet Date: Fri, 21 Mar 2025 13:57:13 -0700 Subject: [PATCH 6/6] Typo --- codeowners.toml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/codeowners.toml b/codeowners.toml index a516046..a859ab6 100644 --- a/codeowners.toml +++ b/codeowners.toml @@ -7,7 +7,7 @@ unskippable_reviewers = ["@BakerNet"] # `ignore` allows you to specify directories that should be ignored by the codeowners check ignore = ["test_project"] # `high_priority_lables` allows you to specify labels that should be considered high priority -high_priority_lables = ["P0"] +high_priority_labels = ["P0"] # `enforcement` allows you to specify how the codeowners check should be enforced [enforcement]