[Rules] Expose extra Branch.PullReq fields on merge DryRyn as requi…

…red by UI (#1117)

app/api/controller/pullreq/merge.go

@@ -262,11 +262,17 @@ func (c *Controller) Merge(
 
  // With in.DryRun=true this function never returns types.MergeViolations
  out := &types.MergeResponse{
- DryRun: true,
  BranchDeleted: ruleOut.DeleteSourceBranch,
- AllowedMethods: ruleOut.AllowedMethods,
- ConflictFiles: pr.MergeConflicts,
  RuleViolations: violations,
+
+ // values only retured by dry run
+ DryRun: true,
+ ConflictFiles: pr.MergeConflicts,
+ AllowedMethods: ruleOut.AllowedMethods,
+ RequiresCodeOwnersApproval: ruleOut.RequiresCodeOwnersApproval,
+ RequiresCommentResolution: ruleOut.RequiresCommentResolution,
+ RequiresNoChangeRequests: ruleOut.RequiresNoChangeRequests,
+ MinimumRequiredApprovalsCount: ruleOut.MinimumRequiredApprovalsCount,
  }
 
  return out, nil, nil

app/services/protection/rule_branch_test.go

@@ -19,6 +19,7 @@ import (
  "reflect"
  "testing"
 
+ "github.com/harness/gitness/app/services/codeowners"
  "github.com/harness/gitness/types"
  "github.com/harness/gitness/types/enum"
 )
@@ -61,8 +62,9 @@ func TestBranch_MergeVerify(t *testing.T) {
  PullReq: &types.PullReq{UnresolvedCount: 1},
  },
  expOut: MergeVerifyOutput{
- DeleteSourceBranch: true,
- AllowedMethods: enum.MergeMethods,
+ DeleteSourceBranch: true,
+ AllowedMethods: enum.MergeMethods,
+ RequiresCommentResolution: true,
  },
  expVs: []types.RuleViolations{
  {
@@ -91,8 +93,9 @@ func TestBranch_MergeVerify(t *testing.T) {
  PullReq: &types.PullReq{UnresolvedCount: 1},
  },
  expOut: MergeVerifyOutput{
- DeleteSourceBranch: true,
- AllowedMethods: enum.MergeMethods,
+ DeleteSourceBranch: true,
+ AllowedMethods: enum.MergeMethods,
+ RequiresCommentResolution: true,
  },
  expVs: []types.RuleViolations{
  {
@@ -120,8 +123,9 @@ func TestBranch_MergeVerify(t *testing.T) {
  PullReq: &types.PullReq{UnresolvedCount: 1},
  },
  expOut: MergeVerifyOutput{
- DeleteSourceBranch: true,
- AllowedMethods: enum.MergeMethods,
+ DeleteSourceBranch: true,
+ AllowedMethods: enum.MergeMethods,
+ RequiresCommentResolution: true,
  },
  expVs: []types.RuleViolations{
  {
@@ -156,6 +160,48 @@ func TestBranch_MergeVerify(t *testing.T) {
  },
  expVs: []types.RuleViolations{},
  },
+ {
+ name: "definition-values",
+ branch: Branch{
+ Bypass: DefBypass{},
+ PullReq: DefPullReq{
+ StatusChecks: DefStatusChecks{},
+ Comments: DefComments{
+ RequireResolveAll: true,
+ },
+ Approvals: DefApprovals{
+ RequireCodeOwners: true,
+ RequireMinimumCount: 2,
+ RequireNoChangeRequest: true,
+ },
+ Merge: DefMerge{
+ DeleteBranch: true,
+ StrategiesAllowed: []enum.MergeMethod{enum.MergeMethodSquash},
+ },
+ },
+ },
+ in: MergeVerifyInput{
+ Actor: user,
+ CodeOwners: &codeowners.Evaluation{},
+ PullReq: &types.PullReq{},
+ Reviewers: []*types.PullReqReviewer{},
+ },
+ expOut: MergeVerifyOutput{
+ DeleteSourceBranch: true,
+ AllowedMethods: []enum.MergeMethod{enum.MergeMethodSquash},
+ RequiresCodeOwnersApproval: true,
+ RequiresNoChangeRequests: true,
+ RequiresCommentResolution: true,
+ MinimumRequiredApprovalsCount: 2,
+ },
+ expVs: []types.RuleViolations{
+ {
+ Violations: []types.Violation{
+ {Code: codePullReqApprovalReqMinCount},
+ },
+ },
+ },
+ },
  }
 
  ctx := context.Background()

app/services/protection/set.go

@@ -50,9 +50,14 @@ func (s ruleSet) MergeVerify(
  return err
  }
 
+ // combine output across rules
  violations = append(violations, backFillRule(rVs, r.RuleInfo)...)
- out.DeleteSourceBranch = out.DeleteSourceBranch || rOut.DeleteSourceBranch
  out.AllowedMethods = intersectSorted(out.AllowedMethods, rOut.AllowedMethods)
+ out.DeleteSourceBranch = out.DeleteSourceBranch || rOut.DeleteSourceBranch
+ out.MinimumRequiredApprovalsCount = maxInt(out.MinimumRequiredApprovalsCount, rOut.MinimumRequiredApprovalsCount)
+ out.RequiresCodeOwnersApproval = out.RequiresCodeOwnersApproval || rOut.RequiresCodeOwnersApproval
+ out.RequiresCommentResolution = out.RequiresCommentResolution || rOut.RequiresCommentResolution
+ out.RequiresNoChangeRequests = out.RequiresNoChangeRequests || rOut.RequiresNoChangeRequests
 
  return nil
  })
@@ -265,3 +270,10 @@ func intersectSorted[T constraints.Ordered](sliceA, sliceB []T) []T {
 
  return sliceA
 }
+
+func maxInt(a int, b int) int {
+ if a > b {
+ return a
+ }
+ return b
+}

app/services/protection/set_test.go

@@ -19,6 +19,7 @@ import (
  "reflect"
  "testing"
 
+ "github.com/harness/gitness/app/services/codeowners"
  "github.com/harness/gitness/types"
  "github.com/harness/gitness/types/enum"
 )
@@ -96,8 +97,9 @@ func TestRuleSet_MergeVerify(t *testing.T) {
  Method: enum.MergeMethodRebase,
  },
  expOut: MergeVerifyOutput{
- DeleteSourceBranch: true,
- AllowedMethods: nil,
+ DeleteSourceBranch: true,
+ MinimumRequiredApprovalsCount: 1,
+ AllowedMethods: nil,
  },
  expViol: []types.RuleViolations{
  {
@@ -169,6 +171,151 @@ func TestRuleSet_MergeVerify(t *testing.T) {
  },
  expViol: []types.RuleViolations{},
  },
+ {
+ name: "combine-definition-values",
+ rules: []types.RuleInfoInternal{
+ {
+ RuleInfo: types.RuleInfo{
+ SpacePath: "",
+ RepoPath: "space/repo",
+ ID: 1,
+ Identifier: "rule1",
+ Type: TypeBranch,
+ State: enum.RuleStateActive,
+ },
+ Pattern: []byte(`{"default":true}`),
+ Definition: []byte(`{
+ "pullreq": {
+ "approvals": {
+ "require_code_owners": false,
+ "require_minimum_count": 2,
+ "require_no_change_request": false
+ },
+ "comments":{
+ "require_resolve_all": false
+ },
+ "merge":{
+ "delete_branch": true,
+ "strategies_allowed": ["merge","rebase"]
+ }
+ }
+ }`),
+ },
+ {
+ RuleInfo: types.RuleInfo{
+ SpacePath: "space",
+ RepoPath: "",
+ ID: 2,
+ Identifier: "rule2",
+ Type: TypeBranch,
+ State: enum.RuleStateActive,
+ },
+ Pattern: []byte(`{"default":true}`),
+ Definition: []byte(`{
+ "pullreq": {
+ "approvals": {
+ "require_code_owners": true,
+ "require_minimum_count": 3,
+ "require_no_change_request": true
+ },
+ "comments":{
+ "require_resolve_all": true
+ },
+ "merge":{
+ "delete_branch": true,
+ "strategies_allowed": ["rebase","squash"]
+ }
+ }
+ }`),
+ },
+ {
+ RuleInfo: types.RuleInfo{
+ SpacePath: "",
+ RepoPath: "space/repo",
+ ID: 3,
+ Identifier: "rule3",
+ Type: TypeBranch,
+ State: enum.RuleStateActive,
+ },
+ Pattern: []byte(`{"default":true}`),
+ Definition: []byte(`{
+ "pullreq": {
+ "approvals": {
+ "require_code_owners": false,
+ "require_minimum_count": 2,
+ "require_no_change_request": false
+ },
+ "comments":{
+ "require_resolve_all": false
+ },
+ "merge":{
+ "delete_branch": false,
+ "strategies_allowed": ["rebase"]
+ }
+ }
+ }`),
+ },
+ },
+ input: MergeVerifyInput{
+ Actor: &types.Principal{ID: 1},
+ TargetRepo: &types.Repository{ID: 1, DefaultBranch: "main"},
+ PullReq: &types.PullReq{ID: 1, SourceBranch: "pr", TargetBranch: "main"},
+ CodeOwners: &codeowners.Evaluation{},
+ Reviewers: []*types.PullReqReviewer{},
+ },
+ expOut: MergeVerifyOutput{
+ AllowedMethods: []enum.MergeMethod{enum.MergeMethodRebase},
+ DeleteSourceBranch: true,
+ MinimumRequiredApprovalsCount: 3,
+ RequiresCodeOwnersApproval: true,
+ RequiresCommentResolution: true,
+ RequiresNoChangeRequests: true,
+ },
+ expViol: []types.RuleViolations{
+ {
+ Rule: types.RuleInfo{
+ SpacePath: "",
+ RepoPath: "space/repo",
+ ID: 1,
+ Identifier: "rule1",
+ Type: TypeBranch,
+ State: enum.RuleStateActive,
+ },
+ Bypassed: false,
+ Violations: []types.Violation{
+ {Code: codePullReqApprovalReqMinCount},
+ },
+ },
+ {
+ Rule: types.RuleInfo{
+ SpacePath: "space",
+ RepoPath: "",
+ ID: 2,
+ Identifier: "rule2",
+ Type: TypeBranch,
+ State: enum.RuleStateActive,
+ },
+ Bypassed: false,
+ Violations: []types.Violation{
+ {Code: codePullReqApprovalReqMinCount},
+ },
+ },
+ {
+ Rule: types.RuleInfo{
+ SpacePath: "",
+ RepoPath: "space/repo",
+ ID: 3,
+ Identifier: "rule3",
+ Type: TypeBranch,
+ State: enum.RuleStateActive,
+ },
+ Bypassed: false,
+ Violations: []types.Violation{
+ {Code: codePullReqApprovalReqMinCount},
+ },
+ },
+ },
+ },
  }
 
  ctx := context.Background()

app/services/protection/verify_pullreq.go

@@ -48,8 +48,12 @@ type (
  }
 
  MergeVerifyOutput struct {
- DeleteSourceBranch bool
- AllowedMethods []enum.MergeMethod
+ AllowedMethods []enum.MergeMethod
+ DeleteSourceBranch bool
+ MinimumRequiredApprovalsCount int
+ RequiresCodeOwnersApproval bool
+ RequiresCommentResolution bool
+ RequiresNoChangeRequests bool
  }
 
  RequiredChecksInput struct {
@@ -97,7 +101,12 @@ func (v *DefPullReq) MergeVerify(
  var out MergeVerifyOutput
  var violations types.RuleViolations
 
+ // set static merge verify output that comes from the PR definition
  out.DeleteSourceBranch = v.Merge.DeleteBranch
+ out.MinimumRequiredApprovalsCount = v.Approvals.RequireMinimumCount
+ out.RequiresCodeOwnersApproval = v.Approvals.RequireCodeOwners
+ out.RequiresCommentResolution = v.Comments.RequireResolveAll
+ out.RequiresNoChangeRequests = v.Approvals.RequireNoChangeRequest
 
  // pullreq.approvals