Fix service team reviews (GoogleCloudPlatform#10200)

* Corrected unknown command messge to clean up stderr * Made tests throw error if team name is empty * Fixed handling of unset github_team name
balanaguharsha · May 2, 2024 · b4ce5d4 · b4ce5d4
1 parent ab3318c
commit b4ce5d4
Show file tree

Hide file tree

Showing 3 changed files with 33 additions and 4 deletions.
diff --git a/.ci/magician/cmd/mock_github_test.go b/.ci/magician/cmd/mock_github_test.go
@@ -15,7 +15,11 @@
  */
 package cmd
 
-import "magician/github"
+import (
+	"errors"
+
+	"magician/github"
+)
 
 type mockGithub struct {
 	pullRequest        github.PullRequest
@@ -53,6 +57,9 @@ func (m *mockGithub) GetPullRequestPreviousReviewers(prNumber string) ([]github.
 
 func (m *mockGithub) GetTeamMembers(organization, team string) ([]github.User, error) {
 	m.calledMethods["GetTeamMembers"] = append(m.calledMethods["GetTeamMembers"], []any{organization, team})
+	if team == "" {
+		return nil, errors.New("No team members set")
+	}
 	return m.teamMembers[team], nil
 }
 

diff --git a/.ci/magician/cmd/mock_runner_test.go b/.ci/magician/cmd/mock_runner_test.go
@@ -20,7 +20,12 @@ import (
 	"errors"
 	"fmt"
 	"log"
+	"os"
 	"path/filepath"
+	"sort"
+	"strings"
+
+	"golang.org/x/exp/maps"
 )
 
 type ParameterList []any
@@ -37,7 +42,24 @@ type mockRunner struct {
 	dirStack      *list.List
 }
 
+func sortedEnvString(env map[string]string) string {
+	keys := maps.Keys(env)
+	sort.Strings(keys)
+	kvs := make([]string, len(keys))
+	for i, k := range keys {
+		kvs[i] = fmt.Sprintf("%s:%s", k, env[k])
+	}
+	return fmt.Sprintf("map[%s]", strings.Join(kvs, " "))
+}
+
 func NewMockRunner() MockRunner {
+	diffProcessorEnv := map[string]string{
+		"NEW_REF": "auto-pr-123456",
+		"OLD_REF": "auto-pr-123456-old",
+		"PATH":    os.Getenv("PATH"),
+		"GOPATH":  os.Getenv("GOPATH"),
+		"HOME":    os.Getenv("HOME"),
+	}
 	return &mockRunner{
 		calledMethods: make(map[string][]ParameterList),
 		cmdResults: map[string]string{
@@ -47,7 +69,7 @@ func NewMockRunner() MockRunner {
 			"/mock/dir/magic-modules/.ci/magician git [clone -b auto-pr-123456 https://modular-magician:*******@github.com/modular-magician/terraform-provider-google-beta /mock/dir/tpgb] map[]": "",
 			"/mock/dir/magic-modules git [diff HEAD origin/main tools/missing-test-detector] map[]":                                                                                               "",
 			"/mock/dir/magic-modules/tools/diff-processor bin/diff-processor [breaking-changes] map[]":                                                                                            "",
-			"/mock/dir/magic-modules/tools/diff-processor make [build] map[NEW_REF:auto-pr-123456 OLD_REF:auto-pr-123456-old]":                                                                    "",
+			"/mock/dir/magic-modules/tools/diff-processor make [build] " + sortedEnvString(diffProcessorEnv):                                                                                      "",
 			"/mock/dir/magic-modules/tools/diff-processor bin/diff-processor [add-labels 123456] map[GITHUB_TOKEN_MAGIC_MODULES:*******]":                                                         "",
 			"/mock/dir/magic-modules/tools/missing-test-detector go [mod edit -replace google/provider/new=/mock/dir/tpgb] map[]":                                                                 "",
 			"/mock/dir/magic-modules/tools/missing-test-detector go [mod edit -replace google/provider/old=/mock/dir/tpgbold] map[]":                                                              "",
@@ -128,7 +150,7 @@ func (mr *mockRunner) PopDir() error {
 
 func (mr *mockRunner) Run(name string, args []string, env map[string]string) (string, error) {
 	mr.calledMethods["Run"] = append(mr.calledMethods["Run"], ParameterList{mr.cwd, name, args, env})
-	cmd := fmt.Sprintf("%s %s %v %v", mr.cwd, name, args, env)
+	cmd := fmt.Sprintf("%s %s %v %s", mr.cwd, name, args, sortedEnvString(env))
 	if result, ok := mr.cmdResults[cmd]; ok {
 		return result, nil
 	}

diff --git a/.ci/magician/cmd/request_service_reviewers.go b/.ci/magician/cmd/request_service_reviewers.go
@@ -89,7 +89,7 @@ func execRequestServiceReviewers(prNumber string, gh GithubClient, enrolledTeams
 			continue
 		}
 		teamCount += 1
-		if labelData, ok := enrolledTeams[label.Name]; ok {
+		if labelData, ok := enrolledTeams[label.Name]; ok && labelData.Team != "" {
 			githubTeamsSet[labelData.Team] = struct{}{}
 		}
 	}