Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Added service team review request support #9681

Merged
merged 12 commits into from
Dec 21, 2023
8 changes: 8 additions & 0 deletions .ci/gcb-generate-diffs-new.yml
Original file line number Diff line number Diff line change
Expand Up @@ -273,6 +273,14 @@ steps:
- $PROJECT_ID
- "22" # Build step

- name: 'gcr.io/graphite-docker-images/go-plus'
entrypoint: '/workspace/.ci/scripts/go-plus/magician/exec.sh'
secretEnv: ["GITHUB_TOKEN"]
waitFor: ["diff"]
args:
- 'request-service-reviewers'
- $_PR_NUMBER

# Long timeout to enable waiting on VCR test
timeout: 20000s
options:
Expand Down
1 change: 1 addition & 0 deletions .ci/magician/cmd/interfaces.go
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ type GithubClient interface {
GetPullRequestRequestedReviewers(prNumber string) ([]github.User, error)
GetPullRequestPreviousReviewers(prNumber string) ([]github.User, error)
GetUserType(user string) github.UserType
GetTeamMembers(organization, team string) ([]github.User, error)
PostBuildStatus(prNumber, title, state, targetURL, commitSha string) error
PostComment(prNumber, comment string) error
RequestPullRequestReviewer(prNumber, assignee string) error
Expand Down
6 changes: 6 additions & 0 deletions .ci/magician/cmd/mock_github_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ type mockGithub struct {
userType github.UserType
requestedReviewers []github.User
previousReviewers []github.User
teamMembers map[string][]github.User
calledMethods map[string][][]any
}

Expand All @@ -45,6 +46,11 @@ func (m *mockGithub) GetPullRequestPreviousReviewers(prNumber string) ([]github.
return m.previousReviewers, nil
}

func (m *mockGithub) GetTeamMembers(organization, team string) ([]github.User, error) {
m.calledMethods["GetTeamMembers"] = append(m.calledMethods["GetTeamMembers"], []any{organization, team})
return m.teamMembers[team], nil
}

func (m *mockGithub) RequestPullRequestReviewer(prNumber string, reviewer string) error {
m.calledMethods["RequestPullRequestReviewer"] = append(m.calledMethods["RequestPullRequestReviewer"], []any{prNumber, reviewer})
return nil
Expand Down
149 changes: 149 additions & 0 deletions .ci/magician/cmd/request_service_reviewers.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,149 @@
/*
* Copyright 2023 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"
"math/rand"
"os"
"strings"

"github.com/GoogleCloudPlatform/magic-modules/tools/issue-labeler/labeler"
"github.com/spf13/cobra"
"gopkg.in/yaml.v2"
)

// requestServiceReviewersCmd represents the requestServiceReviewers command
var requestServiceReviewersCmd = &cobra.Command{
Use: "request-service-reviewers PR_NUMBER",
Short: "Assigns reviewers based on the PR's service labels.",
Long: `This command requests (or re-requests) review based on the PR's service labels.

If a PR has more than 3 service labels, the command will not do anything.
`,
Args: cobra.ExactArgs(1),
Run: func(cmd *cobra.Command, args []string) {
prNumber := args[0]
fmt.Println("PR Number: ", prNumber)

gh := github.NewClient()
execRequestServiceReviewers(prNumber, gh, labeler.EnrolledTeamsYaml)
},
}

func execRequestServiceReviewers(prNumber string, gh GithubClient, enrolledTeamsYaml []byte) {
ScottSuarez marked this conversation as resolved.
Show resolved Hide resolved
pullRequest, err := gh.GetPullRequest(prNumber)
if err != nil {
fmt.Println(err)
os.Exit(1)
}

enrolledTeams := make(map[string]labeler.LabelData)
if err := yaml.Unmarshal(enrolledTeamsYaml, &enrolledTeams); err != nil {
fmt.Printf("Error unmarshalling enrolled teams yaml: %s", err)
os.Exit(1)
}

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

// If more than three service labels are impacted, don't request reviews.
// Only request reviews from unique service teams.
githubTeams := make(map[string]struct{})
teamCount := 0
for _, label := range pullRequest.Labels {
if !strings.HasPrefix(label.Name, "service/") || label.Name == "service/terraform" {
continue
}
teamCount += 1
if labelData, ok := enrolledTeams[label.Name]; ok {
githubTeams[labelData.Team] = struct{}{}
}
}

if teamCount > 3 {
fmt.Println("Provider-wide change (>3 services impacted); not requesting service team reviews")
ScottSuarez marked this conversation as resolved.
Show resolved Hide resolved
return
}

// For each service team, check if one of the team members is already a reviewer. Rerequest
// review if there is and choose a random reviewer from the list if there isn't.
toRequest := []string{}
melinath marked this conversation as resolved.
Show resolved Hide resolved
requestedReviewersMap := make(map[string]struct{})
for _, reviewer := range requestedReviewers {
requestedReviewersMap[reviewer.Login] = struct{}{}
}

previousReviewersMap := make(map[string]struct{})
for _, reviewer := range previousReviewers {
previousReviewersMap[reviewer.Login] = struct{}{}
melinath marked this conversation as resolved.
Show resolved Hide resolved
}

exitCode := 0
for githubTeam, _ := range githubTeams {
members, err := gh.GetTeamMembers("GoogleCloudPlatform", githubTeam)
if err != nil {
fmt.Printf("Error fetching members for GoogleCloudPlatform/%s: %s", githubTeam, err)
exitCode = 1
continue
}
hasReviewer := false
reviewerPool := []string{}
for _, member := range members {
// Exclude PR author
if member.Login != pullRequest.User.Login {
reviewerPool = append(reviewerPool, member.Login)
}
// Don't re-request review if there's an active review request
if _, ok := requestedReviewersMap[member.Login]; ok {
hasReviewer = true
}
if _, ok := previousReviewersMap[member.Login]; ok {
hasReviewer = true
toRequest = append(toRequest, member.Login)
}
}

if !hasReviewer && len(reviewerPool) > 0 {
toRequest = append(toRequest, reviewerPool[rand.Intn(len(reviewerPool))])
}
}

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

func init() {
rootCmd.AddCommand(requestServiceReviewersCmd)
}
147 changes: 147 additions & 0 deletions .ci/magician/cmd/request_service_reviewers_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,147 @@
package cmd

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

var enrolledTeamsYaml = []byte(`
service/google-x:
team: google-x
resources:
- google_x_resource
service/google-y:
team: google-y
resources:
- google_y_resource
service/google-z:
resources:
- google_z_resource
`)

func TestExecRequestServiceReviewersMembershipChecker_CoreContributorFlow(t *testing.T) {
cases := map[string]struct {
pullRequest github.PullRequest
requestedReviewers []string
previousReviewers []string
teamMembers map[string][]string
expectSpecificReviewers []string
}{
"no service labels means no service team reviewers": {
pullRequest: github.PullRequest{
User: github.User{Login: "googler_author"},
},
expectSpecificReviewers: []string{},
},
"unregistered service labels will not trigger review": {
pullRequest: github.PullRequest{
User: github.User{Login: "googler_author"},
Labels: []github.Label{{Name: "service/google-a"}},
},
expectSpecificReviewers: []string{},
},
"no configured team means no reviewers": {
pullRequest: github.PullRequest{
User: github.User{Login: "googler_author"},
Labels: []github.Label{{Name: "service/google-z"}},
},
expectSpecificReviewers: []string{},
},
"no previous reviewers means all reviews will be requested": {
pullRequest: github.PullRequest{
User: github.User{Login: "googler_author"},
Labels: []github.Label{{Name: "service/google-x"}},
},
teamMembers: map[string][]string{"google-x": []string{"googler_team_member"}},
expectSpecificReviewers: []string{"googler_team_member"},
},
"previous reviewers will be re-requested": {
pullRequest: github.PullRequest{
User: github.User{Login: "googler_author"},
Labels: []github.Label{{Name: "service/google-x"}},
},
previousReviewers: []string{"googler_team_member"},
teamMembers: map[string][]string{"google-x": []string{"googler_team_member", "googler_team_member_2", "googler_team_member_3", "googler_team_member_4", "googler_team_member_5"}},
expectSpecificReviewers: []string{"googler_team_member"},
},
"active reviewers will not be re-requested": {
pullRequest: github.PullRequest{
User: github.User{Login: "googler_author"},
Labels: []github.Label{{Name: "service/google-x"}},
},
requestedReviewers: []string{"googler_team_member"},
teamMembers: map[string][]string{"google-x": []string{"googler_team_member"}},
expectSpecificReviewers: []string{},
},
"authors will not be requested on their own PRs": {
pullRequest: github.PullRequest{
User: github.User{Login: "googler_team_member"},
Labels: []github.Label{{Name: "service/google-x"}},
},
teamMembers: map[string][]string{"google-x": []string{"googler_team_member"}},
expectSpecificReviewers: []string{},
},
"other team members be requested even if the author is excluded": {
pullRequest: github.PullRequest{
User: github.User{Login: "googler_team_member"},
Labels: []github.Label{{Name: "service/google-x"}},
},
teamMembers: map[string][]string{"google-x": []string{"googler_team_member", "googler_team_member_2"}},
expectSpecificReviewers: []string{"googler_team_member_2"},
},
"multiple teams can be requested at once": {
pullRequest: github.PullRequest{
User: github.User{Login: "googler_author"},
Labels: []github.Label{{Name: "service/google-x"}, {Name: "service/google-y"}, {Name: "service/google-z"}},
},
teamMembers: map[string][]string{"google-x": []string{"googler_team_member"}, "google-y": []string{"googler_y_team_member"}},
expectSpecificReviewers: []string{"googler_team_member", "googler_y_team_member"},
},
">3 service teams will not be requested": {
pullRequest: github.PullRequest{
User: github.User{Login: "googler_author"},
Labels: []github.Label{{Name: "service/google-x"}, {Name: "service/google-y"}, {Name: "service/google-z"}, {Name: "service/google-a"}},
},
teamMembers: map[string][]string{"google-x": []string{"googler_team_member"}, "google-y": []string{"googler_y_team_member"}},
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})
}
teamMembers := map[string][]github.User{}
for team, logins := range tc.teamMembers {
teamMembers[team] = []github.User{}
for _, login := range logins {
teamMembers[team] = append(teamMembers[team], github.User{Login: login})
}
}
gh := &mockGithub{
pullRequest: tc.pullRequest,
requestedReviewers: requestedReviewers,
previousReviewers: previousReviewers,
teamMembers: teamMembers,
calledMethods: make(map[string][][]any),
}

execRequestServiceReviewers("1", gh, enrolledTeamsYaml)

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

if tc.expectSpecificReviewers != nil {
assert.Equal(t, tc.expectSpecificReviewers, actualReviewers)
}
})
}
}
11 changes: 11 additions & 0 deletions .ci/magician/github/get.go
Original file line number Diff line number Diff line change
Expand Up @@ -85,3 +85,14 @@ func (gh *Client) GetPullRequestPreviousReviewers(prNumber string) ([]User, erro

return result, nil
}

func (gh *Client) GetTeamMembers(organization, team string) ([]User, error) {
url := fmt.Sprintf("https://api.github.com/orgs/%s/teams/%s/members", organization, team)

var members []User
_, err := utils.RequestCall(url, "GET", gh.token, &members, nil)
if err != nil {
return nil, err
}
return members, nil
}
11 changes: 10 additions & 1 deletion .ci/magician/go.mod
Original file line number Diff line number Diff line change
Expand Up @@ -2,11 +2,14 @@ module magician

go 1.20

replace github.com/GoogleCloudPlatform/magic-modules/tools/issue-labeler => ../../tools/issue-labeler

require (
github.com/GoogleCloudPlatform/magic-modules/tools/issue-labeler v0.0.0-00010101000000-000000000000
github.com/inconshreveable/mousetrap v1.1.0 // indirect
github.com/spf13/cobra v1.7.0
github.com/spf13/pflag v1.0.5 // indirect
golang.org/x/exp v0.0.0-20230314191032-db074128a8ec
golang.org/x/exp v0.0.0-20230810033253-352e893a4cad
google.golang.org/api v0.112.0
)

Expand All @@ -15,11 +18,15 @@ require github.com/otiai10/copy v1.12.0
require (
cloud.google.com/go/compute v1.18.0 // indirect
cloud.google.com/go/compute/metadata v0.2.3 // indirect
github.com/davecgh/go-spew v1.1.1 // indirect
github.com/golang/glog v1.1.1 // indirect
github.com/golang/groupcache v0.0.0-20210331224755-41bb18bfe9da // indirect
github.com/golang/protobuf v1.5.2 // indirect
github.com/google/uuid v1.3.0 // indirect
github.com/googleapis/enterprise-certificate-proxy v0.2.3 // indirect
github.com/googleapis/gax-go/v2 v2.7.0 // indirect
github.com/pmezard/go-difflib v1.0.0 // indirect
github.com/stretchr/testify v1.8.4 // indirect
go.opencensus.io v0.24.0 // indirect
golang.org/x/net v0.15.0 // indirect
golang.org/x/oauth2 v0.6.0 // indirect
Expand All @@ -29,4 +36,6 @@ require (
google.golang.org/genproto v0.0.0-20230303212802-e74f57abe488 // indirect
google.golang.org/grpc v1.53.0 // indirect
google.golang.org/protobuf v1.28.1 // indirect
gopkg.in/yaml.v2 v2.4.0 // indirect
gopkg.in/yaml.v3 v3.0.1 // indirect
)
Loading
Loading