Skip to content
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
36 changes: 36 additions & 0 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -261,6 +261,10 @@ disable_review_status_comments = false
# `admin_bypass` allows repository administrators to bypass codeowner requirements
[admin_bypass]
# see "Admin Bypass" below for more details

# `approval_retention` allows you to specify which kinds of changes may keep an existing approval
[approval_retention]
# see "Approval Retention" below for more details
```

When a PR has any of the `high_priority_labels`, the comment will look like this:
Expand Down Expand Up @@ -330,6 +334,38 @@ Codeowners Plus automatically detects and validates the bypass approval, immedia

The bypass text is case-insensitive, so "codeowners bypass", "Codeowners Bypass", or "CODEOWNERS BYPASS" all work.

#### Approval Retention

The `approval_retention` section lists the kinds of changes which may keep an existing approval instead of dismissing it. `enabled` is an umbrella switch for the whole section: when it is off, nothing in the section applies.

`codeowners.toml`:
```toml
[approval_retention]
# `enabled` (default false) is the umbrella switch for the whole section
enabled = true
# `whitespace` (default follows `enabled`) retains approvals across whitespace-only changes
whitespace = true
# `comments` (default follows `enabled`) retains approvals across comment-only changes
comments = true
# `formatting` (default follows `enabled`) retains approvals across formatting-only changes
formatting = true
# `string_literals` (default false) retains approvals across string literal changes
string_literals = false
# `renames` (default false) retains approvals across renames
renames = false
# `fetch_orphaned_approval` (default false) looks for approvals which are no longer
# attached to the current commit
fetch_orphaned_approval = false
```

Each flag may be left unset, set to `true`, or set to `false`, and an explicit value always wins over the umbrella:

- Umbrella off, nothing else set: every flag is off
- Umbrella on, nothing else set: every flag which follows the umbrella is on
- Umbrella on with `comments = false`: every flag which follows the umbrella is on except `comments`

`string_literals`, `renames` and `fetch_orphaned_approval` are opt-in only - the umbrella never turns them on by itself, and they must be set to `true` explicitly. A change to a string literal or a rename can alter behavior without changing the shape of the code the approver reviewed, so retaining an approval across one is a stronger claim than the other categories. `fetch_orphaned_approval` is the odd one out: it is opt-in because it is the only flag in the section which reaches outside the checkout, and enabling the umbrella should not quietly add network calls to a run.

#### Require Both Branch Reviewers (Ownership Handoffs)

The `require_both_branch_reviewers` feature enables self-service ownership transfers by requiring approval from codeowners defined in **BOTH** the base branch and the PR branch. This creates an AND relationship between ownership rules from both branches.
Expand Down
224 changes: 224 additions & 0 deletions internal/app/approval_retention_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,224 @@
package app

import (
"bytes"
"os"
"os/exec"
"path/filepath"
"slices"
"strings"
"testing"

"github.com/google/go-github/v89/github"
"github.com/multimediallc/codeowners-plus/internal/git"
gh "github.com/multimediallc/codeowners-plus/internal/github"
"github.com/multimediallc/codeowners-plus/pkg/codeowners"
)

// The shared mock approves everything without reading the diff, which is the
// decision under test, so the real staleness check is spliced back in.
type realCheckApprovalsClient struct {
*mockGitHubClient
real gh.Client
dismissed []*gh.CurrentApproval
}

func (c *realCheckApprovalsClient) CheckApprovals(
fileReviewerMap map[string][]string,
approvals []*gh.CurrentApproval,
originalDiff git.Diff,
) ([]codeowners.Slug, []*gh.CurrentApproval) {
return c.real.CheckApprovals(fileReviewerMap, approvals, originalDiff)
}

func (c *realCheckApprovalsClient) DismissStaleReviews(approvals []*gh.CurrentApproval) error {
c.dismissed = append(c.dismissed, approvals...)
return c.mockGitHubClient.DismissStaleReviews(approvals)
}

func runGit(t *testing.T, dir string, args ...string) string {
t.Helper()
cmd := exec.Command("git", args...)
cmd.Dir = dir
cmd.Env = append(os.Environ(), "GIT_CONFIG_GLOBAL=/dev/null", "GIT_CONFIG_SYSTEM=/dev/null")
out, err := cmd.CombinedOutput()
if err != nil {
t.Fatalf("git %s (in %s): %v\n%s", strings.Join(args, " "), dir, err, out)
}
return strings.TrimSpace(string(out))
}

func initRepo(t *testing.T, dir string) {
t.Helper()
runGit(t, dir, "init", "-q", "-b", "main")
runGit(t, dir, "config", "user.email", "test@example.invalid")
runGit(t, dir, "config", "user.name", "Test User")
runGit(t, dir, "config", "commit.gpgsign", "false")
}

func writeRepoFile(t *testing.T, dir, name, content string) {
t.Helper()
path := filepath.Join(dir, name)
if err := os.MkdirAll(filepath.Dir(path), 0o755); err != nil {
t.Fatalf("mkdir for %s: %v", name, err)
}
if err := os.WriteFile(path, []byte(content), 0o644); err != nil {
t.Fatalf("write %s: %v", name, err)
}
}

func commitAll(t *testing.T, dir, message string) string {
t.Helper()
runGit(t, dir, "add", "-A")
runGit(t, dir, "commit", "-q", "-m", message)
return runGit(t, dir, "rev-parse", "HEAD")
}

func runApp(t *testing.T, repoDir, baseSHA, headSHA, approvalSHA string) (*OutputData, []*gh.CurrentApproval, string) {
t.Helper()

warnings := &bytes.Buffer{}
info := &bytes.Buffer{}

realClient, err := gh.NewClient("test-owner", "test-repo", "test-token")
if err != nil {
t.Fatalf("failed to build the real client: %v", err)
}
realClient.SetWarningBuffer(warnings)
realClient.SetInfoBuffer(info)

client := &realCheckApprovalsClient{
mockGitHubClient: &mockGitHubClient{
pr: &github.PullRequest{
Number: github.Ptr(1),
Base: &github.PullRequestBranch{SHA: github.Ptr(baseSHA)},
Head: &github.PullRequestBranch{SHA: github.Ptr(headSHA)},
User: &github.User{Login: github.Ptr("author")},
},
currentApprovals: []*gh.CurrentApproval{{
GHLogin: codeowners.NewSlug("@reviewer"),
ReviewID: 1,
Reviewers: []codeowners.Slug{codeowners.NewSlug("@owner")},
CommitID: approvalSHA,
}},
},
real: realClient,
}

app := &App{
config: &Config{
RepoDir: repoDir,
PR: 1,
Quiet: true,
InfoBuffer: info,
WarningBuffer: warnings,
},
client: client,
}

output, err := app.Run()
if err != nil {
t.Fatalf("app.Run failed: %v\nwarnings: %s", err, warnings)
}
return output, client.dismissed, warnings.String()
}

const retentionBaseSource = `package service

func Alpha() int {
return 1
}

func Beta() int {
return 2
}
`

// retentionApprovedSource is the change the reviewer approved.
const retentionApprovedSource = `package service

func Alpha() int {
return 1
}

func Beta() int {
return 20
}
`

// Adds a comment and nothing else, so a comment is all the reviewer has not seen.
const retentionHeadSource = `package service

// Alpha is the first step.
func Alpha() int {
return 1
}

func Beta() int {
return 20
}
`

// configBody is committed as codeowners.toml on the base ref, which is where the
// application reads its configuration from.
func buildCommentOnlyRepo(t *testing.T, configBody string) (repoDir, baseSHA, headSHA, approvalSHA string) {
t.Helper()
repoDir = t.TempDir()
initRepo(t, repoDir)

writeRepoFile(t, repoDir, ".codeowners", "* @owner\n")
writeRepoFile(t, repoDir, "codeowners.toml", configBody)
writeRepoFile(t, repoDir, "service.go", retentionBaseSource)
baseSHA = commitAll(t, repoDir, "base")

writeRepoFile(t, repoDir, "service.go", retentionApprovedSource)
approvalSHA = commitAll(t, repoDir, "approved change")

writeRepoFile(t, repoDir, "service.go", retentionHeadSource)
headSHA = commitAll(t, repoDir, "comment on top of the approved change")

return repoDir, baseSHA, headSHA, approvalSHA
}

const retentionOffConfig = `disable_review_status_comments = true
`

// The feature is inert until asked for: no section and an all-off section have to
// produce the same bytes.
func TestRunWithoutRetentionSectionIsUnchanged(t *testing.T) {
const explicitlyOff = `disable_review_status_comments = true

[approval_retention]
enabled = false
whitespace = false
comments = false
formatting = false
string_literals = false
renames = false
fetch_orphaned_approval = false
`

repoDir, baseSHA, headSHA, approvalSHA := buildCommentOnlyRepo(t, retentionOffConfig)
absentOutput, absentDismissed, absentWarnings := runApp(t, repoDir, baseSHA, headSHA, approvalSHA)

repoDir, baseSHA, headSHA, approvalSHA = buildCommentOnlyRepo(t, explicitlyOff)
offOutput, offDismissed, offWarnings := runApp(t, repoDir, baseSHA, headSHA, approvalSHA)

if absentOutput.Message != offOutput.Message || absentOutput.Success != offOutput.Success {
t.Errorf("expected identical results, got %+v and %+v", absentOutput, offOutput)
}
if !slices.Equal(absentOutput.StillRequired, offOutput.StillRequired) {
t.Errorf("expected identical still required, got %v and %v", absentOutput.StillRequired, offOutput.StillRequired)
}
if len(absentDismissed) != len(offDismissed) {
t.Errorf("expected identical dismissals, got %d and %d", len(absentDismissed), len(offDismissed))
}
if absentWarnings != offWarnings {
t.Errorf("expected identical warnings, got %q and %q", absentWarnings, offWarnings)
}
// Both are the pre-feature behavior, not merely equal to each other.
if len(absentDismissed) != 1 || absentOutput.Success {
t.Errorf("expected the approval to be dismissed as it always was, got %d dismissals, success %t",
len(absentDismissed), absentOutput.Success)
}
}
Loading
Loading