Check GH Automation: check org membership as an alternative to repo c…

…ollaborators (#2874)

* check org membership as an alternative to repo collaborators

* check org membership as an alternative to repo collaborators

cmd/check-gh-automation/main.go

@@ -63,6 +63,7 @@ func (o *options) validate() error {
 }
 
 type collaboratorClient interface {
+	IsMember(org, user string) (bool, error)
 	IsCollaborator(org, repo, user string) (bool, error)
 }
 
@@ -103,6 +104,7 @@ func main() {
 }
 
 func checkRepos(repos []string, bots []string, ignore sets.String, client collaboratorClient, logger *logrus.Entry) ([]string, error) {
+	logger.Infof("checking %d repo(s): %s", len(repos), strings.Join(repos, ", "))
 	var failing []string
 	for _, orgRepo := range repos {
 		split := strings.Split(orgRepo, "/")
@@ -119,6 +121,15 @@ func checkRepos(repos []string, bots []string, ignore sets.String, client collab
 
 		var missingBots []string
 		for _, bot := range bots {
+			isMember, err := client.IsMember(org, bot)
+			if err != nil {
+				return nil, fmt.Errorf("unable to determine if: %s is a member of %s: %v", bot, org, err)
+			}
+			if isMember {
+				repoLogger.WithField("bot", bot).Info("bot is an org member")
+				continue
+			}
+
 			isCollaborator, err := client.IsCollaborator(org, repo, bot)
 			if err != nil {
 				return nil, fmt.Errorf("unable to determine if: %s is a collaborator on %s/%s: %v", bot, org, repo, err)
@@ -132,7 +143,7 @@ func checkRepos(repos []string, bots []string, ignore sets.String, client collab
 			failing = append(failing, orgRepo)
 			repoLogger.Errorf("bots that are not collaborators: %s", strings.Join(missingBots, ", "))
 		} else {
-			repoLogger.Info("all bots are collaborators")
+			repoLogger.Info("all bots are org members or repo collaborators")
 		}
 	}
 

cmd/check-gh-automation/main_test.go

@@ -14,12 +14,26 @@ import (
 )
 
 type fakeCollaboratorClient struct {
-	repos map[string][]string
+	collaboratorsByRepo map[string][]string
+	membersByOrg        map[string][]string
+}
+
+func (c fakeCollaboratorClient) IsMember(org, user string) (bool, error) {
+	for _, member := range c.membersByOrg[org] {
+		if user == member {
+			return true, nil
+		}
+	}
+	if org == "fake" {
+		return false, errors.New("intentional error")
+	}
+
+	return false, nil
 }
 
 func (c fakeCollaboratorClient) IsCollaborator(org, repo, user string) (bool, error) {
 	orgRepo := fmt.Sprintf("%s/%s", org, repo)
-	collaborators := c.repos[orgRepo]
+	collaborators := c.collaboratorsByRepo[orgRepo]
 	for _, collaborator := range collaborators {
 		if collaborator == user {
 			return true, nil
@@ -33,10 +47,15 @@ func (c fakeCollaboratorClient) IsCollaborator(org, repo, user string) (bool, er
 }
 
 func TestCheckRepos(t *testing.T) {
-	client := fakeCollaboratorClient{repos: map[string][]string{
-		"org-1/repo-a": {"a-bot", "b-bot", "c-bot"},
-		"org-2/repo-z": {"c-bot", "some-user"},
-	}}
+	client := fakeCollaboratorClient{
+		collaboratorsByRepo: map[string][]string{
+			"org-1/repo-a": {"a-bot", "b-bot", "c-bot"},
+			"org-2/repo-z": {"c-bot", "some-user"},
+		},
+		membersByOrg: map[string][]string{
+			"org-1": {"a-user", "d-bot", "e-bot"},
+			"org-2": {"some-user", "z-bot"},
+		}}
 
 	testCases := []struct {
 		name        string
@@ -46,13 +65,23 @@ func TestCheckRepos(t *testing.T) {
 		expected    []string
 		expectedErr error
 	}{
+		{
+			name:  "org has bots as members",
+			repos: []string{"org-1/repo-a"},
+			bots:  []string{"d-bot", "e-bot"},
+		},
+		{
+			name:  "org has one bot as member, and one as collaborator",
+			repos: []string{"org-1/repo-a"},
+			bots:  []string{"a-bot", "e-bot"},
+		},
 		{
 			name:  "repo has bots as collaborators",
 			repos: []string{"org-1/repo-a"},
 			bots:  []string{"a-bot", "b-bot"},
 		},
 		{
-			name:     "repo doesn't have bots as collaborators",
+			name:     "org doesn't have bots as members, and repo doesn't have bots as collaborators",
 			repos:    []string{"org-2/repo-z"},
 			bots:     []string{"a-bot", "b-bot"},
 			expected: []string{"org-2/repo-z"},
@@ -75,6 +104,12 @@ func TestCheckRepos(t *testing.T) {
 			bots:   []string{"a-bot", "b-bot"},
 			ignore: sets.NewString("org-2"),
 		},
+		{
+			name:        "org member check returns error",
+			repos:       []string{"fake/repo"},
+			bots:        []string{"a-bot"},
+			expectedErr: errors.New("unable to determine if: a-bot is a member of fake: intentional error"),
+		},
 		{
 			name:        "collaborator check returns error",
 			repos:       []string{"org-1/fake"},