From db8f05fe9474aaff361e9c3748967466e01167b7 Mon Sep 17 00:00:00 2001 From: "Stephen Lewis (Burrows)" Date: Fri, 8 Mar 2024 09:15:24 -0800 Subject: [PATCH] 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 db32066e198daf930006f72bae43602f22d8e3d9. --- .ci/gcb-contributor-membership-checker.yml | 4 -- .ci/magician/cmd/community_checker.go | 24 ++-------- .ci/magician/cmd/community_checker_test.go | 24 ++++++---- .ci/magician/cmd/membership_checker.go | 53 +++------------------ .ci/magician/cmd/membership_checker_test.go | 40 ++++------------ 5 files changed, 36 insertions(+), 109 deletions(-) diff --git a/.ci/gcb-contributor-membership-checker.yml b/.ci/gcb-contributor-membership-checker.yml index f40bc6d69c5b..68219b034e4d 100644 --- 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 index 9eb82a5e3e2e..2834009a5ebb 100644 --- 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 index 2b02e28448f3..9880d83031a3 100644 --- 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 index 97210105dd08..a91ce77c5fca 100644 --- a/.ci/magician/cmd/membership_checker.go +++ b/.ci/magician/cmd/membership_checker.go @@ -33,26 +33,18 @@ 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) @@ -60,18 +52,6 @@ var membershipCheckerCmd = &cobra.Command{ 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") @@ -79,19 +59,11 @@ var membershipCheckerCmd = &cobra.Command{ } 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 index a3a40b4b89bd..81074230ec56 100644 --- 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) }