Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: minimum reviewers on OWNERS file #1635

Open
wants to merge 20 commits into
base: main
Choose a base branch
from
Open

feat: minimum reviewers on OWNERS file #1635

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
20 commits
Select commit Hold shift + click to select a range
ed3e68d
feat: minimum required reviewers on OWNERS
Skisocks Dec 26, 2024
f7287f4
test: update for minRequiredReviewers
Skisocks Dec 27, 2024
e175225
test: add tests for min required approvers
Skisocks Dec 28, 2024
b6f747e
feat: add required approvers text to message
Skisocks Jan 3, 2025
4fc16e2
test: add updated text to tests
Skisocks Jan 3, 2025
d96925a
refactor: simplify AreFilesApproved
Skisocks Jan 3, 2025
8473da9
fix: use the closest file to the change to get MinReviewers
Skisocks Jan 3, 2025
65062ce
test: add tests cases for MinReviewers
Skisocks Jan 3, 2025
e1459fd
fix: use the changed files to get the min required reviewers
Skisocks Jan 3, 2025
1928d45
feat: only set min reviewers if found in config
Skisocks Jan 5, 2025
4a8664e
fix: escape traverse if no min reviewers found
Skisocks Jan 5, 2025
f95ca28
fix: default to 0 if no min reviewers found, in keeping with current …
Skisocks Jan 5, 2025
0cf1e3f
test: add min reviewers to createFakeRepo
Skisocks Jan 5, 2025
a2fa460
test: approve handler for minimum reviewers
Skisocks Jan 5, 2025
0d5911c
chore: remove redundant function
Skisocks Jan 5, 2025
c242327
fix: default to 1 min reviewer if not found
Skisocks Jan 7, 2025
95470e0
fix: handle case where multiple reviewers but not all owners
Skisocks Jan 15, 2025
57c16b1
fix: text rendering in tests
Skisocks Jan 15, 2025
ef54099
test: add case for 2 required approvals from different OWNERS
Skisocks Jan 16, 2025
d55358e
test: add more coverage of minReviewers tests
Skisocks Jan 20, 2025
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
204 changes: 181 additions & 23 deletions pkg/plugins/approve/approve_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,8 @@ package approve

import (
"fmt"
"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"
"net/url"
"os"
"reflect"
Expand Down Expand Up @@ -147,8 +149,15 @@ func newFakeSCMProviderClient(hasLabel, humanApproved, labelComments bool, files
type fakeRepo struct {
approvers, leafApprovers map[string]sets.String
approverOwners map[string]string
minimumReviewers map[string]int
}

func (fr fakeRepo) MinimumReviewersForFile(path string) int {
if n, ok := fr.minimumReviewers[path]; ok {
return n
}
return 1
}
func (fr fakeRepo) Approvers(path string) sets.String {
return fr.approvers[path]
}
Expand Down Expand Up @@ -244,7 +253,8 @@ Approvers can cancel approval by writing ` + "`/approve cancel`" + ` in a commen
expectComment: true,
expectedComment: `[APPROVALNOTIFIER] This PR is **NOT APPROVED**

This pull-request has been approved by:
This pull-request has been approved by:
The changes made require 1 more approval(s).
To complete the [pull request process](https://git.k8s.io/community/contributors/guide/owners.md#the-code-review-process), please assign **cjwagner**
You can assign the PR to them by writing ` + "`/assign @cjwagner`" + ` in a comment when ready.

Expand Down Expand Up @@ -610,6 +620,7 @@ Approvers can cancel approval by writing `+"`/approve cancel`"+` in a comment
Approval requirements bypassed by manually added approval.

This pull-request has been approved by:
The changes made require 1 more approval(s).

The full list of commands accepted by this bot can be found [here](https://jenkins-x.io/v3/develop/reference/chatops/?repo=org%2Frepo).

Expand Down Expand Up @@ -674,7 +685,8 @@ Approvers can cancel approval by writing ` + "`/approve cancel`" + ` in a commen
comments: []*scm.Comment{
newTestComment("k8s-ci-robot", `[APPROVALNOTIFIER] This PR is **NOT APPROVED**

This pull-request has been approved by:
This pull-request has been approved by:
The changes made require 1 more approval(s).
To complete the [pull request process](https://git.k8s.io/community/contributors/guide/owners.md#the-code-review-process), please assign **alice**
You can assign the PR to them by writing `+"`/assign @alice`"+` in a comment when ready.

Expand Down Expand Up @@ -752,7 +764,8 @@ Approvers can cancel approval by writing ` + "`/approve cancel`" + ` in a commen
expectComment: true,
expectedComment: `[APPROVALNOTIFIER] This PR is **NOT APPROVED**

This pull-request has been approved by:
This pull-request has been approved by:
The changes made require 1 more approval(s).
To complete the [pull request process](https://git.k8s.io/community/contributors/guide/owners.md#the-code-review-process), please assign **cjwagner**
You can assign the PR to them by writing ` + "`/assign @cjwagner`" + ` in a comment when ready.

Expand Down Expand Up @@ -822,7 +835,8 @@ Approvers can cancel approval by writing ` + "`/approve cancel`" + ` in a commen
expectComment: true,
expectedComment: `[APPROVALNOTIFIER] This PR is **NOT APPROVED**

This pull-request has been approved by:
This pull-request has been approved by:
The changes made require 1 more approval(s).
To complete the [pull request process](https://git.k8s.io/community/contributors/guide/owners.md#the-code-review-process), please assign **cjwagner**
You can assign the PR to them by writing ` + "`/assign @cjwagner`" + ` in a comment when ready.

Expand Down Expand Up @@ -860,7 +874,8 @@ Approvers can cancel approval by writing ` + "`/approve cancel`" + ` in a commen
expectComment: true,
expectedComment: `[APPROVALNOTIFIER] This PR is **NOT APPROVED**

This pull-request has been approved by:
This pull-request has been approved by:
The changes made require 1 more approval(s).
To complete the [pull request process](https://git.k8s.io/community/contributors/guide/owners.md#the-code-review-process), please assign **cjwagner**
You can assign the PR to them by writing ` + "`/assign @cjwagner`" + ` in a comment when ready.

Expand Down Expand Up @@ -936,7 +951,8 @@ Approvers can cancel approval by writing ` + "`/approve cancel`" + ` in a commen
expectComment: true,
expectedComment: `[APPROVALNOTIFIER] This PR is **NOT APPROVED**

This pull-request has been approved by:
This pull-request has been approved by:
The changes made require 1 more approval(s).
To complete the [pull request process](https://git.k8s.io/community/contributors/guide/owners.md#the-code-review-process), please assign **cjwagner**
You can assign the PR to them by writing ` + "`/assign @cjwagner`" + ` in a comment when ready.

Expand Down Expand Up @@ -971,7 +987,8 @@ Approvers can cancel approval by writing ` + "`/approve cancel`" + ` in a commen
expectComment: true,
expectedComment: `[APPROVALNOTIFIER] This PR is **NOT APPROVED**

This pull-request has been approved by:
This pull-request has been approved by:
The changes made require 1 more approval(s).
To complete the [pull request process](https://git.k8s.io/community/contributors/guide/owners.md#the-code-review-process), please assign **cjwagner**
You can assign the PR to them by writing ` + "`/assign @cjwagner`" + ` in a comment when ready.

Expand Down Expand Up @@ -1127,24 +1144,172 @@ Approvers can cancel approval by writing ` + "`/approve cancel`" + ` in a commen
</details>
<!-- META={"approvers":["alice"]} -->`,
},
{
name: "2 minimum reviewers - no approvals",
hasLabel: false,
files: []string{"d/d.go"},
comments: []*scm.Comment{},
reviews: []*scm.Review{},
selfApprove: false,
needsIssue: false,
lgtmActsAsApprove: false,
reviewActsAsApprove: false,
githubLinkURL: &url.URL{Scheme: "https", Host: "github.com"},

expectDelete: false,
expectToggle: false,
expectComment: true,
expectedComment: `[APPROVALNOTIFIER] This PR is **NOT APPROVED**

This pull-request has been approved by:
The changes made require 2 more approval(s).
To complete the [pull request process](https://git.k8s.io/community/contributors/guide/owners.md#the-code-review-process), please assign **derek**
You can assign the PR to them by writing ` + "`/assign @derek`" + ` in a comment when ready.

The full list of commands accepted by this bot can be found [here](https://jenkins-x.io/v3/develop/reference/chatops/?repo=org%2Frepo).

<details open>
Needs approval from an approver in each of these files:

- **[d/OWNERS](https://github.com/org/repo/blob/master/d/OWNERS)**

Approvers can indicate their approval by writing ` + "`/approve`" + ` in a comment
Approvers can cancel approval by writing ` + "`/approve cancel`" + ` in a comment
</details>
<!-- META={"approvers":["derek"]} -->`,
},
{
name: "2 minimum reviewers - 1 approval",
hasLabel: false,
files: []string{"d/d.go"},
comments: []*scm.Comment{
newTestComment("derek", "/approve"),
},
reviews: []*scm.Review{},
selfApprove: false,
needsIssue: false,
lgtmActsAsApprove: false,
reviewActsAsApprove: false,
githubLinkURL: &url.URL{Scheme: "https", Host: "github.com"},

expectDelete: false,
expectToggle: false,
expectComment: true,
expectedComment: `[APPROVALNOTIFIER] This PR is **NOT APPROVED**

This pull-request has been approved by: *[derek](<> "Approved")*
The changes made require 1 more approval(s).
To complete the [pull request process](https://git.k8s.io/community/contributors/guide/owners.md#the-code-review-process), please assign **derek**
You can assign the PR to them by writing ` + "`/assign @derek`" + ` in a comment when ready.

The full list of commands accepted by this bot can be found [here](https://jenkins-x.io/v3/develop/reference/chatops/?repo=org%2Frepo).

<details open>
Needs approval from an approver in each of these files:

- ~~[d/OWNERS](https://github.com/org/repo/blob/master/d/OWNERS)~~ [derek]

Approvers can indicate their approval by writing ` + "`/approve`" + ` in a comment
Approvers can cancel approval by writing ` + "`/approve cancel`" + ` in a comment
</details>
<!-- META={"approvers":["derek"]} -->`,
},
{
name: "2 minimum reviewers - 2 approval",
hasLabel: false,
files: []string{"d/d.go"},
comments: []*scm.Comment{
newTestComment("derek", "/approve"),
newTestComment("jerry", "/approve"),
},
reviews: []*scm.Review{},
selfApprove: false,
needsIssue: false,
lgtmActsAsApprove: false,
reviewActsAsApprove: false,
githubLinkURL: &url.URL{Scheme: "https", Host: "github.com"},

expectDelete: false,
expectToggle: true,
expectComment: true,
expectedComment: `[APPROVALNOTIFIER] This PR is **APPROVED**

This pull-request has been approved by: *[derek](<> "Approved")*, *[jerry](<> "Approved")*

The full list of commands accepted by this bot can be found [here](https://jenkins-x.io/v3/develop/reference/chatops/?repo=org%2Frepo).

The pull request process is described [here](https://git.k8s.io/community/contributors/guide/owners.md#the-code-review-process)

<details >
Needs approval from an approver in each of these files:

- ~~[d/OWNERS](https://github.com/org/repo/blob/master/d/OWNERS)~~ [derek,jerry]

Approvers can indicate their approval by writing ` + "`/approve` " + `in a comment
Approvers can cancel approval by writing ` + "`/approve cancel` " + `in a comment
</details>
<!-- META={"approvers":[]} -->`,
},
{
name: "2 minimum reviewers - 1 approval, 1 not registered approval",
hasLabel: false,
files: []string{"d/d.go"},
comments: []*scm.Comment{
newTestComment("derek", "/approve"),
newTestComment("alice", "/approve"),
},
reviews: []*scm.Review{},
selfApprove: false,
needsIssue: false,
lgtmActsAsApprove: false,
reviewActsAsApprove: false,
githubLinkURL: &url.URL{Scheme: "https", Host: "github.com"},

expectDelete: false,
expectToggle: false,
expectComment: true,
expectedComment: `[APPROVALNOTIFIER] This PR is **NOT APPROVED**

This pull-request has been approved by: *[alice](<> "Approved")*, *[derek](<> "Approved")*
To complete the [pull request process](https://git.k8s.io/community/contributors/guide/owners.md#the-code-review-process), please assign **derek**
You can assign the PR to them by writing ` + "`/assign @derek`" + ` in a comment when ready.

The full list of commands accepted by this bot can be found [here](https://jenkins-x.io/v3/develop/reference/chatops/?repo=org%2Frepo).

<details open>
Needs approval from an approver in each of these files:

- ~~[d/OWNERS](https://github.com/org/repo/blob/master/d/OWNERS)~~ [derek]

Approvers can indicate their approval by writing ` + "`/approve`" + ` in a comment
Approvers can cancel approval by writing ` + "`/approve cancel`" + ` in a comment
</details>
<!-- META={"approvers":["derek"]} -->`,
},
}

fr := fakeRepo{
approvers: map[string]sets.String{
"a": sets.NewString("alice"),
"a/b": sets.NewString("alice", "bob"),
"c": sets.NewString("cblecker", "cjwagner"),
"d": sets.NewString("derek", "jerry"),
},
leafApprovers: map[string]sets.String{
"a": sets.NewString("alice"),
"a/b": sets.NewString("bob"),
"c": sets.NewString("cblecker", "cjwagner"),
"d": sets.NewString("derek", "jerry"),
},
approverOwners: map[string]string{
"a/a.go": "a",
"a/aa.go": "a",
"a/b/b.go": "a/b",
"c/c.go": "c",
"d/d.go": "d",
},
minimumReviewers: map[string]int{
"d": 2,
},
}

Expand All @@ -1158,7 +1323,7 @@ Approvers can cancel approval by writing ` + "`/approve cancel`" + ` in a commen

rsa := !test.selfApprove
irs := !test.reviewActsAsApprove
if err := handle(
err := handle(
logrus.WithField("plugin", "approve"),
fakeClient,
fr,
Expand All @@ -1179,9 +1344,8 @@ Approvers can cancel approval by writing ` + "`/approve cancel`" + ` in a commen
author: "cjwagner",
assignees: []scm.User{{Login: "spxtr"}},
},
); err != nil {
t.Errorf("[%s] Unexpected error handling event: %v.", test.name, err)
}
)
require.NoError(t, err)

fakeLabel := fmt.Sprintf("org/repo#%v:approved", prNumber)

Expand Down Expand Up @@ -1223,12 +1387,7 @@ Approvers can cancel approval by writing ` + "`/approve cancel`" + ` in a commen
len(fspc.PullRequestCommentsAdded),
)
} else if expect, got := fmt.Sprintf("org/repo#%v:", prNumber)+test.expectedComment, fspc.PullRequestCommentsAdded[0]; test.expectedComment != "" && got != expect {
t.Errorf(
"[%s] Expected the created notification to be:\n%s\n\nbut got:\n%s\n\n",
test.name,
expect,
got,
)
assert.Equal(t, expect, got, "actual notification does not equal expected")
}
} else {
if len(fspc.PullRequestCommentsAdded) != 0 {
Expand Down Expand Up @@ -1262,12 +1421,7 @@ Approvers can cancel approval by writing ` + "`/approve cancel`" + ` in a commen
}
}
if test.expectToggle != toggled {
t.Errorf(
"[%s] Expected 'approved' label toggled: %t, but got %t.",
test.name,
test.expectToggle,
toggled,
)
assert.Equal(t, test.expectToggle, toggled, "actual toggle state does not equal expected")
}
})
}
Expand Down Expand Up @@ -1306,6 +1460,10 @@ func (fro fakeRepoOwners) RequiredReviewers(path string) sets.String {
return sets.NewString()
}

func (fro fakeRepoOwners) MinimumReviewersForFile(path string) int {
return 1
}

func TestHandleGenericComment(t *testing.T) {
tests := []struct {
name string
Expand Down
Loading
Loading