Simplified / clarified approval flow for presubmits

.ci/gcb-contributor-membership-checker.yml

@@ -68,6 +68,7 @@ steps:
       - "membership-checker"
       - $_PR_NUMBER
       - $COMMIT_SHA
+      # The following are unused and should be safe to remove at least 2 weeks after this comment was added.
       - $BRANCH_NAME
       - $_HEAD_REPO_URL
       - $_HEAD_BRANCH

.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")

.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) {

.ci/magician/cmd/membership_checker.go

@@ -52,46 +52,27 @@ var membershipCheckerCmd = &cobra.Command{
 			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 +83,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 {

.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)
 }

mmv1/third_party/terraform/services/compute/metadata.go.erb

@@ -15,6 +15,7 @@ import (
 	transport_tpg "github.com/hashicorp/terraform-provider-google/google/transport"
 )
 
+// force generation
 // Since the google compute API uses optimistic locking, there is a chance
 // we need to resubmit our updated metadata. To do this, you need to provide
 // an update function that attempts to submit your metadata