Skip to content

Commit

Permalink
sync merge from upstream/main
Browse files Browse the repository at this point in the history
  • Loading branch information
maxi-cit committed Feb 19, 2024
2 parents 9fc074b + aa4dd13 commit 074ad81
Show file tree
Hide file tree
Showing 133 changed files with 5,459 additions and 647 deletions.
35 changes: 0 additions & 35 deletions .ci/magician/cmd/membership_checker.go
Original file line number Diff line number Diff line change
Expand Up @@ -97,41 +97,6 @@ func execMembershipChecker(prNumber, commitSha, branchName, headRepoUrl, headBra
authorUserType := gh.GetUserType(author)
trusted := authorUserType == github.CoreContributorUserType || authorUserType == github.GooglerUserType

if authorUserType != github.CoreContributorUserType {
fmt.Println("Not core contributor - assigning reviewer")

requestedReviewers, err := gh.GetPullRequestRequestedReviewers(prNumber)
if err != nil {
fmt.Println(err)
os.Exit(1)
}

previousReviewers, err := gh.GetPullRequestPreviousReviewers(prNumber)
if err != nil {
fmt.Println(err)
os.Exit(1)
}

reviewersToRequest, newPrimaryReviewer := github.ChooseCoreReviewers(requestedReviewers, previousReviewers)

for _, reviewer := range reviewersToRequest {
err = gh.RequestPullRequestReviewer(prNumber, reviewer)
if err != nil {
fmt.Println(err)
os.Exit(1)
}
}

if newPrimaryReviewer != "" {
comment := github.FormatReviewerComment(newPrimaryReviewer, authorUserType, trusted)
err = gh.PostComment(prNumber, comment)
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
if trusted {
Expand Down
60 changes: 2 additions & 58 deletions .ci/magician/cmd/membership_checker_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,6 @@ package cmd
import (
"magician/github"
"reflect"
"regexp"
"testing"
)

Expand Down Expand Up @@ -78,20 +77,7 @@ func TestExecMembershipChecker_GooglerFlow(t *testing.T) {

execMembershipChecker("pr1", "sha1", "branch1", "url1", "head1", "base1", gh, cb)

method := "RequestPullRequestReviewer"
if calls, ok := gh.calledMethods[method]; !ok {
t.Fatal("Review wasn't requested for googler")
} else if len(calls) != 1 {
t.Fatalf("Wrong number of calls for %s, got %d, expected 1", method, len(calls))
} else if params := calls[0]; len(params) != 2 {
t.Fatalf("Wrong number of params for %s, got %d, expected 2", method, len(params))
} else if param := params[0]; param != "pr1" {
t.Fatalf("Wrong first param for %s, got %v, expected pr1", method, param)
} else if param := params[1]; !github.IsTeamReviewer(param.(string)) {
t.Fatalf("Wrong second param for %s, got %v, expected a team reviewer", method, param)
}

method = "TriggerMMPresubmitRuns"
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")
Expand Down Expand Up @@ -126,20 +112,7 @@ func TestExecMembershipChecker_AmbiguousUserFlow(t *testing.T) {

execMembershipChecker("pr1", "sha1", "branch1", "url1", "head1", "base1", gh, cb)

method := "RequestPullRequestReviewer"
if calls, ok := gh.calledMethods[method]; !ok {
t.Fatal("Review wasn't requested for ambiguous user")
} else if len(calls) != 1 {
t.Fatalf("Wrong number of calls for %s, got %d, expected 1", method, len(calls))
} else if params := calls[0]; len(params) != 2 {
t.Fatalf("Wrong number of params for %s, got %d, expected 2", method, len(params))
} else if param := params[0]; param != "pr1" {
t.Fatalf("Wrong first param for %s, got %v, expected pr1", method, param)
} else if param := params[1]; !github.IsTeamReviewer(param.(string)) {
t.Fatalf("Wrong second param for %s, got %v, expected a team reviewer", method, param)
}

method = "AddLabel"
method := "AddLabel"
expected := [][]any{{"pr1", "awaiting-approval"}}
if calls, ok := gh.calledMethods[method]; !ok {
t.Fatal("Label wasn't posted to pull request")
Expand Down Expand Up @@ -181,33 +154,4 @@ func TestExecMembershipChecker_CommentForNewPrimaryReviewer(t *testing.T) {
}

execMembershipChecker("pr1", "sha1", "branch1", "url1", "head1", "base1", gh, cb)

method := "RequestPullRequestReviewer"
if calls, ok := gh.calledMethods[method]; !ok {
t.Fatal("Review wasn't requested for googler")
} else if len(calls) != 1 {
t.Fatalf("Wrong number of calls for %s, got %d, expected 1", method, len(calls))
} else if params := calls[0]; len(params) != 2 {
t.Fatalf("Wrong number of params for %s, got %d, expected 2", method, len(params))
} else if param := params[0]; param != "pr1" {
t.Fatalf("Wrong first param for %s, got %v, expected pr1", method, param)
} else if param := params[1]; !github.IsTeamReviewer(param.(string)) {
t.Fatalf("Wrong second param for %s, got %v, expected a team reviewer", method, param)
}

method = "PostComment"
reviewerExp := regexp.MustCompile(`@(.*?),`)
if calls, ok := gh.calledMethods[method]; !ok {
t.Fatal("Comment wasn't posted stating user status")
} else if len(calls) != 1 {
t.Fatalf("Wrong number of calls for %s, got %d, expected 1", method, len(calls))
} else if params := calls[0]; len(params) != 2 {
t.Fatalf("Wrong number of params for %s, got %d, expected 2", method, len(params))
} else if param := params[0]; param != "pr1" {
t.Fatalf("Wrong first param for %s, got %v, expected pr1", method, param)
} else if param, ok := params[1].(string); !ok {
t.Fatalf("Got non-string second param for %s", method)
} else if submatches := reviewerExp.FindStringSubmatch(param); len(submatches) != 2 || !github.IsTeamReviewer(submatches[1]) {
t.Fatalf("%s called without a team reviewer (found %v) in the comment: %s", method, submatches, param)
}
}
102 changes: 102 additions & 0 deletions .ci/magician/cmd/request_reviewer.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,102 @@
/*
* Copyright 2024 Google LLC. All Rights Reserved.
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/
package cmd

import (
"fmt"
"magician/github"
"os"

"github.com/spf13/cobra"
)

// requestReviewerCmd represents the requestReviewer command
var requestReviewerCmd = &cobra.Command{
Use: "request-reviewer",
Short: "Assigns and re-requests reviewers",
Long: `This command automatically requests (or re-requests) core contributor reviews for a PR based on whether the user is a core contributor.
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. Determines the author of the pull request
2. 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. As appropriate, posts a welcome comment on the PR.
`,
Args: cobra.ExactArgs(1),
Run: func(cmd *cobra.Command, args []string) {
prNumber := args[0]
fmt.Println("PR Number: ", prNumber)
gh := github.NewClient()
execRequestReviewer(prNumber, gh)
},
}

func execRequestReviewer(prNumber string, gh GithubClient) {
pullRequest, err := gh.GetPullRequest(prNumber)
if err != nil {
fmt.Println(err)
os.Exit(1)
}

author := pullRequest.User.Login
if !github.IsCoreContributor(author) {
fmt.Println("Not core contributor - assigning reviewer")

requestedReviewers, err := gh.GetPullRequestRequestedReviewers(prNumber)
if err != nil {
fmt.Println(err)
os.Exit(1)
}

previousReviewers, err := gh.GetPullRequestPreviousReviewers(prNumber)
if err != nil {
fmt.Println(err)
os.Exit(1)
}

reviewersToRequest, newPrimaryReviewer := github.ChooseCoreReviewers(requestedReviewers, previousReviewers)

for _, reviewer := range reviewersToRequest {
err = gh.RequestPullRequestReviewer(prNumber, reviewer)
if err != nil {
fmt.Println(err)
os.Exit(1)
}
}

if newPrimaryReviewer != "" {
comment := github.FormatReviewerComment(newPrimaryReviewer)
err = gh.PostComment(prNumber, comment)
if err != nil {
fmt.Println(err)
os.Exit(1)
}
}
}
}

func init() {
rootCmd.AddCommand(requestReviewerCmd)
}
103 changes: 103 additions & 0 deletions .ci/magician/cmd/request_reviewer_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,103 @@
/*
* Copyright 2024 Google LLC. All Rights Reserved.
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/

package cmd

import (
"github.com/stretchr/testify/assert"
"magician/github"
"testing"
)

func TestExecRequestReviewer(t *testing.T) {
availableReviewers := github.AvailableReviewers()
cases := map[string]struct {
pullRequest github.PullRequest
requestedReviewers []string
previousReviewers []string
teamMembers map[string][]string
expectSpecificReviewers []string
expectReviewersFromList []string
}{
"core contributor author doesn't get a new reviewer, re-request, or comment with no previous reviewers": {
pullRequest: github.PullRequest{
User: github.User{Login: availableReviewers[0]},
},
expectSpecificReviewers: []string{},
},
"core contributor author doesn't get a new reviewer, re-request, or comment with previous reviewers": {
pullRequest: github.PullRequest{
User: github.User{Login: availableReviewers[0]},
},
previousReviewers: []string{availableReviewers[1]},
expectSpecificReviewers: []string{},
},
"non-core-contributor author gets a new reviewer with no previous reviewers": {
pullRequest: github.PullRequest{
User: github.User{Login: "author"},
},
expectReviewersFromList: availableReviewers,
},
"non-core-contributor author doesn't get a new reviewer (but does get re-request) with previous reviewers": {
pullRequest: github.PullRequest{
User: github.User{Login: "author"},
},
previousReviewers: []string{availableReviewers[1], "author2", availableReviewers[2]},
expectSpecificReviewers: []string{availableReviewers[1], availableReviewers[2]},
},
"non-core-contributor author doesn't get a new reviewer or a re-request with already-requested reviewers": {
pullRequest: github.PullRequest{
User: github.User{Login: "author"},
},
requestedReviewers: []string{availableReviewers[1], "author2", availableReviewers[2]},
expectSpecificReviewers: []string{},
},
}
for tn, tc := range cases {
t.Run(tn, func(t *testing.T) {
requestedReviewers := []github.User{}
for _, login := range tc.requestedReviewers {
requestedReviewers = append(requestedReviewers, github.User{Login: login})
}
previousReviewers := []github.User{}
for _, login := range tc.previousReviewers {
previousReviewers = append(previousReviewers, github.User{Login: login})
}
gh := &mockGithub{
pullRequest: tc.pullRequest,
requestedReviewers: requestedReviewers,
previousReviewers: previousReviewers,
calledMethods: make(map[string][][]any),
}

execRequestReviewer("1", gh)

actualReviewers := []string{}
for _, args := range gh.calledMethods["RequestPullRequestReviewer"] {
actualReviewers = append(actualReviewers, args[1].(string))
}

if tc.expectSpecificReviewers != nil {
assert.ElementsMatch(t, tc.expectSpecificReviewers, actualReviewers)
}
if tc.expectReviewersFromList != nil {
for _, reviewer := range actualReviewers {
assert.Contains(t, tc.expectReviewersFromList, reviewer)
}
}
})
}
}
4 changes: 3 additions & 1 deletion .ci/magician/cmd/test_terraform_vcr.go
Original file line number Diff line number Diff line change
Expand Up @@ -168,6 +168,8 @@ Affected tests: ` + fmt.Sprintf("`%d`", len(replayingResult.FailedTests)) + `
recordingResult, recordingErr := vt.RunParallel(vcr.Recording, provider.Beta, testDirs, replayingResult.FailedTests)
if recordingErr != nil {
testState = "failure"
} else {
testState = "success"
}

if err := vt.UploadCassettes("ci-vcr-cassettes", prNumber, provider.Beta); err != nil {
Expand Down Expand Up @@ -200,7 +202,7 @@ Affected tests: ` + fmt.Sprintf("`%d`", len(replayingResult.FailedTests)) + `
testState = "failure"
}

if err := vt.UploadLogs("ci-vcr-logs", prNumber, buildID, true, false, vcr.Recording, provider.Beta); err != nil {
if err := vt.UploadLogs("ci-vcr-logs", prNumber, buildID, true, true, vcr.Replaying, provider.Beta); err != nil {
fmt.Println("Error uploading recording logs: ", err)
os.Exit(1)
}
Expand Down
2 changes: 1 addition & 1 deletion .ci/magician/github/REVIEWER_ASSIGNMENT_COMMENT.md
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
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}}
Hello! I am a robot. Tests will require approval from a repository maintainer to run.

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

Expand Down
Loading

0 comments on commit 074ad81

Please sign in to comment.