From 1d9e903a9a6bfb6be6561935de330f9b672e1a27 Mon Sep 17 00:00:00 2001 From: Jordan Davidson Date: Thu, 10 Apr 2025 12:56:43 -0600 Subject: [PATCH 1/8] fix(build): Initial Patch to fix build for usemotion --- .github/dependabot.yml | 14 -------- .github/workflows/build.yml | 22 ++++-------- .github/workflows/codeql-analysis.yml | 49 --------------------------- godel/config/dist-plugin.yml | 8 ++--- 4 files changed, 9 insertions(+), 84 deletions(-) delete mode 100644 .github/dependabot.yml delete mode 100644 .github/workflows/codeql-analysis.yml 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/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 From 3bb99d00f7f82390acacc354bca40a0ca88c44b2 Mon Sep 17 00:00:00 2001 From: Jordan Davidson Date: Wed, 9 Apr 2025 11:49:36 -0600 Subject: [PATCH 2/8] patch(commit-status): Remove branch from status name --- server/handler/eval_context.go | 3 +-- server/handler/installation.go | 2 +- 2 files changed, 2 insertions(+), 3 deletions(-) diff --git a/server/handler/eval_context.go b/server/handler/eval_context.go index 708b06e1..b618e860 100644 --- a/server/handler/eval_context.go +++ b/server/handler/eval_context.go @@ -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/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{ From 7e06a141946c376531acbe9750129ba63a79c145 Mon Sep 17 00:00:00 2001 From: Jordan Davidson Date: Fri, 11 Apr 2025 08:54:15 -0600 Subject: [PATCH 3/8] patch(status-state): Don't show pending when unapproved Graphite shows a clock and a spinning wheel whenever there are any pending statuses in github. This is very confusing, as you would expect that you can just wait to solve the problem, but in reality you need to reach out. We should roll this back if having a failure marker is more confusing than having a spinning wheel. --- server/handler/eval_context.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/server/handler/eval_context.go b/server/handler/eval_context.go index b618e860..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." From 258dde6ce82cf559fa1ca617028141b92e00b4e6 Mon Sep 17 00:00:00 2001 From: Denys Vitali Date: Fri, 9 Jan 2026 10:15:52 +0100 Subject: [PATCH 4/8] feat: add support for CODEOWNERS --- .gitignore | 3 + go.mod | 1 + go.sum | 2 + policy/approval/approve.go | 1 + policy/common/actor.go | 47 ++- policy/common/actor_test.go | 96 ++++++ policy/common/result.go | 1 + policy/reviewer/reviewer.go | 80 ++++- pull/codeowners_test.go | 120 +++++++ pull/context.go | 46 +++ pull/github.go | 84 +++++ pull/pulltest/context.go | 7 + .../go-codeowners/.golangci.yml | 66 ++++ .../.release-please-manifest.json | 3 + .../hairyhenderson/go-codeowners/CHANGELOG.md | 33 ++ .../hairyhenderson/go-codeowners/LICENSE | 21 ++ .../hairyhenderson/go-codeowners/README.md | 48 +++ .../go-codeowners/codeowners.go | 303 ++++++++++++++++++ .../go-codeowners/release-please-config.json | 41 +++ vendor/modules.txt | 3 + 20 files changed, 993 insertions(+), 13 deletions(-) create mode 100644 pull/codeowners_test.go create mode 100644 vendor/github.com/hairyhenderson/go-codeowners/.golangci.yml create mode 100644 vendor/github.com/hairyhenderson/go-codeowners/.release-please-manifest.json create mode 100644 vendor/github.com/hairyhenderson/go-codeowners/CHANGELOG.md create mode 100644 vendor/github.com/hairyhenderson/go-codeowners/LICENSE create mode 100644 vendor/github.com/hairyhenderson/go-codeowners/README.md create mode 100644 vendor/github.com/hairyhenderson/go-codeowners/codeowners.go create mode 100644 vendor/github.com/hairyhenderson/go-codeowners/release-please-config.json 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/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/policy/approval/approve.go b/policy/approval/approve.go index 69e6212a..600e9f23 100644 --- a/policy/approval/approve.go +++ b/policy/approval/approve.go @@ -151,6 +151,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, 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/result.go b/policy/common/result.go index ce0ca64b..251af3f1 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 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/pull/codeowners_test.go b/pull/codeowners_test.go new file mode 100644 index 00000000..ac23a329 --- /dev/null +++ b/pull/codeowners_test.go @@ -0,0 +1,120 @@ +// 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) + }) +} diff --git a/pull/context.go b/pull/context.go index eb1ac4c3..9b19490e 100644 --- a/pull/context.go +++ b/pull/context.go @@ -15,6 +15,7 @@ package pull import ( + "strings" "time" ) @@ -137,6 +138,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 @@ -267,3 +272,44 @@ 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 +} + +// 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 +} + +// 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 +} diff --git a/pull/github.go b/pull/github.go index 0eb6b35a..55e301a3 100644 --- a/pull/github.go +++ b/pull/github.go @@ -23,6 +23,7 @@ import ( "time" "github.com/google/go-github/v82/github" + "github.com/hairyhenderson/go-codeowners" "github.com/pkg/errors" "github.com/shurcooL/githubv4" ) @@ -146,6 +147,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 @@ -959,6 +964,85 @@ 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 + + // Try to fetch CODEOWNERS from standard locations + var co *codeowners.Codeowners + for _, path := range codeownersLocations { + content, exists, err := ghc.getFileContent(path) + if err != nil { + ghc.codeownersErr = errors.Wrapf(err, "failed to fetch %s", path) + return nil, ghc.codeownersErr + } + if !exists { + continue + } + + co, err = codeowners.FromReader(strings.NewReader(content), "") + if err != nil { + ghc.codeownersErr = errors.Wrapf(err, "failed to parse %s", path) + return nil, ghc.codeownersErr + } + break + } + + // 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 + } + } + + ghc.codeownersResult = result + return result, nil +} + +func (ghc *GitHubContext) getFileContent(path string) (string, bool, error) { + file, _, _, err := ghc.client.Repositories.GetContents( + ghc.ctx, ghc.owner, ghc.repo, path, + &github.RepositoryContentGetOptions{Ref: ghc.pr.HeadRefOID}, + ) + 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 diff --git a/pull/pulltest/context.go b/pull/pulltest/context.go index 8a5a0217..a25ec8ba 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 } @@ -271,5 +274,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/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 From 0ff7a34abcf78d2fd2e0e9d62ae844eeaa76a7ed Mon Sep 17 00:00:00 2001 From: Denys Vitali Date: Fri, 9 Jan 2026 11:49:35 +0100 Subject: [PATCH 5/8] docs: add example config --- README.md | 6 ++++++ config/policy-examples/codeowners-approval.yml | 13 +++++++++++++ 2 files changed, 19 insertions(+) create mode 100644 config/policy-examples/codeowners-approval.yml 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 From 7e68a82213f893f8d23a2147cbeed660bc08777e Mon Sep 17 00:00:00 2001 From: Denys Vitali Date: Thu, 15 Jan 2026 15:04:27 +0100 Subject: [PATCH 6/8] feat(codeowners): require review from all codeowners, improve UI --- policy/approval/approve.go | 231 +++++++++-- policy/approval/approve_test.go | 392 ++++++++++++++++++- policy/common/methods.go | 22 +- policy/common/methods_test.go | 48 +-- policy/common/result.go | 23 ++ policy/disapproval/disapprove.go | 10 +- policy/disapproval/disapprove_test.go | 14 +- policy/simulated/context.go | 36 +- policy/simulated/context_test.go | 66 ++-- policy/simulated/options.go | 4 +- pull/codeowners_test.go | 134 +++++++ pull/context.go | 111 +++++- pull/github.go | 157 ++++++-- pull/github_membership.go | 315 ++++++++++++--- pull/github_test.go | 86 +++- pull/global_cache.go | 130 +++++- pull/pulltest/context.go | 30 ++ server/assets/css/main.css | 20 + server/config.go | 13 + server/handler/base.go | 2 +- server/handler/cross_org.go | 87 ++-- server/handler/details.go | 8 + server/handler/details_reviewers.go | 174 +++++++- server/handler/eval_context_dismissal.go | 4 +- server/handler/eval_context_reviewers.go | 10 +- server/handler/frontend.go | 138 ++++++- server/handler/simulate.go | 2 +- server/server.go | 25 +- server/templates/details.html.tmpl | 66 +++- server/templates/details_reviewers.html.tmpl | 25 +- server/templates/page.html.tmpl | 30 ++ 31 files changed, 2094 insertions(+), 319 deletions(-) diff --git a/policy/approval/approve.go b/policy/approval/approve.go index 600e9f23..d9b3acdb 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" @@ -159,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 } @@ -170,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)) @@ -203,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 { @@ -215,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 } @@ -236,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) { @@ -269,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 } @@ -303,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() { @@ -409,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") @@ -435,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 ") } @@ -451,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 { @@ -521,3 +559,140 @@ 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..f90b0707 100644 --- a/policy/approval/approve_test.go +++ b/policy/approval/approve_test.go @@ -47,49 +47,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 +97,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 +513,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 +1008,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: ptr(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/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 251af3f1..2cf3540c 100644 --- a/policy/common/result.go +++ b/policy/common/result.go @@ -93,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/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 index ac23a329..f88a3f8d 100644 --- a/pull/codeowners_test.go +++ b/pull/codeowners_test.go @@ -118,3 +118,137 @@ func TestCodeownersResultAllOwners(t *testing.T) { 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 9b19490e..8dc5ac04 100644 --- a/pull/context.go +++ b/pull/context.go @@ -15,6 +15,7 @@ package pull import ( + "sort" "strings" "time" ) @@ -32,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) } @@ -206,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 } @@ -227,7 +245,7 @@ type Review struct { ID string CreatedAt time.Time LastEditedAt time.Time - Author string + Author *Author State ReviewState Body string SHA string @@ -264,7 +282,7 @@ type CollaboratorPermission struct { type Body struct { Body string CreatedAt time.Time - Author string + Author *Author LastEditedAt time.Time } @@ -278,6 +296,14 @@ 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. @@ -300,6 +326,22 @@ func (c *CodeownersResult) AllOwners() []string { 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: @@ -313,3 +355,66 @@ func ParseCodeowner(owner string) (ownerType string, name string) { } 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 55e301a3..91b5fe52 100644 --- a/pull/github.go +++ b/pull/github.go @@ -20,6 +20,7 @@ import ( "net/http" "slices" "strings" + "sync" "time" "github.com/google/go-github/v82/github" @@ -271,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 } @@ -582,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) { @@ -978,24 +995,11 @@ func (ghc *GitHubContext) Codeowners() (*CodeownersResult, error) { } ghc.codeownersLoaded = true - // Try to fetch CODEOWNERS from standard locations - var co *codeowners.Codeowners - for _, path := range codeownersLocations { - content, exists, err := ghc.getFileContent(path) - if err != nil { - ghc.codeownersErr = errors.Wrapf(err, "failed to fetch %s", path) - return nil, ghc.codeownersErr - } - if !exists { - continue - } - - co, err = codeowners.FromReader(strings.NewReader(content), "") - if err != nil { - ghc.codeownersErr = errors.Wrapf(err, "failed to parse %s", path) - return nil, ghc.codeownersErr - } - break + // 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 @@ -1017,6 +1021,8 @@ func (ghc *GitHubContext) Codeowners() (*CodeownersResult, error) { owners := co.Owners(f.Filename) if len(owners) > 0 { result.Owners[f.Filename] = owners + } else { + result.OrphanFiles = append(result.OrphanFiles, f.Filename) } } @@ -1024,10 +1030,74 @@ func (ghc *GitHubContext) Codeowners() (*CodeownersResult, error) { return result, nil } -func (ghc *GitHubContext) getFileContent(path string) (string, bool, error) { +// 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: ghc.pr.HeadRefOID}, + &github.RepositoryContentGetOptions{Ref: ref}, ) if err != nil { if isNotFound(err) { @@ -1194,6 +1264,7 @@ type v4PullRequest struct { Owner v4Actor } + BaseRefOID string BaseRefName string BaseRepository struct { DatabaseID int64 @@ -1254,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, @@ -1266,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, } } @@ -1282,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, } } @@ -1350,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 @@ -1366,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 a25ec8ba..116b087b 100644 --- a/pull/pulltest/context.go +++ b/pull/pulltest/context.go @@ -242,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 } 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_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/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}} From 2516c39cc02734d2fb56d6504950c5345dc71081 Mon Sep 17 00:00:00 2001 From: Denys Vitali Date: Mon, 19 Jan 2026 09:51:37 +0100 Subject: [PATCH 7/8] chore: godelw verify --- policy/approval/approve.go | 1 - 1 file changed, 1 deletion(-) diff --git a/policy/approval/approve.go b/policy/approval/approve.go index d9b3acdb..9922b027 100644 --- a/policy/approval/approve.go +++ b/policy/approval/approve.go @@ -695,4 +695,3 @@ func (r *Rule) isApprovedByCodeownerGroups( return satisfiedCount == len(results), approvers, results, nil } - From 1030278cd29a0f7e44641f2c9078c5615120a9f7 Mon Sep 17 00:00:00 2001 From: Jordan Davidson Date: Mon, 30 Mar 2026 14:42:22 -0600 Subject: [PATCH 8/8] test: add ptr helper in approval tests --- policy/approval/approve_test.go | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/policy/approval/approve_test.go b/policy/approval/approve_test.go index f90b0707..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(), } @@ -1243,7 +1247,7 @@ func TestCodeownerGroupApproval(t *testing.T) { r := &Rule{ Options: Options{ Methods: DefaultMethods(), - AllowAuthor: ptr(false), + AllowAuthor: new(false), }, Requires: Requires{ Actors: common.Actors{Codeowners: true},