Repository: palantir/policy-bot
Pull request #214 (2020-08-04 19:25:10)

Only process review requests when enabled
While reviewers were never assigned incorrectly, we previously made
unnecessary requests to GitHub to get collaborator and reviewer details
that were discarded when no enabled rules existed. Now, check for rules
that enable review requests first and only start reviewer assignment if
at least one exists.

This also cleans up some logging and other minor style things in code I
touched to fix the problem.


Patch:
diff --git a/policy/approval/approve.go b/policy/approval/approve.go
index 377d618..3988402 100644
--- a/policy/approval/approve.go
+++ b/policy/approval/approve.go
@@ -113,15 +113,18 @@ func (r *Rule) Evaluate(ctx context.Context, prctx pull.Context) (res common.Res
 		res.Status = common.StatusPending
 
 		if r.Options.RequestReview.Enabled {
-			res.ReviewRequestRule = common.ReviewRequestRule{
+			mode := r.Options.RequestReview.Mode
+			if mode == "" {
+				mode = common.RequestModeRandomUsers
+			}
+			res.ReviewRequestRule = &common.ReviewRequestRule{
 				Users:              r.Requires.Users,
 				Teams:              r.Requires.Teams,
 				Organizations:      r.Requires.Organizations,
 				Admins:             r.Requires.Admins,
 				WriteCollaborators: r.Requires.WriteCollaborators,
 				RequiredCount:      r.Requires.Count,
-
-				Mode: r.Options.RequestReview.Mode,
+				Mode:               mode,
 			}
 		}
 	}
diff --git a/policy/common/result.go b/policy/common/result.go
index 3197ed4..8accdbd 100644
--- a/policy/common/result.go
+++ b/policy/common/result.go
@@ -57,20 +57,12 @@ type ReviewRequestRule struct {
 }
 
 type Result struct {
-	Name              string
-	Description       string
-	Status            EvaluationStatus
-	ReviewRequestRule ReviewRequestRule
+	Name        string
+	Description string
+	Status      EvaluationStatus
+	Error       error
 
-	Error error
+	ReviewRequestRule *ReviewRequestRule
 
 	Children []*Result
 }
-
-func (r Result) GetMode() RequestMode {
-	mode := RequestModeRandomUsers
-	if r.ReviewRequestRule.Mode != "" {
-		mode = r.ReviewRequestRule.Mode
-	}
-	return mode
-}
diff --git a/policy/reviewer/reviewer.go b/policy/reviewer/reviewer.go
index 5475cce..02cba2f 100644
--- a/policy/reviewer/reviewer.go
+++ b/policy/reviewer/reviewer.go
@@ -32,20 +32,20 @@ const (
 	LogKeyLeafNode = "leaf_node"
 )
 
-func findLeafChildren(result common.Result) []common.Result {
-	var r []common.Result
-	if len(result.Children) == 0 {
-		if result.Status == common.StatusPending && result.Error == nil {
-			return []common.Result{result}
-		}
-	} else {
-		for _, c := range result.Children {
-			if c.Status == common.StatusPending {
-				r = append(r, findLeafChildren(*c)...)
-			}
-		}
+// FindRequests returns all pending leaf results with review requests enabled.
+func FindRequests(result *common.Result) []*common.Result {
+	if result.Status != common.StatusPending {
+		return nil
+	}
+
+	var reqs []*common.Result
+	for _, c := range result.Children {
+		reqs = append(reqs, FindRequests(c)...)
+	}
+	if len(result.Children) == 0 && result.ReviewRequestRule != nil && result.Error == nil {
+		reqs = append(reqs, result)
 	}
-	return r
+	return reqs
 }
 
 // select n random values from the list of users without reuse
@@ -153,13 +153,11 @@ func stripOrg(teamWithOrg string) (string, error) {
 	return split[1], nil
 }
 
-func SelectReviewers(ctx context.Context, prctx pull.Context, result common.Result, r *rand.Rand) ([]string, []string, error) {
+func SelectReviewers(ctx context.Context, prctx pull.Context, results []*common.Result, r *rand.Rand) ([]string, []string, error) {
 	usersToRequest := make([]string, 0)
 	teamsToRequest := make([]string, 0)
-	pendingLeafNodes := findLeafChildren(result)
-	zerolog.Ctx(ctx).Debug().Msgf("Found %d pending leaf nodes for review selection", len(pendingLeafNodes))
 
-	for _, child := range pendingLeafNodes {
+	for _, child := range results {
 		logger := zerolog.Ctx(ctx).With().Str(LogKeyLeafNode, child.Name).Logger()
 
 		allUsers := make(map[string]struct{})
@@ -222,7 +220,7 @@ func SelectReviewers(ctx context.Context, prctx pull.Context, result common.Resu
 			}
 		}
 
-		switch child.GetMode() {
+		switch child.ReviewRequestRule.Mode {
 		case common.RequestModeTeams:
 			var possibleTeams []string
 			permissionedTeams, err := prctx.Teams()
diff --git a/server/handler/base.go b/server/handler/base.go
index 8c921a3..9b12174 100644
--- a/server/handler/base.go
+++ b/server/handler/base.go
@@ -202,42 +202,58 @@ func (b *Base) EvaluateFetchedConfig(ctx context.Context, prctx pull.Context, re
 	}
 
 	if requestReviews && statusState == "pending" && !prctx.IsDraft() {
-		hasReviewers, err := prctx.HasReviewers()
-		if err != nil {
-			return errors.Wrap(err, "Unable to list request reviewers")
+		if reqs := reviewer.FindRequests(&result); len(reqs) > 0 {
+			logger.Debug().Msgf("Found %d pending rules with review requests enabled", len(reqs))
+			return b.requestReviews(ctx, prctx, client, reqs)
 		}
+		logger.Debug().Msgf("No pending rules have review requests enabled, skipping reviewer assignment")
+	}
 
-		if !hasReviewers {
-			r := rand.New(rand.NewSource(time.Now().UnixNano()))
-			requestedUsers, requestedTeams, err := reviewer.SelectReviewers(ctx, prctx, result, r)
-			if err != nil {
-				return errors.Wrap(err, "Unable to select random request reviewers")
-			}
-
-			// check again if someone assigned a reviewer while we were calculating users to request
-			hasReviewersAfter, err := prctx.HasReviewers()
-			if err != nil {
-				return errors.Wrap(err, "Unable to double-check existing reviewers, assuming original state is still valid")
-			}
-
-			if (len(requestedUsers) > 0 || len(requestedTeams) > 0) && !hasReviewersAfter {
-				reviewers := github.ReviewersRequest{
-					Reviewers:     requestedUsers,
-					TeamReviewers: requestedTeams,
-				}
-
-				logger.Debug().Msgf("PR is not in draft, there are no current reviewers, and reviews are requested from %q users", requestedUsers)
-				_, _, err = client.PullRequests.RequestReviewers(ctx, prctx.RepositoryOwner(), prctx.RepositoryName(), prctx.Number(), reviewers)
-				if err != nil {
-					return errors.Wrap(err, "Unable to request reviewers")
-				}
-			} else {
-				logger.Debug().Msg("No users found for review, no users were requested, or users were assigned during review calculation")
-			}
-		} else {
-			logger.Debug().Msg("PR has existing reviewers, not adding anyone")
-		}
+	return nil
+}
+
+func (b *Base) requestReviews(ctx context.Context, prctx pull.Context, client *github.Client, reqs []*common.Result) error {
+	logger := zerolog.Ctx(ctx)
+
+	hasReviewers, err := prctx.HasReviewers()
+	if err != nil {
+		return errors.Wrap(err, "failed to check existing reviewers")
+	}
+	if hasReviewers {
+		logger.Debug().Msg("PR has existing reviewers, skipping reviewer assignment")
+		return nil
 	}
 
+	r := rand.New(rand.NewSource(time.Now().UnixNano()))
+	requestedUsers, requestedTeams, err := reviewer.SelectReviewers(ctx, prctx, reqs, r)
+	if err != nil {
+		return errors.Wrap(err, "failed to select reviewers")
+	}
+
+	// check again if someone assigned a reviewer while we were calculating users to request
+	hasReviewersAfter, err := prctx.HasReviewers()
+	if err != nil {
+		logger.Warn().Err(err).Msg("Failed to double-check existing reviewers, assuming there are still no reviewers")
+		hasReviewersAfter = false
+	}
+
+	if (len(requestedUsers) > 0 || len(requestedTeams) > 0) && !hasReviewersAfter {
+		reviewers := github.ReviewersRequest{
+			Reviewers:     requestedUsers,
+			TeamReviewers: requestedTeams,
+		}
+
+		logger.Debug().
+			Strs("users", requestedUsers).
+			Strs("teams", requestedTeams).
+			Msgf("Requesting reviews from %d users and %d teams", len(requestedUsers), len(requestedTeams))
+
+		_, _, err = client.PullRequests.RequestReviewers(ctx, prctx.RepositoryOwner(), prctx.RepositoryName(), prctx.Number(), reviewers)
+		if err != nil {
+			return errors.Wrap(err, "failed to request reviewers")
+		}
+	} else {
+		logger.Debug().Msg("No eligible users or teams found for review, or reviewers were assigned during processing")
+	}
 	return nil
 }


Test patch:
diff --git a/policy/reviewer/reviewer_test.go b/policy/reviewer/reviewer_test.go
index d647aea..523d96a 100644
--- a/policy/reviewer/reviewer_test.go
+++ b/policy/reviewer/reviewer_test.go
@@ -30,14 +30,14 @@ import (
 )
 
 func TestFindLeafResults(t *testing.T) {
-	results := makeResults(&common.Result{
+	result := makeResult(&common.Result{
 		Name:        "Skipped",
 		Description: "",
 		Status:      common.StatusSkipped,
 		Error:       nil,
 		Children:    nil,
 	}, "random-users")
-	actualResults := findLeafChildren(results)
+	actualResults := FindRequests(result)
 	require.Len(t, actualResults, 2, "incorrect number of leaf results")
 }
 
@@ -75,9 +75,10 @@ func TestSelectReviewers(t *testing.T) {
 		Name:        "Owner",
 		Description: "",
 		Status:      common.StatusPending,
-		ReviewRequestRule: common.ReviewRequestRule{
+		ReviewRequestRule: &common.ReviewRequestRule{
 			Admins:        true,
 			RequiredCount: 1,
+			Mode:          common.RequestModeRandomUsers,
 		},
 		Error:    nil,
 		Children: nil,
@@ -100,7 +101,7 @@ func TestSelectAdminTeam(t *testing.T) {
 		Name:        "Owner",
 		Description: "",
 		Status:      common.StatusPending,
-		ReviewRequestRule: common.ReviewRequestRule{
+		ReviewRequestRule: &common.ReviewRequestRule{
 			Admins:        true,
 			RequiredCount: 1,
 			Mode:          "teams",
@@ -125,10 +126,11 @@ func TestSelectReviewers_Team(t *testing.T) {
 		Name:        "Team",
 		Description: "",
 		Status:      common.StatusPending,
-		ReviewRequestRule: common.ReviewRequestRule{
+		ReviewRequestRule: &common.ReviewRequestRule{
 			// Require a team approval
 			Teams:         []string{"everyone/team-write"},
 			RequiredCount: 1,
+			Mode:          common.RequestModeRandomUsers,
 		},
 		Error:    nil,
 		Children: nil,
@@ -151,7 +153,7 @@ func TestSelectReviewers_Team_teams(t *testing.T) {
 		Name:        "Team",
 		Description: "",
 		Status:      common.StatusPending,
-		ReviewRequestRule: common.ReviewRequestRule{
+		ReviewRequestRule: &common.ReviewRequestRule{
 			// Require a team approval
 			Teams:         []string{"everyone/team-write", "everyone/team-not-collaborators"},
 			Users:         []string{"user-team-write"},
@@ -180,7 +182,7 @@ func TestSelectReviewers_Team_teamsDefaultsToNothing(t *testing.T) {
 		Name:        "Team",
 		Description: "",
 		Status:      common.StatusPending,
-		ReviewRequestRule: common.ReviewRequestRule{
+		ReviewRequestRule: &common.ReviewRequestRule{
 			// Require a team approval
 			Teams:         []string{"everyone/team-not-collaborators"},
 			Users:         []string{"user-team-write"},
@@ -204,10 +206,11 @@ func TestSelectReviewers_Org(t *testing.T) {
 		Name:        "Team",
 		Description: "",
 		Status:      common.StatusPending,
-		ReviewRequestRule: common.ReviewRequestRule{
+		ReviewRequestRule: &common.ReviewRequestRule{
 			// Require everyone org approval
 			Organizations: []string{"everyone"},
 			RequiredCount: 1,
+			Mode:          common.RequestModeRandomUsers,
 		},
 		Error:    nil,
 		Children: nil,
@@ -222,35 +225,40 @@ func TestSelectReviewers_Org(t *testing.T) {
 	require.NotContains(t, reviewers, "not-a-collaborator", "a non collaborator cannot be requested")
 }
 
-func makeResults(result *common.Result, mode string) common.Result {
-	results := common.Result{
+func makeResults(result *common.Result, mode string) []*common.Result {
+	return FindRequests(makeResult(result, mode))
+}
+
+func makeResult(result *common.Result, mode string) *common.Result {
+	return &common.Result{
 		Name:        "One",
 		Description: "",
 		Status:      common.StatusPending,
-		ReviewRequestRule: common.ReviewRequestRule{
+		ReviewRequestRule: &common.ReviewRequestRule{
 			Users:         []string{"neverappears"},
 			RequiredCount: 0,
 			Mode:          common.RequestMode(mode),
 		},
 		Error: nil,
-		Children: []*common.Result{{
-			Name:        "Two",
-			Description: "",
-			Status:      common.StatusPending,
-			ReviewRequestRule: common.ReviewRequestRule{
-				Users:         []string{"mhaypenny", "review-approver"},
-				RequiredCount: 1,
-				Mode:          common.RequestMode(mode),
+		Children: []*common.Result{
+			{
+				Name:        "Two",
+				Description: "",
+				Status:      common.StatusPending,
+				ReviewRequestRule: &common.ReviewRequestRule{
+					Users:         []string{"mhaypenny", "review-approver"},
+					RequiredCount: 1,
+					Mode:          common.RequestMode(mode),
+				},
+				Error:    nil,
+				Children: nil,
 			},
-			Error:    nil,
-			Children: nil,
-		},
 			result,
 			{
 				Name:              "Three",
 				Description:       "",
 				Status:            common.StatusDisapproved,
-				ReviewRequestRule: common.ReviewRequestRule{},
+				ReviewRequestRule: &common.ReviewRequestRule{},
 				Error:             errors.New("foo"),
 				Children:          nil,
 			},
@@ -258,27 +266,27 @@ func makeResults(result *common.Result, mode string) common.Result {
 				Name:              "Four",
 				Description:       "",
 				Status:            common.StatusPending,
-				ReviewRequestRule: common.ReviewRequestRule{},
+				ReviewRequestRule: &common.ReviewRequestRule{},
 				Error:             nil,
-				Children: []*common.Result{{
-					Name:        "Five",
-					Description: "",
-					Status:      common.StatusPending,
-					ReviewRequestRule: common.ReviewRequestRule{
-						Users:              []string{"contributor-committer", "contributor-author", "not-a-collaborator"},
-						RequiredCount:      1,
-						WriteCollaborators: true,
-						Admins:             false,
-						Mode:               common.RequestMode(mode),
+				Children: []*common.Result{
+					{
+						Name:        "Five",
+						Description: "",
+						Status:      common.StatusPending,
+						ReviewRequestRule: &common.ReviewRequestRule{
+							Users:              []string{"contributor-committer", "contributor-author", "not-a-collaborator"},
+							RequiredCount:      1,
+							WriteCollaborators: true,
+							Admins:             false,
+							Mode:               common.RequestMode(mode),
+						},
+						Error:    nil,
+						Children: nil,
 					},
-					Error:    nil,
-					Children: nil,
-				},
 				},
 			},
 		},
 	}
-	return results
 }
 
 func makeContext() pull.Context {