Repository: palantir/policy-bot
Pull request #215 (2020-08-04 20:08:40)

Use v3 API to list repository collaborators
We don't use any v4-only fields for this operation and using normal REST
requests lets us cache the responses in the global HTTP cache.


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/pull/github.go b/pull/github.go
index ff31df6..e2c6aea 100644
--- a/pull/github.go
+++ b/pull/github.go
@@ -269,13 +269,44 @@ func (ghc *GitHubContext) Reviews() ([]*Review, error) {
 
 func (ghc *GitHubContext) RepositoryCollaborators() (map[string]string, error) {
 	if ghc.collaborators == nil {
-		if err := ghc.loadCollaboratorData(); err != nil {
-			return nil, err
+		opts := github.ListCollaboratorsOptions{
+			ListOptions: github.ListOptions{
+				PerPage: 100,
+			},
+		}
+
+		collaborators := make(map[string]string)
+		for {
+			users, res, err := ghc.client.Repositories.ListCollaborators(ghc.ctx, ghc.owner, ghc.repo, &opts)
+			if err != nil {
+				return nil, errors.Wrap(err, "failed to list repository collaborators")
+			}
+			for _, u := range users {
+				collaborators[u.GetLogin()] = coalescePermission(u.GetPermissions())
+			}
+			if res.NextPage == 0 {
+				break
+			}
+			opts.Page = res.NextPage
 		}
+		ghc.collaborators = collaborators
 	}
 	return ghc.collaborators, nil
 }
 
+func coalescePermission(perms map[string]bool) string {
+	// standardize to new-style values used by GraphQL and other endpoints
+	switch {
+	case perms["admin"]:
+		return "admin"
+	case perms["push"] || perms["write"]:
+		return "write"
+	case perms["pull"] || perms["read"]:
+		return "read"
+	}
+	return "none"
+}
+
 func (ghc *GitHubContext) HasReviewers() (bool, error) {
 	// Intentionally kept to a small result size, since we just want to determine if there are existing reviewers
 	subsetCurrentReviewers, _, err := ghc.client.PullRequests.ListReviewers(ghc.ctx, ghc.owner, ghc.repo, ghc.number, &github.ListOptions{
@@ -464,44 +495,6 @@ func (ghc *GitHubContext) loadPagedData() error {
 	return nil
 }
 
-func (ghc *GitHubContext) loadCollaboratorData() error {
-	var q struct {
-		Repository struct {
-			Collaborators struct {
-				PageInfo v4PageInfo
-				Edges    []struct {
-					Permission string  `graphql:"permission"`
-					Node       v4Actor `graphql:"node"`
-				}
-			} `graphql:"collaborators(first: 100, after: $collaboratorCursor)"`
-		} `graphql:"repository(owner: $owner, name: $name)"`
-	}
-
-	qvars := map[string]interface{}{
-		"owner": githubv4.String(ghc.owner),
-		"name":  githubv4.String(ghc.repo),
-
-		"collaboratorCursor": (*githubv4.String)(nil),
-	}
-
-	collaborators := make(map[string]string)
-	for {
-		if err := ghc.v4client.Query(ghc.ctx, &q, qvars); err != nil {
-			return errors.Wrap(err, "failed to load pull request data")
-		}
-
-		for _, c := range q.Repository.Collaborators.Edges {
-			collaborators[c.Node.Login] = strings.ToLower(c.Permission)
-		}
-		if !q.Repository.Collaborators.PageInfo.UpdateCursor(qvars, "collaboratorCursor") {
-			break
-		}
-	}
-
-	ghc.collaborators = collaborators
-	return nil
-}
-
 func (ghc *GitHubContext) loadCommits() ([]*Commit, error) {
 	log := zerolog.Ctx(ghc.ctx)
 
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 {