diff --git a/.github/dependabot.yml b/.github/dependabot.yml deleted file mode 100644 index a7d1a54a..00000000 --- a/.github/dependabot.yml +++ /dev/null @@ -1,14 +0,0 @@ -version: 2 - -updates: - - package-ecosystem: github-actions - directory: / - labels: ["no changelog", "merge when ready", "dependencies", "github_actions"] - schedule: - interval: weekly - - - package-ecosystem: npm - directory: / - labels: ["no changelog", "merge when ready", "dependencies", "javascript"] - schedule: - interval: monthly diff --git a/.github/workflows/build.yml b/.github/workflows/build.yml index ea970bca..5d7e333c 100644 --- a/.github/workflows/build.yml +++ b/.github/workflows/build.yml @@ -113,26 +113,16 @@ jobs: # Steps after this point should only run when publishing # Include them here to avoid exporting the Docker container as an artifact # - - - name: Login to Docker Hub - if: ${{ github.event_name == 'push' || github.event_name == 'release' }} - uses: docker/login-action@v4 + - name: Login to GCR + uses: docker/login-action@v3 with: - username: ${{ secrets.DOCKERHUB_USERNAME }} - password: ${{ secrets.DOCKERHUB_PASSWORD }} - - - name: Push snapshot image to Docker Hub - if: ${{ github.event_name == 'push' && github.ref == 'refs/heads/develop' }} - run: ./godelw docker push --tags=snapshot + registry: gcr.io + username: _json_key + password: ${{ secrets.GCR_JSON_KEY }} - - name: Push release image to Docker Hub - if: ${{ github.event_name == 'release' }} + - name: Push release image to gcr run: ./godelw docker push --tags=latest,version - - name: Publish release assets - if: ${{ github.event_name == 'release' }} - run: ./godelw publish github --add-v-prefix --api-url=${GITHUB_API_URL} --user=palantir --repository=policy-bot --token=${{ secrets.GITHUB_TOKEN }} - ci-all: runs-on: ubuntu-latest if: ${{ !cancelled() }} diff --git a/.github/workflows/codeql-analysis.yml b/.github/workflows/codeql-analysis.yml deleted file mode 100644 index b70e5eb9..00000000 --- a/.github/workflows/codeql-analysis.yml +++ /dev/null @@ -1,49 +0,0 @@ -name: "CodeQL" - -on: - push: - branches: [ develop ] - pull_request: - branches: [ develop ] - schedule: - - cron: '25 12 * * 1' - -jobs: - analyze: - permissions: - security-events: write - - name: Analyze - runs-on: ubuntu-latest - - strategy: - fail-fast: false - matrix: - language: [ 'go', 'javascript' ] - - steps: - - name: Checkout repository - uses: actions/checkout@v6 - - # Initializes the CodeQL tools for scanning. - - name: Initialize CodeQL - uses: github/codeql-action/init@v4 - with: - languages: ${{ matrix.language }} - - - name: Set Go version - id: go_version - run: | - GO_VERSION=$(sed -E -n '/^go / s/^go ([0-9]+\.[0-9]+)(\.[0-9]+)?$/\1/p' < go.mod) - echo "version=${GO_VERSION}" >> $GITHUB_OUTPUT - - - name: Set up Go - uses: actions/setup-go@v6 - with: - go-version: ${{ steps.go_version.outputs.version }} - - - name: Autobuild for CodeQL - uses: github/codeql-action/autobuild@v4 - - - name: Perform CodeQL Analysis - uses: github/codeql-action/analyze@v4 diff --git a/.gitignore b/.gitignore index 4b990b85..fea17744 100644 --- a/.gitignore +++ b/.gitignore @@ -15,3 +15,6 @@ gotest.out # default config location config/policy-bot.yml +.opencode + +.claude/settings.local.json diff --git a/README.md b/README.md index 3e059b82..f41234b9 100644 --- a/README.md +++ b/README.md @@ -657,6 +657,12 @@ requires: organizations: ["org1", "org2"] teams: ["org1/team1", "org2/team2"] + # If true, the codeowners of the changed files (as defined in the + # repository's CODEOWNERS file) are considered valid approvers. The + # CODEOWNERS file is read from .github/CODEOWNERS, CODEOWNERS, or + # docs/CODEOWNERS in the repository. + codeowners: true + # A user must have at least the minimum permission in this list for their # approval to count for this rule. Valid permissions are "admin", "maintain", # "write", "triage", and "read". diff --git a/config/policy-examples/codeowners-approval.yml b/config/policy-examples/codeowners-approval.yml new file mode 100644 index 00000000..24b39dbc --- /dev/null +++ b/config/policy-examples/codeowners-approval.yml @@ -0,0 +1,13 @@ +# This policy requires at least one approval from a codeowner of the +# changed files. Codeowners are determined from the repository's +# CODEOWNERS file (.github/CODEOWNERS, CODEOWNERS, or docs/CODEOWNERS). + +policy: + approval: + - a codeowner has approved + +approval_rules: +- name: a codeowner has approved + requires: + count: 1 + codeowners: true diff --git a/go.mod b/go.mod index 1c6c4e93..6918d864 100644 --- a/go.mod +++ b/go.mod @@ -12,6 +12,7 @@ require ( github.com/google/go-github/v82 v82.0.0 github.com/google/go-querystring v1.2.0 github.com/gregjones/httpcache v0.0.0-20190611155906-901d90724c79 + github.com/hairyhenderson/go-codeowners v0.7.0 github.com/hashicorp/golang-lru v1.0.2 github.com/palantir/go-baseapp v0.6.0 github.com/palantir/go-githubapp v0.41.0 diff --git a/go.sum b/go.sum index f9f9ed5d..433e0223 100644 --- a/go.sum +++ b/go.sum @@ -43,6 +43,8 @@ github.com/google/go-querystring v1.2.0 h1:yhqkPbu2/OH+V9BfpCVPZkNmUXhb2gBxJArfh github.com/google/go-querystring v1.2.0/go.mod h1:8IFJqpSRITyJ8QhQ13bmbeMBDfmeEJZD5A0egEOmkqU= github.com/gregjones/httpcache v0.0.0-20190611155906-901d90724c79 h1:+ngKgrYPPJrOjhax5N+uePQ0Fh1Z7PheYoUI/0nzkPA= github.com/gregjones/httpcache v0.0.0-20190611155906-901d90724c79/go.mod h1:FecbI9+v66THATjSRHfNgh1IVFe/9kFxbXtjV0ctIMA= +github.com/hairyhenderson/go-codeowners v0.7.0 h1:s0W4wF8bdsBEjTWzwzSlsatSthWtTAF2xLgo4a4RwAo= +github.com/hairyhenderson/go-codeowners v0.7.0/go.mod h1:wUlNgQ3QjqC4z8DnM5nnCYVq/icpqXJyJOukKx5U8/Q= github.com/hashicorp/golang-lru v1.0.2 h1:dV3g9Z/unq5DpblPpw+Oqcv4dU/1omnb4Ok8iPY6p1c= github.com/hashicorp/golang-lru v1.0.2/go.mod h1:iADmTwqILo4mZ8BN3D2Q6+9jd8WM5uGBxy+E8yxSoD4= github.com/inconshreveable/mousetrap v1.1.0 h1:wN+x4NVGpMsO7ErUn/mUI3vEoE6Jt13X2s0bqwp9tc8= diff --git a/godel/config/dist-plugin.yml b/godel/config/dist-plugin.yml index c2282bb4..fbefdeb5 100644 --- a/godel/config/dist-plugin.yml +++ b/godel/config/dist-plugin.yml @@ -7,7 +7,7 @@ products: build: output-dir: build main-pkg: . - version-var: github.com/palantir/policy-bot/version.version + version-var: develop.version environment: CGO_ENABLED: "0" os-archs: @@ -41,8 +41,6 @@ products: cp -R docker/* ${CONTEXT_DIR} cp -R config ${CONTEXT_DIR} tag-templates: - version: "{{Repository}}palantirtechnologies/policy-bot:{{Version}}" - latest: "{{Repository}}palantirtechnologies/policy-bot:latest" - snapshot: "{{Repository}}palantirtechnologies/policy-bot:snapshot" + version: "{{Repository}}us-central1-docker.pkg.dev/gcp-motion-mgmt/external/policy-bot:{{Version}}" publish: - group-id: com.palantir.policy-bot + group-id: com.palantir.policy-bo22838 diff --git a/policy/approval/approve.go b/policy/approval/approve.go index 69e6212a..9922b027 100644 --- a/policy/approval/approve.go +++ b/policy/approval/approve.go @@ -19,6 +19,7 @@ import ( "fmt" "sort" "strings" + "sync" "time" "github.com/palantir/policy-bot/policy/common" @@ -151,6 +152,7 @@ func (r *Rule) getReviewRequestRule() *common.ReviewRequestRule { Teams: r.Requires.Actors.Teams, Organizations: r.Requires.Actors.Organizations, Permissions: r.Requires.Actors.GetPermissions(), + Codeowners: r.Requires.Actors.Codeowners, RequiredCount: r.Requires.Count, RequestedCount: requestedCount, Mode: mode, @@ -158,7 +160,7 @@ func (r *Rule) getReviewRequestRule() *common.ReviewRequestRule { } func (r *Rule) IsApproved(ctx context.Context, prctx pull.Context, candidates []*common.Candidate) (bool, common.RequiresResult, error) { - approvedByActors, approvers, err := r.isApprovedByActors(ctx, prctx, candidates) + approvedByActors, approvers, ownershipGroups, err := r.isApprovedByActors(ctx, prctx, candidates) if err != nil { return false, common.RequiresResult{}, err } @@ -169,20 +171,21 @@ func (r *Rule) IsApproved(ctx context.Context, prctx pull.Context, candidates [] } result := common.RequiresResult{ - Count: r.Requires.Count, - Actors: r.Requires.Actors, - Approvers: approvers, - Conditions: conditions, + Count: r.Requires.Count, + Actors: r.Requires.Actors, + Approvers: approvers, + Conditions: conditions, + OwnershipGroups: ownershipGroups, } return approvedByActors && approvedByConditions, result, nil } -func (r *Rule) isApprovedByActors(ctx context.Context, prctx pull.Context, candidates []*common.Candidate) (bool, []*common.Candidate, error) { +func (r *Rule) isApprovedByActors(ctx context.Context, prctx pull.Context, candidates []*common.Candidate) (bool, []*common.Candidate, []common.OwnershipGroupResult, error) { log := zerolog.Ctx(ctx) - if r.Requires.Count <= 0 { + if r.Requires.Count <= 0 && !r.Requires.Actors.Codeowners { log.Debug().Msg("rule requires no approvals") - return true, nil, nil + return true, nil, nil, nil } log.Debug().Msgf("found %d candidates for approval", len(candidates)) @@ -202,7 +205,7 @@ func (r *Rule) isApprovedByActors(ctx context.Context, prctx pull.Context, candi if !r.Options.IsAllowContributor() && !r.Options.IsAllowNonAuthorContributor() { commits, err := r.filteredCommits(ctx, prctx) if err != nil { - return false, nil, err + return false, nil, nil, err } for _, c := range commits { @@ -214,20 +217,25 @@ func (r *Rule) isApprovedByActors(ctx context.Context, prctx pull.Context, candi } } + // If codeowners is enabled, delegate to codeowner group logic + if r.Requires.Actors.Codeowners { + return r.isApprovedByCodeownerGroups(ctx, prctx, candidates, banned) + } + // filter real approvers using banned status and required membership var approvers []*common.Candidate for _, c := range candidates { - if banned[c.User] { - log.Debug().Str("user", c.User).Msg("rejecting approval by banned user") + if banned[c.User()] { + log.Debug().Str("user", c.User()).Msg("rejecting approval by banned user") continue } - isApprover, err := r.Requires.Actors.IsActor(ctx, prctx, c.User) + isApprover, err := r.Requires.Actors.IsActor(ctx, prctx, c.User()) if err != nil { - return false, nil, errors.Wrap(err, "failed to check candidate status") + return false, nil, nil, errors.Wrap(err, "failed to check candidate status") } if !isApprover { - log.Debug().Str("user", c.User).Msg("ignoring approval by non-required user") + log.Debug().Str("user", c.User()).Msg("ignoring approval by non-required user") continue } @@ -235,7 +243,7 @@ func (r *Rule) isApprovedByActors(ctx context.Context, prctx pull.Context, candi } log.Debug().Msgf("found %d/%d required approvers", len(approvers), r.Requires.Count) - return len(approvers) >= r.Requires.Count, approvers, nil + return len(approvers) >= r.Requires.Count, approvers, nil, nil } func (r *Rule) isApprovedByConditions(ctx context.Context, prctx pull.Context) (bool, []*common.PredicateResult, error) { @@ -268,7 +276,7 @@ func (r *Rule) isApprovedByConditions(ctx context.Context, prctx pull.Context) ( // FilteredCandidates returns the potential approval candidates and any // candidates that should be dimissed due to rule options. func (r *Rule) FilteredCandidates(ctx context.Context, prctx pull.Context) ([]*common.Candidate, []*common.Dismissal, error) { - if r.Requires.Count <= 0 { + if r.Requires.Count <= 0 && !r.Requires.Actors.Codeowners { return nil, nil, nil } @@ -302,7 +310,7 @@ func (r *Rule) FilteredCandidates(ctx context.Context, prctx pull.Context) ([]*c return candidates, dismissals, nil } -func (r *Rule) filterEditedCandidates(ctx context.Context, prctx pull.Context, candidates []*common.Candidate) ([]*common.Candidate, []*common.Dismissal, error) { +func (r *Rule) filterEditedCandidates(ctx context.Context, _ pull.Context, candidates []*common.Candidate) ([]*common.Candidate, []*common.Dismissal, error) { log := zerolog.Ctx(ctx) if !r.Options.IsIgnoreEditedComments() { @@ -408,23 +416,27 @@ func (r *Rule) filteredCommits(ctx context.Context, prctx pull.Context) ([]*pull func statusDescription(approved bool, result common.RequiresResult, candidates []*common.Candidate) string { hasActors := result.Count > 0 hasConditions := len(result.Conditions) > 0 + hasOwnershipGroups := len(result.OwnershipGroups) > 0 if approved { - if !hasActors && !hasConditions { + if !hasActors && !hasConditions && !hasOwnershipGroups { return "No approval required" } var desc strings.Builder - desc.WriteString("Approved by ") - - for i, c := range result.Approvers { - if i > 0 { - desc.WriteString(", ") + if hasOwnershipGroups { + desc.WriteString(ownershipGroupsStatusDescription(result.OwnershipGroups)) + } else { + desc.WriteString("Approved by ") + for i, c := range result.Approvers { + if i > 0 { + desc.WriteString(", ") + } + desc.WriteString(c.User()) } - desc.WriteString(c.User) } if hasConditions { - if hasActors { + if hasActors || hasOwnershipGroups { desc.WriteString(" and ") } desc.WriteString("required conditions") @@ -434,11 +446,13 @@ func statusDescription(approved bool, result common.RequiresResult, candidates [ } var desc strings.Builder - if hasActors { + if hasOwnershipGroups { + desc.WriteString(ownershipGroupsStatusDescription(result.OwnershipGroups)) + } else if hasActors { fmt.Fprintf(&desc, "%d/%d required approvals", len(result.Approvers), result.Count) } if hasConditions { - if hasActors { + if hasActors || hasOwnershipGroups { desc.WriteString(" and ") } @@ -450,12 +464,37 @@ func statusDescription(approved bool, result common.RequiresResult, candidates [ } fmt.Fprintf(&desc, "%d/%d required conditions", successful, len(result.Conditions)) } - if disqualified := len(candidates) - len(result.Approvers); hasActors && disqualified > 0 { + if disqualified := len(candidates) - len(result.Approvers); hasActors && !hasOwnershipGroups && disqualified > 0 { fmt.Fprintf(&desc, ". Ignored %s from disqualified users", numberOfApprovals(disqualified)) } return desc.String() } +func ownershipGroupsStatusDescription(groups []common.OwnershipGroupResult) string { + var satisfied int + approverSet := make(map[string]struct{}) + + for _, g := range groups { + if g.Satisfied { + satisfied++ + for _, a := range g.Approvers { + approverSet[a] = struct{}{} + } + } + } + + if satisfied < len(groups) { + return fmt.Sprintf("%d/%d ownership groups covered", satisfied, len(groups)) + } + + approvers := make([]string, 0, len(approverSet)) + for a := range approverSet { + approvers = append(approvers, a) + } + sort.Strings(approvers) + return fmt.Sprintf("Approved by %s covering %d ownership groups", strings.Join(approvers, ", "), len(groups)) +} + func isUpdateMerge(commits []*pull.Commit, c *pull.Commit) bool { // must be a simple merge commit (exactly 2 parents) if len(c.Parents) != 2 { @@ -520,3 +559,139 @@ func sortCommits(commits []*pull.Commit, head string) []*pull.Commit { } return ordered } + +// isApprovedByCodeownerGroups checks if all ownership groups have at least one +// approved candidate. Returns true if all groups are satisfied, the list of +// approvers, and the results for each ownership group. +func (r *Rule) isApprovedByCodeownerGroups( + ctx context.Context, + prctx pull.Context, + candidates []*common.Candidate, + banned map[string]bool, +) (bool, []*common.Candidate, []common.OwnershipGroupResult, error) { + log := zerolog.Ctx(ctx) + + co, err := prctx.Codeowners() + if err != nil { + return false, nil, nil, errors.Wrap(err, "failed to get codeowners") + } + if co == nil { + // No CODEOWNERS file - no codeowner requirements + log.Debug().Msg("no CODEOWNERS file found, skipping codeowner group check") + return true, nil, nil, nil + } + + groups := co.OwnershipGroups() + if len(groups) == 0 { + log.Debug().Msg("no ownership groups found, skipping codeowner group check") + return true, nil, nil, nil + } + + log.Debug().Msgf("found %d ownership groups to satisfy", len(groups)) + + // Pre-filter candidates to exclude banned users + validCandidates := make([]*common.Candidate, 0, len(candidates)) + for _, c := range candidates { + if !banned[c.User()] { + validCandidates = append(validCandidates, c) + } + } + + // Collect all unique (team, user) pairs that need membership checks + type memberCheck struct { + team string + user string + } + checksNeeded := make(map[memberCheck]struct{}) + for _, group := range groups { + for _, owner := range group.Owners { + ownerType, name := pull.ParseCodeowner(owner) + if ownerType == "team" { + for _, c := range validCandidates { + checksNeeded[memberCheck{team: name, user: c.User()}] = struct{}{} + } + } + } + } + + // Run all team membership checks in parallel + membershipResults := make(map[memberCheck]bool) + var mu sync.Mutex + var wg sync.WaitGroup + errChan := make(chan error, len(checksNeeded)) + + for check := range checksNeeded { + wg.Go(func() { + isMember, err := prctx.IsTeamMember(check.team, check.user) + if err != nil { + errChan <- errors.Wrapf(err, "failed to check team membership for %s in %s", check.user, check.team) + return + } + mu.Lock() + membershipResults[check] = isMember + mu.Unlock() + }) + } + + wg.Wait() + close(errChan) + + // Check for any errors + if err := <-errChan; err != nil { + return false, nil, nil, err + } + + // Process results using the cached membership data + results := make([]common.OwnershipGroupResult, len(groups)) + var approvers []*common.Candidate + approverSet := make(map[string]struct{}) + + for i, group := range groups { + results[i] = common.OwnershipGroupResult{ + Key: group.Key, + Owners: group.Owners, + Files: group.Files, + } + + for _, c := range validCandidates { + isMember := false + for _, owner := range group.Owners { + ownerType, name := pull.ParseCodeowner(owner) + switch ownerType { + case "user": + if strings.EqualFold(c.User(), name) { + isMember = true + } + case "team": + if membershipResults[memberCheck{team: name, user: c.User()}] { + isMember = true + } + } + if isMember { + break + } + } + + if isMember { + results[i].Satisfied = true + results[i].Approvers = append(results[i].Approvers, c.User()) + if _, exists := approverSet[c.User()]; !exists { + approvers = append(approvers, c) + approverSet[c.User()] = struct{}{} + } + } + } + } + + // Count satisfied groups + satisfiedCount := 0 + for _, result := range results { + if result.Satisfied { + satisfiedCount++ + } + } + + log.Debug().Msgf("%d/%d ownership groups satisfied", satisfiedCount, len(results)) + + return satisfiedCount == len(results), approvers, results, nil +} diff --git a/policy/approval/approve_test.go b/policy/approval/approve_test.go index e4b4fd25..2c927021 100644 --- a/policy/approval/approve_test.go +++ b/policy/approval/approve_test.go @@ -31,6 +31,10 @@ import ( "github.com/stretchr/testify/require" ) +func ptr[T any](v T) *T { + return &v +} + var defaultOptions = Options{ Methods: DefaultMethods(), } @@ -47,49 +51,49 @@ func TestIsApproved(t *testing.T) { Body: "/no-platform", CreatedAt: now.Add(10 * time.Second), LastEditedAt: now.Add(20 * time.Second), - Author: "body-editor", + Author: pull.NewAuthor("body-editor"), }, CommentsValue: []*pull.Comment{ { CreatedAt: now.Add(10 * time.Second), LastEditedAt: time.Time{}, - Author: "other-user", + Author: pull.NewAuthor("other-user"), Body: "Why did you do this?", }, { CreatedAt: now.Add(20 * time.Second), LastEditedAt: time.Time{}, - Author: "comment-approver", + Author: pull.NewAuthor("comment-approver"), Body: "LGTM :+1: :shipit:", }, { CreatedAt: now.Add(30 * time.Second), LastEditedAt: time.Time{}, - Author: "disapprover", + Author: pull.NewAuthor("disapprover"), Body: "I don't like things! :-1:", }, { CreatedAt: now.Add(40 * time.Second), LastEditedAt: time.Time{}, - Author: "mhaypenny", + Author: pull.NewAuthor("mhaypenny"), Body: ":+1: my stuff is cool", }, { CreatedAt: now.Add(50 * time.Second), LastEditedAt: time.Time{}, - Author: "contributor-author", + Author: pull.NewAuthor("contributor-author"), Body: ":+1: I added to this PR", }, { CreatedAt: now.Add(60 * time.Second), LastEditedAt: time.Time{}, - Author: "contributor-committer", + Author: pull.NewAuthor("contributor-committer"), Body: ":+1: I also added to this PR", }, { CreatedAt: now.Add(70 * time.Second), LastEditedAt: now.Add(71 * time.Second), - Author: "comment-editor", + Author: pull.NewAuthor("comment-editor"), Body: "LGTM :+1: :shipit:", }, }, @@ -97,21 +101,21 @@ func TestIsApproved(t *testing.T) { { CreatedAt: now.Add(70 * time.Second), LastEditedAt: time.Time{}, - Author: "disapprover", + Author: pull.NewAuthor("disapprover"), State: pull.ReviewChangesRequested, Body: "I _really_ don't like things!", }, { CreatedAt: now.Add(80 * time.Second), LastEditedAt: time.Time{}, - Author: "review-approver", + Author: pull.NewAuthor("review-approver"), State: pull.ReviewApproved, Body: "I LIKE THIS", }, { CreatedAt: now.Add(90 * time.Second), LastEditedAt: now.Add(91 * time.Second), - Author: "review-comment-editor", + Author: pull.NewAuthor("review-comment-editor"), State: pull.ReviewApproved, Body: "I LIKE THIS", }, @@ -513,7 +517,7 @@ func TestIsApproved(t *testing.T) { }) prctx.CommentsValue = append(prctx.CommentsValue, &pull.Comment{ CreatedAt: now.Add(100 * time.Second), - Author: "merge-committer", + Author: pull.NewAuthor("merge-committer"), Body: ":+1:", }) @@ -1008,3 +1012,371 @@ func TestSortCommits(t *testing.T) { }) } } + +func TestCodeownerGroupApproval(t *testing.T) { + logger := zerolog.New(os.Stdout) + ctx := logger.WithContext(context.Background()) + + now := time.Now() + + t.Run("allGroupsApprovedBySingleReviewer", func(t *testing.T) { + // Single reviewer is member of both team-a and team-c, satisfying both groups + prctx := &pulltest.Context{ + AuthorValue: "author", + TeamMemberships: map[string][]string{ + "reviewer1": {"org/team-a", "org/team-c"}, + }, + ReviewsValue: []*pull.Review{ + {Author: pull.NewAuthor("reviewer1"), State: pull.ReviewApproved, CreatedAt: now}, + }, + CommitsValue: []*pull.Commit{ + {SHA: "abc123", Author: "author"}, + }, + HeadSHAValue: "abc123", + CodeownersValue: &pull.CodeownersResult{ + Owners: map[string][]string{ + "a/file.go": {"@org/team-a"}, + "c/file.go": {"@org/team-c"}, + }, + }, + } + + r := &Rule{ + Options: Options{ + Methods: DefaultMethods(), + }, + Requires: Requires{ + Actors: common.Actors{Codeowners: true}, + }, + } + + candidates, _, err := r.FilteredCandidates(ctx, prctx) + require.NoError(t, err) + + approved, result, err := r.IsApproved(ctx, prctx, candidates) + require.NoError(t, err) + assert.True(t, approved) + assert.Len(t, result.OwnershipGroups, 2) + for _, g := range result.OwnershipGroups { + assert.True(t, g.Satisfied, "group %s should be satisfied", g.Key) + } + }) + + t.Run("partialGroupApproval", func(t *testing.T) { + // reviewer1 is only in team-a, so team-b group is not satisfied + prctx := &pulltest.Context{ + AuthorValue: "author", + TeamMemberships: map[string][]string{ + "reviewer1": {"org/team-a"}, + }, + ReviewsValue: []*pull.Review{ + {Author: pull.NewAuthor("reviewer1"), State: pull.ReviewApproved, CreatedAt: now}, + }, + CommitsValue: []*pull.Commit{ + {SHA: "abc123", Author: "author"}, + }, + HeadSHAValue: "abc123", + CodeownersValue: &pull.CodeownersResult{ + Owners: map[string][]string{ + "a/file.go": {"@org/team-a"}, + "b/file.go": {"@org/team-b"}, + }, + }, + } + + r := &Rule{ + Options: Options{ + Methods: DefaultMethods(), + }, + Requires: Requires{ + Actors: common.Actors{Codeowners: true}, + }, + } + + candidates, _, err := r.FilteredCandidates(ctx, prctx) + require.NoError(t, err) + + approved, result, err := r.IsApproved(ctx, prctx, candidates) + require.NoError(t, err) + assert.False(t, approved) + assert.Len(t, result.OwnershipGroups, 2) + + // Check which group is satisfied + groupByKey := make(map[string]common.OwnershipGroupResult) + for _, g := range result.OwnershipGroups { + groupByKey[g.Key] = g + } + assert.True(t, groupByKey["@org/team-a"].Satisfied) + assert.False(t, groupByKey["@org/team-b"].Satisfied) + }) + + t.Run("multipleReviewersSatisfyAllGroups", func(t *testing.T) { + // Two reviewers, each covering different groups + prctx := &pulltest.Context{ + AuthorValue: "author", + TeamMemberships: map[string][]string{ + "reviewer1": {"org/team-a"}, + "reviewer2": {"org/team-b", "org/team-c"}, + }, + ReviewsValue: []*pull.Review{ + {Author: pull.NewAuthor("reviewer1"), State: pull.ReviewApproved, CreatedAt: now}, + {Author: pull.NewAuthor("reviewer2"), State: pull.ReviewApproved, CreatedAt: now.Add(time.Second)}, + }, + CommitsValue: []*pull.Commit{ + {SHA: "abc123", Author: "author"}, + }, + HeadSHAValue: "abc123", + CodeownersValue: &pull.CodeownersResult{ + Owners: map[string][]string{ + "a/file.go": {"@org/team-a"}, + "b/file.go": {"@org/team-b"}, + "c/file.go": {"@org/team-c"}, + }, + }, + } + + r := &Rule{ + Options: Options{ + Methods: DefaultMethods(), + }, + Requires: Requires{ + Actors: common.Actors{Codeowners: true}, + }, + } + + candidates, _, err := r.FilteredCandidates(ctx, prctx) + require.NoError(t, err) + + approved, result, err := r.IsApproved(ctx, prctx, candidates) + require.NoError(t, err) + assert.True(t, approved) + assert.Len(t, result.OwnershipGroups, 3) + for _, g := range result.OwnershipGroups { + assert.True(t, g.Satisfied, "group %s should be satisfied", g.Key) + } + }) + + t.Run("userCodeownerApproval", func(t *testing.T) { + // Direct user codeowner (not team) + prctx := &pulltest.Context{ + AuthorValue: "author", + ReviewsValue: []*pull.Review{ + {Author: pull.NewAuthor("johndoe"), State: pull.ReviewApproved, CreatedAt: now}, + }, + CommitsValue: []*pull.Commit{ + {SHA: "abc123", Author: "author"}, + }, + HeadSHAValue: "abc123", + CodeownersValue: &pull.CodeownersResult{ + Owners: map[string][]string{ + "file.go": {"@johndoe"}, + }, + }, + } + + r := &Rule{ + Options: Options{ + Methods: DefaultMethods(), + }, + Requires: Requires{ + Actors: common.Actors{Codeowners: true}, + }, + } + + candidates, _, err := r.FilteredCandidates(ctx, prctx) + require.NoError(t, err) + + approved, result, err := r.IsApproved(ctx, prctx, candidates) + require.NoError(t, err) + assert.True(t, approved) + assert.Len(t, result.OwnershipGroups, 1) + assert.True(t, result.OwnershipGroups[0].Satisfied) + assert.Contains(t, result.OwnershipGroups[0].Approvers, "johndoe") + }) + + t.Run("noCodeownersFile", func(t *testing.T) { + // No CODEOWNERS file - should pass (no codeowner requirements) + prctx := &pulltest.Context{ + AuthorValue: "author", + CodeownersValue: nil, // No CODEOWNERS file + CommitsValue: []*pull.Commit{ + {SHA: "abc123", Author: "author"}, + }, + HeadSHAValue: "abc123", + } + + r := &Rule{ + Options: Options{ + Methods: DefaultMethods(), + }, + Requires: Requires{ + Actors: common.Actors{Codeowners: true}, + }, + } + + candidates, _, err := r.FilteredCandidates(ctx, prctx) + require.NoError(t, err) + + approved, result, err := r.IsApproved(ctx, prctx, candidates) + require.NoError(t, err) + assert.True(t, approved) + assert.Empty(t, result.OwnershipGroups) + }) + + t.Run("authorCannotApproveOwnGroup", func(t *testing.T) { + // Author is a codeowner but shouldn't be able to approve (default behavior) + prctx := &pulltest.Context{ + AuthorValue: "johndoe", + TeamMemberships: map[string][]string{ + "johndoe": {"org/team-a"}, + }, + ReviewsValue: []*pull.Review{ + {Author: pull.NewAuthor("johndoe"), State: pull.ReviewApproved, CreatedAt: now}, + }, + CommitsValue: []*pull.Commit{ + {SHA: "abc123", Author: "johndoe"}, + }, + HeadSHAValue: "abc123", + CodeownersValue: &pull.CodeownersResult{ + Owners: map[string][]string{ + "a/file.go": {"@org/team-a"}, + }, + }, + } + + r := &Rule{ + Options: Options{ + Methods: DefaultMethods(), + AllowAuthor: new(false), + }, + Requires: Requires{ + Actors: common.Actors{Codeowners: true}, + }, + } + + candidates, _, err := r.FilteredCandidates(ctx, prctx) + require.NoError(t, err) + + approved, result, err := r.IsApproved(ctx, prctx, candidates) + require.NoError(t, err) + assert.False(t, approved) + assert.Len(t, result.OwnershipGroups, 1) + assert.False(t, result.OwnershipGroups[0].Satisfied) + }) + + t.Run("filesWithSameOwnersGroupedTogether", func(t *testing.T) { + // Multiple files with the same owner should be in one group + prctx := &pulltest.Context{ + AuthorValue: "author", + TeamMemberships: map[string][]string{ + "reviewer1": {"org/team-a"}, + }, + ReviewsValue: []*pull.Review{ + {Author: pull.NewAuthor("reviewer1"), State: pull.ReviewApproved, CreatedAt: now}, + }, + CommitsValue: []*pull.Commit{ + {SHA: "abc123", Author: "author"}, + }, + HeadSHAValue: "abc123", + CodeownersValue: &pull.CodeownersResult{ + Owners: map[string][]string{ + "a/file1.go": {"@org/team-a"}, + "a/file2.go": {"@org/team-a"}, + "a/file3.go": {"@org/team-a"}, + }, + }, + } + + r := &Rule{ + Options: Options{ + Methods: DefaultMethods(), + }, + Requires: Requires{ + Actors: common.Actors{Codeowners: true}, + }, + } + + candidates, _, err := r.FilteredCandidates(ctx, prctx) + require.NoError(t, err) + + approved, result, err := r.IsApproved(ctx, prctx, candidates) + require.NoError(t, err) + assert.True(t, approved) + assert.Len(t, result.OwnershipGroups, 1) // All files in one group + assert.Len(t, result.OwnershipGroups[0].Files, 3) + }) + + t.Run("statusDescriptionShowsOwnershipGroups", func(t *testing.T) { + // Test status description for pending with ownership groups + prctx := &pulltest.Context{ + AuthorValue: "author", + TeamMemberships: map[string][]string{ + "reviewer1": {"org/team-a"}, + }, + ReviewsValue: []*pull.Review{ + {Author: pull.NewAuthor("reviewer1"), State: pull.ReviewApproved, CreatedAt: now}, + }, + CommitsValue: []*pull.Commit{ + {SHA: "abc123", Author: "author"}, + }, + HeadSHAValue: "abc123", + CodeownersValue: &pull.CodeownersResult{ + Owners: map[string][]string{ + "a/file.go": {"@org/team-a"}, + "b/file.go": {"@org/team-b"}, + "c/file.go": {"@org/team-c"}, + }, + }, + } + + r := &Rule{ + Options: Options{ + Methods: DefaultMethods(), + }, + Requires: Requires{ + Actors: common.Actors{Codeowners: true}, + }, + } + + result := r.Evaluate(ctx, prctx) + assert.Equal(t, common.StatusPending, result.Status) + assert.Contains(t, result.StatusDescription, "1/3 ownership groups covered") + }) + + t.Run("statusDescriptionShowsApprovedWithGroups", func(t *testing.T) { + // Test status description for approved with ownership groups + prctx := &pulltest.Context{ + AuthorValue: "author", + TeamMemberships: map[string][]string{ + "reviewer1": {"org/team-a", "org/team-b"}, + }, + ReviewsValue: []*pull.Review{ + {Author: pull.NewAuthor("reviewer1"), State: pull.ReviewApproved, CreatedAt: now}, + }, + CommitsValue: []*pull.Commit{ + {SHA: "abc123", Author: "author"}, + }, + HeadSHAValue: "abc123", + CodeownersValue: &pull.CodeownersResult{ + Owners: map[string][]string{ + "a/file.go": {"@org/team-a"}, + "b/file.go": {"@org/team-b"}, + }, + }, + } + + r := &Rule{ + Options: Options{ + Methods: DefaultMethods(), + }, + Requires: Requires{ + Actors: common.Actors{Codeowners: true}, + }, + } + + result := r.Evaluate(ctx, prctx) + assert.Equal(t, common.StatusApproved, result.Status) + assert.Contains(t, result.StatusDescription, "Approved by reviewer1") + assert.Contains(t, result.StatusDescription, "2 ownership groups") + }) +} diff --git a/policy/common/actor.go b/policy/common/actor.go index 2bdc5404..ad327d6e 100644 --- a/policy/common/actor.go +++ b/policy/common/actor.go @@ -38,12 +38,17 @@ type Actors struct { // A list of GitHub collaborator permissions that are allowed. Values may // be any of "admin", "maintain", "write", "triage", and "read". Permissions []pull.Permission `yaml:"permissions,omitempty" json:"permissions"` + + // Codeowners specifies that codeowners of changed files are allowed actors. + // If true, users who are listed as codeowners of any changed file in the + // CODEOWNERS file can satisfy this actor requirement. + Codeowners bool `yaml:"codeowners,omitempty" json:"codeowners"` } // IsZero returns true if no conditions for actors are defined. func (a *Actors) IsZero() bool { return a == nil || (len(a.Users) == 0 && len(a.Teams) == 0 && len(a.Organizations) == 0 && - len(a.Permissions) == 0 && !a.Admins && !a.WriteCollaborators) + len(a.Permissions) == 0 && !a.Admins && !a.WriteCollaborators && !a.Codeowners) } // GetPermissions returns unique permissions ordered from most to least @@ -115,5 +120,45 @@ func (a *Actors) IsActor(ctx context.Context, prctx pull.Context, user string) ( } } + if a.Codeowners { + isOwner, err := a.isCodeowner(prctx, user) + if err != nil { + return false, errors.Wrap(err, "failed to check codeowner status") + } + if isOwner { + return true, nil + } + } + + return false, nil +} + +// isCodeowner checks if the user is a codeowner of any changed file. +func (a *Actors) isCodeowner(prctx pull.Context, user string) (bool, error) { + co, err := prctx.Codeowners() + if err != nil { + return false, err + } + if co == nil { + return false, nil + } + + for _, owner := range co.AllOwners() { + ownerType, name := pull.ParseCodeowner(owner) + switch ownerType { + case "user": + if user == name { + return true, nil + } + case "team": + isMember, err := prctx.IsTeamMember(name, user) + if err != nil { + return false, errors.Wrapf(err, "failed to check team membership for %s", name) + } + if isMember { + return true, nil + } + } + } return false, nil } diff --git a/policy/common/actor_test.go b/policy/common/actor_test.go index 82ccfff1..f6a61da2 100644 --- a/policy/common/actor_test.go +++ b/policy/common/actor_test.go @@ -115,6 +115,99 @@ func TestIsActor(t *testing.T) { }) } +func TestIsActorCodeowners(t *testing.T) { + ctx := context.Background() + + t.Run("user codeowner", func(t *testing.T) { + prctx := &pulltest.Context{ + CodeownersValue: &pull.CodeownersResult{ + Owners: map[string][]string{ + "src/main.go": {"@mhaypenny", "@other-user"}, + }, + }, + } + + a := &Actors{Codeowners: true} + + isActor, err := a.IsActor(ctx, prctx, "mhaypenny") + require.NoError(t, err) + assert.True(t, isActor, "codeowner should be an actor") + + isActor, err = a.IsActor(ctx, prctx, "other-user") + require.NoError(t, err) + assert.True(t, isActor, "codeowner should be an actor") + + isActor, err = a.IsActor(ctx, prctx, "random-user") + require.NoError(t, err) + assert.False(t, isActor, "non-codeowner should not be an actor") + }) + + t.Run("team codeowner", func(t *testing.T) { + prctx := &pulltest.Context{ + TeamMemberships: map[string][]string{ + "team-member": {"myorg/my-team"}, + }, + CodeownersValue: &pull.CodeownersResult{ + Owners: map[string][]string{ + "src/main.go": {"@myorg/my-team"}, + }, + }, + } + + a := &Actors{Codeowners: true} + + isActor, err := a.IsActor(ctx, prctx, "team-member") + require.NoError(t, err) + assert.True(t, isActor, "team member should be an actor") + + isActor, err = a.IsActor(ctx, prctx, "non-member") + require.NoError(t, err) + assert.False(t, isActor, "non-team-member should not be an actor") + }) + + t.Run("no codeowners file", func(t *testing.T) { + prctx := &pulltest.Context{ + CodeownersValue: nil, + } + + a := &Actors{Codeowners: true} + + isActor, err := a.IsActor(ctx, prctx, "anyone") + require.NoError(t, err) + assert.False(t, isActor, "should not be an actor when no CODEOWNERS file") + }) + + t.Run("empty owners for file", func(t *testing.T) { + prctx := &pulltest.Context{ + CodeownersValue: &pull.CodeownersResult{ + Owners: map[string][]string{}, + }, + } + + a := &Actors{Codeowners: true} + + isActor, err := a.IsActor(ctx, prctx, "anyone") + require.NoError(t, err) + assert.False(t, isActor, "should not be an actor when no owners defined") + }) + + t.Run("codeowners disabled", func(t *testing.T) { + prctx := &pulltest.Context{ + CodeownersValue: &pull.CodeownersResult{ + Owners: map[string][]string{ + "src/main.go": {"@mhaypenny"}, + }, + }, + } + + a := &Actors{Codeowners: false} + + isActor, err := a.IsActor(ctx, prctx, "mhaypenny") + require.NoError(t, err) + assert.False(t, isActor, "should not be an actor when codeowners is disabled") + }) +} + func TestIsEmpty(t *testing.T) { a := &Actors{} assert.True(t, a.IsZero(), "Actors struct was not empty") @@ -128,6 +221,9 @@ func TestIsEmpty(t *testing.T) { a = &Actors{Organizations: []string{"org"}} assert.False(t, a.IsZero(), "Actors struct was empty") + a = &Actors{Codeowners: true} + assert.False(t, a.IsZero(), "Actors struct was empty") + a = nil assert.True(t, a.IsZero(), "nil struct was not empty") } diff --git a/policy/common/methods.go b/policy/common/methods.go index 59ea4294..1643aa5e 100644 --- a/policy/common/methods.go +++ b/policy/common/methods.go @@ -101,11 +101,19 @@ const ( type Candidate struct { Type CandidateType ReviewID string - User string + Author *pull.Author CreatedAt time.Time LastEditedAt time.Time } +// User returns the author's login for backward compatibility. +func (c *Candidate) User() string { + if c.Author == nil { + return "" + } + return c.Author.Login +} + type CandidatesByCreationTime []*Candidate func (cs CandidatesByCreationTime) Len() int { return len(cs) } @@ -131,7 +139,7 @@ func (m *Methods) Candidates(ctx context.Context, prctx pull.Context) ([]*Candid if m.CommentMatches(c.Body) { candidates = append(candidates, &Candidate{ Type: CommentCandidate, - User: c.Author, + Author: c.Author, CreatedAt: c.CreatedAt, LastEditedAt: c.LastEditedAt, }) @@ -146,7 +154,7 @@ func (m *Methods) Candidates(ctx context.Context, prctx pull.Context) ([]*Candid } if m.BodyMatches(prBody.Body) { candidates = append(candidates, &Candidate{ - User: prBody.Author, + Author: prBody.Author, CreatedAt: prBody.CreatedAt, LastEditedAt: prBody.LastEditedAt, }) @@ -166,7 +174,7 @@ func (m *Methods) Candidates(ctx context.Context, prctx pull.Context) ([]*Candid candidates = append(candidates, &Candidate{ Type: ReviewCandidate, ReviewID: r.ID, - User: r.Author, + Author: r.Author, CreatedAt: r.CreatedAt, LastEditedAt: r.LastEditedAt, }) @@ -175,7 +183,7 @@ func (m *Methods) Candidates(ctx context.Context, prctx pull.Context) ([]*Candid candidates = append(candidates, &Candidate{ Type: ReviewCandidate, ReviewID: r.ID, - User: r.Author, + Author: r.Author, CreatedAt: r.CreatedAt, LastEditedAt: r.LastEditedAt, }) @@ -190,9 +198,9 @@ func (m *Methods) Candidates(ctx context.Context, prctx pull.Context) ([]*Candid func deduplicateCandidates(all []*Candidate) []*Candidate { users := make(map[string]*Candidate) for _, c := range all { - last, ok := users[c.User] + last, ok := users[c.User()] if !ok || last.CreatedAt.Before(c.CreatedAt) { - users[c.User] = c + users[c.User()] = c } } diff --git a/policy/common/methods_test.go b/policy/common/methods_test.go index bcf177f9..3f0e3f1f 100644 --- a/policy/common/methods_test.go +++ b/policy/common/methods_test.go @@ -36,50 +36,50 @@ func TestCandidates(t *testing.T) { { CreatedAt: now.Add(0 * time.Minute), Body: "I like to comment!", - Author: "rrandom", + Author: pull.NewAuthor("rrandom"), }, { CreatedAt: now.Add(2 * time.Minute), Body: "Looks good to me :+1:", - Author: "mhaypenny", + Author: pull.NewAuthor("mhaypenny"), }, { CreatedAt: now.Add(4 * time.Minute), Body: ":lgtm:", - Author: "ttest", + Author: pull.NewAuthor("ttest"), }, { CreatedAt: now.Add(8 * time.Minute), Body: "I approve this, because it looks good to me.", - Author: "wstrawmoney", + Author: pull.NewAuthor("wstrawmoney"), }, }, ReviewsValue: []*pull.Review{ { CreatedAt: now.Add(1 * time.Minute), - Author: "rrandom", + Author: pull.NewAuthor("rrandom"), State: pull.ReviewCommented, }, { CreatedAt: now.Add(3 * time.Minute), - Author: "mhaypenny", + Author: pull.NewAuthor("mhaypenny"), State: pull.ReviewChangesRequested, Body: "pr needs work", }, { CreatedAt: now.Add(5 * time.Minute), - Author: "ttest", + Author: pull.NewAuthor("ttest"), State: pull.ReviewApproved, }, { CreatedAt: now.Add(7 * time.Minute), - Author: "santaclaus", + Author: pull.NewAuthor("santaclaus"), Body: "nice", State: pull.ReviewApproved, }, { CreatedAt: now.Add(9 * time.Minute), - Author: "dasherdancer", + Author: pull.NewAuthor("dasherdancer"), Body: "nIcE", State: pull.ReviewApproved, }, @@ -97,8 +97,8 @@ func TestCandidates(t *testing.T) { sort.Sort(CandidatesByCreationTime(cs)) require.Len(t, cs, 2, "incorrect number of candidates found") - assert.Equal(t, "mhaypenny", cs[0].User) - assert.Equal(t, "ttest", cs[1].User) + assert.Equal(t, "mhaypenny", cs[0].User()) + assert.Equal(t, "ttest", cs[1].User()) }) t.Run("commentPatterns", func(t *testing.T) { @@ -114,7 +114,7 @@ func TestCandidates(t *testing.T) { sort.Sort(CandidatesByCreationTime(cs)) require.Len(t, cs, 1, "incorrect number of candidates found") - assert.Equal(t, "mhaypenny", cs[0].User) + assert.Equal(t, "mhaypenny", cs[0].User()) }) t.Run("githubReviewCommentPatterns", func(t *testing.T) { @@ -133,8 +133,8 @@ func TestCandidates(t *testing.T) { sort.Sort(CandidatesByCreationTime(cs)) require.Len(t, cs, 2, "incorrect number of candidates found") - assert.Equal(t, "santaclaus", cs[0].User) - assert.Equal(t, "dasherdancer", cs[1].User) + assert.Equal(t, "santaclaus", cs[0].User()) + assert.Equal(t, "dasherdancer", cs[1].User()) }) t.Run("reviews", func(t *testing.T) { @@ -150,7 +150,7 @@ func TestCandidates(t *testing.T) { sort.Sort(CandidatesByCreationTime(cs)) require.Len(t, cs, 1, "incorrect number of candidates found") - assert.Equal(t, "mhaypenny", cs[0].User) + assert.Equal(t, "mhaypenny", cs[0].User()) }) t.Run("deduplicate", func(t *testing.T) { @@ -167,29 +167,29 @@ func TestCandidates(t *testing.T) { sort.Sort(CandidatesByCreationTime(cs)) require.Len(t, cs, 4, "incorrect number of candidates found") - assert.Equal(t, "mhaypenny", cs[0].User) - assert.Equal(t, "ttest", cs[1].User) - assert.Equal(t, "santaclaus", cs[2].User) - assert.Equal(t, "dasherdancer", cs[3].User) + assert.Equal(t, "mhaypenny", cs[0].User()) + assert.Equal(t, "ttest", cs[1].User()) + assert.Equal(t, "santaclaus", cs[2].User()) + assert.Equal(t, "dasherdancer", cs[3].User()) }) } func TestCandidatesByCreationTime(t *testing.T) { cs := []*Candidate{ { - User: "c", + Author: pull.NewAuthor("c"), CreatedAt: time.Date(2018, 6, 29, 12, 0, 0, 0, time.UTC), }, { - User: "a", + Author: pull.NewAuthor("a"), CreatedAt: time.Date(2018, 6, 28, 0, 0, 0, 0, time.UTC), }, { - User: "d", + Author: pull.NewAuthor("d"), CreatedAt: time.Date(2018, 6, 29, 14, 0, 0, 0, time.UTC), }, { - User: "b", + Author: pull.NewAuthor("b"), CreatedAt: time.Date(2018, 6, 29, 10, 0, 0, 0, time.UTC), }, } @@ -197,7 +197,7 @@ func TestCandidatesByCreationTime(t *testing.T) { sort.Sort(CandidatesByCreationTime(cs)) for i, u := range []string{"a", "b", "c", "d"} { - assert.Equalf(t, u, cs[i].User, "candidate at position %d is incorrect", i) + assert.Equalf(t, u, cs[i].User(), "candidate at position %d is incorrect", i) } } diff --git a/policy/common/result.go b/policy/common/result.go index ce0ca64b..2cf3540c 100644 --- a/policy/common/result.go +++ b/policy/common/result.go @@ -54,6 +54,7 @@ type ReviewRequestRule struct { Users []string Organizations []string Permissions []pull.Permission + Codeowners bool RequiredCount int RequestedCount int @@ -92,6 +93,29 @@ type RequiresResult struct { // Conditions contains the results of all required conditions Conditions []*PredicateResult + + // OwnershipGroups contains the results for each codeowner group when + // codeowners approval is required. Each group must have at least one + // approver for the rule to pass. + OwnershipGroups []OwnershipGroupResult +} + +// OwnershipGroupResult contains the approval status for a codeowner group. +type OwnershipGroupResult struct { + // Key is a deterministic identifier for this ownership group. + Key string + + // Owners contains the codeowners for this group (e.g., "@team-a", "@user1"). + Owners []string + + // Files contains the file paths that belong to this ownership group. + Files []string + + // Satisfied is true if at least one approver is a member of this group. + Satisfied bool + + // Approvers contains the usernames of approvers who are members of this group. + Approvers []string } type Dismissal struct { diff --git a/policy/disapproval/disapprove.go b/policy/disapproval/disapprove.go index d8b2a16f..65903fde 100644 --- a/policy/disapproval/disapprove.go +++ b/policy/disapproval/disapprove.go @@ -196,16 +196,16 @@ func (p *Policy) IsDisapproved(ctx context.Context, prctx pull.Context) (disappr // someone disapproved, but nobody has revoked case revoker == nil: disapproved = true - msg = fmt.Sprintf("Disapproved by %s", disapprover.User) + msg = fmt.Sprintf("Disapproved by %s", disapprover.User()) // the new disapproval appears after a revocation case disapprover.CreatedAt.After(revoker.CreatedAt): disapproved = true - msg = fmt.Sprintf("Disapproved by %s", disapprover.User) + msg = fmt.Sprintf("Disapproved by %s", disapprover.User()) // a disapproval has been revoked default: - msg = fmt.Sprintf("Disapproval revoked by %s", revoker.User) + msg = fmt.Sprintf("Disapproval revoked by %s", revoker.User()) } return } @@ -235,13 +235,13 @@ func (p *Policy) filter(ctx context.Context, prctx pull.Context, candidates []*c var filtered []*common.Candidate for _, c := range candidates { - ok, err := p.Requires.IsActor(ctx, prctx, c.User) + ok, err := p.Requires.IsActor(ctx, prctx, c.User()) if err != nil { return nil, errors.WithMessage(err, "failed to check candidate status") } if !ok { - log.Debug().Str("user", c.User).Msg("ignoring disapproval/revocation by non-whitelisted user") + log.Debug().Str("user", c.User()).Msg("ignoring disapproval/revocation by non-whitelisted user") continue } diff --git a/policy/disapproval/disapprove_test.go b/policy/disapproval/disapprove_test.go index 929d8762..08af4927 100644 --- a/policy/disapproval/disapprove_test.go +++ b/policy/disapproval/disapprove_test.go @@ -38,39 +38,39 @@ func TestIsDisapproved(t *testing.T) { TitleValue: "test: add disapproval predicate test", CommentsValue: []*pull.Comment{ { - Author: "disapprover-1", + Author: pull.NewAuthor("disapprover-1"), Body: "me no like :-1:", CreatedAt: date(0), }, { - Author: "disapprover-1", + Author: pull.NewAuthor("disapprover-1"), Body: "nah, is fine :+1:", CreatedAt: date(1), }, { - Author: "disapprover-2", + Author: pull.NewAuthor("disapprover-2"), Body: "me also no like :-1:", CreatedAt: date(2), }, { - Author: "disapprover-3", + Author: pull.NewAuthor("disapprover-3"), Body: "and me :-1:", CreatedAt: date(3), }, { - Author: "revoker-1", + Author: pull.NewAuthor("revoker-1"), Body: "you all wrong :+1:", CreatedAt: date(4), }, }, ReviewsValue: []*pull.Review{ { - Author: "disapprover-4", + Author: pull.NewAuthor("disapprover-4"), State: pull.ReviewChangesRequested, CreatedAt: date(5), }, { - Author: "revoker-2", + Author: pull.NewAuthor("revoker-2"), State: pull.ReviewApproved, CreatedAt: date(6), }, diff --git a/policy/reviewer/reviewer.go b/policy/reviewer/reviewer.go index b449271b..f38a81bf 100644 --- a/policy/reviewer/reviewer.go +++ b/policy/reviewer/reviewer.go @@ -17,6 +17,7 @@ package reviewer import ( "context" "fmt" + "maps" "math/rand" "slices" "sort" @@ -123,17 +124,25 @@ func selectRandomUsers(n int, users []string, r *rand.Rand) []string { return selections } -func selectTeamMembers(prctx pull.Context, allTeams []string) (map[string][]string, error) { - allTeamsMembers := make(map[string][]string) - for _, team := range allTeams { - teamMembers, err := prctx.TeamMembers(team) +func selectTeamMembers(prctx pull.Context, teams []string) (map[string][]string, error) { + teamMembers := make(map[string][]string) + for _, team := range teams { + members, err := prctx.TeamMembers(team) if err != nil { return nil, errors.Wrapf(err, "failed to get member listing for team %s", team) } - allTeamsMembers[team] = teamMembers + teamMembers[team] = members } + return teamMembers, nil +} - return allTeamsMembers, nil +// addTeamMembersToSet adds all members from the given teams to the user set. +func addTeamMembersToSet(teamMembers map[string][]string, userSet map[string]struct{}) { + for _, members := range teamMembers { + for _, user := range members { + userSet[user] = struct{}{} + } + } } func selectOrgMembers(prctx pull.Context, allOrgs []string) ([]string, error) { @@ -220,14 +229,11 @@ func selectUserReviewers(ctx context.Context, prctx pull.Context, selection *Sel if len(result.ReviewRequestRule.Teams) > 0 { logger.Debug().Msg("Selecting from teams for review") - teamsToUsers, err := selectTeamMembers(prctx, result.ReviewRequestRule.Teams) + teamMembers, err := selectTeamMembers(prctx, result.ReviewRequestRule.Teams) if err != nil { logger.Warn().Err(err).Msgf("failed to get member listing for teams, skipping team member selection") - } - for _, users := range teamsToUsers { - for _, user := range users { - allUsers[user] = struct{}{} - } + } else { + addTeamMembersToSet(teamMembers, allUsers) } } @@ -242,6 +248,26 @@ func selectUserReviewers(ctx context.Context, prctx pull.Context, selection *Sel } } + if result.ReviewRequestRule.Codeowners { + logger.Debug().Msg("Selecting from codeowners for review") + codeownerUsers, codeownerTeams, err := selectCodeowners(prctx) + if err != nil { + logger.Warn().Err(err).Msg("failed to get codeowners, skipping codeowner selection") + } else { + for _, user := range codeownerUsers { + allUsers[user] = struct{}{} + } + if len(codeownerTeams) > 0 { + teamMembers, err := selectTeamMembers(prctx, codeownerTeams) + if err != nil { + logger.Warn().Err(err).Msg("failed to get codeowner team members, skipping team member selection") + } else { + addTeamMembersToSet(teamMembers, allUsers) + } + } + } + } + minPerm := pull.PermissionNone if len(result.ReviewRequestRule.Permissions) > 0 { minPerm = slices.Min(result.ReviewRequestRule.Permissions) @@ -290,3 +316,33 @@ func requestsTeam(r *common.Result, team string) bool { func requestsPermission(r *common.Result, perm pull.Permission) bool { return slices.Contains(r.ReviewRequestRule.Permissions, perm) } + +// selectCodeowners returns the users and teams that are codeowners of the +// changed files in the pull request. +func selectCodeowners(prctx pull.Context) ([]string, []string, error) { + co, err := prctx.Codeowners() + if err != nil { + return nil, nil, err + } + if co == nil { + return nil, nil, nil + } + + userSet := make(map[string]struct{}) + teamSet := make(map[string]struct{}) + + for _, owner := range co.AllOwners() { + ownerType, name := pull.ParseCodeowner(owner) + switch ownerType { + case "user": + userSet[name] = struct{}{} + case "team": + teamSet[name] = struct{}{} + } + } + + users := slices.Sorted(maps.Keys(userSet)) + teams := slices.Sorted(maps.Keys(teamSet)) + + return users, teams, nil +} diff --git a/policy/simulated/context.go b/policy/simulated/context.go index 54e642c8..1ca69727 100644 --- a/policy/simulated/context.go +++ b/policy/simulated/context.go @@ -50,21 +50,23 @@ func (c *Context) filterIgnoredComments(prCtx pull.Context, comments []*pull.Com return comments, nil } - var filteredComments []*pull.Comment + var filtered []*pull.Comment for _, comment := range comments { - isActor, err := c.options.IgnoreComments.IsActor(c.ctx, prCtx, comment.Author) + if comment.Author == nil { + filtered = append(filtered, comment) + continue + } + + isIgnored, err := c.options.IgnoreComments.IsActor(c.ctx, prCtx, comment.Author.Login) if err != nil { return nil, err } - - if isActor { - continue + if !isIgnored { + filtered = append(filtered, comment) } - - filteredComments = append(filteredComments, comment) } - return filteredComments, nil + return filtered, nil } func (c *Context) addApprovalComment(comments []*pull.Comment) []*pull.Comment { @@ -96,21 +98,23 @@ func (c *Context) filterIgnoredReviews(prCtx pull.Context, reviews []*pull.Revie return reviews, nil } - var filteredReviews []*pull.Review + var filtered []*pull.Review for _, review := range reviews { - isActor, err := c.options.IgnoreReviews.IsActor(c.ctx, prCtx, review.Author) + if review.Author == nil { + filtered = append(filtered, review) + continue + } + + isIgnored, err := c.options.IgnoreReviews.IsActor(c.ctx, prCtx, review.Author.Login) if err != nil { return nil, err } - - if isActor { - continue + if !isIgnored { + filtered = append(filtered, review) } - - filteredReviews = append(filteredReviews, review) } - return filteredReviews, nil + return filtered, nil } func (c *Context) addApprovalReview(reviews []*pull.Review) []*pull.Review { diff --git a/policy/simulated/context_test.go b/policy/simulated/context_test.go index e97fe5ec..8bf5f8ac 100644 --- a/policy/simulated/context_test.go +++ b/policy/simulated/context_test.go @@ -37,8 +37,8 @@ func TestComments(t *testing.T) { }{ "ignore comments by user": { Comments: []*pull.Comment{ - {Author: "rrandom"}, - {Author: "iignore"}, + {Author: pull.NewAuthor("rrandom")}, + {Author: pull.NewAuthor("iignore")}, }, ExpectedCommentAuthors: []string{"rrandom"}, Options: Options{ @@ -49,8 +49,8 @@ func TestComments(t *testing.T) { }, "ignore comments by team membership": { Comments: []*pull.Comment{ - {Author: "rrandom"}, - {Author: "iignore"}, + {Author: pull.NewAuthor("rrandom")}, + {Author: pull.NewAuthor("iignore")}, }, Options: Options{ IgnoreComments: &common.Actors{ @@ -64,8 +64,8 @@ func TestComments(t *testing.T) { }, "ignore comments by org membership": { Comments: []*pull.Comment{ - {Author: "rrandom"}, - {Author: "iignore"}, + {Author: pull.NewAuthor("rrandom")}, + {Author: pull.NewAuthor("iignore")}, }, Options: Options{ IgnoreComments: &common.Actors{ @@ -79,8 +79,8 @@ func TestComments(t *testing.T) { }, "ignore comments by permission": { Comments: []*pull.Comment{ - {Author: "rrandom"}, - {Author: "iignore"}, + {Author: pull.NewAuthor("rrandom")}, + {Author: pull.NewAuthor("iignore")}, }, Options: Options{ IgnoreComments: &common.Actors{ @@ -96,15 +96,15 @@ func TestComments(t *testing.T) { }, "do not ignore any comments": { Comments: []*pull.Comment{ - {Author: "rrandom"}, - {Author: "iignore"}, + {Author: pull.NewAuthor("rrandom")}, + {Author: pull.NewAuthor("iignore")}, }, ExpectedCommentAuthors: []string{"rrandom", "iignore"}, }, "add new comment by sperson": { Comments: []*pull.Comment{ - {Author: "rrandom"}, - {Author: "iignore"}, + {Author: pull.NewAuthor("rrandom")}, + {Author: pull.NewAuthor("iignore")}, }, Options: Options{ AddComments: []Comment{ @@ -115,8 +115,8 @@ func TestComments(t *testing.T) { }, "add new comment by sperson and ignore one from iignore": { Comments: []*pull.Comment{ - {Author: "rrandom"}, - {Author: "iignore"}, + {Author: pull.NewAuthor("rrandom")}, + {Author: pull.NewAuthor("iignore")}, }, Options: Options{ AddComments: []Comment{ @@ -157,7 +157,9 @@ func TestComments(t *testing.T) { func getCommentAuthors(comments []*pull.Comment) []string { var authors []string for _, c := range comments { - authors = append(authors, c.Author) + if c.Author != nil { + authors = append(authors, c.Author.Login) + } } sort.Strings(authors) @@ -176,8 +178,8 @@ func TestReviews(t *testing.T) { }{ "ignore reviews by iignore": { Reviews: []*pull.Review{ - {Author: "rrandom"}, - {Author: "iignore"}, + {Author: pull.NewAuthor("rrandom")}, + {Author: pull.NewAuthor("iignore")}, }, ExpectedReviewAuthors: []string{"rrandom"}, Options: Options{ @@ -188,8 +190,8 @@ func TestReviews(t *testing.T) { }, "ignore reviews by team membership": { Reviews: []*pull.Review{ - {Author: "rrandom"}, - {Author: "iignore"}, + {Author: pull.NewAuthor("rrandom")}, + {Author: pull.NewAuthor("iignore")}, }, Options: Options{ IgnoreReviews: &common.Actors{ @@ -203,8 +205,8 @@ func TestReviews(t *testing.T) { }, "ignore reviews by org membership": { Reviews: []*pull.Review{ - {Author: "rrandom"}, - {Author: "iignore"}, + {Author: pull.NewAuthor("rrandom")}, + {Author: pull.NewAuthor("iignore")}, }, Options: Options{ IgnoreReviews: &common.Actors{ @@ -218,8 +220,8 @@ func TestReviews(t *testing.T) { }, "ignore reviews by permission": { Reviews: []*pull.Review{ - {Author: "rrandom"}, - {Author: "iignore"}, + {Author: pull.NewAuthor("rrandom")}, + {Author: pull.NewAuthor("iignore")}, }, Options: Options{ IgnoreReviews: &common.Actors{ @@ -235,15 +237,15 @@ func TestReviews(t *testing.T) { }, "do not ignore any reviews": { Reviews: []*pull.Review{ - {Author: "rrandom"}, - {Author: "iignore"}, + {Author: pull.NewAuthor("rrandom")}, + {Author: pull.NewAuthor("iignore")}, }, ExpectedReviewAuthors: []string{"rrandom", "iignore"}, }, "add new review by sperson": { Reviews: []*pull.Review{ - {Author: "rrandom"}, - {Author: "iignore"}, + {Author: pull.NewAuthor("rrandom")}, + {Author: pull.NewAuthor("iignore")}, }, Options: Options{ AddReviews: []Review{ @@ -254,8 +256,8 @@ func TestReviews(t *testing.T) { }, "add new review by sperson and ignore one from iignore": { Reviews: []*pull.Review{ - {Author: "rrandom"}, - {Author: "iignore"}, + {Author: pull.NewAuthor("rrandom")}, + {Author: pull.NewAuthor("iignore")}, }, Options: Options{ AddReviews: []Review{ @@ -295,8 +297,10 @@ func TestReviews(t *testing.T) { func getReviewAuthors(reviews []*pull.Review) []string { var authors []string - for _, c := range reviews { - authors = append(authors, c.Author) + for _, r := range reviews { + if r.Author != nil { + authors = append(authors, r.Author.Login) + } } sort.Strings(authors) diff --git a/policy/simulated/options.go b/policy/simulated/options.go index 1a104b6d..12c36f05 100644 --- a/policy/simulated/options.go +++ b/policy/simulated/options.go @@ -88,7 +88,7 @@ func (c *Comment) toPullComment() *pull.Comment { return &pull.Comment{ CreatedAt: *c.CreatedAt, LastEditedAt: *c.LastEditedAt, - Author: c.Author, + Author: pull.NewAuthor(c.Author), Body: c.Body, } } @@ -125,7 +125,7 @@ func (r *Review) toPullReview() *pull.Review { SHA: r.SHA, CreatedAt: *r.CreatedAt, LastEditedAt: *r.LastEditedAt, - Author: r.Author, + Author: pull.NewAuthor(r.Author), State: pull.ReviewState(r.State), Body: r.Body, Teams: r.Teams, diff --git a/pull/codeowners_test.go b/pull/codeowners_test.go new file mode 100644 index 00000000..f88a3f8d --- /dev/null +++ b/pull/codeowners_test.go @@ -0,0 +1,254 @@ +// Copyright 2018 Palantir Technologies, Inc. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package pull + +import ( + "sort" + "testing" + + "github.com/stretchr/testify/assert" +) + +func TestParseCodeowner(t *testing.T) { + tests := []struct { + name string + input string + expectedType string + expectedName string + }{ + { + name: "user with @", + input: "@username", + expectedType: "user", + expectedName: "username", + }, + { + name: "user without @", + input: "username", + expectedType: "user", + expectedName: "username", + }, + { + name: "team with @", + input: "@org/team-name", + expectedType: "team", + expectedName: "org/team-name", + }, + { + name: "team without @", + input: "org/team-name", + expectedType: "team", + expectedName: "org/team-name", + }, + { + name: "team with nested path", + input: "@org/parent/child-team", + expectedType: "team", + expectedName: "org/parent/child-team", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + ownerType, name := ParseCodeowner(tt.input) + assert.Equal(t, tt.expectedType, ownerType) + assert.Equal(t, tt.expectedName, name) + }) + } +} + +func TestCodeownersResultAllOwners(t *testing.T) { + t.Run("nil result", func(t *testing.T) { + var result *CodeownersResult + owners := result.AllOwners() + assert.Nil(t, owners) + }) + + t.Run("empty owners", func(t *testing.T) { + result := &CodeownersResult{ + Owners: map[string][]string{}, + } + owners := result.AllOwners() + assert.Empty(t, owners) + }) + + t.Run("single file single owner", func(t *testing.T) { + result := &CodeownersResult{ + Owners: map[string][]string{ + "file.go": {"@user1"}, + }, + } + owners := result.AllOwners() + assert.Equal(t, []string{"@user1"}, owners) + }) + + t.Run("multiple files with overlapping owners", func(t *testing.T) { + result := &CodeownersResult{ + Owners: map[string][]string{ + "file1.go": {"@user1", "@user2"}, + "file2.go": {"@user2", "@user3"}, + "file3.go": {"@org/team1"}, + }, + } + owners := result.AllOwners() + sort.Strings(owners) // Sort for deterministic comparison + assert.Equal(t, []string{"@org/team1", "@user1", "@user2", "@user3"}, owners) + }) + + t.Run("deduplication", func(t *testing.T) { + result := &CodeownersResult{ + Owners: map[string][]string{ + "file1.go": {"@user1", "@user1"}, + "file2.go": {"@user1"}, + }, + } + owners := result.AllOwners() + assert.Equal(t, []string{"@user1"}, owners) + }) +} + +func TestCodeownersResultOwnershipGroups(t *testing.T) { + t.Run("nil result", func(t *testing.T) { + var result *CodeownersResult + groups := result.OwnershipGroups() + assert.Nil(t, groups) + }) + + t.Run("empty owners", func(t *testing.T) { + result := &CodeownersResult{ + Owners: map[string][]string{}, + } + groups := result.OwnershipGroups() + assert.Nil(t, groups) + }) + + t.Run("single file single owner", func(t *testing.T) { + result := &CodeownersResult{ + Owners: map[string][]string{ + "file.go": {"@team-a"}, + }, + } + groups := result.OwnershipGroups() + assert.Len(t, groups, 1) + assert.Equal(t, "@team-a", groups[0].Key) + assert.Equal(t, []string{"@team-a"}, groups[0].Owners) + assert.Equal(t, []string{"file.go"}, groups[0].Files) + }) + + t.Run("multiple files same owner grouped together", func(t *testing.T) { + result := &CodeownersResult{ + Owners: map[string][]string{ + "a/file1.go": {"@team-a"}, + "a/file2.go": {"@team-a"}, + "a/file3.go": {"@team-a"}, + }, + } + groups := result.OwnershipGroups() + assert.Len(t, groups, 1) + assert.Equal(t, "@team-a", groups[0].Key) + assert.Equal(t, []string{"@team-a"}, groups[0].Owners) + assert.Equal(t, []string{"a/file1.go", "a/file2.go", "a/file3.go"}, groups[0].Files) + }) + + t.Run("multiple distinct ownership groups", func(t *testing.T) { + result := &CodeownersResult{ + Owners: map[string][]string{ + "a/file.go": {"@team-a"}, + "b/file.go": {"@team-b"}, + "c/file.go": {"@team-c"}, + }, + } + groups := result.OwnershipGroups() + assert.Len(t, groups, 3) + + // Groups should be sorted by key + assert.Equal(t, "@team-a", groups[0].Key) + assert.Equal(t, "@team-b", groups[1].Key) + assert.Equal(t, "@team-c", groups[2].Key) + }) + + t.Run("owner order independent grouping", func(t *testing.T) { + // Files with the same owners but listed in different order + // should be grouped together + result := &CodeownersResult{ + Owners: map[string][]string{ + "file1.go": {"@user1", "@user2"}, + "file2.go": {"@user2", "@user1"}, + }, + } + groups := result.OwnershipGroups() + assert.Len(t, groups, 1) + assert.Equal(t, "@user1,@user2", groups[0].Key) + assert.Equal(t, []string{"@user1", "@user2"}, groups[0].Owners) + assert.Equal(t, []string{"file1.go", "file2.go"}, groups[0].Files) + }) + + t.Run("mixed single and multiple owners", func(t *testing.T) { + result := &CodeownersResult{ + Owners: map[string][]string{ + "a/file.go": {"@team-a"}, + "b/file.go": {"@team-b", "@team-c"}, + "ab/file.go": {"@team-a", "@team-b"}, + "ab/file2.go": {"@team-b", "@team-a"}, // Same as above, different order + }, + } + groups := result.OwnershipGroups() + assert.Len(t, groups, 3) + + // Find each group by key + groupByKey := make(map[string]OwnershipGroup) + for _, g := range groups { + groupByKey[g.Key] = g + } + + // Single owner group + assert.Equal(t, []string{"a/file.go"}, groupByKey["@team-a"].Files) + + // Two owners group (@team-a, @team-b) - files should be grouped + abGroup := groupByKey["@team-a,@team-b"] + assert.Equal(t, []string{"@team-a", "@team-b"}, abGroup.Owners) + assert.Equal(t, []string{"ab/file.go", "ab/file2.go"}, abGroup.Files) + + // Two owners group (@team-b, @team-c) + bcGroup := groupByKey["@team-b,@team-c"] + assert.Equal(t, []string{"@team-b", "@team-c"}, bcGroup.Owners) + assert.Equal(t, []string{"b/file.go"}, bcGroup.Files) + }) + + t.Run("files with empty owners are skipped", func(t *testing.T) { + result := &CodeownersResult{ + Owners: map[string][]string{ + "file1.go": {"@team-a"}, + "file2.go": {}, + }, + } + groups := result.OwnershipGroups() + assert.Len(t, groups, 1) + assert.Equal(t, "@team-a", groups[0].Key) + }) + + t.Run("user and team owners mixed", func(t *testing.T) { + result := &CodeownersResult{ + Owners: map[string][]string{ + "file1.go": {"@johndoe", "@org/team-a"}, + "file2.go": {"@org/team-a", "@johndoe"}, // Same owners, different order + }, + } + groups := result.OwnershipGroups() + assert.Len(t, groups, 1) + assert.Equal(t, "@johndoe,@org/team-a", groups[0].Key) + assert.Equal(t, []string{"file1.go", "file2.go"}, groups[0].Files) + }) +} diff --git a/pull/context.go b/pull/context.go index eb1ac4c3..8dc5ac04 100644 --- a/pull/context.go +++ b/pull/context.go @@ -15,6 +15,8 @@ package pull import ( + "sort" + "strings" "time" ) @@ -31,6 +33,12 @@ type MembershipContext interface { // TeamMembers returns the list of usernames in the given organization's team. TeamMembers(team string) ([]string, error) + // TeamMembersWithDetails returns team members with full metadata (avatars, URLs). + TeamMembersWithDetails(team string) ([]TeamMember, error) + + // TeamInfo returns team metadata for display purposes. + TeamInfo(team string) (*TeamInfo, error) + // OrganizationMembers returns the list of org member usernames in the given organization. OrganizationMembers(org string) ([]string, error) } @@ -137,6 +145,10 @@ type Context interface { // Labels returns a list of labels applied on the Pull Request Labels() ([]string, error) + + // Codeowners returns the codeowners for files changed in this pull request. + // Returns nil if no CODEOWNERS file exists in the repository. + Codeowners() (*CodeownersResult, error) } type FileStatus int @@ -201,10 +213,21 @@ type Signature struct { State string } +// Author represents a GitHub user who performed an action. +type Author struct { + Login string + AvatarURL string +} + +// NewAuthor creates an Author with the given login. Useful for tests. +func NewAuthor(login string) *Author { + return &Author{Login: login} +} + type Comment struct { CreatedAt time.Time LastEditedAt time.Time - Author string + Author *Author Body string } @@ -222,7 +245,7 @@ type Review struct { ID string CreatedAt time.Time LastEditedAt time.Time - Author string + Author *Author State ReviewState Body string SHA string @@ -259,7 +282,7 @@ type CollaboratorPermission struct { type Body struct { Body string CreatedAt time.Time - Author string + Author *Author LastEditedAt time.Time } @@ -267,3 +290,131 @@ type CustomProperty struct { String *string Array []string } + +// CodeownersResult contains the owners for each changed file in a pull request. +type CodeownersResult struct { + // Owners maps file paths to their owners. Owners are in the format + // "@username" for users or "@org/team" for teams. + Owners map[string][]string + + // OrphanFiles contains files that don't match any CODEOWNERS pattern. + OrphanFiles []string +} + +// HasOrphanFiles returns true if there are files without codeowners. +func (c *CodeownersResult) HasOrphanFiles() bool { + return c != nil && len(c.OrphanFiles) > 0 +} + +// AllOwners returns a deduplicated list of all owners across all files. +func (c *CodeownersResult) AllOwners() []string { + if c == nil { + return nil + } + + ownerSet := make(map[string]struct{}) + for _, fileOwners := range c.Owners { + for _, owner := range fileOwners { + ownerSet[owner] = struct{}{} + } + } + + owners := make([]string, 0, len(ownerSet)) + for owner := range ownerSet { + owners = append(owners, owner) + } + return owners +} + +// OwnersByOwner returns an inverted mapping of owner -> files. +// This is useful for displaying which files each owner is responsible for. +func (c *CodeownersResult) OwnersByOwner() map[string][]string { + if c == nil { + return nil + } + + result := make(map[string][]string) + for file, owners := range c.Owners { + for _, owner := range owners { + result[owner] = append(result[owner], file) + } + } + return result +} + +// ParseCodeowner parses a CODEOWNERS owner string and returns whether it's +// a user or team, along with the normalized name (without @ prefix). +// Examples: +// +// "@username" -> ("user", "username") +// "@org/team-name" -> ("team", "org/team-name") +func ParseCodeowner(owner string) (ownerType string, name string) { + owner = strings.TrimPrefix(owner, "@") + if strings.Contains(owner, "/") { + return "team", owner + } + return "user", owner +} + +// OwnershipGroup represents a unique set of owners for one or more files. +// Files with identical owner sets are grouped together. +type OwnershipGroup struct { + // Key is a deterministic string representation of the sorted owner set, + // used for grouping files with identical owners. + Key string + + // Owners contains the owners in this group (e.g., "@team-a", "@user1"). + Owners []string + + // Files contains the file paths that belong to this ownership group. + Files []string +} + +// OwnershipGroups returns the unique ownership groups across all changed files. +// Files with identical owner sets are grouped together. The groups are returned +// in a deterministic order based on the sorted owner keys. +func (c *CodeownersResult) OwnershipGroups() []OwnershipGroup { + if c == nil || len(c.Owners) == 0 { + return nil + } + + // Group files by their sorted owner set + groupMap := make(map[string]*OwnershipGroup) + + for file, owners := range c.Owners { + if len(owners) == 0 { + continue + } + + // Create a deterministic key by sorting owners + sortedOwners := make([]string, len(owners)) + copy(sortedOwners, owners) + sort.Strings(sortedOwners) + key := strings.Join(sortedOwners, ",") + + if group, exists := groupMap[key]; exists { + group.Files = append(group.Files, file) + } else { + groupMap[key] = &OwnershipGroup{ + Key: key, + Owners: sortedOwners, + Files: []string{file}, + } + } + } + + // Convert map to slice and sort for deterministic output + groups := make([]OwnershipGroup, 0, len(groupMap)) + for _, group := range groupMap { + // Sort files within each group for deterministic output + sort.Strings(group.Files) + groups = append(groups, *group) + } + + // Sort groups by key for deterministic output + sort.Slice(groups, func(i, j int) bool { + return groups[i].Key < groups[j].Key + }) + + return groups +} diff --git a/pull/github.go b/pull/github.go index 0eb6b35a..91b5fe52 100644 --- a/pull/github.go +++ b/pull/github.go @@ -20,9 +20,11 @@ import ( "net/http" "slices" "strings" + "sync" "time" "github.com/google/go-github/v82/github" + "github.com/hairyhenderson/go-codeowners" "github.com/pkg/errors" "github.com/shurcooL/githubv4" ) @@ -146,6 +148,10 @@ type GitHubContext struct { pushedAt map[string]time.Time workflowRuns map[string][]string repositoryCustomProperties map[string]CustomProperty + + codeownersResult *CodeownersResult + codeownersLoaded bool + codeownersErr error } // NewGitHubContext creates a new pull.Context that makes GitHub requests to @@ -266,7 +272,7 @@ func (ghc *GitHubContext) Body() (*Body, error) { return &Body{ Body: graphqlResponse.Body, CreatedAt: graphqlResponse.CreatedAt, - Author: graphqlResponse.Author.GetV3Login(), + Author: graphqlResponse.Author.ToAuthor(), LastEditedAt: graphqlResponse.LastEditedAt, }, nil } @@ -577,16 +583,32 @@ func (ghc *GitHubContext) RepositoryCollaborators(minPermission Permission) ([]* return nil, err } + // Fetch team members in parallel teamMembership := make(map[string][]string) + var membershipMu sync.Mutex + var membershipWg sync.WaitGroup + var membershipErr error + var errOnce sync.Once + for team := range teamPerms { - members, err := ghc.TeamMembers(ghc.owner + "/" + team) - if err != nil { - return nil, err - } + membershipWg.Go(func() { + members, err := ghc.TeamMembers(ghc.owner + "/" + team) + if err != nil { + errOnce.Do(func() { membershipErr = err }) + return + } + membershipMu.Lock() + for _, member := range members { + teamMembership[member] = append(teamMembership[member], team) + } + membershipMu.Unlock() + }) + } - for _, member := range members { - teamMembership[member] = append(teamMembership[member], team) - } + membershipWg.Wait() + + if membershipErr != nil { + return nil, membershipErr } fillPermissions := func(c *Collaborator) { @@ -959,6 +981,138 @@ func (ghc *GitHubContext) Labels() ([]string, error) { return ghc.labels, nil } +// codeownersLocations are the standard locations for CODEOWNERS files +// as documented by GitHub. +var codeownersLocations = []string{ + ".github/CODEOWNERS", + "CODEOWNERS", + "docs/CODEOWNERS", +} + +func (ghc *GitHubContext) Codeowners() (*CodeownersResult, error) { + if ghc.codeownersLoaded { + return ghc.codeownersResult, ghc.codeownersErr + } + ghc.codeownersLoaded = true + + // Load CODEOWNERS from the base branch (not the PR head) for security + co, err := ghc.loadCodeowners() + if err != nil { + ghc.codeownersErr = err + return nil, ghc.codeownersErr + } + + // No CODEOWNERS file found + if co == nil { + return nil, nil + } + + // Get changed files and compute owners for each + files, err := ghc.ChangedFiles() + if err != nil { + ghc.codeownersErr = errors.Wrap(err, "failed to get changed files for codeowners") + return nil, ghc.codeownersErr + } + + result := &CodeownersResult{ + Owners: make(map[string][]string), + } + for _, f := range files { + owners := co.Owners(f.Filename) + if len(owners) > 0 { + result.Owners[f.Filename] = owners + } else { + result.OrphanFiles = append(result.OrphanFiles, f.Filename) + } + } + + ghc.codeownersResult = result + return result, nil +} + +// loadCodeowners attempts to load the CODEOWNERS file from the base branch. +// It uses the base branch ref to prevent PRs from modifying their own approval +// requirements by changing CODEOWNERS in the PR. Returns nil if no CODEOWNERS +// file exists. +func (ghc *GitHubContext) loadCodeowners() (*codeowners.Codeowners, error) { + baseRef := ghc.pr.BaseRefOID + repoID := ghc.pr.BaseRepository.DatabaseID + + // Check cache for parsed CODEOWNERS content + if gc := ghc.globalCache; gc != nil { + if co, ok := gc.GetCodeowners(repoID, baseRef); ok { + return co, nil + } + } + + type codeownersFetchResult struct { + co *codeowners.Codeowners + err error + } + results := make([]codeownersFetchResult, len(codeownersLocations)) + var wg sync.WaitGroup + + for i, path := range codeownersLocations { + wg.Go(func() { + co, err := ghc.fetchCodeowners(path, baseRef) + results[i] = codeownersFetchResult{co: co, err: err} + }) + } + wg.Wait() + + // Return first result by priority order (check errors and results together) + for _, r := range results { + if r.err != nil { + return nil, r.err + } + if r.co != nil { + if gc := ghc.globalCache; gc != nil { + gc.SetCodeowners(repoID, baseRef, r.co) + } + return r.co, nil + } + } + + return nil, nil +} + +// fetchCodeowners retrieves and parses a CODEOWNERS file at the given path and ref. +// Returns nil without error if the file does not exist. +func (ghc *GitHubContext) fetchCodeowners(path, ref string) (*codeowners.Codeowners, error) { + content, exists, err := ghc.getFileContent(path, ref) + if err != nil { + return nil, errors.Wrapf(err, "failed to fetch %s", path) + } + if !exists { + return nil, nil + } + + co, err := codeowners.FromReader(strings.NewReader(content), "") + if err != nil { + return nil, errors.Wrapf(err, "failed to parse %s", path) + } + return co, nil +} + +func (ghc *GitHubContext) getFileContent(path, ref string) (string, bool, error) { + file, _, _, err := ghc.client.Repositories.GetContents( + ghc.ctx, ghc.owner, ghc.repo, path, + &github.RepositoryContentGetOptions{Ref: ref}, + ) + if err != nil { + if isNotFound(err) { + return "", false, nil + } + return "", false, err + } + + content, err := file.GetContent() + if err != nil { + return "", false, err + } + return content, true, nil +} + func (ghc *GitHubContext) loadPagedData() error { // This query is tuned to use a single GraphQL rate limit point per // execution. For most PRs, this means loading all commits, comments, and @@ -1110,6 +1264,7 @@ type v4PullRequest struct { Owner v4Actor } + BaseRefOID string BaseRefName string BaseRepository struct { DatabaseID int64 @@ -1170,7 +1325,7 @@ func (r *v4PullRequestReview) ToReview() *Review { ID: r.ID, CreatedAt: r.SubmittedAt, LastEditedAt: r.LastEditedAt, - Author: r.Author.GetV3Login(), + Author: r.Author.ToAuthor(), State: ReviewState(strings.ToLower(r.State)), Body: r.Body, SHA: r.Commit.OID, @@ -1182,7 +1337,7 @@ func (r *v4PullRequestReview) ToComment() *Comment { return &Comment{ CreatedAt: r.SubmittedAt, LastEditedAt: r.LastEditedAt, - Author: r.Author.GetV3Login(), + Author: r.Author.ToAuthor(), Body: r.Body, } } @@ -1198,7 +1353,7 @@ func (c *v4IssueComment) ToComment() *Comment { return &Comment{ CreatedAt: c.CreatedAt, LastEditedAt: c.LastEditedAt, - Author: c.Author.GetV3Login(), + Author: c.Author.ToAuthor(), Body: c.Body, } } @@ -1266,8 +1421,9 @@ type v4Team struct { } type v4Actor struct { - Type string `graphql:"__typename"` - Login string + Type string `graphql:"__typename"` + Login string + AvatarURL string `graphql:"avatarUrl"` } // GetV3Login returns a V3-compatible login string. These login strings contain @@ -1282,6 +1438,25 @@ func (a *v4Actor) GetV3Login() string { return a.Login } +// GetAvatarURL returns the actor's avatar URL. +func (a *v4Actor) GetAvatarURL() string { + if a == nil { + return "" + } + return a.AvatarURL +} + +// ToAuthor converts the actor to an Author struct. +func (a *v4Actor) ToAuthor() *Author { + if a == nil { + return nil + } + return &Author{ + Login: a.GetV3Login(), + AvatarURL: a.AvatarURL, + } +} + type v4GitActor struct { User *v4Actor } diff --git a/pull/github_membership.go b/pull/github_membership.go index 711406ae..b218729b 100644 --- a/pull/github_membership.go +++ b/pull/github_membership.go @@ -17,27 +17,42 @@ package pull import ( "context" "strings" + "sync" "github.com/google/go-github/v82/github" "github.com/pkg/errors" ) type GitHubMembershipContext struct { - ctx context.Context - client *github.Client + ctx context.Context + client *github.Client + globalCache GlobalCache - membership map[string]bool - orgMembers map[string][]string - teamMembers map[string][]string + membership sync.Map // map[string]bool + orgMembers sync.Map // map[string][]string + teamMembers sync.Map // map[string][]string + teamData sync.Map // map[string]*teamLoader } -func NewGitHubMembershipContext(ctx context.Context, client *github.Client) *GitHubMembershipContext { +// cachedTeamData holds rich team data including member details +type cachedTeamData struct { + Info *TeamInfo + Members []TeamMember +} + +// teamLoader coordinates loading of team data to prevent duplicate API calls +type teamLoader struct { + once sync.Once + data *cachedTeamData + err error +} + +func NewGitHubMembershipContext(ctx context.Context, client *github.Client, globalCache GlobalCache) *GitHubMembershipContext { return &GitHubMembershipContext{ ctx: ctx, client: client, - membership: make(map[string]bool), - orgMembers: make(map[string][]string), - teamMembers: make(map[string][]string), + globalCache: globalCache, + // sync.Map zero value is ready to use } } @@ -56,23 +71,85 @@ func splitTeam(team string) (org, slug string, err error) { func (mc *GitHubMembershipContext) IsTeamMember(team, user string) (bool, error) { key := membershipKey(team, user) - org, slug, err := splitTeam(team) + // Check per-request cache first (fast path) + if val, ok := mc.membership.Load(key); ok { + return val.(bool), nil + } + + // Check global caches + if mc.globalCache != nil { + if isMember, found := mc.checkGlobalCaches(key, team, user); found { + return isMember, nil + } + } + + // Fetch full team members list (batch approach is more efficient for multiple checks) + data, err := mc.loadTeamWithMembers(team) if err != nil { - return false, err + // Fall back to individual API call on error + return mc.checkTeamMembershipViaAPI(key, team, user) } - isMember, ok := mc.membership[key] - if ok { - return isMember, nil + // populateLocalCaches already stored true for all members, but we need to + // also store false for non-members to avoid re-checking the list + isMember := mc.findMemberInList(data.Members, user) + if !isMember { + mc.membership.Store(key, false) + } + return isMember, nil +} + +// checkGlobalCaches checks both team members cache and individual membership cache. +// Returns (isMember, found) where found indicates if a cached value was found. +func (mc *GitHubMembershipContext) checkGlobalCaches(key, team, user string) (bool, bool) { + // Check full team members list first + if members, _, found := mc.globalCache.GetTeamMembers(team); found { + isMember := mc.findMemberInList(members, user) + mc.membership.Store(key, isMember) + return isMember, true + } + + // Fall back to individual membership cache + if isMember, found := mc.globalCache.GetTeamMembership(team, user); found { + mc.membership.Store(key, isMember) + return isMember, true + } + + return false, false +} + +// findMemberInList searches for a user in a team member list using case-insensitive comparison. +func (mc *GitHubMembershipContext) findMemberInList(members []TeamMember, user string) bool { + for _, m := range members { + if strings.EqualFold(m.Login, user) { + return true + } + } + return false +} + +// checkTeamMembershipViaAPI checks team membership using individual API call. +// Used as fallback when batch fetch fails. +func (mc *GitHubMembershipContext) checkTeamMembershipViaAPI(key, team, user string) (bool, error) { + org, slug, err := splitTeam(team) + if err != nil { + return false, err } membership, _, err := mc.client.Teams.GetTeamMembershipBySlug(mc.ctx, org, slug, user) - if err != nil && !isNotFound(err) { + if err != nil { + if isNotFound(err) { + mc.membership.Store(key, false) + return false, nil + } return false, errors.Wrap(err, "failed to get team membership") } - isMember = membership != nil && membership.GetState() == "active" - mc.membership[key] = isMember + isMember := membership.GetState() == "active" + mc.membership.Store(key, isMember) + if mc.globalCache != nil { + mc.globalCache.SetTeamMembership(team, user, isMember) + } return isMember, nil } @@ -80,9 +157,8 @@ func (mc *GitHubMembershipContext) IsTeamMember(team, user string) (bool, error) func (mc *GitHubMembershipContext) IsOrgMember(org, user string) (bool, error) { key := membershipKey(org, user) - isMember, ok := mc.membership[key] - if ok { - return isMember, nil + if val, ok := mc.membership.Load(key); ok { + return val.(bool), nil } isMember, _, err := mc.client.Organizations.IsMember(mc.ctx, org, user) @@ -90,69 +166,194 @@ func (mc *GitHubMembershipContext) IsOrgMember(org, user string) (bool, error) { return false, errors.Wrap(err, "failed to get organization membership") } - mc.membership[key] = isMember + mc.membership.Store(key, isMember) return isMember, nil } func (mc *GitHubMembershipContext) OrganizationMembers(org string) ([]string, error) { - members, ok := mc.orgMembers[org] - if !ok { - opt := &github.ListMembersOptions{ - ListOptions: github.ListOptions{ - PerPage: 100, - }, - } + if val, ok := mc.orgMembers.Load(org); ok { + return val.([]string), nil + } - for { - users, resp, err := mc.client.Organizations.ListMembers(mc.ctx, org, opt) - if err != nil { - return nil, errors.Wrapf(err, "failed to list members of org %s page %d", org, opt.Page) - } - for _, u := range users { - members = append(members, u.GetLogin()) - // And cache these values for later lookups - mc.membership[membershipKey(org, u.GetLogin())] = true - } - if resp.NextPage == 0 { - break - } - opt.Page = resp.NextPage + opt := &github.ListMembersOptions{ + ListOptions: github.ListOptions{ + PerPage: 100, + }, + } + + var members []string + for { + users, resp, err := mc.client.Organizations.ListMembers(mc.ctx, org, opt) + if err != nil { + return nil, errors.Wrapf(err, "failed to list members of org %s page %d", org, opt.Page) + } + for _, u := range users { + members = append(members, u.GetLogin()) + // And cache these values for later lookups + mc.membership.Store(membershipKey(org, u.GetLogin()), true) } - mc.orgMembers[org] = members + if resp.NextPage == 0 { + break + } + opt.Page = resp.NextPage } + mc.orgMembers.Store(org, members) return members, nil } func (mc *GitHubMembershipContext) TeamMembers(team string) ([]string, error) { - members, ok := mc.teamMembers[team] - if !ok { - opt := &github.TeamListTeamMembersOptions{ - ListOptions: github.ListOptions{ - PerPage: 100, - }, + if val, ok := mc.teamMembers.Load(team); ok { + return val.([]string), nil + } + + org, slug, err := splitTeam(team) + if err != nil { + return nil, err + } + + opt := &github.TeamListTeamMembersOptions{ + ListOptions: github.ListOptions{PerPage: 100}, + } + + var members []string + for { + users, resp, err := mc.client.Teams.ListTeamMembersBySlug(mc.ctx, org, slug, opt) + if err != nil { + return nil, errors.Wrapf(err, "failed to list team %s members page %d", team, opt.Page) + } + for _, u := range users { + login := u.GetLogin() + members = append(members, login) + mc.membership.Store(membershipKey(team, login), true) + } + if resp.NextPage == 0 { + break + } + opt.Page = resp.NextPage + } + mc.teamMembers.Store(team, members) + return members, nil +} + +// loadTeamWithMembers fetches and caches team data with full member details. +// It checks global cache first, then uses sync.Once to deduplicate concurrent API calls. +func (mc *GitHubMembershipContext) loadTeamWithMembers(team string) (*cachedTeamData, error) { + // Check global cache first (shared across requests) + if mc.globalCache != nil { + if members, info, found := mc.globalCache.GetTeamMembers(team); found { + mc.populateLocalCaches(team, members) + return &cachedTeamData{Info: info, Members: members}, nil } + } + + // Use LoadOrStore to get-or-create a loader, ensuring only one fetch per team + loaderVal, _ := mc.teamData.LoadOrStore(team, &teamLoader{}) + loader := loaderVal.(*teamLoader) + + // Execute the fetch exactly once + loader.once.Do(func() { + loader.data, loader.err = mc.fetchTeamFromAPI(team) + }) + + return loader.data, loader.err +} + +// populateLocalCaches updates per-request caches from team member data. +func (mc *GitHubMembershipContext) populateLocalCaches(team string, members []TeamMember) { + usernames := make([]string, len(members)) + for i, m := range members { + mc.membership.Store(membershipKey(team, m.Login), true) + usernames[i] = m.Login + } + mc.teamMembers.Store(team, usernames) +} + +// fetchTeamFromAPI fetches team info and members from the GitHub API in parallel. +func (mc *GitHubMembershipContext) fetchTeamFromAPI(team string) (*cachedTeamData, error) { + org, slug, err := splitTeam(team) + if err != nil { + return nil, err + } - org, slug, err := splitTeam(team) + var teamObj *github.Team + var members []TeamMember + var teamErr, membersErr error + var wg sync.WaitGroup + + // Fetch team info + wg.Go(func() { + t, _, err := mc.client.Teams.GetTeamBySlug(mc.ctx, org, slug) if err != nil { - return nil, err + teamErr = errors.Wrap(err, "failed to get team info") + return } + teamObj = t + }) + // Fetch team members + wg.Go(func() { + opt := &github.TeamListTeamMembersOptions{ + ListOptions: github.ListOptions{PerPage: 100}, + } for { users, resp, err := mc.client.Teams.ListTeamMembersBySlug(mc.ctx, org, slug, opt) if err != nil { - return nil, errors.Wrapf(err, "failed to list team %s members page %d", team, opt.Page) + membersErr = errors.Wrapf(err, "failed to list team %s members page %d", team, opt.Page) + return } for _, u := range users { - members = append(members, u.GetLogin()) - // And cache these values for later lookups - mc.membership[membershipKey(team, u.GetLogin())] = true + members = append(members, TeamMember{ + Login: u.GetLogin(), + AvatarURL: u.GetAvatarURL(), + HTMLURL: u.GetHTMLURL(), + }) } if resp.NextPage == 0 { break } opt.Page = resp.NextPage } - mc.teamMembers[team] = members + }) + + wg.Wait() + + // Return first error encountered + if teamErr != nil { + return nil, teamErr } - return members, nil + if membersErr != nil { + return nil, membersErr + } + + info := &TeamInfo{ + ID: teamObj.GetID(), + Slug: teamObj.GetSlug(), + HTMLURL: teamObj.GetHTMLURL(), + } + + mc.populateLocalCaches(team, members) + + if mc.globalCache != nil { + mc.globalCache.SetTeamMembers(team, info, members) + } + + return &cachedTeamData{Info: info, Members: members}, nil +} + +// TeamMembersWithDetails returns team members with full metadata (avatars, URLs). +func (mc *GitHubMembershipContext) TeamMembersWithDetails(team string) ([]TeamMember, error) { + data, err := mc.loadTeamWithMembers(team) + if err != nil { + return nil, err + } + return data.Members, nil +} + +// TeamInfo returns team metadata for display purposes. +func (mc *GitHubMembershipContext) TeamInfo(team string) (*TeamInfo, error) { + data, err := mc.loadTeamWithMembers(team) + if err != nil { + return nil, err + } + return data.Info, nil } diff --git a/pull/github_test.go b/pull/github_test.go index 11b77590..142e8f29 100644 --- a/pull/github_test.go +++ b/pull/github_test.go @@ -24,6 +24,7 @@ import ( "time" "github.com/google/go-github/v82/github" + "github.com/hairyhenderson/go-codeowners" "github.com/shurcooL/githubv4" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" @@ -176,19 +177,19 @@ func TestReviews(t *testing.T) { expectedTime, err := time.Parse(time.RFC3339, "2018-06-27T20:33:26Z") assert.NoError(t, err) - assert.Equal(t, "mhaypenny", reviews[0].Author) + assert.Equal(t, "mhaypenny", reviews[0].Author.Login) assert.Equal(t, expectedTime, reviews[0].CreatedAt) assert.Equal(t, expectedTime, reviews[0].LastEditedAt) assert.Equal(t, ReviewChangesRequested, reviews[0].State) assert.Equal(t, "", reviews[0].Body) - assert.Equal(t, "bkeyes", reviews[1].Author) + assert.Equal(t, "bkeyes", reviews[1].Author.Login) assert.Equal(t, expectedTime.Add(time.Second), reviews[1].CreatedAt) assert.Equal(t, expectedTime.Add(time.Second), reviews[1].LastEditedAt) assert.Equal(t, ReviewApproved, reviews[1].State) assert.Equal(t, "the body", reviews[1].Body) - assert.Equal(t, "jgiannuzzi", reviews[2].Author) + assert.Equal(t, "jgiannuzzi", reviews[2].Author.Login) assert.Equal(t, expectedTime.Add(-4*time.Second).Add(5*time.Minute), reviews[2].CreatedAt) assert.Equal(t, expectedTime.Add(-4*time.Second).Add(5*time.Minute), reviews[2].LastEditedAt) assert.Equal(t, ReviewCommented, reviews[2].State) @@ -240,7 +241,7 @@ func TestBody(t *testing.T) { assert.NoError(t, err) assert.Equal(t, "/no-platform", prBody.Body) - assert.Equal(t, "agirlnamedsophia", prBody.Author) + assert.Equal(t, "agirlnamedsophia", prBody.Author.Login) assert.Equal(t, expectedTime, prBody.CreatedAt) assert.Equal(t, expectedTime, prBody.LastEditedAt) } @@ -263,22 +264,22 @@ func TestComments(t *testing.T) { expectedTime, err := time.Parse(time.RFC3339, "2018-06-27T20:28:22Z") assert.NoError(t, err) - assert.Equal(t, "bkeyes", comments[0].Author) + assert.Equal(t, "bkeyes", comments[0].Author.Login) assert.Equal(t, expectedTime, comments[0].CreatedAt) assert.Equal(t, expectedTime, comments[0].LastEditedAt) assert.Equal(t, ":+1:", comments[0].Body) - assert.Equal(t, "mhaypenny", comments[1].Author) + assert.Equal(t, "mhaypenny", comments[1].Author.Login) assert.Equal(t, expectedTime.Add(30*time.Second), comments[1].CreatedAt) assert.Equal(t, expectedTime.Add(30*time.Second), comments[1].LastEditedAt) assert.Equal(t, "I comment!", comments[1].Body) - assert.Equal(t, "bulldozer[bot]", comments[2].Author) + assert.Equal(t, "bulldozer[bot]", comments[2].Author.Login) assert.Equal(t, expectedTime.Add(time.Minute), comments[2].CreatedAt) assert.Equal(t, expectedTime.Add(time.Minute), comments[2].LastEditedAt) assert.Equal(t, "I merge!", comments[2].Body) - assert.Equal(t, "jgiannuzzi", comments[3].Author) + assert.Equal(t, "jgiannuzzi", comments[3].Author.Login) assert.Equal(t, expectedTime.Add(10*time.Minute), comments[3].CreatedAt) assert.Equal(t, expectedTime.Add(10*time.Minute), comments[3].LastEditedAt) assert.Equal(t, "A review comment", comments[3].Body) @@ -725,7 +726,7 @@ func makeContext(t *testing.T, rp *ResponsePlayer, pr *github.PullRequest, gc Gl base, _ := url.Parse("http://github.localhost/") client.BaseURL = base - mbrCtx := NewGitHubMembershipContext(ctx, client) + mbrCtx := NewGitHubMembershipContext(ctx, client, gc) if pr == nil { pr = defaultTestPR() } @@ -777,12 +778,29 @@ func defaultTestPR() *github.PullRequest { } type MockGlobalCache struct { - PushedAt map[string]time.Time + PushedAt map[string]time.Time + Codeowners map[string]*codeowners.Codeowners + Membership map[string]mockMembershipEntry + Teams map[string]mockTeamMembersEntry +} + +type mockTeamMembersEntry struct { + info *TeamInfo + members []TeamMember + expiresAt time.Time +} + +type mockMembershipEntry struct { + isMember bool + expiresAt time.Time } func NewMockGlobalCache() *MockGlobalCache { return &MockGlobalCache{ - PushedAt: make(map[string]time.Time), + PushedAt: make(map[string]time.Time), + Codeowners: make(map[string]*codeowners.Codeowners), + Membership: make(map[string]mockMembershipEntry), + Teams: make(map[string]mockTeamMembersEntry), } } @@ -794,3 +812,49 @@ func (c *MockGlobalCache) GetPushedAt(repoID int64, sha string) (time.Time, bool func (c *MockGlobalCache) SetPushedAt(repoID int64, sha string, t time.Time) { c.PushedAt[fmt.Sprintf("%d:%s", repoID, sha)] = t } + +func (c *MockGlobalCache) GetCodeowners(repoID int64, baseRefOID string) (*codeowners.Codeowners, bool) { + key := fmt.Sprintf("%d:%s", repoID, baseRefOID) + co, ok := c.Codeowners[key] + return co, ok +} + +func (c *MockGlobalCache) SetCodeowners(repoID int64, baseRefOID string, co *codeowners.Codeowners) { + key := fmt.Sprintf("%d:%s", repoID, baseRefOID) + c.Codeowners[key] = co +} + +func (c *MockGlobalCache) GetTeamMembership(team, user string) (bool, bool) { + key := team + ":" + user + if entry, ok := c.Membership[key]; ok { + if time.Now().Before(entry.expiresAt) { + return entry.isMember, true + } + } + return false, false +} + +func (c *MockGlobalCache) SetTeamMembership(team, user string, isMember bool) { + key := team + ":" + user + c.Membership[key] = mockMembershipEntry{ + isMember: isMember, + expiresAt: time.Now().Add(5 * time.Minute), + } +} + +func (c *MockGlobalCache) GetTeamMembers(team string) ([]TeamMember, *TeamInfo, bool) { + if entry, ok := c.Teams[team]; ok { + if time.Now().Before(entry.expiresAt) { + return entry.members, entry.info, true + } + } + return nil, nil, false +} + +func (c *MockGlobalCache) SetTeamMembers(team string, info *TeamInfo, members []TeamMember) { + c.Teams[team] = mockTeamMembersEntry{ + info: info, + members: members, + expiresAt: time.Now().Add(5 * time.Minute), + } +} diff --git a/pull/global_cache.go b/pull/global_cache.go index 0eeb5a9b..31160d54 100644 --- a/pull/global_cache.go +++ b/pull/global_cache.go @@ -18,9 +18,26 @@ import ( "fmt" "time" + "github.com/hairyhenderson/go-codeowners" lru "github.com/hashicorp/golang-lru" ) +const DefaultMembershipTTL = 5 * time.Minute + +// TeamMember represents a GitHub user with basic metadata +type TeamMember struct { + Login string + AvatarURL string + HTMLURL string +} + +// TeamInfo represents team metadata +type TeamInfo struct { + ID int64 + Slug string + HTMLURL string +} + // GlobalCache implementations provide a way to cache values that are safe to // cache at the application level. Values in the global cache should not become // stale due to external changes and should only expire to prevent the cache @@ -28,21 +45,70 @@ import ( type GlobalCache interface { GetPushedAt(repoID int64, sha string) (time.Time, bool) SetPushedAt(repoID int64, sha string, t time.Time) + + // GetCodeowners returns the cached parsed CODEOWNERS for a repository at a + // specific base branch commit. Since commit SHAs are immutable, caching the + // parsed content is safe and avoids repeated HTTP requests. + GetCodeowners(repoID int64, baseRefOID string) (*codeowners.Codeowners, bool) + SetCodeowners(repoID int64, baseRefOID string, co *codeowners.Codeowners) + + // GetTeamMembership returns cached team membership status. + // Returns (isMember, found). If found is false, the value is not in cache. + GetTeamMembership(team, user string) (bool, bool) + SetTeamMembership(team, user string, isMember bool) + + // GetTeamMembers returns cached team members with metadata. + // Returns (members, info, found). If found is false, the value is not in cache. + GetTeamMembers(team string) ([]TeamMember, *TeamInfo, bool) + SetTeamMembers(team string, info *TeamInfo, members []TeamMember) +} + +type membershipEntry struct { + isMember bool + expiresAt time.Time +} + +type teamMembersEntry struct { + info *TeamInfo + members []TeamMember + expiresAt time.Time } // LRUGlobalCache is a GlobalCache where each data type is stored in a separate // LRU cache. This prevents frequently used data of one type from evicting less // frequently used data of a different type. type LRUGlobalCache struct { - pushedAt *lru.Cache + pushedAt *lru.Cache + codeowners *lru.Cache + membership *lru.Cache + teams *lru.Cache + memberTTL time.Duration } -func NewLRUGlobalCache(pushedAtSize int) (*LRUGlobalCache, error) { +func NewLRUGlobalCache(pushedAtSize, codeownersSize, membershipSize, teamsSize int) (*LRUGlobalCache, error) { pushedAt, err := lru.New(pushedAtSize) if err != nil { return nil, err } - return &LRUGlobalCache{pushedAt: pushedAt}, nil + codeownersCache, err := lru.New(codeownersSize) + if err != nil { + return nil, err + } + membership, err := lru.New(membershipSize) + if err != nil { + return nil, err + } + teams, err := lru.New(teamsSize) + if err != nil { + return nil, err + } + return &LRUGlobalCache{ + pushedAt: pushedAt, + codeowners: codeownersCache, + membership: membership, + teams: teams, + memberTTL: DefaultMembershipTTL, + }, nil } func (c *LRUGlobalCache) GetPushedAt(repoID int64, sha string) (time.Time, bool) { @@ -61,3 +127,61 @@ func (c *LRUGlobalCache) SetPushedAt(repoID int64, sha string, t time.Time) { func pushedAtKey(repoID int64, sha string) string { return fmt.Sprintf("%d:%s", repoID, sha) } + +func (c *LRUGlobalCache) GetCodeowners(repoID int64, baseRefOID string) (*codeowners.Codeowners, bool) { + key := fmt.Sprintf("%d:%s", repoID, baseRefOID) + if val, ok := c.codeowners.Get(key); ok { + if co, ok := val.(*codeowners.Codeowners); ok { + return co, true + } + } + return nil, false +} + +func (c *LRUGlobalCache) SetCodeowners(repoID int64, baseRefOID string, co *codeowners.Codeowners) { + key := fmt.Sprintf("%d:%s", repoID, baseRefOID) + c.codeowners.Add(key, co) +} + +func (c *LRUGlobalCache) GetTeamMembership(team, user string) (bool, bool) { + key := team + ":" + user + if val, ok := c.membership.Get(key); ok { + if entry, ok := val.(membershipEntry); ok { + if time.Now().Before(entry.expiresAt) { + return entry.isMember, true + } + // Expired - remove and return not found + c.membership.Remove(key) + } + } + return false, false +} + +func (c *LRUGlobalCache) SetTeamMembership(team, user string, isMember bool) { + key := team + ":" + user + c.membership.Add(key, membershipEntry{ + isMember: isMember, + expiresAt: time.Now().Add(c.memberTTL), + }) +} + +func (c *LRUGlobalCache) GetTeamMembers(team string) ([]TeamMember, *TeamInfo, bool) { + if val, ok := c.teams.Get(team); ok { + if entry, ok := val.(teamMembersEntry); ok { + if time.Now().Before(entry.expiresAt) { + return entry.members, entry.info, true + } + // Expired - remove and return not found + c.teams.Remove(team) + } + } + return nil, nil, false +} + +func (c *LRUGlobalCache) SetTeamMembers(team string, info *TeamInfo, members []TeamMember) { + c.teams.Add(team, teamMembersEntry{ + info: info, + members: members, + expiresAt: time.Now().Add(c.memberTTL), + }) +} diff --git a/pull/pulltest/context.go b/pull/pulltest/context.go index 8a5a0217..116b087b 100644 --- a/pull/pulltest/context.go +++ b/pull/pulltest/context.go @@ -81,6 +81,9 @@ type Context struct { RepositoryCustomPropertiesValue map[string]pull.CustomProperty RepositoryCustomPropertiesError error + CodeownersValue *pull.CodeownersResult + CodeownersError error + Draft bool } @@ -239,6 +242,36 @@ func (c *Context) TeamMembers(team string) ([]string, error) { return inverted[team], nil } +func (c *Context) TeamMembersWithDetails(team string) ([]pull.TeamMember, error) { + if c.TeamMembershipError != nil { + return nil, c.TeamMembershipError + } + + members, err := c.TeamMembers(team) + if err != nil { + return nil, err + } + + result := make([]pull.TeamMember, len(members)) + for i, login := range members { + result[i] = pull.TeamMember{Login: login} + } + return result, nil +} + +func (c *Context) TeamInfo(team string) (*pull.TeamInfo, error) { + if c.TeamMembershipError != nil { + return nil, c.TeamMembershipError + } + + // Return basic team info from the team string + return &pull.TeamInfo{ + ID: 0, + Slug: team, + HTMLURL: "", + }, nil +} + func (c *Context) RequestedReviewers() ([]*pull.Reviewer, error) { return c.RequestedReviewersValue, c.RequestedReviewersError } @@ -271,5 +304,9 @@ func (c *Context) RepositoryCustomProperties() (map[string]pull.CustomProperty, return c.RepositoryCustomPropertiesValue, c.RepositoryCustomPropertiesError } +func (c *Context) Codeowners() (*pull.CodeownersResult, error) { + return c.CodeownersValue, c.CodeownersError +} + // assert that the test object implements the full interface var _ pull.Context = &Context{} diff --git a/server/assets/css/main.css b/server/assets/css/main.css index d6ae32b6..ce05e806 100644 --- a/server/assets/css/main.css +++ b/server/assets/css/main.css @@ -154,6 +154,26 @@ details summary > * { .status-stripe.skipped { @apply border-gray3; } .status-stripe.error { @apply border-red3; } +.status-dot { + @apply inline-block w-2 h-2 rounded-full mr-1; + vertical-align: middle; +} +.status-dot.approved { @apply bg-green3; } +.status-dot.pending { @apply bg-orange3; } + +.ownership-groups { + @apply flex flex-col gap-1 mt-2; +} +.ownership-group { + @apply flex items-center text-sm; +} +.ownership-group .group-owner { + @apply font-mono text-sm-mono; +} +.ownership-group .group-approvers { + @apply text-dark-gray3 ml-1; +} + .hero { @apply p-8 bg-dark-gray5; background-image: diff --git a/server/config.go b/server/config.go index 6f078d7d..a154f5e9 100644 --- a/server/config.go +++ b/server/config.go @@ -71,6 +71,19 @@ type CachingConfig struct { // The size of the global cache for commit push times. Each entry uses // roughly 100 bytes of memory. PushedAtSize int `yaml:"pushed_at_size"` + + // The size of the global cache for parsed CODEOWNERS content. This caches + // the parsed CODEOWNERS file for a repository at a given base branch commit + // to avoid repeated HTTP requests. + CodeownersSize int `yaml:"codeowners_size"` + + // The size of the global cache for team membership checks. Each entry + // caches the result of a team membership API call with a 5 minute TTL. + MembershipSize int `yaml:"membership_size"` + + // The size of the global cache for team member lists. Each entry caches + // the full list of team members with metadata (avatars, URLs) with a 5 minute TTL. + TeamsSize int `yaml:"teams_size"` } type WorkerConfig struct { diff --git a/server/handler/base.go b/server/handler/base.go index b1bad1fb..4415f471 100644 --- a/server/handler/base.go +++ b/server/handler/base.go @@ -69,7 +69,7 @@ func (b *Base) NewEvalContext(ctx context.Context, installationID int64, loc pul return nil, err } - mbrCtx := NewCrossOrgMembershipContext(ctx, client, loc.Owner, b.Installations, b.ClientCreator) + mbrCtx := NewCrossOrgMembershipContext(ctx, client, loc.Owner, b.Installations, b.ClientCreator, b.GlobalCache) prctx, err := pull.NewGitHubContext(ctx, mbrCtx, b.GlobalCache, client, v4client, loc) if err != nil { return nil, err diff --git a/server/handler/cross_org.go b/server/handler/cross_org.go index 79899949..9d0fee44 100644 --- a/server/handler/cross_org.go +++ b/server/handler/cross_org.go @@ -17,6 +17,7 @@ package handler import ( "context" "strings" + "sync" "github.com/google/go-github/v82/github" "github.com/palantir/go-githubapp/githubapp" @@ -29,50 +30,65 @@ type CrossOrgMembershipContext struct { lookupClient *github.Client installations githubapp.InstallationsService clientCreator githubapp.ClientCreator + globalCache pull.GlobalCache - mbrCtxs map[string]pull.MembershipContext + mbrCtxs sync.Map // map[string]pull.MembershipContext } -func NewCrossOrgMembershipContext(ctx context.Context, client *github.Client, orgName string, installations githubapp.InstallationsService, clientCreator githubapp.ClientCreator) *CrossOrgMembershipContext { +func NewCrossOrgMembershipContext(ctx context.Context, client *github.Client, orgName string, installations githubapp.InstallationsService, clientCreator githubapp.ClientCreator, globalCache pull.GlobalCache) *CrossOrgMembershipContext { mbrCtx := &CrossOrgMembershipContext{ ctx: ctx, lookupClient: client, installations: installations, clientCreator: clientCreator, - mbrCtxs: make(map[string]pull.MembershipContext), + globalCache: globalCache, } - mbrCtx.mbrCtxs[orgName] = pull.NewGitHubMembershipContext(ctx, client) + mbrCtx.mbrCtxs.Store(orgName, pull.NewGitHubMembershipContext(ctx, client, globalCache)) return mbrCtx } +func (c *CrossOrgMembershipContext) getCtxForTeam(team string) (pull.MembershipContext, error) { + return c.getCtxForOrg(strings.Split(team, "/")[0]) +} + func (c *CrossOrgMembershipContext) getCtxForOrg(name string) (pull.MembershipContext, error) { - mbrCtx, ok := c.mbrCtxs[name] - if !ok { - org, _, err := c.lookupClient.Organizations.Get(c.ctx, name) - if err != nil { - return nil, errors.WithStack(err) - } - - installation, err := c.installations.GetByOwner(c.ctx, org.GetLogin()) - if err != nil { - return nil, errors.Wrapf(err, "failed to lookup installation ID for org '%s' or there is no such installation", name) - } - - client, err := c.clientCreator.NewInstallationClient(installation.ID) - if err != nil { - return nil, err - } - - mbrCtx = pull.NewGitHubMembershipContext(c.ctx, client) - c.mbrCtxs[name] = mbrCtx + if val, ok := c.mbrCtxs.Load(name); ok { + return val.(pull.MembershipContext), nil + } + + // Create a new membership context for this org + mbrCtx, err := c.createMembershipContext(name) + if err != nil { + return nil, err + } + + // Use LoadOrStore to handle concurrent creation - if another goroutine + // already stored a context, use that one instead + actual, _ := c.mbrCtxs.LoadOrStore(name, mbrCtx) + return actual.(pull.MembershipContext), nil +} + +func (c *CrossOrgMembershipContext) createMembershipContext(name string) (pull.MembershipContext, error) { + org, _, err := c.lookupClient.Organizations.Get(c.ctx, name) + if err != nil { + return nil, errors.WithStack(err) + } + + installation, err := c.installations.GetByOwner(c.ctx, org.GetLogin()) + if err != nil { + return nil, errors.Wrapf(err, "failed to lookup installation ID for org '%s' or there is no such installation", name) + } + + client, err := c.clientCreator.NewInstallationClient(installation.ID) + if err != nil { + return nil, err } - return mbrCtx, nil + return pull.NewGitHubMembershipContext(c.ctx, client, c.globalCache), nil } func (c *CrossOrgMembershipContext) IsTeamMember(team, user string) (bool, error) { - org := strings.Split(team, "/")[0] - mbrCtx, err := c.getCtxForOrg(org) + mbrCtx, err := c.getCtxForTeam(team) if err != nil { return false, err } @@ -96,10 +112,25 @@ func (c *CrossOrgMembershipContext) OrganizationMembers(org string) ([]string, e } func (c *CrossOrgMembershipContext) TeamMembers(team string) ([]string, error) { - org := strings.Split(team, "/")[0] - mbrCtx, err := c.getCtxForOrg(org) + mbrCtx, err := c.getCtxForTeam(team) if err != nil { return nil, err } return mbrCtx.TeamMembers(team) } + +func (c *CrossOrgMembershipContext) TeamMembersWithDetails(team string) ([]pull.TeamMember, error) { + mbrCtx, err := c.getCtxForTeam(team) + if err != nil { + return nil, err + } + return mbrCtx.TeamMembersWithDetails(team) +} + +func (c *CrossOrgMembershipContext) TeamInfo(team string) (*pull.TeamInfo, error) { + mbrCtx, err := c.getCtxForTeam(team) + if err != nil { + return nil, err + } + return mbrCtx.TeamInfo(team) +} diff --git a/server/handler/details.go b/server/handler/details.go index 9cca4e23..c4121722 100644 --- a/server/handler/details.go +++ b/server/handler/details.go @@ -74,6 +74,8 @@ func (h *Details) ServeHTTP(w http.ResponseWriter, r *http.Request) error { PullRequest *github.PullRequest Result *common.Result + Codeowners *pull.CodeownersResult + PullContext pull.Context } data.BasePath = getBasePath(h.BaseConfig.PublicURL) @@ -81,6 +83,7 @@ func (h *Details) ServeHTTP(w http.ResponseWriter, r *http.Request) error { data.PolicyURL = getPolicyURL(state.PullRequest, evalCtx.Config) data.ExpandRequiredReviewers = h.PullOpts.ExpandRequiredReviewers data.PullRequest = state.PullRequest + data.PullContext = evalCtx.PullContext evaluator, err := evalCtx.ParseConfig(ctx, common.TriggerAll) if err != nil { @@ -102,6 +105,11 @@ func (h *Details) ServeHTTP(w http.ResponseWriter, r *http.Request) error { data.Error = err } + // Fetch codeowners data for display in the UI + if evalCtx.PullContext != nil { + data.Codeowners, _ = evalCtx.PullContext.Codeowners() + } + // Intentionally skip evalCtx.RunPostEvaluateActions() for details // evaluations to minimize side-effects when viewing policy status. These // actions are best-effort, so if they were missed by normal event diff --git a/server/handler/details_reviewers.go b/server/handler/details_reviewers.go index b840bcc5..75373744 100644 --- a/server/handler/details_reviewers.go +++ b/server/handler/details_reviewers.go @@ -17,6 +17,7 @@ package handler import ( "net/http" "slices" + "strings" "github.com/google/go-github/v82/github" "github.com/palantir/policy-bot/policy" @@ -29,13 +30,96 @@ type DetailsReviewers struct { Details } +type ReviewerInfo struct { + Name string // Username/login + Username string // Secondary identifier (usually same as Name) + Link string + AvatarURL string +} + +type ReviewerGroup struct { + Name string // Group display name (team slug, org name, or "Other") + Reviewers []ReviewerInfo // Reviewers in this group +} + type DetailsReviewersData struct { PullRequest *github.PullRequest - Reviewers []string + Groups []ReviewerGroup Incomplete bool } +// reviewerCollector collects reviewers grouped by source. +type reviewerCollector struct { + groups map[string]*reviewerGroupData + seen map[string]struct{} // track all seen usernames to avoid duplicates +} + +type reviewerGroupData struct { + label string + reviewers []ReviewerInfo +} + +func newReviewerCollector() *reviewerCollector { + return &reviewerCollector{ + groups: make(map[string]*reviewerGroupData), + seen: make(map[string]struct{}), + } +} + +func (c *reviewerCollector) add(groupKey, groupLabel, name, link, avatarURL string) { + if _, ok := c.seen[name]; ok { + return + } + c.seen[name] = struct{}{} + + group := c.groups[groupKey] + if group == nil { + group = &reviewerGroupData{label: groupLabel} + c.groups[groupKey] = group + } + + group.reviewers = append(group.reviewers, ReviewerInfo{ + Name: name, + Username: name, + Link: link, + AvatarURL: avatarURL, + }) +} + +func (c *reviewerCollector) toGroups() []ReviewerGroup { + // Collect and sort group keys by label, then key for stability. + groupKeys := make([]string, 0, len(c.groups)) + for key := range c.groups { + groupKeys = append(groupKeys, key) + } + slices.SortFunc(groupKeys, func(a, b string) int { + if c.groups[a].label == c.groups[b].label { + return strings.Compare(a, b) + } + return strings.Compare(c.groups[a].label, c.groups[b].label) + }) + + // Build sorted groups + result := make([]ReviewerGroup, 0, len(c.groups)) + for _, groupKey := range groupKeys { + group := c.groups[groupKey] + reviewers := group.reviewers + slices.SortFunc(reviewers, func(a, b ReviewerInfo) int { + return strings.Compare(a.Name, b.Name) + }) + result = append(result, ReviewerGroup{ + Name: group.label, + Reviewers: reviewers, + }) + } + return result +} + func (h *DetailsReviewers) ServeHTTP(w http.ResponseWriter, r *http.Request) error { + if !h.PullOpts.ExpandRequiredReviewers { + return h.renderEmptyReviewers(w, r, nil) + } + state := h.getStateIfAllowed(w, r) if state == nil { return nil @@ -49,17 +133,14 @@ func (h *DetailsReviewers) ServeHTTP(w http.ResponseWriter, r *http.Request) err switch { case config.LoadError != nil: logger.Warn().Err(config.LoadError).Msgf("Error loading policy from %s, reviewers will be incomplete", config.Source) - return h.renderReviewers(w, r, DetailsReviewersData{ - PullRequest: state.PullRequest, - Incomplete: true, - }) + return h.renderReviewers(w, r, DetailsReviewersData{PullRequest: state.PullRequest, Incomplete: true}) case config.Config == nil: // The repository either has no policy or the policy was invalid. This // should never happen in normal use, because the expected way to hit // this endpoint requires viewing a valid details page first. Hitting // this case means something unexpected is hitting the API directly. - return h.renderReviewers(w, r, DetailsReviewersData{PullRequest: state.PullRequest}) + return h.renderEmptyReviewers(w, r, state.PullRequest) } ruleName := r.URL.Query().Get("rule") @@ -68,14 +149,18 @@ func (h *DetailsReviewers) ServeHTTP(w http.ResponseWriter, r *http.Request) err if requires == nil || requires.Count == 0 || requires.Actors.IsZero() { // If the rule does not exist, it does not require approval, or it has // no actors specified, there's no need to list reviewers - return h.renderReviewers(w, r, DetailsReviewersData{PullRequest: state.PullRequest}) + return h.renderEmptyReviewers(w, r, state.PullRequest) } - var reviewers []string - var incomplete bool + collector := newReviewerCollector() + incomplete := false - // Add direct users - reviewers = append(reviewers, requires.Actors.Users...) + githubURL := defaultGitHubURL(evalctx.PublicURL) + + // Add direct users (group: "Users") + for _, user := range requires.Actors.Users { + collector.add("Users", "Users", user, githubURL+"/"+user, "") + } // Add organization members for _, org := range requires.Actors.Organizations { @@ -84,18 +169,23 @@ func (h *DetailsReviewers) ServeHTTP(w http.ResponseWriter, r *http.Request) err logger.Warn().Err(err).Str("organization", org).Msg("Error listing organization members, reviewers will be incomplete") incomplete = true } else { - reviewers = append(reviewers, members...) + for _, m := range members { + collector.add(org, org, m, githubURL+"/"+m, "") + } } } - // Add team members + // Add team members with details (avatars, display names) for _, team := range requires.Actors.Teams { - members, err := prctx.TeamMembers(team) + groupKey, groupLabel := teamGroupKeyLabel(team) + members, err := prctx.TeamMembersWithDetails(team) if err != nil { logger.Warn().Err(err).Str("team", team).Msg("Error listing team members, reviewers will be incomplete") incomplete = true } else { - reviewers = append(reviewers, members...) + for _, m := range members { + collector.add(groupKey, groupLabel, m.Login, m.HTMLURL, m.AvatarURL) + } } } @@ -109,23 +199,51 @@ func (h *DetailsReviewers) ServeHTTP(w http.ResponseWriter, r *http.Request) err } else { for _, user := range userCollaborators { if userHasReviewerPermission(user, perms) { - reviewers = append(reviewers, user.Name) + collector.add("Collaborators", "Collaborators", user.Name, githubURL+"/"+user.Name, "") } } } } - // Order the reviewers and remove any duplicates - slices.Sort(reviewers) - reviewers = slices.Compact(reviewers) + // Add codeowners (both direct users and team members) + if requires.Actors.Codeowners { + co, err := prctx.Codeowners() + if err != nil { + logger.Warn().Err(err).Msg("Error loading codeowners, reviewers will be incomplete") + incomplete = true + } else if co != nil { + for _, owner := range co.AllOwners() { + ownerType, name := pull.ParseCodeowner(owner) + switch ownerType { + case "user": + collector.add("Codeowners", "Codeowners", name, githubURL+"/"+name, "") + case "team": + groupKey, groupLabel := teamGroupKeyLabel(name) + members, err := prctx.TeamMembersWithDetails(name) + if err != nil { + logger.Warn().Err(err).Str("team", name).Msg("Error listing codeowner team members, reviewers will be incomplete") + incomplete = true + } else { + for _, m := range members { + collector.add(groupKey, groupLabel, m.Login, m.HTMLURL, m.AvatarURL) + } + } + } + } + } + } return h.renderReviewers(w, r, DetailsReviewersData{ PullRequest: state.PullRequest, - Reviewers: reviewers, + Groups: collector.toGroups(), Incomplete: incomplete, }) } +func (h *DetailsReviewers) renderEmptyReviewers(w http.ResponseWriter, r *http.Request, pr *github.PullRequest) error { + return h.renderReviewers(w, r, DetailsReviewersData{PullRequest: pr}) +} + func (h *DetailsReviewers) renderReviewers(w http.ResponseWriter, r *http.Request, data DetailsReviewersData) error { tmpl, ok := h.Templates["details_reviewers.html.tmpl"] if !ok { @@ -158,3 +276,19 @@ func findRuleRequires(config *policy.Config, ruleName string) *approval.Requires } return nil } + +func defaultGitHubURL(publicURL string) string { + if publicURL == "" { + return "https://github.com" + } + return publicURL +} + +// teamGroupKeyLabel returns the key (full team name) and label (team slug only) +// for grouping reviewers by team. For "org/team-slug", returns ("org/team-slug", "team-slug"). +func teamGroupKeyLabel(team string) (key, label string) { + if idx := strings.LastIndex(team, "/"); idx != -1 { + return team, team[idx+1:] + } + return team, team +} diff --git a/server/handler/eval_context.go b/server/handler/eval_context.go index 708b06e1..6eb64878 100644 --- a/server/handler/eval_context.go +++ b/server/handler/eval_context.go @@ -145,7 +145,7 @@ func (ec *EvalContext) EvaluatePolicy(ctx context.Context, evaluator common.Eval case common.StatusDisapproved: statusState = "failure" case common.StatusPending: - statusState = "pending" + statusState = "failure" case common.StatusSkipped: statusState = "error" statusDescription = "All rules were skipped. At least one rule must match." @@ -183,14 +183,13 @@ func (ec *EvalContext) PostStatus(ctx context.Context, state, message string) { owner := ec.PullContext.RepositoryOwner() repo := ec.PullContext.RepositoryName() sha := ec.PullContext.HeadSHA() - base, _ := ec.PullContext.Branches() publicURL := strings.TrimSuffix(ec.PublicURL, "/") detailsURL := fmt.Sprintf("%s/details/%s/%s/%d", publicURL, owner, repo, ec.PullContext.Number()) status := github.RepoStatus{ State: &state, - Context: new(fmt.Sprintf("%s: %s", ec.Options.StatusCheckContext, base)), + Context: github.Ptr(fmt.Sprintf("%s", ec.Options.StatusCheckContext)), Description: &message, TargetURL: &detailsURL, } diff --git a/server/handler/eval_context_dismissal.go b/server/handler/eval_context_dismissal.go index 1e3cd380..d4dbb90a 100644 --- a/server/handler/eval_context_dismissal.go +++ b/server/handler/eval_context_dismissal.go @@ -36,7 +36,7 @@ func (ec *EvalContext) dismissStaleReviewsForResult(ctx context.Context, result } if !ec.Options.StrictReviewDismissal { // Only dismiss reviews from users who are not currently approvers - if approvers[d.Candidate.User] { + if approvers[d.Candidate.User()] { continue } } @@ -73,7 +73,7 @@ func findAllApprovers(result *common.Result) map[string]bool { if len(result.Children) == 0 && result.Error == nil { for _, a := range result.Requires.Approvers { - approvers[a.User] = true + approvers[a.User()] = true } } for _, c := range result.Children { diff --git a/server/handler/eval_context_reviewers.go b/server/handler/eval_context_reviewers.go index b95e7e27..3fded3e1 100644 --- a/server/handler/eval_context_reviewers.go +++ b/server/handler/eval_context_reviewers.go @@ -104,10 +104,12 @@ func (ec *EvalContext) requestReviews(ctx context.Context, reqs []*common.Result } for _, r := range reviews { if r.SHA == head { - reviewers = append(reviewers, &pull.Reviewer{ - Type: pull.ReviewerUser, - Name: r.Author, - }) + if r.Author != nil && r.Author.Login != "" { + reviewers = append(reviewers, &pull.Reviewer{ + Type: pull.ReviewerUser, + Name: r.Author.Login, + }) + } for _, team := range r.Teams { reviewers = append(reviewers, &pull.Reviewer{ diff --git a/server/handler/frontend.go b/server/handler/frontend.go index 00e6709c..8951bef5 100644 --- a/server/handler/frontend.go +++ b/server/handler/frontend.go @@ -29,6 +29,7 @@ import ( "github.com/bluekeyes/templatetree" "github.com/palantir/policy-bot/policy/common" + "github.com/palantir/policy-bot/pull" "github.com/pkg/errors" ) @@ -45,8 +46,10 @@ type FilesConfig struct { } type Membership struct { - Name string - Link string + Name string // Secondary identifier or last-resort label + Username string // Preferred visible label (e.g., "org/team-slug" or "username") + Link string + AvatarURL string } func LoadTemplates(c *FilesConfig, basePath string, githubURL string) (templatetree.Tree[*template.Template], error) { @@ -108,7 +111,38 @@ func LoadTemplates(c *FilesConfig, basePath string, githubURL string) (templatet return r }, "hasActors": func(requires common.RequiresResult) bool { - return len(requires.Actors.Users) > 0 || len(requires.Actors.Teams) > 0 || len(requires.Actors.Organizations) > 0 + return len(requires.Actors.Users) > 0 || + len(requires.Actors.Teams) > 0 || + len(requires.Actors.Organizations) > 0 || + requires.Actors.Codeowners + }, + "hasCodeowners": func(requires common.RequiresResult) bool { + return requires.Actors.Codeowners + }, + "hasOwnershipGroups": func(requires common.RequiresResult) bool { + return len(requires.OwnershipGroups) > 0 + }, + "getOwnershipGroups": func(requires common.RequiresResult) []common.OwnershipGroupResult { + return requires.OwnershipGroups + }, + "getEnrichedOwnershipGroups": func(prctx pull.Context, requires common.RequiresResult) []EnrichedOwnershipGroup { + return getEnrichedOwnershipGroups(prctx, requires, githubURL) + }, + "getCodeownersBreakdown": func(co *pull.CodeownersResult) map[string][]string { + if co == nil { + return nil + } + return co.OwnersByOwner() + }, + "codeownerLink": func(owner string) string { + ownerType, name := pull.ParseCodeowner(owner) + if ownerType == "team" { + parts := strings.SplitN(name, "/", 2) + if len(parts) == 2 { + return githubURL + "/orgs/" + parts[0] + "/teams/" + parts[1] + } + } + return githubURL + "/" + name }, "getMethods": func(results *common.Result) map[string][]string { return getMethods(results) @@ -209,15 +243,15 @@ func getActors(result *common.Result, githubURL string) map[string][]Membership membershipInfo := make(map[string][]Membership) for _, org := range result.Requires.Actors.Organizations { - membershipInfo[orgKey] = append(membershipInfo[orgKey], Membership{Name: org, Link: githubURL + "/orgs/" + org + "/people"}) + membershipInfo[orgKey] = append(membershipInfo[orgKey], Membership{Name: org, Username: org, Link: githubURL + "/orgs/" + org + "/people"}) } for _, team := range result.Requires.Actors.Teams { teamName := strings.Split(team, "/") - membershipInfo[teamKey] = append(membershipInfo[teamKey], Membership{Name: team, Link: githubURL + "/orgs/" + teamName[0] + "/teams/" + teamName[1] + "/members"}) + membershipInfo[teamKey] = append(membershipInfo[teamKey], Membership{Name: team, Username: team, Link: githubURL + "/orgs/" + teamName[0] + "/teams/" + teamName[1] + "/members"}) } for _, user := range result.Requires.Actors.Users { - membershipInfo[userKey] = append(membershipInfo[userKey], Membership{Name: user, Link: githubURL + "/" + user}) + membershipInfo[userKey] = append(membershipInfo[userKey], Membership{Name: user, Username: user, Link: githubURL + "/" + user}) } return membershipInfo } @@ -230,3 +264,95 @@ func getPermissions(result *common.Result) []string { } return permStrings } + +// EnrichedOwnershipGroup contains enriched ownership group data for display. +type EnrichedOwnershipGroup struct { + Key string + Files []string + Satisfied bool + Owners []Membership + Approvers []Membership +} + +func getEnrichedOwnershipGroups(prctx pull.Context, requires common.RequiresResult, githubURL string) []EnrichedOwnershipGroup { + approversByLogin := buildApproversMap(requires.Approvers) + teamInfoCache := make(map[string]*pull.TeamInfo) + + result := make([]EnrichedOwnershipGroup, 0, len(requires.OwnershipGroups)) + for _, group := range requires.OwnershipGroups { + enriched := EnrichedOwnershipGroup{ + Key: group.Key, + Files: group.Files, + Satisfied: group.Satisfied, + Owners: enrichOwners(group.Owners, githubURL, prctx, teamInfoCache), + Approvers: enrichApprovers(group.Approvers, approversByLogin, githubURL), + } + result = append(result, enriched) + } + + return result +} + +func buildApproversMap(approvers []*common.Candidate) map[string]*common.Candidate { + m := make(map[string]*common.Candidate, len(approvers)) + for _, c := range approvers { + m[c.User()] = c + } + return m +} + +func enrichOwners(owners []string, githubURL string, prctx pull.Context, teamInfoCache map[string]*pull.TeamInfo) []Membership { + result := make([]Membership, 0, len(owners)) + for _, owner := range owners { + ownerType, name := pull.ParseCodeowner(owner) + m := Membership{Name: name, Username: name} + + switch ownerType { + case "team": + parts := strings.Split(name, "/") + if len(parts) == 2 { + org, team := parts[0], parts[1] + m.Link = githubURL + "/orgs/" + org + "/teams/" + team + if prctx != nil { + info, ok := teamInfoCache[name] + if !ok { + info, _ = prctx.TeamInfo(name) + teamInfoCache[name] = info + } + if info != nil && info.ID != 0 { + m.AvatarURL = teamAvatarURL(info.ID) + } + } + if m.AvatarURL == "" { + m.AvatarURL = githubURL + "/" + org + ".png" + } + } + case "user": + m.Link = githubURL + "/" + name + m.AvatarURL = githubURL + "/" + name + ".png" + } + + result = append(result, m) + } + return result +} + +func teamAvatarURL(teamID int64) string { + return fmt.Sprintf("https://avatars.githubusercontent.com/t/%d?s=116&v=4", teamID) +} + +func enrichApprovers(approverNames []string, approversByLogin map[string]*common.Candidate, githubURL string) []Membership { + result := make([]Membership, 0, len(approverNames)) + for _, name := range approverNames { + m := Membership{ + Name: name, + Username: name, + Link: githubURL + "/" + name, + } + if candidate, ok := approversByLogin[name]; ok && candidate.Author != nil { + m.AvatarURL = candidate.Author.AvatarURL + } + result = append(result, m) + } + return result +} diff --git a/server/handler/installation.go b/server/handler/installation.go index 0811694b..f715f2f1 100644 --- a/server/handler/installation.go +++ b/server/handler/installation.go @@ -98,7 +98,7 @@ func (h *Installation) postRepoInstallationStatus(ctx context.Context, client *g } head := branch.GetCommit().GetSHA() - contextWithBranch := fmt.Sprintf("%s: %s", h.PullOpts.StatusCheckContext, defaultBranch) + contextWithBranch := fmt.Sprintf("%s", h.PullOpts.StatusCheckContext) state := "success" message := fmt.Sprintf("%s successfully installed.", h.AppName) status := github.RepoStatus{ diff --git a/server/handler/simulate.go b/server/handler/simulate.go index 2e4ba15a..ed6f3cd4 100644 --- a/server/handler/simulate.go +++ b/server/handler/simulate.go @@ -150,7 +150,7 @@ func (h *Simulate) newSimulatedContext(ctx context.Context, installationID int64 return nil, nil, err } - mbrCtx := NewCrossOrgMembershipContext(ctx, client, loc.Owner, h.Installations, h.ClientCreator) + mbrCtx := NewCrossOrgMembershipContext(ctx, client, loc.Owner, h.Installations, h.ClientCreator, h.GlobalCache) prctx, err := pull.NewGitHubContext(ctx, mbrCtx, h.GlobalCache, client, v4client, loc) if err != nil { return nil, nil, err diff --git a/server/server.go b/server/server.go index 1fab1f1c..c01922c3 100644 --- a/server/server.go +++ b/server/server.go @@ -48,8 +48,11 @@ const ( DefaultWebhookWorkers = 10 DefaultWebhookQueueSize = 100 - DefaultHTTPCacheSize = 50 * datasize.MB - DefaultPushedAtCacheSize = 100_000 + DefaultHTTPCacheSize = 50 * datasize.MB + DefaultPushedAtCacheSize = 100_000 + DefaultCodeownersCacheSize = 10_000 + DefaultMembershipCacheSize = 10_000 + DefaultTeamsCacheSize = 1_000 ) type Server struct { @@ -138,12 +141,12 @@ func New(c *Config) (*Server, error) { return nil, errors.Wrap(err, "failed to get configured GitHub app") } - pushedAtSize := c.Cache.PushedAtSize - if pushedAtSize == 0 { - pushedAtSize = DefaultPushedAtCacheSize - } + pushedAtSize := cacheSize(c.Cache.PushedAtSize, DefaultPushedAtCacheSize) + codeownersSize := cacheSize(c.Cache.CodeownersSize, DefaultCodeownersCacheSize) + membershipSize := cacheSize(c.Cache.MembershipSize, DefaultMembershipCacheSize) + teamsSize := cacheSize(c.Cache.TeamsSize, DefaultTeamsCacheSize) - globalCache, err := pull.NewLRUGlobalCache(pushedAtSize) + globalCache, err := pull.NewLRUGlobalCache(pushedAtSize, codeownersSize, membershipSize, teamsSize) if err != nil { return nil, errors.Wrap(err, "failed to initialize global cache") } @@ -287,3 +290,11 @@ func (s *Server) Start() error { } return s.base.Start() } + +// cacheSize returns the configured size or the default if not set. +func cacheSize(configured, defaultSize int) int { + if configured > 0 { + return configured + } + return defaultSize +} diff --git a/server/templates/details.html.tmpl b/server/templates/details.html.tmpl index d8b07b2f..5706fe63 100644 --- a/server/templates/details.html.tmpl +++ b/server/templates/details.html.tmpl @@ -67,7 +67,7 @@ {{ $showReviewers := (and $root.ExpandRequiredReviewers (eq $s "pending")) }}
  • - {{template "result-details" .}} + {{template "result-details" (args $root .)}} {{if (or (.PredicateResults) (hasActors .Requires) (hasActorsPermissions .Requires) (gt (len .Requires.Conditions) 0))}}
    - {{template "result-approver-details" .}} + {{template "result-approver-details" (args $root .)}}
    {{template "result-methods-details" .}} @@ -127,6 +127,8 @@ {{end}} {{define "result-details"}} + {{ $root := index . 0 }} + {{with index . 1}} {{ $s := (or (and .Error "error") (.Status | print)) }}

    {{.Name}} @@ -135,7 +137,36 @@ {{if (and .Description (ne $s "skipped"))}}

    {{.Description}}

    {{end}} -

    {{or .Error .StatusDescription}}

    + {{if hasOwnershipGroups .Requires}} + {{template "ownership-groups" (args $root .Requires)}} + {{else}} +

    {{or .Error .StatusDescription}}

    + {{end}} + {{end}} +{{end}} + +{{define "ownership-groups"}} + {{ $root := index . 0 }} + {{with index . 1}} +
    + {{range getEnrichedOwnershipGroups $root.PullContext .}} + {{$status := "pending"}}{{if .Satisfied}}{{$status = "approved"}}{{end}} +
    + + {{with index .Owners 0}} + {{- template "component/actor" . -}} + {{end}} + {{if .Satisfied}} + + ({{range $i, $a := .Approvers}}{{if $i}}, {{end -}} + {{- template "component/actor" $a -}} + {{- end}}) + + {{end}} +
    + {{end}} +
    + {{end}} {{end}} {{define "result-predicates-details"}} @@ -194,8 +225,11 @@ {{end}} {{define "result-approver-details"}} + {{ $root := index . 0 }} + {{with index . 1}} {{$hasActors := hasActors .Requires}} {{$hasPerms := hasActorsPermissions .Requires}} + {{$hasCodeowners := hasCodeowners .Requires}} {{if or $hasActors $hasPerms}} {{template "result-reviews-count" .Requires}} from:
    @@ -207,6 +241,31 @@ {{end}} {{end}} + {{if $hasCodeowners}} +
    Codeowners:
    +
    + {{if $root.Codeowners}} + {{range $owner, $files := getCodeownersBreakdown $root.Codeowners}} +
    + {{$owner}} +
      + {{range $files}}
    • {{.}}
      • {{end}} +
    +
    + {{end}} + {{if $root.Codeowners.HasOrphanFiles}} +
    + No codeowners: +
      + {{range $root.Codeowners.OrphanFiles}}
    • {{.}}
      • {{end}} +
    +
    + {{end}} + {{else}} +

    Unable to load codeowners

    + {{end}} +
    + {{end}} {{if $hasPerms}}
    Users with the permissions:
    @@ -217,6 +276,7 @@ {{else}} {{template "result-reviews-count" .Requires}} {{end}} + {{end}} {{end}} {{define "result-reviews-count"}}This rule requires at least {{.Count}} approval{{if gt .Count 1}}s{{end}}{{end}} diff --git a/server/templates/details_reviewers.html.tmpl b/server/templates/details_reviewers.html.tmpl index c3c2d812..16f15634 100644 --- a/server/templates/details_reviewers.html.tmpl +++ b/server/templates/details_reviewers.html.tmpl @@ -1,12 +1,21 @@ {{/* templatetree:extends page.html.tmpl */}} -{{define "title"}}{{.PullRequest.GetBase.GetRepo.GetFullName}}#{{.PullRequest.GetNumber}} - Reviewers | PolicyBot{{end}} +{{define "title"}}{{if .PullRequest}}{{.PullRequest.GetBase.GetRepo.GetFullName}}#{{.PullRequest.GetNumber}} - Reviewers | {{end}}PolicyBot{{end}} {{define "body"}} -
      - {{- range .Reviewers}} -
    • {{.}}
      • - {{- else}} -
    • No reviewers found
      • +{{- if .Groups}} + {{- range .Groups}} +
      +
      {{.Name}}:
      +
      + {{- range .Reviewers -}} + {{- template "component/actor" . -}} + {{- end}} +
      +
      {{- end}} -
    -{{if .Incomplete}}

    Due to an error, the reviewer list may be incomplete

    {{end}} +{{- else}} +

    No reviewers found

    +{{- end}} +{{if .Incomplete}} +

    Due to an error, the reviewer list may be incomplete

    +{{end}} {{end}} diff --git a/server/templates/page.html.tmpl b/server/templates/page.html.tmpl index d4fe9e72..d0ab0030 100644 --- a/server/templates/page.html.tmpl +++ b/server/templates/page.html.tmpl @@ -15,3 +15,33 @@ {{block "body" .}}{{end}} + +{{/* + Reusable UI components. + + Note: templatetree parses templates per-file (with inheritance), so shared + components must live in a parent template like this one. +*/}} + +{{/* + component/actor + + Renders either a user or a team identity. Expected fields (all optional except Link/Name): + - AvatarURL string + - Link string + - Username string (preferred visible label; often username/slug) + - Name string (secondary identifier or last-resort label) +*/}} +{{define "component/actor"}} + {{- $display := .Name -}} + {{- if .Username -}} + {{- $display = .Username -}} + {{- end -}} + + + {{- if .AvatarURL -}} + {{$display}} + {{- end -}} + {{$display}} + +{{end}} diff --git a/vendor/github.com/hairyhenderson/go-codeowners/.golangci.yml b/vendor/github.com/hairyhenderson/go-codeowners/.golangci.yml new file mode 100644 index 00000000..393d1c36 --- /dev/null +++ b/vendor/github.com/hairyhenderson/go-codeowners/.golangci.yml @@ -0,0 +1,66 @@ +linters-settings: + govet: + enable-all: true + gocyclo: + min-complexity: 10 + dupl: + threshold: 100 + goconst: + min-len: 2 + min-occurrences: 4 + lll: + line-length: 140 + nolintlint: + allow-unused: false # report any unused nolint directives + require-explanation: false # don't require an explanation for nolint directives + require-specific: false # don't require nolint directives to be specific about which linter is being skipped + +linters: + disable-all: true + enable: + - asciicheck + - bodyclose + - copyloopvar + # - dogsled + - dupl + - errcheck + - exhaustive + - funlen + - gci + - gochecknoglobals + - gochecknoinits + - gocognit + - goconst + - gocritic + - gocyclo + - godox + - gofmt + # - gofumpt + - goheader + - goimports + # - gomnd + - gomodguard + - goprintffuncname + - gosec + - gosimple + - govet + - ineffassign + - lll + - misspell + - nakedret + - nestif + # - nlreturn + - noctx + - nolintlint + - prealloc + - revive + - rowserrcheck + - sqlclosecheck + - staticcheck + - stylecheck + - typecheck + - unconvert + - unparam + - unused + - whitespace + # - wsl diff --git a/vendor/github.com/hairyhenderson/go-codeowners/.release-please-manifest.json b/vendor/github.com/hairyhenderson/go-codeowners/.release-please-manifest.json new file mode 100644 index 00000000..77c0d292 --- /dev/null +++ b/vendor/github.com/hairyhenderson/go-codeowners/.release-please-manifest.json @@ -0,0 +1,3 @@ +{ + ".": "0.7.0" +} diff --git a/vendor/github.com/hairyhenderson/go-codeowners/CHANGELOG.md b/vendor/github.com/hairyhenderson/go-codeowners/CHANGELOG.md new file mode 100644 index 00000000..ac35ef70 --- /dev/null +++ b/vendor/github.com/hairyhenderson/go-codeowners/CHANGELOG.md @@ -0,0 +1,33 @@ +# Changelog + +## [0.7.0](https://github.com/hairyhenderson/go-codeowners/compare/v0.6.1...v0.7.0) (2024-12-07) + + +### Features + +* add PatternIndex method ([#48](https://github.com/hairyhenderson/go-codeowners/issues/48)) ([a5c2e59](https://github.com/hairyhenderson/go-codeowners/commit/a5c2e593dcbe38fa52c24c95a72d1ca3f5e47ee8)) + + +### Dependencies + +* **go:** Bump github.com/stretchr/testify from 1.9.0 to 1.10.0 ([#49](https://github.com/hairyhenderson/go-codeowners/issues/49)) ([4f55176](https://github.com/hairyhenderson/go-codeowners/commit/4f551769326fc31237d06b29a4a0497c78bd882f)) + +## [0.6.1](https://github.com/hairyhenderson/go-codeowners/compare/v0.6.0...v0.6.1) (2024-10-25) + + +### Bug Fixes + +* **parsing:** Check for CODEOWNERS files for both GitHub and GitLab ([#45](https://github.com/hairyhenderson/go-codeowners/issues/45)) ([db9c01e](https://github.com/hairyhenderson/go-codeowners/commit/db9c01eb61a0e5975521721a102e8c54b6dc2876)), closes [#44](https://github.com/hairyhenderson/go-codeowners/issues/44) +* **parsing:** Ignore block and inline comments ([#46](https://github.com/hairyhenderson/go-codeowners/issues/46)) ([968c9ea](https://github.com/hairyhenderson/go-codeowners/commit/968c9eaf0924c1912731d2729f26d2c691b8d4b1)) + +## [0.6.0](https://github.com/hairyhenderson/go-codeowners/compare/v0.5.0...v0.6.0) (2024-09-27) + + +### Features + +* **errors:** Allow checking codeowners file not found ([#41](https://github.com/hairyhenderson/go-codeowners/issues/41)) ([d659b73](https://github.com/hairyhenderson/go-codeowners/commit/d659b73a08c1a1111c5e9c4c1136472c8ca28a4b)) + + +### Bug Fixes + +* **perf:** Reduce allocations ([#39](https://github.com/hairyhenderson/go-codeowners/issues/39)) ([2ca66e0](https://github.com/hairyhenderson/go-codeowners/commit/2ca66e0194d2b9077c63a9d1eace8f1083675fa9)) diff --git a/vendor/github.com/hairyhenderson/go-codeowners/LICENSE b/vendor/github.com/hairyhenderson/go-codeowners/LICENSE new file mode 100644 index 00000000..80265d0a --- /dev/null +++ b/vendor/github.com/hairyhenderson/go-codeowners/LICENSE @@ -0,0 +1,21 @@ +The MIT License (MIT) + +Copyright (c) 2018-2024 Dave Henderson + +Permission is hereby granted, free of charge, to any person obtaining a copy +of this software and associated documentation files (the “Software”), to deal +in the Software without restriction, including without limitation the rights +to use, copy, modify, merge, publish, distribute, sublicense, and/or sell +copies of the Software, and to permit persons to whom the Software is +furnished to do so, subject to the following conditions: + +The above copyright notice and this permission notice shall be included in +all copies or substantial portions of the Software. + +THE SOFTWARE IS PROVIDED “AS IS”, WITHOUT WARRANTY OF ANY KIND, EXPRESS OR +IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, +FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE +AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER +LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, +OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN +THE SOFTWARE. diff --git a/vendor/github.com/hairyhenderson/go-codeowners/README.md b/vendor/github.com/hairyhenderson/go-codeowners/README.md new file mode 100644 index 00000000..798a3ba3 --- /dev/null +++ b/vendor/github.com/hairyhenderson/go-codeowners/README.md @@ -0,0 +1,48 @@ +[![Go Reference][doc-image]][docs] +[![Build][build-image]][build-url] + +# go-codeowners + +A package that finds and parses [`CODEOWNERS`](https://help.github.com/articles/about-codeowners/) files. + +Features: +- operates on local repos +- doesn't require a cloned repo (i.e. doesn't need a `.git` directory to be + present at the repo's root) +- can be called from within a repo (doesn't have to be at the root) +- will find `CODEOWNERS` files in all documented locations: the repo's root, + `docs/`, and `.github/` (or `.gitlab/` for GitLab repos) + +## Usage + +```console +go get -u github.com/hairyhenderson/go-codeowners +``` + +To find the owner of the README.md file: + +```go +import "github.com/hairyhenderson/go-codeowners" + +func main() { + c, _ := FromFile(cwd()) + owners := c.Owners("README.md") + for i, o := range owners { + fmt.Printf("Owner #%d is %s\n", i, o) + } +} +``` + +See the [docs][] for more information. + +## License + +[The MIT License](http://opensource.org/licenses/MIT) + +Copyright (c) 2018-2023 Dave Henderson + +[docs]: https://pkg.go.dev/github.com/hairyhenderson/go-codeowners +[doc-image]: https://pkg.go.dev/badge/github.com/hairyhenderson/go-codeowners.svg + +[build-image]: https://github.com/hairyhenderson/go-codeowners/actions/workflows/build.yml/badge.svg +[build-url]: https://github.com/hairyhenderson/go-codeowners/actions/workflows/build.yml diff --git a/vendor/github.com/hairyhenderson/go-codeowners/codeowners.go b/vendor/github.com/hairyhenderson/go-codeowners/codeowners.go new file mode 100644 index 00000000..3a5c7b2f --- /dev/null +++ b/vendor/github.com/hairyhenderson/go-codeowners/codeowners.go @@ -0,0 +1,303 @@ +package codeowners + +import ( + "bufio" + "errors" + "fmt" + "io" + "io/fs" + "os" + "path" + "path/filepath" + "regexp" + "strings" +) + +// ErrNoCodeownersFound is returned when no CODEOWNERS file is found +var ErrNoCodeownersFound = errors.New("no CODEOWNERS found") + +// Codeowners - patterns/owners mappings for the given repo +type Codeowners struct { + repoRoot string + Patterns []Codeowner +} + +// Codeowner - owners for a given pattern +type Codeowner struct { + Pattern string + re *regexp.Regexp + Owners []string +} + +func (c Codeowner) String() string { + return fmt.Sprintf("%s\t%v", c.Pattern, strings.Join(c.Owners, ", ")) +} + +func dirExists(fsys fs.FS, path string) (bool, error) { + fi, err := fs.Stat(fsys, path) + if err == nil && fi.IsDir() { + return true, nil + } + + if errors.Is(err, fs.ErrNotExist) { + return false, nil + } + + return false, err +} + +// findCodeownersFile - find a CODEOWNERS file somewhere within or below +// the working directory (wd), and open it. +func findCodeownersFile(fsys fs.FS, wd string) (io.Reader, string, error) { + dir := wd + for { + for _, p := range []string{".github", ".", "docs", ".gitlab"} { + pth := path.Join(dir, p) + exists, err := dirExists(fsys, pth) + if err != nil { + return nil, "", err + } + if exists { + f := path.Join(pth, "CODEOWNERS") + _, err := fs.Stat(fsys, f) + if err != nil { + if errors.Is(err, fs.ErrNotExist) { + continue + } + return nil, "", err + } + r, err := fsys.Open(f) + return r, dir, err + } + } + odir := dir + dir = path.Dir(odir) + // if we can't go up any further... + if odir == dir { + break + } + // if we're heading above the volume name (relevant on Windows)... + if len(dir) < len(filepath.VolumeName(odir)) { + break + } + } + return nil, "", nil +} + +// Deprecated: Use [FromFile] instead. +func NewCodeowners(path string) (*Codeowners, error) { + return FromFile(path) +} + +// FromFile creates a Codeowners from the path to a local file. Consider using +// [FromFileWithFS] instead. +func FromFile(path string) (*Codeowners, error) { + base := "/" + if filepath.IsAbs(path) && filepath.VolumeName(path) != "" { + base = path[:len(filepath.VolumeName(path))+1] + } + path = path[len(base):] + + return FromFileWithFS(os.DirFS(base), path) +} + +// FromFileWithFS creates a Codeowners from the path to a file relative to the +// given filesystem. +func FromFileWithFS(fsys fs.FS, path string) (*Codeowners, error) { + r, root, err := findCodeownersFile(fsys, path) + if err != nil { + return nil, err + } + if r == nil { + return nil, fmt.Errorf("%w in %s", ErrNoCodeownersFound, path) + } + return FromReader(r, root) +} + +// FromReader creates a Codeowners from a given Reader instance and root path. +func FromReader(r io.Reader, repoRoot string) (*Codeowners, error) { + co := &Codeowners{ + repoRoot: repoRoot, + } + patterns, err := parseCodeowners(r) + if err != nil { + return nil, err + } + co.Patterns = patterns + return co, nil +} + +func isSection(line string) bool { + return strings.HasPrefix(line, "^") || strings.HasPrefix(line, "[") +} + +// parseCodeowners parses a list of Codeowners from a Reader +func parseCodeowners(r io.Reader) ([]Codeowner, error) { + co := []Codeowner{} + s := bufio.NewScanner(r) + var defaultOwners []string + for s.Scan() { + line := s.Text() + if isSection(line) { + defaultOwners = parseDefaultOwners(line) + continue + } + fields := strings.Fields(line) + + fields = removeComments(fields) + if len(fields) == 0 { + continue + } + if len(fields) > 1 { + fields = combineEscapedSpaces(fields) + c, err := NewCodeowner(fields[0], fields[1:]) + if err != nil { + return nil, err + } + co = append(co, c) + } else if len(fields) == 1 && defaultOwners != nil { + c, err := NewCodeowner(fields[0], defaultOwners) + if err != nil { + return nil, err + } + co = append(co, c) + } + } + return co, nil +} + +func removeComments(fields []string) []string { + for i := range fields { + if strings.HasPrefix(fields[i], "#") { + fields = fields[:i] + break + } + } + return fields +} + +func parseDefaultOwners(line string) []string { + index := strings.LastIndex(line, "]") + if index != -1 && len(line) > index+1 { + return strings.Fields(strings.TrimSpace(line[index+1:])) + } + return nil +} + +// if any of the elements ends with a \, it was an escaped space +// put it back together properly so it's not treated as separate fields +func combineEscapedSpaces(fields []string) []string { + outFields := make([]string, 0) + escape := `\` + for i := 0; i < len(fields); i++ { + outField := fields[i] + for strings.HasSuffix(fields[i], escape) && i+1 < len(fields) { + outField = strings.Join([]string{strings.TrimRight(outField, escape), fields[i+1]}, " ") + i++ + } + outFields = append(outFields, outField) + } + + return outFields +} + +// NewCodeowner - +func NewCodeowner(pattern string, owners []string) (Codeowner, error) { + re, err := getPattern(pattern) + if err != nil { + return Codeowner{}, err + } + c := Codeowner{ + Pattern: pattern, + re: re, + Owners: owners, + } + return c, nil +} + +// Owners - return the list of code owners for the given path +// (within the repo root) +func (c *Codeowners) Owners(path string) []string { + if i := c.PatternIndex(path); i >= 0 { + return c.Patterns[i].Owners + } + return nil +} + +// PatternIndex - return the index of the pattern that matches the given path +// (within the repo root) +func (c *Codeowners) PatternIndex(path string) int { + if strings.HasPrefix(path, c.repoRoot) { + path = strings.Replace(path, c.repoRoot, "", 1) + } + + // Order is important; the last matching pattern takes the most precedence. + for i := len(c.Patterns) - 1; i >= 0; i-- { + if c.Patterns[i].re.MatchString(path) { + return i + } + } + + return -1 +} + +// precompile all regular expressions +var ( + rePrependSlash = regexp.MustCompile(`([^\/+])/.*\*\.`) +) + +// based on github.com/sabhiram/go-gitignore +// but modified so that 'dir/*' only matches files in 'dir/' +func getPattern(line string) (*regexp.Regexp, error) { + // when # or ! is escaped with a \ + if strings.HasPrefix(line, `\#`) || strings.HasPrefix(line, `\!`) { + line = line[1:] + } + + // If we encounter a foo/*.blah in a folder, prepend the / char + if rePrependSlash.MatchString(line) && line[0] != '/' { + line = "/" + line + } + + // Handle escaping the "." char + line = strings.ReplaceAll(line, ".", `\.`) + + magicStar := "#$~" + + // Handle "/**/" usage + if strings.HasPrefix(line, "/**/") { + line = line[1:] + } + line = strings.ReplaceAll(line, `/**/`, `(/|/.+/)`) + line = strings.ReplaceAll(line, `**/`, `(|.`+magicStar+`/)`) + line = strings.ReplaceAll(line, `/**`, `(|/.`+magicStar+`)`) + + // Handle escaping the "*" char + line = strings.ReplaceAll(line, `\*`, `\`+magicStar) + line = strings.ReplaceAll(line, `*`, `([^/]*)`) + + // Handle escaping the "?" char + line = strings.ReplaceAll(line, "?", `\?`) + + line = strings.ReplaceAll(line, magicStar, "*") + + // Temporary regex + expr := "" + + switch { + case strings.HasSuffix(line, "/"): + expr = line + "(|.*)$" + case strings.HasSuffix(line, "/([^/]*)"): + expr = line + "$" + default: + expr = line + "($|/.*$)" + } + + if strings.HasPrefix(expr, "/") { + expr = "^(|/)" + expr[1:] + } else { + expr = "^(|.*/)" + expr + } + + return regexp.Compile(expr) +} diff --git a/vendor/github.com/hairyhenderson/go-codeowners/release-please-config.json b/vendor/github.com/hairyhenderson/go-codeowners/release-please-config.json new file mode 100644 index 00000000..b803e307 --- /dev/null +++ b/vendor/github.com/hairyhenderson/go-codeowners/release-please-config.json @@ -0,0 +1,41 @@ +{ + "$schema": "https://raw.githubusercontent.com/googleapis/release-please/main/schemas/config.json", + "release-type": "go", + "changelog-type": "default", + "changelog-sections": [ + { + "type": "feat", + "section": "Features" + }, + { + "type": "fix", + "section": "Bug Fixes" + }, + { + "type": "docs", + "section": "Documentation" + }, + { + "type": "deps", + "section": "Dependencies" + }, + { + "type": "chore", + "section": "Miscellaneous Chores", + "hidden": true + }, + { + "type": "build", + "section": "Build System", + "hidden": true + }, + { + "type": "ci", + "section": "Continuous Integration", + "hidden": true + } + ], + "packages": { + ".": {} + } +} diff --git a/vendor/modules.txt b/vendor/modules.txt index add69476..8ec4a697 100644 --- a/vendor/modules.txt +++ b/vendor/modules.txt @@ -54,6 +54,9 @@ github.com/google/go-querystring/query # github.com/gregjones/httpcache v0.0.0-20190611155906-901d90724c79 ## explicit github.com/gregjones/httpcache +# github.com/hairyhenderson/go-codeowners v0.7.0 +## explicit; go 1.22.0 +github.com/hairyhenderson/go-codeowners # github.com/hashicorp/golang-lru v1.0.2 ## explicit; go 1.12 github.com/hashicorp/golang-lru