Repository: palantir/policy-bot
Pull request #636 (2023-09-26 08:53:07)

Implemented RequestedCount
Made a small change in sugested implementation, instead of `requested_reviewers` i used `requested_count` which more clearly points to a number. 

Fixes: https://github.com/palantir/policy-bot/issues/634

palantirtech: Thanks for your interest in [palantir/policy-bot](https://github.com/palantir/policy-bot), @AtzeDeVries! Before we can accept your pull request, you need to sign our contributor license agreement - just visit https://cla.palantir.com/ and follow the instructions. Once you sign, I'll automatically update this pull request.

Patch:
diff --git a/README.md b/README.md
index 0df2afa..8bc9ca8 100644
--- a/README.md
+++ b/README.md
@@ -356,6 +356,12 @@ options:
     # at least until https://github.com/palantir/policy-bot/issues/165 is fixed.
     # Defaults to 'random-users'.
     mode: all-users|random-users|teams
+    
+    # count sets the number of users requested to review the pull request when
+    # using the `random-users` mode. If count is not set or set to 0, request the
+    # number of users set by requires.count. Setting this is useful when you want
+    # to request more reviewers than the required count. Defaults to 0.
+    count: 0
 
   # "methods" defines how users may express approval.
   methods:
diff --git a/policy/approval/approve.go b/policy/approval/approve.go
index 8b40202..9f44ba1 100644
--- a/policy/approval/approve.go
+++ b/policy/approval/approve.go
@@ -54,6 +54,7 @@ type Options struct {
 type RequestReview struct {
 	Enabled bool               `yaml:"enabled"`
 	Mode    common.RequestMode `yaml:"mode"`
+	Count   int                `yaml:"count"`
 }
 
 func (opts *Options) GetMethods() *common.Methods {
@@ -168,13 +169,19 @@ func (r *Rule) getReviewRequestRule() *common.ReviewRequestRule {
 		mode = common.RequestModeRandomUsers
 	}
 
+	requestedCount := r.Options.RequestReview.Count
+	if requestedCount == 0 {
+		requestedCount = r.Requires.Count
+	}
+
 	return &common.ReviewRequestRule{
-		Users:         r.Requires.Actors.Users,
-		Teams:         r.Requires.Actors.Teams,
-		Organizations: r.Requires.Actors.Organizations,
-		Permissions:   r.Requires.Actors.GetPermissions(),
-		RequiredCount: r.Requires.Count,
-		Mode:          mode,
+		Users:          r.Requires.Actors.Users,
+		Teams:          r.Requires.Actors.Teams,
+		Organizations:  r.Requires.Actors.Organizations,
+		Permissions:    r.Requires.Actors.GetPermissions(),
+		RequiredCount:  r.Requires.Count,
+		RequestedCount: requestedCount,
+		Mode:           mode,
 	}
 }
 
diff --git a/policy/common/result.go b/policy/common/result.go
index 3d3ab50..0c7942d 100644
--- a/policy/common/result.go
+++ b/policy/common/result.go
@@ -50,11 +50,12 @@ const (
 )
 
 type ReviewRequestRule struct {
-	Teams         []string
-	Users         []string
-	Organizations []string
-	Permissions   []pull.Permission
-	RequiredCount int
+	Teams          []string
+	Users          []string
+	Organizations  []string
+	Permissions    []pull.Permission
+	RequiredCount  int
+	RequestedCount int
 
 	Mode RequestMode
 }
diff --git a/policy/reviewer/reviewer.go b/policy/reviewer/reviewer.go
index 56d0687..a01ca86 100644
--- a/policy/reviewer/reviewer.go
+++ b/policy/reviewer/reviewer.go
@@ -269,7 +269,7 @@ func selectUserReviewers(ctx context.Context, prctx pull.Context, selection *Sel
 		selection.Users = append(selection.Users, possibleReviewers...)
 
 	case common.RequestModeRandomUsers:
-		count := result.ReviewRequestRule.RequiredCount
+		count := result.ReviewRequestRule.RequestedCount
 		selectedUsers := selectRandomUsers(count, possibleReviewers, r)
 
 		logger.Debug().Msgf("Found %d eligible reviewers; randomly selecting %d", len(possibleReviewers), count)


Test patch:
diff --git a/policy/reviewer/reviewer_test.go b/policy/reviewer/reviewer_test.go
index bbc3315..82a4268 100644
--- a/policy/reviewer/reviewer_test.go
+++ b/policy/reviewer/reviewer_test.go
@@ -192,18 +192,20 @@ func TestSelectReviewers(t *testing.T) {
 			Name:   "users",
 			Status: common.StatusPending,
 			ReviewRequestRule: &common.ReviewRequestRule{
-				Users:         []string{"mhaypenny", "review-approver", "contributor-committer"},
-				RequiredCount: 2,
-				Mode:          common.RequestModeRandomUsers,
+				Users:          []string{"mhaypenny", "review-approver", "contributor-committer"},
+				RequiredCount:  2,
+				RequestedCount: 2,
+				Mode:           common.RequestModeRandomUsers,
 			},
 		},
 		{
 			Name:   "admin-users",
 			Status: common.StatusPending,
 			ReviewRequestRule: &common.ReviewRequestRule{
-				Permissions:   []pull.Permission{pull.PermissionAdmin},
-				RequiredCount: 1,
-				Mode:          common.RequestModeRandomUsers,
+				Permissions:    []pull.Permission{pull.PermissionAdmin},
+				RequiredCount:  1,
+				RequestedCount: 1,
+				Mode:           common.RequestModeRandomUsers,
 			},
 		},
 	}
@@ -219,6 +221,32 @@ func TestSelectReviewers(t *testing.T) {
 	require.NotContains(t, selection.Users, "org-owner", "org-owner should not be requested")
 }
 
+func TestSelectRequestedReviewers(t *testing.T) {
+	r := rand.New(rand.NewSource(42))
+	results := []*common.Result{
+		{
+			Name:   "users",
+			Status: common.StatusPending,
+			ReviewRequestRule: &common.ReviewRequestRule{
+				Users:          []string{"mhaypenny", "review-approver", "contributor-committer"},
+				RequiredCount:  1,
+				RequestedCount: 2,
+				Mode:           common.RequestModeRandomUsers,
+			},
+		},
+	}
+
+	prctx := makeContext()
+
+	selection, err := SelectReviewers(context.Background(), prctx, results, r)
+	require.NoError(t, err)
+	require.Len(t, selection.Users, 2, "policy should request two people")
+	require.Contains(t, selection.Users, "review-approver", "at least review-approver must be selected")
+	require.NotContains(t, selection.Users, "mhaypenny", "the author cannot be requested")
+	require.NotContains(t, selection.Users, "not-a-collaborator", "a non collaborator cannot be requested")
+	require.NotContains(t, selection.Users, "org-owner", "org-owner should not be requested")
+}
+
 func TestSelectReviewers_UserPermission(t *testing.T) {
 	r := rand.New(rand.NewSource(42))
 	results := []*common.Result{
@@ -226,9 +254,10 @@ func TestSelectReviewers_UserPermission(t *testing.T) {
 			Name:   "user-permissions",
 			Status: common.StatusPending,
 			ReviewRequestRule: &common.ReviewRequestRule{
-				Permissions:   []pull.Permission{pull.PermissionTriage, pull.PermissionMaintain},
-				RequiredCount: 2,
-				Mode:          common.RequestModeAllUsers,
+				Permissions:    []pull.Permission{pull.PermissionTriage, pull.PermissionMaintain},
+				RequiredCount:  2,
+				RequestedCount: 2,
+				Mode:           common.RequestModeAllUsers,
 			},
 		},
 	}
@@ -278,9 +307,10 @@ func TestSelectReviewers_TeamMembers(t *testing.T) {
 			Name:   "team-users",
 			Status: common.StatusPending,
 			ReviewRequestRule: &common.ReviewRequestRule{
-				Teams:         []string{"everyone/team-write"},
-				RequiredCount: 1,
-				Mode:          common.RequestModeRandomUsers,
+				Teams:          []string{"everyone/team-write"},
+				RequiredCount:  1,
+				RequestedCount: 1,
+				Mode:           common.RequestModeRandomUsers,
 			},
 		},
 	}
@@ -345,9 +375,10 @@ func TestSelectReviewers_Org(t *testing.T) {
 			Name:   "org",
 			Status: common.StatusPending,
 			ReviewRequestRule: &common.ReviewRequestRule{
-				Organizations: []string{"everyone"},
-				RequiredCount: 1,
-				Mode:          common.RequestModeRandomUsers,
+				Organizations:  []string{"everyone"},
+				RequiredCount:  1,
+				RequestedCount: 1,
+				Mode:           common.RequestModeRandomUsers,
 			},
 		},
 	}