fix: [CODE-3379]: Remove PR author from default reviewer evaluation (#3578)

This commit is contained in:
Johannes Batzill 2025-03-20 19:33:56 +00:00 committed by Harness
parent 83cf33bbb2
commit 9d5ceb0033
12 changed files with 358 additions and 92 deletions

View File

@ -225,6 +225,14 @@ func (c *Controller) Merge(
} }
if in.DryRunRules { if in.DryRunRules {
for _, approvals := range ruleOut.DefaultReviewerApprovals {
principalInfos, err := c.principalInfoCache.Map(ctx, approvals.PrincipalIDs)
if err != nil {
return nil, nil, fmt.Errorf("failed to fetch principal infos from info cache: %w", err)
}
approvals.PrincipalInfos = maps.Values(principalInfos)
}
return &types.MergeResponse{ return &types.MergeResponse{
BranchDeleted: ruleOut.DeleteSourceBranch, BranchDeleted: ruleOut.DeleteSourceBranch,
RuleViolations: violations, RuleViolations: violations,

View File

@ -54,21 +54,11 @@ func (s ruleSet) MergeVerify(
out.DeleteSourceBranch = out.DeleteSourceBranch || rOut.DeleteSourceBranch out.DeleteSourceBranch = out.DeleteSourceBranch || rOut.DeleteSourceBranch
out.MinimumRequiredApprovalsCount = maxInt(out.MinimumRequiredApprovalsCount, rOut.MinimumRequiredApprovalsCount) out.MinimumRequiredApprovalsCount = maxInt(out.MinimumRequiredApprovalsCount, rOut.MinimumRequiredApprovalsCount)
out.MinimumRequiredApprovalsCountLatest = maxInt(out.MinimumRequiredApprovalsCountLatest, rOut.MinimumRequiredApprovalsCountLatest) //nolint:lll out.MinimumRequiredApprovalsCountLatest = maxInt(out.MinimumRequiredApprovalsCountLatest, rOut.MinimumRequiredApprovalsCountLatest) //nolint:lll
out.DefaultReviewerIDs = append(out.DefaultReviewerIDs, rOut.DefaultReviewerIDs...)
out.RequiresCodeOwnersApproval = out.RequiresCodeOwnersApproval || rOut.RequiresCodeOwnersApproval out.RequiresCodeOwnersApproval = out.RequiresCodeOwnersApproval || rOut.RequiresCodeOwnersApproval
out.RequiresCodeOwnersApprovalLatest = out.RequiresCodeOwnersApprovalLatest || rOut.RequiresCodeOwnersApprovalLatest out.RequiresCodeOwnersApprovalLatest = out.RequiresCodeOwnersApprovalLatest || rOut.RequiresCodeOwnersApprovalLatest
out.RequiresCommentResolution = out.RequiresCommentResolution || rOut.RequiresCommentResolution out.RequiresCommentResolution = out.RequiresCommentResolution || rOut.RequiresCommentResolution
out.RequiresNoChangeRequests = out.RequiresNoChangeRequests || rOut.RequiresNoChangeRequests out.RequiresNoChangeRequests = out.RequiresNoChangeRequests || rOut.RequiresNoChangeRequests
out.DefaultReviewerApprovals = append(out.DefaultReviewerApprovals, rOut.DefaultReviewerApprovals...)
if len(out.DefaultReviewerIDs) > 0 {
out.DefaultReviewerApprovals = append(out.DefaultReviewerApprovals, &types.DefaultReviewerApprovalsResponse{
RuleInfo: r.RuleInfo,
MinimumRequiredCount: rOut.MinimumRequiredDefaultReviewerApprovalsCount,
MinimumRequiredCountLatest: rOut.MinimumRequiredDefaultReviewerApprovalsCountLatest,
PrincipalIDs: rOut.DefaultReviewerIDs,
CurrentCount: rOut.DefaultReviewerApprovalsCount,
})
}
return nil return nil
}) })

View File

@ -49,19 +49,15 @@ type (
} }
MergeVerifyOutput struct { MergeVerifyOutput struct {
AllowedMethods []enum.MergeMethod AllowedMethods []enum.MergeMethod
DeleteSourceBranch bool DeleteSourceBranch bool
MinimumRequiredApprovalsCount int MinimumRequiredApprovalsCount int
MinimumRequiredApprovalsCountLatest int MinimumRequiredApprovalsCountLatest int
MinimumRequiredDefaultReviewerApprovalsCount int RequiresCodeOwnersApproval bool
MinimumRequiredDefaultReviewerApprovalsCountLatest int RequiresCodeOwnersApprovalLatest bool
RequiresCodeOwnersApproval bool RequiresCommentResolution bool
RequiresCodeOwnersApprovalLatest bool RequiresNoChangeRequests bool
RequiresCommentResolution bool DefaultReviewerApprovals []*types.DefaultReviewerApprovalsResponse
RequiresNoChangeRequests bool
DefaultReviewerIDs []int64
DefaultReviewerApprovalsCount int
DefaultReviewerApprovals []*types.DefaultReviewerApprovalsResponse
} }
RequiredChecksInput struct { RequiredChecksInput struct {
@ -109,9 +105,8 @@ var (
const ( const (
codePullReqApprovalReqMinCount = "pullreq.approvals.require_minimum_count" codePullReqApprovalReqMinCount = "pullreq.approvals.require_minimum_count"
codePullReqApprovalReqMinCountLatest = "pullreq.approvals.require_minimum_count:latest_commit" codePullReqApprovalReqMinCountLatest = "pullreq.approvals.require_minimum_count:latest_commit"
codePullReqDefaultReviewerApprovalReqMinCount = "pullreq.approvals.require_default_reviewer_minimum_count" codePullReqApprovalReqDefaultReviewerMinCount = "pullreq.approvals.require_default_reviewer_minimum_count"
codePullReqDefaultReviewerApprovalReqMinCountLatest = "" + codePullReqApprovalReqDefaultReviewerMinCountLatest = "pullreq.approvals.require_default_reviewer_minimum_count:latest_commit" //nolint:lll
"pullreq.approvals.require_default_reviewer_minimum_count:latest_commit"
codePullReqApprovalReqLatestCommit = "pullreq.approvals.require_latest_commit" codePullReqApprovalReqLatestCommit = "pullreq.approvals.require_latest_commit"
codePullReqApprovalReqChangeRequested = "pullreq.approvals.require_change_requested" codePullReqApprovalReqChangeRequested = "pullreq.approvals.require_change_requested"
@ -146,23 +141,21 @@ func (v *DefPullReq) MergeVerify(
if v.Approvals.RequireLatestCommit { if v.Approvals.RequireLatestCommit {
out.RequiresCodeOwnersApprovalLatest = v.Approvals.RequireCodeOwners out.RequiresCodeOwnersApprovalLatest = v.Approvals.RequireCodeOwners
out.MinimumRequiredApprovalsCountLatest = v.Approvals.RequireMinimumCount out.MinimumRequiredApprovalsCountLatest = v.Approvals.RequireMinimumCount
out.MinimumRequiredDefaultReviewerApprovalsCountLatest = v.Approvals.RequireMinimumDefaultReviewerCount
} else { } else {
out.RequiresCodeOwnersApproval = v.Approvals.RequireCodeOwners out.RequiresCodeOwnersApproval = v.Approvals.RequireCodeOwners
out.MinimumRequiredApprovalsCount = v.Approvals.RequireMinimumCount out.MinimumRequiredApprovalsCount = v.Approvals.RequireMinimumCount
out.MinimumRequiredDefaultReviewerApprovalsCount = v.Approvals.RequireMinimumDefaultReviewerCount
} }
// pullreq.approvals // pullreq.approvals
approvedBy := make([]types.PrincipalInfo, 0, len(in.Reviewers)) approvedBy := make(map[int64]struct{})
for _, reviewer := range in.Reviewers { for _, reviewer := range in.Reviewers {
switch reviewer.ReviewDecision { switch reviewer.ReviewDecision {
case enum.PullReqReviewDecisionApproved: case enum.PullReqReviewDecisionApproved:
if v.Approvals.RequireLatestCommit && reviewer.SHA != in.PullReq.SourceSHA { if v.Approvals.RequireLatestCommit && reviewer.SHA != in.PullReq.SourceSHA {
continue continue
} }
approvedBy = append(approvedBy, reviewer.Reviewer) approvedBy[reviewer.Reviewer.ID] = struct{}{}
case enum.PullReqReviewDecisionChangeReq: case enum.PullReqReviewDecisionChangeReq:
if v.Approvals.RequireNoChangeRequest { if v.Approvals.RequireNoChangeRequest {
if reviewer.SHA == in.PullReq.SourceSHA { if reviewer.SHA == in.PullReq.SourceSHA {
@ -196,29 +189,51 @@ func (v *DefPullReq) MergeVerify(
} }
} }
defaultReviewerMap := make(map[int64]struct{}) effectiveDefaultReviewerIDs := make([]int64, 0, len(v.Reviewers.DefaultReviewerIDs))
for _, approver := range approvedBy {
defaultReviewerMap[approver.ID] = struct{}{}
}
var defaultReviewersCount int
for _, id := range v.Reviewers.DefaultReviewerIDs { for _, id := range v.Reviewers.DefaultReviewerIDs {
if _, ok := defaultReviewerMap[id]; ok { if id == in.PullReq.Author.ID {
defaultReviewersCount++ continue
} }
effectiveDefaultReviewerIDs = append(effectiveDefaultReviewerIDs, id)
} }
if defaultReviewersCount < v.Approvals.RequireMinimumDefaultReviewerCount {
// if author is default reviewer and required minimum == number of default reviewers, reduce minimum by one.
effectiveMinimumRequiredDefaultReviewerCount := v.Approvals.RequireMinimumDefaultReviewerCount
if len(effectiveDefaultReviewerIDs) < len(v.Reviewers.DefaultReviewerIDs) &&
len(v.Reviewers.DefaultReviewerIDs) == v.Approvals.RequireMinimumDefaultReviewerCount {
effectiveMinimumRequiredDefaultReviewerCount--
}
//nolint:nestif
if effectiveMinimumRequiredDefaultReviewerCount > 0 {
var defaultReviewerApprovalCount int
for _, id := range effectiveDefaultReviewerIDs {
if _, ok := approvedBy[id]; ok {
defaultReviewerApprovalCount++
}
}
if defaultReviewerApprovalCount < effectiveMinimumRequiredDefaultReviewerCount {
if v.Approvals.RequireLatestCommit {
violations.Addf(codePullReqApprovalReqDefaultReviewerMinCountLatest,
"Insufficient number of default reviewer approvals of the latest commit. Have %d but need at least %d.",
defaultReviewerApprovalCount, effectiveMinimumRequiredDefaultReviewerCount)
} else {
violations.Addf(codePullReqApprovalReqDefaultReviewerMinCount,
"Insufficient number of default reviewer approvals. Have %d but need at least %d.",
defaultReviewerApprovalCount, effectiveMinimumRequiredDefaultReviewerCount)
}
}
out.DefaultReviewerApprovals = []*types.DefaultReviewerApprovalsResponse{{
PrincipalIDs: effectiveDefaultReviewerIDs,
CurrentCount: defaultReviewerApprovalCount,
}}
if v.Approvals.RequireLatestCommit { if v.Approvals.RequireLatestCommit {
violations.Addf(codePullReqDefaultReviewerApprovalReqMinCount, out.DefaultReviewerApprovals[0].MinimumRequiredCountLatest = effectiveMinimumRequiredDefaultReviewerCount
"Insufficient number of default reviewer approvals of the latest commit. Have %d but need at least %d.",
defaultReviewersCount, v.Approvals.RequireMinimumDefaultReviewerCount)
} else { } else {
violations.Addf(codePullReqDefaultReviewerApprovalReqMinCountLatest, out.DefaultReviewerApprovals[0].MinimumRequiredCount = effectiveMinimumRequiredDefaultReviewerCount
"Insufficient number of default reviewer approvals. Have %d but need at least %d.",
defaultReviewersCount, v.Approvals.RequireMinimumDefaultReviewerCount)
} }
} }
out.DefaultReviewerIDs = v.Reviewers.DefaultReviewerIDs
out.DefaultReviewerApprovalsCount = defaultReviewersCount
if v.Approvals.RequireCodeOwners { if v.Approvals.RequireCodeOwners {
for _, entry := range in.CodeOwners.EvaluationEntries { for _, entry := range in.CodeOwners.EvaluationEntries {

View File

@ -24,6 +24,12 @@ import (
"github.com/harness/gitness/types/enum" "github.com/harness/gitness/types/enum"
) )
var (
reviewer1 = types.PrincipalInfo{ID: 1, DisplayName: "Reviewer 1", UID: "reviewer-1"}
reviewer2 = types.PrincipalInfo{ID: 2, DisplayName: "Reviewer 2", UID: "reviewer-2"}
reviewer3 = types.PrincipalInfo{ID: 3, DisplayName: "Reviewer 3", UID: "reviewer-3"}
)
// nolint:gocognit // it's a unit test // nolint:gocognit // it's a unit test
func TestDefPullReq_MergeVerify(t *testing.T) { func TestDefPullReq_MergeVerify(t *testing.T) {
tests := []struct { tests := []struct {
@ -58,7 +64,7 @@ func TestDefPullReq_MergeVerify(t *testing.T) {
in: MergeVerifyInput{ in: MergeVerifyInput{
PullReq: &types.PullReq{UnresolvedCount: 0, SourceSHA: "abc"}, PullReq: &types.PullReq{UnresolvedCount: 0, SourceSHA: "abc"},
Reviewers: []*types.PullReqReviewer{ Reviewers: []*types.PullReqReviewer{
{ReviewDecision: enum.PullReqReviewDecisionChangeReq, SHA: "abc"}, {ReviewDecision: enum.PullReqReviewDecisionChangeReq, SHA: "abc", Reviewer: reviewer1},
}, },
Method: enum.MergeMethodMerge, Method: enum.MergeMethodMerge,
}, },
@ -75,8 +81,8 @@ func TestDefPullReq_MergeVerify(t *testing.T) {
in: MergeVerifyInput{ in: MergeVerifyInput{
PullReq: &types.PullReq{UnresolvedCount: 0, SourceSHA: "abc"}, PullReq: &types.PullReq{UnresolvedCount: 0, SourceSHA: "abc"},
Reviewers: []*types.PullReqReviewer{ Reviewers: []*types.PullReqReviewer{
{ReviewDecision: enum.PullReqReviewDecisionApproved, SHA: "abc"}, {ReviewDecision: enum.PullReqReviewDecisionApproved, SHA: "abc", Reviewer: reviewer1},
{ReviewDecision: enum.PullReqReviewDecisionApproved, SHA: "abc"}, {ReviewDecision: enum.PullReqReviewDecisionApproved, SHA: "abc", Reviewer: reviewer2},
}, },
Method: enum.MergeMethodMerge, Method: enum.MergeMethodMerge,
}, },
@ -109,9 +115,9 @@ func TestDefPullReq_MergeVerify(t *testing.T) {
in: MergeVerifyInput{ in: MergeVerifyInput{
PullReq: &types.PullReq{UnresolvedCount: 0, SourceSHA: "abc"}, PullReq: &types.PullReq{UnresolvedCount: 0, SourceSHA: "abc"},
Reviewers: []*types.PullReqReviewer{ Reviewers: []*types.PullReqReviewer{
{ReviewDecision: enum.PullReqReviewDecisionPending, SHA: "abc"}, {ReviewDecision: enum.PullReqReviewDecisionPending, SHA: "abc", Reviewer: reviewer1},
{ReviewDecision: enum.PullReqReviewDecisionApproved, SHA: "abc"}, {ReviewDecision: enum.PullReqReviewDecisionApproved, SHA: "abc", Reviewer: reviewer2},
{ReviewDecision: enum.PullReqReviewDecisionApproved, SHA: "abc"}, {ReviewDecision: enum.PullReqReviewDecisionApproved, SHA: "abc", Reviewer: reviewer3},
}, },
Method: enum.MergeMethodMerge, Method: enum.MergeMethodMerge,
}, },
@ -120,6 +126,252 @@ func TestDefPullReq_MergeVerify(t *testing.T) {
MinimumRequiredApprovalsCountLatest: 2, MinimumRequiredApprovalsCountLatest: 2,
}, },
}, },
{
name: codePullReqApprovalReqDefaultReviewerMinCount + "-fail",
def: DefPullReq{
Approvals: DefApprovals{RequireMinimumDefaultReviewerCount: 1},
Reviewers: DefReviewers{DefaultReviewerIDs: []int64{reviewer1.ID}},
},
in: MergeVerifyInput{
PullReq: &types.PullReq{UnresolvedCount: 0, SourceSHA: "abc"},
Reviewers: []*types.PullReqReviewer{
{ReviewDecision: enum.PullReqReviewDecisionChangeReq, SHA: "abc", Reviewer: reviewer1},
{ReviewDecision: enum.PullReqReviewDecisionApproved, SHA: "abc", Reviewer: reviewer2},
},
Method: enum.MergeMethodMerge,
},
expCodes: []string{codePullReqApprovalReqDefaultReviewerMinCount},
expParams: [][]any{{0, 1}},
expOut: MergeVerifyOutput{
AllowedMethods: enum.MergeMethods,
DefaultReviewerApprovals: []*types.DefaultReviewerApprovalsResponse{{
PrincipalIDs: []int64{reviewer1.ID},
CurrentCount: 0,
MinimumRequiredCount: 1,
}},
},
},
{
name: codePullReqApprovalReqDefaultReviewerMinCount + "-success",
def: DefPullReq{
Approvals: DefApprovals{RequireMinimumDefaultReviewerCount: 1},
Reviewers: DefReviewers{DefaultReviewerIDs: []int64{reviewer1.ID}},
},
in: MergeVerifyInput{
PullReq: &types.PullReq{UnresolvedCount: 0, SourceSHA: "abc"},
Reviewers: []*types.PullReqReviewer{
{ReviewDecision: enum.PullReqReviewDecisionApproved, SHA: "abc", Reviewer: reviewer1},
{ReviewDecision: enum.PullReqReviewDecisionChangeReq, SHA: "abc", Reviewer: reviewer2},
},
Method: enum.MergeMethodMerge,
},
expOut: MergeVerifyOutput{
AllowedMethods: enum.MergeMethods,
DefaultReviewerApprovals: []*types.DefaultReviewerApprovalsResponse{{
PrincipalIDs: []int64{reviewer1.ID},
CurrentCount: 1,
MinimumRequiredCount: 1,
}},
},
},
{
name: codePullReqApprovalReqDefaultReviewerMinCount + "-with-author-count-1-exact",
def: DefPullReq{
Approvals: DefApprovals{RequireMinimumDefaultReviewerCount: 1},
Reviewers: DefReviewers{DefaultReviewerIDs: []int64{reviewer1.ID}},
},
in: MergeVerifyInput{
PullReq: &types.PullReq{UnresolvedCount: 0, SourceSHA: "abc", Author: reviewer1},
Reviewers: nil,
Method: enum.MergeMethodMerge,
},
expOut: MergeVerifyOutput{
AllowedMethods: enum.MergeMethods,
DefaultReviewerApprovals: nil,
},
},
{
name: codePullReqApprovalReqDefaultReviewerMinCount + "-with-author-count-1-more-fail",
def: DefPullReq{
Approvals: DefApprovals{RequireMinimumDefaultReviewerCount: 1},
Reviewers: DefReviewers{DefaultReviewerIDs: []int64{reviewer1.ID, reviewer2.ID}},
},
in: MergeVerifyInput{
PullReq: &types.PullReq{UnresolvedCount: 0, SourceSHA: "abc", Author: reviewer1},
Reviewers: nil,
Method: enum.MergeMethodMerge,
},
expCodes: []string{codePullReqApprovalReqDefaultReviewerMinCount},
expParams: [][]any{{0, 1}},
expOut: MergeVerifyOutput{
AllowedMethods: enum.MergeMethods,
DefaultReviewerApprovals: []*types.DefaultReviewerApprovalsResponse{{
PrincipalIDs: []int64{reviewer2.ID},
CurrentCount: 0,
MinimumRequiredCount: 1,
}},
},
},
{
name: codePullReqApprovalReqDefaultReviewerMinCount + "-with-author-count-1-more-success",
def: DefPullReq{
Approvals: DefApprovals{RequireMinimumDefaultReviewerCount: 1},
Reviewers: DefReviewers{DefaultReviewerIDs: []int64{reviewer1.ID, reviewer2.ID}},
},
in: MergeVerifyInput{
PullReq: &types.PullReq{UnresolvedCount: 0, SourceSHA: "abc", Author: reviewer1},
Reviewers: []*types.PullReqReviewer{
{ReviewDecision: enum.PullReqReviewDecisionApproved, SHA: "abc", Reviewer: reviewer2},
},
Method: enum.MergeMethodMerge,
},
expOut: MergeVerifyOutput{
AllowedMethods: enum.MergeMethods,
DefaultReviewerApprovals: []*types.DefaultReviewerApprovalsResponse{{
PrincipalIDs: []int64{reviewer2.ID},
CurrentCount: 1,
MinimumRequiredCount: 1,
}},
},
},
{
name: codePullReqApprovalReqDefaultReviewerMinCount + "-with-author-count-2-exact-fail",
def: DefPullReq{
Approvals: DefApprovals{RequireMinimumDefaultReviewerCount: 2},
Reviewers: DefReviewers{DefaultReviewerIDs: []int64{reviewer1.ID, reviewer2.ID}},
},
in: MergeVerifyInput{
PullReq: &types.PullReq{UnresolvedCount: 0, SourceSHA: "abc", Author: reviewer1},
Reviewers: []*types.PullReqReviewer{},
Method: enum.MergeMethodMerge,
},
expCodes: []string{codePullReqApprovalReqDefaultReviewerMinCount},
expParams: [][]any{{0, 1}},
expOut: MergeVerifyOutput{
AllowedMethods: enum.MergeMethods,
DefaultReviewerApprovals: []*types.DefaultReviewerApprovalsResponse{{
PrincipalIDs: []int64{reviewer2.ID},
CurrentCount: 0,
MinimumRequiredCount: 1,
}},
},
},
{
name: codePullReqApprovalReqDefaultReviewerMinCount + "-with-author-count-2-exact-success",
def: DefPullReq{
Approvals: DefApprovals{RequireMinimumDefaultReviewerCount: 2},
Reviewers: DefReviewers{DefaultReviewerIDs: []int64{reviewer1.ID, reviewer2.ID}},
},
in: MergeVerifyInput{
PullReq: &types.PullReq{UnresolvedCount: 0, SourceSHA: "abc", Author: reviewer1},
Reviewers: []*types.PullReqReviewer{
{ReviewDecision: enum.PullReqReviewDecisionApproved, SHA: "abc", Reviewer: reviewer2},
},
Method: enum.MergeMethodMerge,
},
expOut: MergeVerifyOutput{
AllowedMethods: enum.MergeMethods,
DefaultReviewerApprovals: []*types.DefaultReviewerApprovalsResponse{{
PrincipalIDs: []int64{reviewer2.ID},
CurrentCount: 1,
MinimumRequiredCount: 1,
}},
},
},
{
name: codePullReqApprovalReqDefaultReviewerMinCount + "-with-author-count-2-more-fail",
def: DefPullReq{
Approvals: DefApprovals{RequireMinimumDefaultReviewerCount: 2},
Reviewers: DefReviewers{DefaultReviewerIDs: []int64{reviewer1.ID, reviewer2.ID, reviewer3.ID}},
},
in: MergeVerifyInput{
PullReq: &types.PullReq{UnresolvedCount: 0, SourceSHA: "abc", Author: reviewer1},
Reviewers: []*types.PullReqReviewer{
{ReviewDecision: enum.PullReqReviewDecisionApproved, SHA: "abc", Reviewer: reviewer2},
{ReviewDecision: enum.PullReqReviewDecisionChangeReq, SHA: "abc", Reviewer: reviewer3},
},
Method: enum.MergeMethodMerge,
},
expCodes: []string{codePullReqApprovalReqDefaultReviewerMinCount},
expParams: [][]any{{1, 2}},
expOut: MergeVerifyOutput{
AllowedMethods: enum.MergeMethods,
DefaultReviewerApprovals: []*types.DefaultReviewerApprovalsResponse{{
PrincipalIDs: []int64{reviewer2.ID, reviewer3.ID},
CurrentCount: 1,
MinimumRequiredCount: 2,
}},
},
},
{
name: codePullReqApprovalReqDefaultReviewerMinCount + "-with-author-count-2-more-success",
def: DefPullReq{
Approvals: DefApprovals{RequireMinimumDefaultReviewerCount: 2},
Reviewers: DefReviewers{DefaultReviewerIDs: []int64{reviewer1.ID, reviewer2.ID, reviewer3.ID}},
},
in: MergeVerifyInput{
PullReq: &types.PullReq{UnresolvedCount: 0, SourceSHA: "abc", Author: reviewer1},
Reviewers: []*types.PullReqReviewer{
{ReviewDecision: enum.PullReqReviewDecisionApproved, SHA: "abc", Reviewer: reviewer2},
{ReviewDecision: enum.PullReqReviewDecisionApproved, SHA: "abc", Reviewer: reviewer3},
},
Method: enum.MergeMethodMerge,
},
expOut: MergeVerifyOutput{
AllowedMethods: enum.MergeMethods,
DefaultReviewerApprovals: []*types.DefaultReviewerApprovalsResponse{{
PrincipalIDs: []int64{reviewer2.ID, reviewer3.ID},
CurrentCount: 2,
MinimumRequiredCount: 2,
}},
},
},
{
name: codePullReqApprovalReqDefaultReviewerMinCountLatest + "-fail",
def: DefPullReq{
Approvals: DefApprovals{RequireMinimumDefaultReviewerCount: 1, RequireLatestCommit: true},
Reviewers: DefReviewers{DefaultReviewerIDs: []int64{reviewer1.ID}},
},
in: MergeVerifyInput{
PullReq: &types.PullReq{UnresolvedCount: 0, SourceSHA: "abc"},
Reviewers: []*types.PullReqReviewer{
{ReviewDecision: enum.PullReqReviewDecisionApproved, SHA: "def", Reviewer: reviewer1},
},
Method: enum.MergeMethodMerge,
},
expCodes: []string{codePullReqApprovalReqDefaultReviewerMinCountLatest},
expParams: [][]any{{0, 1}},
expOut: MergeVerifyOutput{
AllowedMethods: enum.MergeMethods,
DefaultReviewerApprovals: []*types.DefaultReviewerApprovalsResponse{{
PrincipalIDs: []int64{reviewer1.ID},
CurrentCount: 0,
MinimumRequiredCountLatest: 1,
}},
},
},
{
name: codePullReqApprovalReqDefaultReviewerMinCountLatest + "-success",
def: DefPullReq{
Approvals: DefApprovals{RequireMinimumDefaultReviewerCount: 1, RequireLatestCommit: true},
Reviewers: DefReviewers{DefaultReviewerIDs: []int64{reviewer1.ID}},
},
in: MergeVerifyInput{
PullReq: &types.PullReq{UnresolvedCount: 0, SourceSHA: "abc"},
Reviewers: []*types.PullReqReviewer{
{ReviewDecision: enum.PullReqReviewDecisionApproved, SHA: "abc", Reviewer: reviewer1},
},
Method: enum.MergeMethodMerge,
},
expOut: MergeVerifyOutput{
AllowedMethods: enum.MergeMethods,
DefaultReviewerApprovals: []*types.DefaultReviewerApprovalsResponse{{
PrincipalIDs: []int64{reviewer1.ID},
CurrentCount: 1,
MinimumRequiredCountLatest: 1,
}},
},
},
{ {
name: codePullReqApprovalReqCodeOwnersNoApproval + "-fail", name: codePullReqApprovalReqCodeOwnersNoApproval + "-fail",
def: DefPullReq{Approvals: DefApprovals{RequireCodeOwners: true}}, def: DefPullReq{Approvals: DefApprovals{RequireCodeOwners: true}},
@ -390,7 +642,7 @@ func TestDefPullReq_MergeVerify(t *testing.T) {
in: MergeVerifyInput{ in: MergeVerifyInput{
PullReq: &types.PullReq{SourceSHA: "abc"}, PullReq: &types.PullReq{SourceSHA: "abc"},
Reviewers: []*types.PullReqReviewer{ Reviewers: []*types.PullReqReviewer{
{ReviewDecision: enum.PullReqReviewDecisionChangeReq, SHA: "abc"}, {ReviewDecision: enum.PullReqReviewDecisionChangeReq, SHA: "abc", Reviewer: reviewer1},
}, },
Method: enum.MergeMethodMerge, Method: enum.MergeMethodMerge,
}, },
@ -408,14 +660,14 @@ func TestDefPullReq_MergeVerify(t *testing.T) {
Reviewers: []*types.PullReqReviewer{ Reviewers: []*types.PullReqReviewer{
{ {
ReviewDecision: enum.PullReqReviewDecisionChangeReq, ReviewDecision: enum.PullReqReviewDecisionChangeReq,
Reviewer: types.PrincipalInfo{DisplayName: "John"}, Reviewer: reviewer1,
SHA: "abc", SHA: "abc",
}, },
}, },
Method: enum.MergeMethodMerge, Method: enum.MergeMethodMerge,
}, },
expCodes: []string{codePullReqApprovalReqChangeRequested}, expCodes: []string{codePullReqApprovalReqChangeRequested},
expParams: [][]any{{"John"}}, expParams: [][]any{{reviewer1.DisplayName}},
expOut: MergeVerifyOutput{ expOut: MergeVerifyOutput{
AllowedMethods: enum.MergeMethods, AllowedMethods: enum.MergeMethods,
RequiresNoChangeRequests: true, RequiresNoChangeRequests: true,
@ -431,14 +683,14 @@ func TestDefPullReq_MergeVerify(t *testing.T) {
Reviewers: []*types.PullReqReviewer{ Reviewers: []*types.PullReqReviewer{
{ {
ReviewDecision: enum.PullReqReviewDecisionChangeReq, ReviewDecision: enum.PullReqReviewDecisionChangeReq,
Reviewer: types.PrincipalInfo{DisplayName: "John"}, Reviewer: reviewer1,
SHA: "def", SHA: "def",
}, },
}, },
Method: enum.MergeMethodMerge, Method: enum.MergeMethodMerge,
}, },
expCodes: []string{codePullReqApprovalReqChangeRequestedOldSHA}, expCodes: []string{codePullReqApprovalReqChangeRequestedOldSHA},
expParams: [][]any{{"John"}}, expParams: [][]any{{reviewer1.DisplayName}},
expOut: MergeVerifyOutput{ expOut: MergeVerifyOutput{
AllowedMethods: enum.MergeMethods, AllowedMethods: enum.MergeMethods,
RequiresNoChangeRequests: true, RequiresNoChangeRequests: true,

View File

@ -252,7 +252,6 @@ type PullReqFileView struct {
} }
type DefaultReviewerApprovalsResponse struct { type DefaultReviewerApprovalsResponse struct {
RuleInfo RuleInfo `json:"rule_info"`
MinimumRequiredCount int `json:"minimum_required_count"` MinimumRequiredCount int `json:"minimum_required_count"`
MinimumRequiredCountLatest int `json:"minimum_required_count_latest"` MinimumRequiredCountLatest int `json:"minimum_required_count_latest"`
CurrentCount int `json:"current_count"` CurrentCount int `json:"current_count"`

View File

@ -13,7 +13,7 @@
* See the License for the specific language governing permissions and * See the License for the specific language governing permissions and
* limitations under the License. * limitations under the License.
*/ */
import React from 'react' import React, { useMemo } from 'react'
import cx from 'classnames' import cx from 'classnames'
import { Container, FormInput, SelectOption, Text } from '@harnessio/uicore' import { Container, FormInput, SelectOption, Text } from '@harnessio/uicore'
import { Color } from '@harnessio/design-system' import { Color } from '@harnessio/design-system'
@ -49,6 +49,28 @@ const DefaultReviewersSection = (props: {
(item: SelectOption) => !defaultReviewersList?.includes(item.value as string) (item: SelectOption) => !defaultReviewersList?.includes(item.value as string)
) )
const defReviewerWarning = useMemo(() => {
const minReviewers = Number(formik.values.minDefaultReviewers)
const reviewerCount = defaultReviewersList?.length || 0
if (formik.values.requireMinDefaultReviewers && minReviewers === reviewerCount) {
let message = ''
let showWarning = false
if (reviewerCount === 1) {
message = getString('branchProtection.defaultReviewerWarning')
showWarning = true
} else if (reviewerCount > 1) {
message = getString('branchProtection.defaultReviewersWarning')
showWarning = true
}
return { message, showWarning }
}
return { message: '', showWarning: false }
}, [formik.values, defaultReviewersList])
return ( return (
<> <>
<FormInput.CheckBox <FormInput.CheckBox
@ -90,6 +112,11 @@ const DefaultReviewersSection = (props: {
{formik.errors.defaultReviewersList} {formik.errors.defaultReviewersList}
</Text> </Text>
)} )}
<Render when={defReviewerWarning.showWarning}>
<Text color={Color.WARNING} padding={{ bottom: 'medium' }} style={{ width: '35%' }}>
{defReviewerWarning.message}
</Text>
</Render>
<DefaultReviewersList defaultReviewersList={defaultReviewersList} setFieldValue={formik.setFieldValue} /> <DefaultReviewersList defaultReviewersList={defaultReviewersList} setFieldValue={formik.setFieldValue} />
<FormInput.CheckBox <FormInput.CheckBox

View File

@ -94,6 +94,8 @@ export interface StringsMap {
'branchProtection.createBranchBlockText': string 'branchProtection.createBranchBlockText': string
'branchProtection.createRule': string 'branchProtection.createRule': string
'branchProtection.defaultBranch': string 'branchProtection.defaultBranch': string
'branchProtection.defaultReviewerWarning': string
'branchProtection.defaultReviewersWarning': string
'branchProtection.deleteBranchAlertBtn': string 'branchProtection.deleteBranchAlertBtn': string
'branchProtection.deleteBranchAlertText': string 'branchProtection.deleteBranchAlertText': string
'branchProtection.deleteBranchBlockText': string 'branchProtection.deleteBranchBlockText': string

View File

@ -1085,6 +1085,8 @@ branchProtection:
requireMinDefaultReviewersContent: Require approval on pull requests from a minimum number of default reviewers requireMinDefaultReviewersContent: Require approval on pull requests from a minimum number of default reviewers
atLeastMinReviewers: 'Select at least {{count}} default reviewers' atLeastMinReviewers: 'Select at least {{count}} default reviewers'
atLeastMinReviewer: 'Select at least {{count}} default reviewer' atLeastMinReviewer: 'Select at least {{count}} default reviewer'
defaultReviewerWarning: Pull requests authored by the selected default reviewer will skip the required default reviewer check as there aren't enough default reviewers to satisfy the condition - consider adding additional default reviewers.
defaultReviewersWarning: Pull requests authored by the selected default reviewers will have the required default reviewer check reduced by one as there aren't enough default reviewers to satisfy the condition - consider adding additional default reviewers.
codeOwner: codeOwner:
title: Code Owner title: Code Owner
changesRequested: '{count} {count|1:change,changes} requested' changesRequested: '{count} {count|1:change,changes} requested'

View File

@ -151,8 +151,7 @@ const ChangesSection = (props: ChangesSectionProps) => {
defReviewerApprovalRequiredByRule, defReviewerApprovalRequiredByRule,
defReviewerLatestApprovalRequiredByRule, defReviewerLatestApprovalRequiredByRule,
defReviewerApprovedLatestChanges, defReviewerApprovedLatestChanges,
defReviewerApprovedChanges, defReviewerApprovedChanges
changesRequestedByDefReviewersArr
} = getUnifiedDefaultReviewersState(updatedDefaultApprovalRes) } = getUnifiedDefaultReviewersState(updatedDefaultApprovalRes)
const extractInfoForCodeOwnerContent = () => { const extractInfoForCodeOwnerContent = () => {
@ -690,29 +689,7 @@ const ChangesSection = (props: ChangesSectionProps) => {
(defReviewerApprovalRequiredByRule || defReviewerLatestApprovalRequiredByRule) && ( (defReviewerApprovalRequiredByRule || defReviewerLatestApprovalRequiredByRule) && (
<Container className={css.borderContainer} padding={{ left: 'xlarge', right: 'small' }}> <Container className={css.borderContainer} padding={{ left: 'xlarge', right: 'small' }}>
<Layout.Horizontal className={css.paddingContainer} flex={{ justifyContent: 'space-between' }}> <Layout.Horizontal className={css.paddingContainer} flex={{ justifyContent: 'space-between' }}>
{changesRequestedByDefReviewersArr && changesRequestedByDefReviewersArr?.length > 0 ? ( {renderDefaultReviewersStatus()}
<Text
className={cx(
css.sectionSubheader,
defReviewerApprovalRequiredByRule || defReviewerLatestApprovalRequiredByRule
? css.redIcon
: css.greyIcon
)}
icon={'error-transparent-no-outline'}
iconProps={{
size: 17,
color:
defReviewerApprovalRequiredByRule || defReviewerLatestApprovalRequiredByRule
? Color.RED_600
: '',
padding: { right: 'medium' }
}}
padding={{ left: 'large' }}>
{getString('changesSection.defaultReviewersChangesToPr')}
</Text>
) : (
renderDefaultReviewersStatus()
)}
{(defReviewerApprovalRequiredByRule || defReviewerLatestApprovalRequiredByRule) && ( {(defReviewerApprovalRequiredByRule || defReviewerLatestApprovalRequiredByRule) && (
<Container className={css.changeContainerPadding}> <Container className={css.changeContainerPadding}>
<Container className={css.requiredContainer}> <Container className={css.requiredContainer}>

View File

@ -79,10 +79,7 @@ export const DefaultReviewersPanel: React.FC<DefaultReviewersPanelProps> = ({
accessor: 'DefaultReviewers', accessor: 'DefaultReviewers',
Cell: ({ row }: CellProps<TypesDefaultReviewerApprovalsResponseWithRevDecision>) => { Cell: ({ row }: CellProps<TypesDefaultReviewerApprovalsResponseWithRevDecision>) => {
return ( return (
<Layout.Horizontal <Layout.Horizontal key={`keyContainer-${row.index}`} className={css.ownerContainer} spacing="tiny">
key={`keyContainer-${row.original.rule_info?.identifier}`}
className={css.ownerContainer}
spacing="tiny">
{row.original.principals?.map((principal, idx) => { {row.original.principals?.map((principal, idx) => {
if (idx < 2) { if (idx < 2) {
return ( return (

View File

@ -1127,7 +1127,6 @@ export interface TypesDefaultReviewerApprovalsResponse {
minimum_required_count?: number minimum_required_count?: number
minimum_required_count_latest?: number minimum_required_count_latest?: number
principals?: TypesPrincipalInfo[] | null principals?: TypesPrincipalInfo[] | null
rule_info?: TypesRuleInfo
} }
export interface TypesDeleteBranchOutput { export interface TypesDeleteBranchOutput {

View File

@ -14185,8 +14185,6 @@ components:
$ref: '#/components/schemas/TypesPrincipalInfo' $ref: '#/components/schemas/TypesPrincipalInfo'
nullable: true nullable: true
type: array type: array
rule_info:
$ref: '#/components/schemas/TypesRuleInfo'
type: object type: object
TypesDeleteBranchOutput: TypesDeleteBranchOutput:
properties: properties: