skip reviewers who are no longer in the organization

bot/internal/bot/assign.go

@@ -88,6 +88,11 @@ func (b *Bot) backportReviewers(ctx context.Context) ([]string, error) {
 
 	var originalReviewers []string
 
+	currentOrgUserMap, err := b.makeMapOfExistingMembers(ctx, b.c.Environment.Organization)
+	if err != nil {
+		return nil, trace.Wrap(err)
+	}
+
 	// Append list of reviewers that have yet to submit a review.
 	reviewers, err := b.c.GitHub.ListReviewers(ctx,
 		b.c.Environment.Organization,
@@ -98,7 +103,10 @@ func (b *Bot) backportReviewers(ctx context.Context) ([]string, error) {
 	}
 	for _, reviewer := range reviewers {
 		// Don't request reviews from bots.
-		if !strings.Contains(reviewer, "[bot]") {
+		if strings.Contains(reviewer, "[bot]") {
+			continue
+		}
+		if _, found := currentOrgUserMap[reviewer]; found {
 			originalReviewers = append(originalReviewers, reviewer)
 		}
 	}
@@ -111,9 +119,14 @@ func (b *Bot) backportReviewers(ctx context.Context) ([]string, error) {
 	if err != nil {
 		return nil, trace.Wrap(err)
 	}
+
 	for _, review := range reviews {
 		// Don't request reviews from bots.
-		if !strings.Contains(review.Author, "[bot]") {
+		if strings.Contains(review.Author, "[bot]") {
+			continue
+		}
+
+		if _, found := currentOrgUserMap[review.Author]; found {
 			originalReviewers = append(originalReviewers, review.Author)
 		}
 	}
@@ -170,6 +183,22 @@ func (b *Bot) findOriginalPR(ctx context.Context, organization string, repositor
 	return n, nil
 }
 
+// makeMapOfExistingMembers fetches all members in the provided org, and builds a map of users for fast lookups
+func (b *Bot) makeMapOfExistingMembers(ctx context.Context, organization string) (map[string]bool, error) {
+	currentOrgMemberUserNameList, err := b.c.GitHub.FetchAllOrgMembers(ctx, organization)
+	if err != nil {
+		log.Printf("Assign: Failed to fetch org members %v: %v.", organization, err)
+		return nil, trace.Wrap(err)
+	}
+
+	memberLoginToIsExistingOrgUser := make(map[string]bool)
+	for _, currentOrgMember := range currentOrgMemberUserNameList {
+		memberLoginToIsExistingOrgUser[currentOrgMember] = true
+	}
+
+	return memberLoginToIsExistingOrgUser, nil
+}
+
 func dedup(author string, reviewers []string) []string {
 	m := map[string]bool{}
 

bot/internal/bot/assign_test.go

@@ -39,117 +39,103 @@ func TestBackportReviewers(t *testing.T) {
 	})
 	require.NoError(t, err)
 
+	const (
+		botUser           = "espa[bot]lini"
+		luke              = "luke"
+		anakin            = "anakin"
+		palpatine         = "palpatine"
+		noLongerInOrgUser = "reviewer-no-longer-in-org-user"
+		author            = "someUserForTest"
+	)
+
 	tests := []struct {
-		desc      string
-		pull      github.PullRequest
-		reviewers []string
-		reviews   []github.Review
-		err       bool
-		expected  []string
+		desc       string
+		pull       github.PullRequest
+		reviewers  []string
+		reviews    []github.Review
+		orgMembers []string
+		err        bool
+		expected   []string
 	}{
 		{
-			desc: "backport-original-pr-number-approved",
-			pull: github.PullRequest{
-				Author:     "baz",
-				Repository: "bar",
-				UnsafeHead: github.Branch{
-					Ref: "baz/fix",
-				},
-				UnsafeTitle: "Backport #0 to branch/v8",
-				UnsafeBody:  "",
-				Fork:        false,
-			},
-			reviewers: []string{"3", "this is a bot[bot]"},
+			desc:      "backport-original-pr-number-approved",
+			pull:      pr("Backport #0 to branch/v8", ""),
+			reviewers: []string{luke, botUser},
 			reviews: []github.Review{
-				{Author: "4", State: review.Approved},
+				{Author: anakin, State: review.Approved},
 			},
-			err:      false,
-			expected: []string{"3", "4"},
+			orgMembers: []string{author, luke, anakin},
+			err:        false,
+			expected:   []string{luke, anakin},
 		},
 		{
-			desc: "backport-original-url-approved",
-			pull: github.PullRequest{
-				Author:     "baz",
-				Repository: "bar",
-				UnsafeHead: github.Branch{
-					Ref: "baz/fix",
-				},
-				UnsafeTitle: "Fixed an issue",
-				UnsafeBody:  "https://github.com/gravitational/teleport/pull/0",
-				Fork:        false,
-			},
-			reviewers: []string{"3"},
+			desc:      "backport-original-url-approved",
+			pull:      pr("Fixed an issue", "https://github.com/gravitational/teleport/pull/0"),
+			reviewers: []string{luke},
 			reviews: []github.Review{
-				{Author: "4", State: review.Approved},
+				{Author: anakin, State: review.Approved},
 			},
-			err:      false,
-			expected: []string{"3", "4"},
+			orgMembers: []string{author, luke, anakin},
+			err:        false,
+			expected:   []string{luke, anakin},
 		},
 		{
-			desc: "backport-reviewed-by-bot",
-			pull: github.PullRequest{
-				Author:     "baz",
-				Repository: "bar",
-				UnsafeHead: github.Branch{
-					Ref: "baz/fix",
-				},
-				UnsafeTitle: "Fixed an issue",
-				UnsafeBody:  "https://github.com/gravitational/teleport/pull/0",
-				Fork:        false,
+			desc:      "backport-reviewed-by-bot",
+			pull:      pr("Fixed an issue", "https://github.com/gravitational/teleport/pull/0"),
+			reviewers: []string{luke},
+			reviews: []github.Review{
+				{Author: anakin, State: review.Approved},
+				{Author: botUser, State: review.Approved},
 			},
+			orgMembers: []string{author, luke, anakin},
+			err:        false,
+			expected:   []string{luke, anakin},
+		},
+		{
+			desc:      "backport-multiple-reviews",
+			pull:      pr("Fixed an issue", ""),
 			reviewers: []string{"3"},
 			reviews: []github.Review{
-				{Author: "4", State: review.Approved},
-				{Author: "espa[bot]lini", State: review.Approved},
+				{Author: anakin, State: review.Commented},
+				{Author: anakin, State: review.ChangesRequested},
+				{Author: anakin, State: review.Approved},
+				{Author: palpatine, State: review.Approved},
 			},
-			err:      false,
-			expected: []string{"3", "4"},
+			orgMembers: []string{author, luke, anakin},
+			err:        true,
+			expected:   []string{},
 		},
 		{
-			desc: "backport-multiple-reviews",
-			pull: github.PullRequest{
-				Author:     "baz",
-				Repository: "bar",
-				UnsafeHead: github.Branch{
-					Ref: "baz/fix",
-				},
-
-				UnsafeTitle: "Fixed feature",
-				UnsafeBody:  "",
-				Fork:        false,
-			},
-			reviewers: []string{"3"},
+			desc:       "backport-original-not-found",
+			pull:       pr("Fixed feature", ""),
+			orgMembers: []string{anakin, author},
+			reviewers:  []string{luke},
 			reviews: []github.Review{
-				{Author: "4", State: review.Commented},
-				{Author: "4", State: review.ChangesRequested},
-				{Author: "4", State: review.Approved},
-				{Author: "9", State: review.Approved},
+				{Author: anakin, State: review.Approved},
 			},
 			err:      true,
 			expected: []string{},
 		},
 		{
-			desc: "backport-original-not-found",
-			pull: github.PullRequest{
-				Author:     "baz",
-				Repository: "bar",
-				UnsafeHead: github.Branch{
-					Ref: "baz/fix",
-				},
-				UnsafeTitle: "Fixed feature",
-				UnsafeBody:  "",
-				Fork:        false,
-			},
-			reviewers: []string{"3"},
+			desc:       "backport-reviewer-left-org",
+			pull:       pr("Backport #0 to branch/v8", ""),
+			orgMembers: []string{luke, anakin, author},
+			reviewers:  []string{luke, anakin, noLongerInOrgUser},
 			reviews: []github.Review{
-				{Author: "4", State: review.Approved},
+				{Author: luke, State: review.Approved},
+				{Author: anakin, State: review.Approved},
+				{Author: noLongerInOrgUser, State: review.Approved},
 			},
-			err:      true,
-			expected: []string{},
+			err:      false,
+			expected: []string{luke, anakin},
 		},
 	}
 	for _, test := range tests {
 		t.Run(test.desc, func(t *testing.T) {
+			githubUserIDToIsMember := make(map[string]struct{})
+			for _, member := range test.orgMembers {
+				githubUserIDToIsMember[member] = struct{}{}
+			}
 			b := &Bot{
 				c: &Config{
 					Environment: &env.Environment{
@@ -162,9 +148,10 @@ func TestBackportReviewers(t *testing.T) {
 					},
 					Review: r,
 					GitHub: &fakeGithub{
-						pull:      test.pull,
-						reviewers: test.reviewers,
-						reviews:   test.reviews,
+						pull:       test.pull,
+						reviewers:  test.reviewers,
+						reviews:    test.reviews,
+						orgMembers: githubUserIDToIsMember,
 					},
 				},
 			}
@@ -174,3 +161,16 @@ func TestBackportReviewers(t *testing.T) {
 		})
 	}
 }
+
+func pr(title, body string) github.PullRequest {
+	return github.PullRequest{
+		Author:     "baz",
+		Repository: "repositoryNameForTest",
+		UnsafeHead: github.Branch{
+			Ref: "baz/fix",
+		},
+		UnsafeTitle: title,
+		UnsafeBody:  body,
+		Fork:        false,
+	}
+}

bot/internal/bot/backport.go

@@ -301,7 +301,7 @@ func git(args ...string) error {
 	cmd := exec.Command("git", args...)
 	out, err := cmd.CombinedOutput()
 	if err != nil {
-		return trace.BadParameter(string(bytes.TrimSpace(out)))
+		return trace.BadParameter("%s", string(bytes.TrimSpace(out)))
 	}
 	return nil
 }

bot/internal/bot/bot.go

@@ -95,6 +95,9 @@ type Client interface {
 
 	// ListCommitFiles returns all filenames recursively from the tree at a given commit SHA whose prefix matches pathPrefix.
 	ListCommitFiles(ctx context.Context, organization string, repository string, sha string, path string) ([]string, error)
+
+	// FetchAllOrgMembers fetches all members in the provided org, it will continuously execute requests until it fetches all members
+	FetchAllOrgMembers(ctx context.Context, org string) ([]string, error)
 }
 
 // Config contains configuration for the bot.

bot/internal/bot/bot_test.go

@@ -536,6 +536,14 @@ func (f *fakeGithub) ListCommitFiles(ctx context.Context, organization string, r
 	return f.commitFiles, nil
 }
 
+func (f *fakeGithub) FetchAllOrgMembers(ctx context.Context, org string) ([]string, error) {
+	orgMemberUserNameList := make([]string, 0)
+	for member := range f.orgMembers {
+		orgMemberUserNameList = append(orgMemberUserNameList, member)
+	}
+	return orgMemberUserNameList, nil
+}
+
 func TestSkipFileForSizeCheck(t *testing.T) {
 	generatedFilePaths := []string{
 		// go types from proto

bot/internal/github/github.go

@@ -783,6 +783,36 @@ func (c *Client) ListCommitFiles(ctx context.Context, organization string, repos
 	return findTreeBlobEntries(tree, pathPrefix), nil
 }
 
+func (c *Client) FetchAllOrgMembers(ctx context.Context, org string) ([]string, error) {
+	var allMembers []*go_github.User
+
+	opts := &go_github.ListMembersOptions{
+		ListOptions: go_github.ListOptions{PerPage: perPageFetchAllMembers},
+	}
+
+	for {
+		members, resp, err := c.client.Organizations.ListMembers(ctx, org, opts)
+		if err != nil {
+			return nil, trace.Wrap(err)
+		}
+
+		allMembers = append(allMembers, members...)
+
+		if resp.NextPage == 0 {
+			break
+		}
+
+		opts.Page = resp.NextPage
+	}
+
+	allMemberUsernames := make([]string, 0, len(allMembers))
+	for _, member := range allMembers {
+		allMemberUsernames = append(allMemberUsernames, member.GetLogin())
+	}
+
+	return allMemberUsernames, nil
+}
+
 // findTreeBlobEntries returns all blob entries in tree whose path matches pathPrefix.
 // All blob entries are returned when pathPrefix is empty. Called bby GetCommitFilenames.
 func findTreeBlobEntries(tree *go_github.Tree, pathPrefix string) []string {
@@ -809,5 +839,6 @@ type Job struct {
 
 const (
 	// perPage is the number of items per page to request.
-	perPage = 100
+	perPage                = 100
+	perPageFetchAllMembers = 100
 )