Simplified / clarified approval flow for presubmits (#10142)

* Simplified / clarified approval flow for presubmits * Force generation * Removed unused command args and cleaned up documentation for membership-checker * Revert "Force generation" This reverts commit db32066.
GoogleCloudPlatform · Mar 8, 2024 · c6e3d15 · c6e3d15
1 parent 5989a16
commit c6e3d15
Show file tree

Hide file tree

Showing 5 changed files with 36 additions and 109 deletions.
diff --git a/.ci/gcb-contributor-membership-checker.yml b/.ci/gcb-contributor-membership-checker.yml
@@ -68,10 +68,6 @@ steps:
       - "membership-checker"
       - $_PR_NUMBER
       - $COMMIT_SHA
-      - $BRANCH_NAME
-      - $_HEAD_REPO_URL
-      - $_HEAD_BRANCH
-      - $_BASE_BRANCH
 
 availableSecrets:
   secretManager:

diff --git a/.ci/magician/cmd/community_checker.go b/.ci/magician/cmd/community_checker.go
@@ -39,11 +39,8 @@ var communityApprovalCmd = &cobra.Command{
 	6. Base Branch
 
 	The command performs the following steps:
-	1. Retrieve and print the provided pull request details.
-	2. Get the author of the pull request and determine their user type.
-	3. If the author is not a trusted user (neither a Core Contributor nor a Googler):
-			a. Trigger cloud builds with specific substitutions for the PR.
-	4. For all pull requests, the 'awaiting-approval' label is removed.
+	1. Trigger cloud presubmits with specific substitutions for the PR.
+	2. Remove the 'awaiting-approval' label from the PR.
 	`,
 	Run: func(cmd *cobra.Command, args []string) {
 		prNumber := args[0]
@@ -84,25 +81,14 @@ func execCommunityChecker(prNumber, commitSha, branchName, headRepoUrl, headBran
 		"_BASE_BRANCH":   baseBranch,
 	}
 
-	pullRequest, err := gh.GetPullRequest(prNumber)
+	// trigger presubmit builds - community-checker requires approval
+	// (explicitly or via membership-checker)
+	err := cb.TriggerMMPresubmitRuns(commitSha, substitutions)
 	if err != nil {
 		fmt.Println(err)
 		os.Exit(1)
 	}
 
-	author := pullRequest.User.Login
-	authorUserType := gh.GetUserType(author)
-	trusted := authorUserType == github.CoreContributorUserType || authorUserType == github.GooglerUserType
-
-	// only triggers build for untrusted users (because trusted users will be handled by membership-checker)
-	if !trusted {
-		err = cb.TriggerMMPresubmitRuns(commitSha, substitutions)
-		if err != nil {
-			fmt.Println(err)
-			os.Exit(1)
-		}
-	}
-
 	// in community-checker job:
 	// remove awaiting-approval label from external contributor PRs
 	gh.RemoveLabel(prNumber, "awaiting-approval")

diff --git a/.ci/magician/cmd/community_checker_test.go b/.ci/magician/cmd/community_checker_test.go
@@ -37,12 +37,16 @@ func TestExecCommunityChecker_CoreContributorFlow(t *testing.T) {
 
 	execCommunityChecker("pr1", "sha1", "branch1", "url1", "head1", "base1", gh, cb)
 
-	if _, ok := cb.calledMethods["TriggerMMPresubmitRuns"]; ok {
-		t.Fatal("Presubmit runs redundantly triggered for core contributor")
+	method := "TriggerMMPresubmitRuns"
+	expected := [][]any{{"sha1", map[string]string{"BRANCH_NAME": "branch1", "_BASE_BRANCH": "base1", "_HEAD_BRANCH": "head1", "_HEAD_REPO_URL": "url1", "_PR_NUMBER": "pr1"}}}
+	if calls, ok := cb.calledMethods[method]; !ok {
+		t.Fatal("Presubmit runs not triggered for core contributor")
+	} else if !reflect.DeepEqual(calls, expected) {
+		t.Fatalf("Wrong calls for %s, got %v, expected %v", method, calls, expected)
 	}
 
-	method := "RemoveLabel"
-	expected := [][]any{{"pr1", "awaiting-approval"}}
+	method = "RemoveLabel"
+	expected = [][]any{{"pr1", "awaiting-approval"}}
 	if calls, ok := gh.calledMethods[method]; !ok {
 		t.Fatal("awaiting-approval label not removed for PR ")
 	} else if !reflect.DeepEqual(calls, expected) {
@@ -69,12 +73,16 @@ func TestExecCommunityChecker_GooglerFlow(t *testing.T) {
 
 	execCommunityChecker("pr1", "sha1", "branch1", "url1", "head1", "base1", gh, cb)
 
-	if _, ok := cb.calledMethods["TriggerMMPresubmitRuns"]; ok {
-		t.Fatal("Presubmit runs redundantly triggered for googler")
+	method := "TriggerMMPresubmitRuns"
+	expected := [][]any{{"sha1", map[string]string{"BRANCH_NAME": "branch1", "_BASE_BRANCH": "base1", "_HEAD_BRANCH": "head1", "_HEAD_REPO_URL": "url1", "_PR_NUMBER": "pr1"}}}
+	if calls, ok := cb.calledMethods[method]; !ok {
+		t.Fatal("Presubmit runs not triggered for googler")
+	} else if !reflect.DeepEqual(calls, expected) {
+		t.Fatalf("Wrong calls for %s, got %v, expected %v", method, calls, expected)
 	}
 
-	method := "RemoveLabel"
-	expected := [][]any{{"pr1", "awaiting-approval"}}
+	method = "RemoveLabel"
+	expected = [][]any{{"pr1", "awaiting-approval"}}
 	if calls, ok := gh.calledMethods[method]; !ok {
 		t.Fatal("awaiting-approval label not removed for PR ")
 	} else if !reflect.DeepEqual(calls, expected) {

diff --git a/.ci/magician/cmd/membership_checker.go b/.ci/magician/cmd/membership_checker.go
@@ -33,65 +33,37 @@ var membershipCheckerCmd = &cobra.Command{
 	The command expects the following pull request details as arguments:
 	1. PR Number
 	2. Commit SHA
-	3. Branch Name
-	4. Head Repo URL
-	5. Head Branch
-	6. Base Branch
 
 	It then performs the following operations:
 	1. Extracts and displays the pull request details.
 	2. Fetches the author of the pull request and determines their contribution type.
-	3. If the author is not a core contributor:
-			a. Identifies the initially requested reviewer and those who previously reviewed this PR.
-			b. Determines and requests reviewers based on the above.
-			c. Posts comments tailored to the contribution type, the trust level of the contributor, and the primary reviewer.
-	4. For trusted authors (Core Contributors and Googlers):
-			a. Triggers generate-diffs using the provided PR details.
-			b. Automatically approves the community-checker run.
-	5. For external or untrusted contributors:
+	3. For trusted authors (Core Contributors and Googlers):
+			a. Automatically approves the community-checker run.
+	4. For external or untrusted contributors:
 			a. Adds the 'awaiting-approval' label.
 			b. Posts a link prompting approval for the build.
 	`,
-	Args: cobra.ExactArgs(6),
+	// This can change to cobra.ExactArgs(2) after at least a 2-week soak
+	Args: cobra.RangeArgs(2, 6),
 	Run: func(cmd *cobra.Command, args []string) {
 		prNumber := args[0]
 		fmt.Println("PR Number: ", prNumber)
 
 		commitSha := args[1]
 		fmt.Println("Commit SHA: ", commitSha)
 
-		branchName := args[2]
-		fmt.Println("Branch Name: ", branchName)
-
-		headRepoUrl := args[3]
-		fmt.Println("Head Repo URL: ", headRepoUrl)
-
-		headBranch := args[4]
-		fmt.Println("Head Branch: ", headBranch)
-
-		baseBranch := args[5]
-		fmt.Println("Base Branch: ", baseBranch)
-
 		githubToken, ok := lookupGithubTokenOrFallback("GITHUB_TOKEN_MAGIC_MODULES")
 		if !ok {
 			fmt.Println("Did not provide GITHUB_TOKEN_MAGIC_MODULES or GITHUB_TOKEN environment variables")
 			os.Exit(1)
 		}
 		gh := github.NewClient(githubToken)
 		cb := cloudbuild.NewClient()
-		execMembershipChecker(prNumber, commitSha, branchName, headRepoUrl, headBranch, baseBranch, gh, cb)
+		execMembershipChecker(prNumber, commitSha, gh, cb)
 	},
 }
 
-func execMembershipChecker(prNumber, commitSha, branchName, headRepoUrl, headBranch, baseBranch string, gh GithubClient, cb CloudbuildClient) {
-	substitutions := map[string]string{
-		"BRANCH_NAME":    branchName,
-		"_PR_NUMBER":     prNumber,
-		"_HEAD_REPO_URL": headRepoUrl,
-		"_HEAD_BRANCH":   headBranch,
-		"_BASE_BRANCH":   baseBranch,
-	}
-
+func execMembershipChecker(prNumber, commitSha string, gh GithubClient, cb CloudbuildClient) {
 	pullRequest, err := gh.GetPullRequest(prNumber)
 	if err != nil {
 		fmt.Println(err)
@@ -102,17 +74,6 @@ func execMembershipChecker(prNumber, commitSha, branchName, headRepoUrl, headBra
 	authorUserType := gh.GetUserType(author)
 	trusted := authorUserType == github.CoreContributorUserType || authorUserType == github.GooglerUserType
 
-	// auto_run(contributor-membership-checker) will be run on every commit or /gcbrun:
-	// only triggers builds for trusted users
-	if trusted {
-		err = cb.TriggerMMPresubmitRuns(commitSha, substitutions)
-		if err != nil {
-			fmt.Println(err)
-			os.Exit(1)
-		}
-	}
-
-	// in contributor-membership-checker job:
 	// 1. auto approve community-checker run for trusted users
 	// 2. add awaiting-approval label to external contributor PRs
 	if trusted {

diff --git a/.ci/magician/cmd/membership_checker_test.go b/.ci/magician/cmd/membership_checker_test.go
@@ -35,22 +35,10 @@ func TestExecMembershipChecker_CoreContributorFlow(t *testing.T) {
 		calledMethods: make(map[string][][]any),
 	}
 
-	execMembershipChecker("pr1", "sha1", "branch1", "url1", "head1", "base1", gh, cb)
+	execMembershipChecker("pr1", "sha1", gh, cb)
 
-	if _, ok := gh.calledMethods["RequestPullRequestReviewer"]; ok {
-		t.Fatal("Incorrectly requested review for core contributor")
-	}
-
-	method := "TriggerMMPresubmitRuns"
-	expected := [][]any{{"sha1", map[string]string{"BRANCH_NAME": "branch1", "_BASE_BRANCH": "base1", "_HEAD_BRANCH": "head1", "_HEAD_REPO_URL": "url1", "_PR_NUMBER": "pr1"}}}
-	if calls, ok := cb.calledMethods[method]; !ok {
-		t.Fatal("Presubmit runs not triggered for core author")
-	} else if !reflect.DeepEqual(calls, expected) {
-		t.Fatalf("Wrong calls for %s, got %v, expected %v", method, calls, expected)
-	}
-
-	method = "ApproveCommunityChecker"
-	expected = [][]any{{"pr1", "sha1"}}
+	method := "ApproveCommunityChecker"
+	expected := [][]any{{"pr1", "sha1"}}
 	if calls, ok := cb.calledMethods[method]; !ok {
 		t.Fatal("Community checker not approved for core author")
 	} else if !reflect.DeepEqual(calls, expected) {
@@ -75,18 +63,10 @@ func TestExecMembershipChecker_GooglerFlow(t *testing.T) {
 		calledMethods: make(map[string][][]any),
 	}
 
-	execMembershipChecker("pr1", "sha1", "branch1", "url1", "head1", "base1", gh, cb)
+	execMembershipChecker("pr1", "sha1", gh, cb)
 
-	method := "TriggerMMPresubmitRuns"
-	expected := [][]any{{"sha1", map[string]string{"BRANCH_NAME": "branch1", "_BASE_BRANCH": "base1", "_HEAD_BRANCH": "head1", "_HEAD_REPO_URL": "url1", "_PR_NUMBER": "pr1"}}}
-	if calls, ok := cb.calledMethods[method]; !ok {
-		t.Fatal("Presubmit runs not triggered for googler")
-	} else if !reflect.DeepEqual(calls, expected) {
-		t.Fatalf("Wrong calls for %s, got %v, expected %v", method, calls, expected)
-	}
-
-	method = "ApproveCommunityChecker"
-	expected = [][]any{{"pr1", "sha1"}}
+	method := "ApproveCommunityChecker"
+	expected := [][]any{{"pr1", "sha1"}}
 	if calls, ok := cb.calledMethods[method]; !ok {
 		t.Fatal("Community checker not approved for googler")
 	} else if !reflect.DeepEqual(calls, expected) {
@@ -110,7 +90,7 @@ func TestExecMembershipChecker_AmbiguousUserFlow(t *testing.T) {
 		calledMethods: make(map[string][][]any),
 	}
 
-	execMembershipChecker("pr1", "sha1", "branch1", "url1", "head1", "base1", gh, cb)
+	execMembershipChecker("pr1", "sha1", gh, cb)
 
 	method := "AddLabel"
 	expected := [][]any{{"pr1", "awaiting-approval"}}
@@ -131,10 +111,6 @@ func TestExecMembershipChecker_AmbiguousUserFlow(t *testing.T) {
 	if _, ok := gh.calledMethods["ApproveCommunityChecker"]; ok {
 		t.Fatal("Incorrectly approved community checker for ambiguous user")
 	}
-
-	if _, ok := gh.calledMethods["TriggerMMPresubmitRuns"]; ok {
-		t.Fatal("Incorrectly triggered presubmit runs for ambiguous user")
-	}
 }
 
 func TestExecMembershipChecker_CommentForNewPrimaryReviewer(t *testing.T) {
@@ -153,5 +129,5 @@ func TestExecMembershipChecker_CommentForNewPrimaryReviewer(t *testing.T) {
 		calledMethods: make(map[string][][]any),
 	}
 
-	execMembershipChecker("pr1", "sha1", "branch1", "url1", "head1", "base1", gh, cb)
+	execMembershipChecker("pr1", "sha1", gh, cb)
 }