Merge branch 'main' into main

.ci/containers/membership-checker/REVIEWER_ASSIGNMENT_COMMENT.md

@@ -1,3 +1,5 @@
-Hello! I am a robot. It looks like you are a community contributor. @{{reviewer}}, a repository maintainer, has been assigned to review your changes. If you have not received review feedback within 2 business days, please leave a comment on this PR asking them to take a look.
+Hello! I am a robot. It looks like you are a: {{if eq .authorUserType "Community Contributor"}}Community Contributor{{else}}~Community Contributor~{{end}} {{if eq .authorUserType "Googler"}}Googler{{else}}~Googler~{{end}} {{if eq .authorUserType "Core Contributor"}}Core Contributor{{else}}~Core Contributor~{{end}}. {{if .trusted}}Tests will run automatically.{{else}}Tests will require approval to run.{{end}}
+
+@{{.reviewer}}, a repository maintainer, has been assigned to review your changes. If you have not received review feedback within 2 business days, please leave a comment on this PR asking them to take a look.
 
 You can help make sure that review is quick by [doing a self-review](https://googlecloudplatform.github.io/magic-modules/contribute/review-pr/) and by [running impacted tests locally](https://googlecloudplatform.github.io/magic-modules/get-started/run-provider-tests/).

.ci/containers/membership-checker/main.go

@@ -54,15 +54,43 @@ func main() {
 		os.Exit(1)
 	}
 
-	if target == "auto_run" {
-		err = requestReviewer(author, prNumber, GITHUB_TOKEN)
+	authorUserType := getUserType(author, GITHUB_TOKEN)
+	trusted := authorUserType == coreContributorUserType || authorUserType == googlerUserType
+
+	if target == "auto_run" && authorUserType != coreContributorUserType {
+		fmt.Println("Not core contributor - assigning reviewer")
+
+		firstRequestedReviewer, err := getPullRequestRequestedReviewer(prNumber, GITHUB_TOKEN)
 		if err != nil {
 			fmt.Println(err)
 			os.Exit(1)
 		}
-	}
 
-	trusted := isTrustedUser(author, GITHUB_TOKEN)
+		previouslyInvolvedReviewers, err := getPullRequestPreviousAssignedReviewers(prNumber, GITHUB_TOKEN)
+		if err != nil {
+			fmt.Println(err)
+			os.Exit(1)
+		}
+
+		reviewersToRequest, newPrimaryReviewer := chooseReviewers(firstRequestedReviewer, previouslyInvolvedReviewers)
+
+		for _, reviewer := range reviewersToRequest {
+			err = requestPullRequestReviewer(prNumber, reviewer, GITHUB_TOKEN)
+			if err != nil {
+				fmt.Println(err)
+				os.Exit(1)
+			}
+		}
+
+		if newPrimaryReviewer != "" {
+			comment := formatReviewerComment(newPrimaryReviewer, authorUserType, trusted)
+			err = postComment(prNumber, comment, GITHUB_TOKEN, authorUserType)
+			if err != nil {
+				fmt.Println(err)
+				os.Exit(1)
+			}
+		}
+	}
 
 	// auto_run(contributor-membership-checker) will be run on every commit or /gcbrun:
 	// only triggers builds for trusted users

.ci/containers/membership-checker/membership.go

@@ -30,13 +30,31 @@ var (
 
 	// This is for reviewers who are "on vacation": will not receive new review assignments but will still receive re-requests for assigned PRs.
 	onVacationReviewers = []string{
-		"SarahFrench",
-		"roaks3",
 		"rileykarson",
 		"shuyama1",
+		"NickElliot",
 	}
 )
 
+type userType int64
+
+const (
+	communityUserType userType = iota
+	googlerUserType
+	coreContributorUserType
+)
+
+func (ut userType) String() string {
+	switch ut {
+	case googlerUserType:
+		return "Googler"
+	case coreContributorUserType:
+		return "Core Contributor"
+	default:
+		return "Community Contributor"
+	}
+}
+
 // Check if a user is team member to not request a random reviewer
 func isTeamMember(author string) bool {
 	return slices.Contains(reviewerRotation, author) || slices.Contains(trustedContributors, author)
@@ -46,24 +64,23 @@ func isTeamReviewer(reviewer string) bool {
 	return slices.Contains(reviewerRotation, reviewer)
 }
 
-// Check if a user is safe to run tests automatically
-func isTrustedUser(author, GITHUB_TOKEN string) bool {
-	if isTeamMember(author) {
+func getUserType(user, GITHUB_TOKEN string) userType {
+	if isTeamMember(user) {
 		fmt.Println("User is a team member")
-		return true
+		return coreContributorUserType
 	}
 
-	if isOrgMember(author, "GoogleCloudPlatform", GITHUB_TOKEN) {
+	if isOrgMember(user, "GoogleCloudPlatform", GITHUB_TOKEN) {
 		fmt.Println("User is a GCP org member")
-		return true
+		return googlerUserType
 	}
 
-	if isOrgMember(author, "googlers", GITHUB_TOKEN) {
+	if isOrgMember(user, "googlers", GITHUB_TOKEN) {
 		fmt.Println("User is a googlers org member")
-		return true
+		return googlerUserType
 	}
 
-	return false
+	return communityUserType
 }
 
 func isOrgMember(author, org, GITHUB_TOKEN string) bool {

.ci/containers/membership-checker/reviewer_assignment.go

@@ -4,6 +4,7 @@ import (
 	"fmt"
 	"net/http"
 	"strings"
+	"text/template"
 
 	_ "embed"
 )
@@ -13,48 +14,30 @@ var (
 	reviewerAssignmentComment string
 )
 
-func requestReviewer(author, prNumber, GITHUB_TOKEN string) error {
-	if isTeamMember(author) {
-		fmt.Println("author is a team member, not assigning")
-		return nil
-	}
-
-	firstRequestedReviewer, err := getPullRequestRequestedReviewer(prNumber, GITHUB_TOKEN)
-	if err != nil {
-		return err
-	}
-
-	previouslyInvolvedReviewers, err := getPullRequestPreviousAssignedReviewers(prNumber, GITHUB_TOKEN)
-	if err != nil {
-		return err
-	}
-
-	foundTeamReviewer := false
+// Returns a list of users to request review from, as well as a new primary reviewer if this is the first run.
+func chooseReviewers(firstRequestedReviewer string, previouslyInvolvedReviewers []string) (reviewersToRequest []string, newPrimaryReviewer string) {
+	hasPrimaryReviewer := false
+	newPrimaryReviewer = ""
 
 	if firstRequestedReviewer != "" {
-		foundTeamReviewer = true
+		hasPrimaryReviewer = true
 	}
 
 	if previouslyInvolvedReviewers != nil {
 		for _, reviewer := range previouslyInvolvedReviewers {
 			if isTeamReviewer(reviewer) {
-				foundTeamReviewer = true
-				err = requestPullRequestReviewer(prNumber, reviewer, GITHUB_TOKEN)
-				if err != nil {
-					return err
-				}
+				hasPrimaryReviewer = true
+				reviewersToRequest = append(reviewersToRequest, reviewer)
 			}
 		}
 	}
 
-	if !foundTeamReviewer {
-		err = requestRandomReviewer(prNumber, GITHUB_TOKEN)
-		if err != nil {
-			return err
-		}
+	if !hasPrimaryReviewer {
+		newPrimaryReviewer = getRandomReviewer()
+		reviewersToRequest = append(reviewersToRequest, newPrimaryReviewer)
 	}
 
-	return nil
+	return reviewersToRequest, newPrimaryReviewer
 }
 
 func getPullRequestAuthor(prNumber, GITHUB_TOKEN string) (string, error) {
@@ -144,25 +127,23 @@ func requestPullRequestReviewer(prNumber, assignee, GITHUB_TOKEN string) error {
 	return nil
 }
 
-func requestRandomReviewer(prNumber, GITHUB_TOKEN string) error {
-	assignee := getRandomReviewer()
-	err := requestPullRequestReviewer(prNumber, assignee, GITHUB_TOKEN)
-	if err != nil {
-		return err
-	}
-	err = postComment(prNumber, assignee, GITHUB_TOKEN)
+func formatReviewerComment(newPrimaryReviewer string, authorUserType userType, trusted bool) string {
+	tmpl, err := template.New("REVIEWER_ASSIGNMENT_COMMENT.md").Parse(reviewerAssignmentComment)
 	if err != nil {
-		return err
-	}
-	return nil
-
+		panic(fmt.Sprintf("Unable to parse REVIEWER_ASSIGNMENT_COMMENT.md: %s", err))
+	}
+	sb := new(strings.Builder)
+	tmpl.Execute(sb, map[string]interface{}{
+		"reviewer":       newPrimaryReviewer,
+		"authorUserType": authorUserType.String(),
+		"trusted":        trusted,
+	})
+	return sb.String()
 }
 
-func postComment(prNumber, reviewer, GITHUB_TOKEN string) error {
+func postComment(prNumber, comment, GITHUB_TOKEN string, authorUserType userType) error {
 	url := fmt.Sprintf("https://api.github.com/repos/GoogleCloudPlatform/magic-modules/issues/%s/comments", prNumber)
 
-	comment := strings.Replace(reviewerAssignmentComment, "{{reviewer}}", reviewer, 1)
-
 	body := map[string]string{
 		"body": comment,
 	}
@@ -173,10 +154,10 @@ func postComment(prNumber, reviewer, GITHUB_TOKEN string) error {
 	}
 
 	if reqStatusCode != http.StatusCreated {
-		return fmt.Errorf("Error posting reviewer assignment comment for PR %s", prNumber)
+		return fmt.Errorf("Error posting comment for PR %s", prNumber)
 	}
 
-	fmt.Printf("Successfully posted reviewer assignment comment to pull request %s\n", prNumber)
+	fmt.Printf("Successfully posted comment to pull request %s\n", prNumber)
 
 	return nil
 }

.ci/containers/membership-checker/reviewer_assignment_test.go

@@ -0,0 +1,135 @@
+package main
+
+import (
+	"fmt"
+	"strings"
+	"testing"
+
+	"golang.org/x/exp/slices"
+)
+
+func TestChooseReviewers(t *testing.T) {
+	cases := map[string]struct {
+		FirstRequestedReviewer                           string
+		PreviouslyInvolvedReviewers                      []string
+		ExpectReviewersFromList, ExpectSpecificReviewers []string
+		ExpectPrimaryReviewer                            bool
+	}{
+		"no previous review requests assigns new reviewer from team": {
+			FirstRequestedReviewer:      "",
+			PreviouslyInvolvedReviewers: []string{},
+			ExpectReviewersFromList:     removes(reviewerRotation, onVacationReviewers),
+			ExpectPrimaryReviewer:       true,
+		},
+		"first requested reviewer means that primary reviewer was already selected": {
+			FirstRequestedReviewer:      "foobar",
+			PreviouslyInvolvedReviewers: []string{},
+			ExpectPrimaryReviewer:       false,
+		},
+		"previously involved team member reviewers should have review requested and mean that primary reviewer was already selected": {
+			FirstRequestedReviewer:      "",
+			PreviouslyInvolvedReviewers: []string{reviewerRotation[0]},
+			ExpectSpecificReviewers:     []string{reviewerRotation[0]},
+			ExpectPrimaryReviewer:       false,
+		},
+		"previously involved reviewers that are not team members are ignored": {
+			FirstRequestedReviewer:      "",
+			PreviouslyInvolvedReviewers: []string{"foobar"},
+			ExpectReviewersFromList:     removes(reviewerRotation, onVacationReviewers),
+			ExpectPrimaryReviewer:       true,
+		},
+		"only previously involved team member reviewers will have review requested": {
+			FirstRequestedReviewer:      "",
+			PreviouslyInvolvedReviewers: []string{reviewerRotation[0], "foobar", reviewerRotation[1]},
+			ExpectSpecificReviewers:     []string{reviewerRotation[0], reviewerRotation[1]},
+			ExpectPrimaryReviewer:       false,
+		},
+		"primary reviewer will not have review requested even if other team members previously reviewed": {
+			FirstRequestedReviewer:      reviewerRotation[1],
+			PreviouslyInvolvedReviewers: []string{reviewerRotation[0]},
+			ExpectSpecificReviewers:     []string{reviewerRotation[0]},
+			ExpectPrimaryReviewer:       false,
+		},
+	}
+
+	for tn, tc := range cases {
+		tc := tc
+		t.Run(tn, func(t *testing.T) {
+			t.Parallel()
+			reviewers, primaryReviewer := chooseReviewers(tc.FirstRequestedReviewer, tc.PreviouslyInvolvedReviewers)
+			if tc.ExpectPrimaryReviewer && primaryReviewer == "" {
+				t.Error("wanted primary reviewer to be returned; got none")
+			}
+			if !tc.ExpectPrimaryReviewer && primaryReviewer != "" {
+				t.Errorf("wanted no primary reviewer; got %s", primaryReviewer)
+			}
+			if len(tc.ExpectReviewersFromList) > 0 {
+				for _, reviewer := range reviewers {
+					if !slices.Contains(tc.ExpectReviewersFromList, reviewer) {
+						t.Errorf("wanted reviewer %s to be in list %v but they were not", reviewer, tc.ExpectReviewersFromList)
+					}
+				}
+			}
+			if len(tc.ExpectSpecificReviewers) > 0 {
+				if !slices.Equal(reviewers, tc.ExpectSpecificReviewers) {
+					t.Errorf("wanted reviewers to be %v; instead got %v", tc.ExpectSpecificReviewers, reviewers)
+				}
+			}
+		})
+	}
+}
+
+func TestFormatReviewerComment(t *testing.T) {
+	cases := map[string]struct {
+		Reviewer       string
+		AuthorUserType userType
+		Trusted        bool
+	}{
+		"community contributor": {
+			Reviewer:       "foobar",
+			AuthorUserType: communityUserType,
+			Trusted:        false,
+		},
+		"googler": {
+			Reviewer:       "foobar",
+			AuthorUserType: googlerUserType,
+			Trusted:        true,
+		},
+		"core contributor": {
+			Reviewer:       "foobar",
+			AuthorUserType: coreContributorUserType,
+			Trusted:        true,
+		},
+	}
+
+	for tn, tc := range cases {
+		tc := tc
+		t.Run(tn, func(t *testing.T) {
+			t.Parallel()
+			comment := formatReviewerComment(tc.Reviewer, tc.AuthorUserType, tc.Trusted)
+			t.Log(comment)
+			if !strings.Contains(comment, fmt.Sprintf("@%s", tc.Reviewer)) {
+				t.Errorf("wanted comment to contain @%s; does not.", tc.Reviewer)
+			}
+			if !strings.Contains(comment, tc.AuthorUserType.String()) {
+				t.Errorf("wanted comment to contain user type (%s); does not.", tc.AuthorUserType.String())
+			}
+			if strings.Contains(comment, fmt.Sprintf("~%s~", tc.AuthorUserType.String())) {
+				t.Errorf("wanted user type (%s) in comment to not be crossed out, but it is", tc.AuthorUserType.String())
+			}
+			for _, ut := range []userType{communityUserType, googlerUserType, coreContributorUserType} {
+				if ut != tc.AuthorUserType && !strings.Contains(comment, fmt.Sprintf("~%s~", ut.String())) {
+					t.Errorf("wanted other user type (%s) in comment to be crossed out, but it is not", ut)
+				}
+			}
+
+			if tc.Trusted && !strings.Contains(comment, "Tests will run automatically") {
+				t.Errorf("wanted comment to say tests will run automatically; does not")
+			}
+			if !tc.Trusted && !strings.Contains(comment, "Tests will require approval") {
+				t.Errorf("wanted comment to say tests will require approval; does not")
+			}
+		})
+
+	}
+}

.ci/gcb-run-rake-tests.yml

@@ -1,6 +1,6 @@
 ---
 steps:
-- name: 'gcr.io/$PROJECT_ID/downstream-builder'
+- name: 'gcr.io/graphite-docker-images/build-environment'
   id: run-rake-tests
   entrypoint: bundle
   dir: mmv1