Switched from diff-processor adding labels to just computing and retu…

…rning them as JSON (GoogleCloudPlatform#10211)

* Switched diff-processor from add-labels command to changed-schema-labels command

* Force update to redis cluster

* Revert "Force update to redis cluster"

This reverts commit 750c39d.

.ci/magician/cmd/generate_comment.go

@@ -16,18 +16,20 @@
 package cmd
 
 import (
+	"encoding/json"
 	"fmt"
-	"magician/exec"
-	"magician/github"
-	"magician/provider"
-	"magician/source"
 	"os"
 	"path/filepath"
 	"sort"
 	"strconv"
 	"strings"
 	"text/template"
 
+	"magician/exec"
+	"magician/github"
+	"magician/provider"
+	"magician/source"
+
 	"github.com/spf13/cobra"
 	"golang.org/x/exp/maps"
 
@@ -141,6 +143,14 @@ func listGCEnvironmentVariables() string {
 }
 
 func execGenerateComment(prNumber int, ghTokenMagicModules, buildId, buildStep, projectId, commitSha string, gh GithubClient, rnr ExecRunner, ctlr *source.Controller) {
+	errors := map[string][]string{"Other": []string{}}
+
+	pullRequest, err := gh.GetPullRequest(strconv.Itoa(prNumber))
+	if err != nil {
+		fmt.Printf("Error getting pull request: %v\n", err)
+		errors["Other"] = append(errors["Other"], "Failed to fetch PR data")
+	}
+
 	newBranch := fmt.Sprintf("auto-pr-%d", prNumber)
 	oldBranch := fmt.Sprintf("auto-pr-%d-old", prNumber)
 	wd := rnr.GetCWD()
@@ -174,8 +184,6 @@ func execGenerateComment(prNumber int, ghTokenMagicModules, buildId, buildStep,
 	data := diffCommentData{
 		PrNumber: prNumber,
 	}
-	errors := map[string][]string{"Other": []string{}}
-	var err error
 	for _, repo := range []*source.Repo{&tpgRepo, &tpgbRepo, &tgcRepo, &tfoicsRepo} {
 		errors[repo.Title] = []string{}
 		repo.Branch = newBranch
@@ -210,6 +218,7 @@ func execGenerateComment(prNumber int, ghTokenMagicModules, buildId, buildStep,
 
 	// The breaking changes are unique across both provider versions
 	uniqueBreakingChanges := map[string]struct{}{}
+	uniqueServiceLabels := map[string]struct{}{}
 	diffProcessorPath := filepath.Join(mmLocalPath, "tools", "diff-processor")
 	diffProcessorEnv := map[string]string{
 		"OLD_REF": oldBranch,
@@ -240,14 +249,20 @@ func execGenerateComment(prNumber int, ghTokenMagicModules, buildId, buildStep,
 			uniqueBreakingChanges[breakingChange] = struct{}{}
 		}
 
-		addLabelsEnv := map[string]string{
-			"GITHUB_TOKEN_MAGIC_MODULES": ghTokenMagicModules,
+		// If fetching the PR failed, Labels will be empty
+		labels := make([]string, len(pullRequest.Labels))
+		for i, label := range pullRequest.Labels {
+			labels[i] = label.Name
 		}
-		err = addLabels(prNumber, diffProcessorPath, addLabelsEnv, rnr)
+		serviceLabels, err := changedSchemaLabels(prNumber, labels, diffProcessorPath, gh, rnr)
 		if err != nil {
-			fmt.Println("adding service labels: ", err)
-			errors[repo.Title] = append(errors[repo.Title], "The diff processor crashed while adding labels.")
+			fmt.Println("computing changed schema labels: ", err)
+			errors[repo.Title] = append(errors[repo.Title], "The diff processor crashed while computing changed schema labels.")
+		}
+		for _, serviceLabel := range serviceLabels {
+			uniqueServiceLabels[serviceLabel] = struct{}{}
 		}
+
 		err = cleanDiffProcessor(diffProcessorPath, rnr)
 		if err != nil {
 			fmt.Println("cleaning up diff processor: ", err)
@@ -258,21 +273,25 @@ func execGenerateComment(prNumber int, ghTokenMagicModules, buildId, buildStep,
 	sort.Strings(breakingChangesSlice)
 	data.BreakingChanges = breakingChangesSlice
 
+	// Add service labels to PR
+	if len(uniqueServiceLabels) > 0 {
+		serviceLabelsSlice := maps.Keys(uniqueServiceLabels)
+		sort.Strings(serviceLabelsSlice)
+		if err = gh.AddLabels(strconv.Itoa(prNumber), serviceLabelsSlice); err != nil {
+			fmt.Printf("Error posting new service labels %q: %s", serviceLabelsSlice, err)
+			errors["Other"] = append(errors["Other"], "Failed to update service labels")
+		}
+	}
+
 	// Update breaking changes status on PR
 	breakingState := "success"
 	if len(uniqueBreakingChanges) > 0 {
 		breakingState = "failure"
-
-		pullRequest, err := gh.GetPullRequest(strconv.Itoa(prNumber))
-		if err != nil {
-			fmt.Printf("Error getting pull request: %v\n", err)
-			errors["Other"] = append(errors["Other"], "Failed to check for `override-breaking-change` label")
-		} else {
-			for _, label := range pullRequest.Labels {
-				if label.Name == allowBreakingChangesLabel {
-					breakingState = "success"
-					break
-				}
+		// If fetching the PR failed, Labels will be empty
+		for _, label := range pullRequest.Labels {
+			if label.Name == allowBreakingChangesLabel {
+				breakingState = "success"
+				break
 			}
 		}
 	}
@@ -387,16 +406,40 @@ func computeBreakingChanges(diffProcessorPath string, rnr ExecRunner) ([]string,
 	return strings.Split(strings.TrimSuffix(output, "\n"), "\n"), rnr.PopDir()
 }
 
-func addLabels(prNumber int, diffProcessorPath string, env map[string]string, rnr ExecRunner) error {
+func changedSchemaLabels(prNumber int, currentLabels []string, diffProcessorPath string, gh GithubClient, rnr ExecRunner) ([]string, error) {
 	if err := rnr.PushDir(diffProcessorPath); err != nil {
-		return err
+		return nil, err
+	}
+
+	// short-circuit if service labels have already been added to the PR
+	hasServiceLabels := false
+	oldLabels := make(map[string]struct{}, len(currentLabels))
+	for _, label := range currentLabels {
+		oldLabels[label] = struct{}{}
+		if strings.HasPrefix(label, "service/") {
+			hasServiceLabels = true
+		}
 	}
-	output, err := rnr.Run("bin/diff-processor", []string{"add-labels", strconv.Itoa(prNumber)}, env)
-	fmt.Println(output)
+	if hasServiceLabels {
+		return nil, nil
+	}
+
+	output, err := rnr.Run("bin/diff-processor", []string{"changed-schema-labels"}, nil)
 	if err != nil {
-		return err
+		return nil, err
 	}
-	return rnr.PopDir()
+
+	fmt.Println("Labels for changed schema: " + output)
+
+	var labels []string
+	if err = json.Unmarshal([]byte(output), &labels); err != nil {
+		return nil, err
+	}
+
+	if err = rnr.PopDir(); err != nil {
+		return nil, err
+	}
+	return labels, nil
 }
 
 func cleanDiffProcessor(diffProcessorPath string, rnr ExecRunner) error {

.ci/magician/cmd/generate_comment_test.go

@@ -37,9 +37,6 @@ func TestExecGenerateComment(t *testing.T) {
 		"GOPATH":  os.Getenv("GOPATH"),
 		"HOME":    os.Getenv("HOME"),
 	}
-	addLabelsEnv := map[string]string{
-		"GITHUB_TOKEN_MAGIC_MODULES": "*******",
-	}
 	execGenerateComment(
 		123456,
 		"*******",
@@ -83,10 +80,10 @@ func TestExecGenerateComment(t *testing.T) {
 			{"/mock/dir/tfoics", "git", []string{"diff", "origin/auto-pr-123456-old", "origin/auto-pr-123456", "--shortstat"}, map[string]string(nil)},
 			{"/mock/dir/magic-modules/tools/diff-processor", "make", []string{"build"}, diffProcessorEnv},
 			{"/mock/dir/magic-modules/tools/diff-processor", "bin/diff-processor", []string{"breaking-changes"}, map[string]string(nil)},
-			{"/mock/dir/magic-modules/tools/diff-processor", "bin/diff-processor", []string{"add-labels", "123456"}, addLabelsEnv},
+			{"/mock/dir/magic-modules/tools/diff-processor", "bin/diff-processor", []string{"changed-schema-labels"}, map[string]string(nil)},
 			{"/mock/dir/magic-modules/tools/diff-processor", "make", []string{"build"}, diffProcessorEnv},
 			{"/mock/dir/magic-modules/tools/diff-processor", "bin/diff-processor", []string{"breaking-changes"}, map[string]string(nil)},
-			{"/mock/dir/magic-modules/tools/diff-processor", "bin/diff-processor", []string{"add-labels", "123456"}, addLabelsEnv},
+			{"/mock/dir/magic-modules/tools/diff-processor", "bin/diff-processor", []string{"changed-schema-labels"}, map[string]string(nil)},
 			{"/mock/dir/tpgbold", "git", []string{"checkout", "origin/auto-pr-123456-old"}, map[string]string(nil)},
 			{"/mock/dir/tpgbold", "find", []string{".", "-type", "f", "-name", "*.go", "-exec", "sed", "-i.bak", "s~github.com/hashicorp/terraform-provider-google-beta~google/provider/old~g", "{}", "+"}, map[string]string(nil)},
 			{"/mock/dir/tpgbold", "sed", []string{"-i.bak", "s|github.com/hashicorp/terraform-provider-google-beta|google/provider/old|g", "go.mod"}, map[string]string(nil)},
@@ -117,6 +114,7 @@ func TestExecGenerateComment(t *testing.T) {
 	for method, expectedCalls := range map[string][][]any{
 		"PostBuildStatus": {{"123456", "terraform-provider-breaking-change-test", "success", "https://console.cloud.google.com/cloud-build/builds;region=global/build1;step=17?project=project1", "sha1"}},
 		"PostComment":     {{"123456", "Hi there, I'm the Modular magician. I've detected the following information about your changes:\n\n## Diff report\n\nYour PR generated some diffs in downstreams - here they are.\n\n`google` provider: [Diff](https://github.com/modular-magician/terraform-provider-google/compare/auto-pr-123456-old..auto-pr-123456) ( 2 files changed, 40 insertions(+))\n`google-beta` provider: [Diff](https://github.com/modular-magician/terraform-provider-google-beta/compare/auto-pr-123456-old..auto-pr-123456) ( 2 files changed, 40 insertions(+))\n`terraform-google-conversion`: [Diff](https://github.com/modular-magician/terraform-google-conversion/compare/auto-pr-123456-old..auto-pr-123456) ( 1 file changed, 10 insertions(+))\n\n## Missing test report\nYour PR includes resource fields which are not covered by any test.\n\nResource: `google_folder_access_approval_settings` (3 total tests)\nPlease add an acceptance test which includes these fields. The test should include the following:\n\n```hcl\nresource \"google_folder_access_approval_settings\" \"primary\" {\n  uncovered_field = # value needed\n}\n\n```\n"}},
+		"AddLabels":       {{"123456", []string{"service/google-x"}}},
 	} {
 		if actualCalls, ok := gh.calledMethods[method]; !ok {
 			t.Fatalf("Found no calls for %s", method)

.ci/magician/cmd/interfaces.go

@@ -30,7 +30,7 @@ type GithubClient interface {
 	PostBuildStatus(prNumber, title, state, targetURL, commitSha string) error
 	PostComment(prNumber, comment string) error
 	RequestPullRequestReviewer(prNumber, assignee string) error
-	AddLabel(prNumber, label string) error
+	AddLabels(prNumber string, labels []string) error
 	RemoveLabel(prNumber, label string) error
 	CreateWorkflowDispatchEvent(workflowFileName string, inputs map[string]any) error
 }

.ci/magician/cmd/membership_checker.go

@@ -83,7 +83,7 @@ func execMembershipChecker(prNumber, commitSha string, gh GithubClient, cb Cloud
 			os.Exit(1)
 		}
 	} else {
-		gh.AddLabel(prNumber, "awaiting-approval")
+		gh.AddLabels(prNumber, []string{"awaiting-approval"})
 		targetUrl, err := cb.GetAwaitingApprovalBuildLink(prNumber, commitSha)
 		if err != nil {
 			fmt.Println(err)

.ci/magician/cmd/membership_checker_test.go

@@ -92,8 +92,8 @@ func TestExecMembershipChecker_AmbiguousUserFlow(t *testing.T) {
 
 	execMembershipChecker("pr1", "sha1", gh, cb)
 
-	method := "AddLabel"
-	expected := [][]any{{"pr1", "awaiting-approval"}}
+	method := "AddLabels"
+	expected := [][]any{{"pr1", []string{"awaiting-approval"}}}
 	if calls, ok := gh.calledMethods[method]; !ok {
 		t.Fatal("Label wasn't posted to pull request")
 	} else if !reflect.DeepEqual(calls, expected) {

.ci/magician/cmd/mock_github_test.go

@@ -73,8 +73,8 @@ func (m *mockGithub) PostComment(prNumber string, comment string) error {
 	return nil
 }
 
-func (m *mockGithub) AddLabel(prNumber string, label string) error {
-	m.calledMethods["AddLabel"] = append(m.calledMethods["AddLabel"], []any{prNumber, label})
+func (m *mockGithub) AddLabels(prNumber string, labels []string) error {
+	m.calledMethods["AddLabels"] = append(m.calledMethods["AddLabels"], []any{prNumber, labels})
 	return nil
 }
 

.ci/magician/cmd/mock_runner_test.go

@@ -70,7 +70,7 @@ func NewMockRunner() MockRunner {
 			"/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] " + 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/diff-processor bin/diff-processor [changed-schema-labels] map[]":                                                                                       "[\"service/google-x\"]",
 			"/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[]":                                                              "",
 			"/mock/dir/magic-modules/tools/missing-test-detector go [mod tidy] map[]":                                                                                                             "",

.ci/magician/github/set.go

@@ -74,16 +74,16 @@ func (gh *Client) RequestPullRequestReviewer(prNumber, assignee string) error {
 	return nil
 }
 
-func (gh *Client) AddLabel(prNumber, label string) error {
+func (gh *Client) AddLabels(prNumber string, labels []string) error {
 	url := fmt.Sprintf("https://api.github.com/repos/GoogleCloudPlatform/magic-modules/issues/%s/labels", prNumber)
 
 	body := map[string][]string{
-		"labels": {label},
+		"labels": labels,
 	}
 	err := utils.RequestCall(url, "POST", gh.token, nil, body)
 
 	if err != nil {
-		return fmt.Errorf("failed to add %s label: %s", label, err)
+		return fmt.Errorf("failed to add %q labels: %s", labels, err)
 	}
 
 	return nil

.ci/magician/source/repo.go

@@ -8,13 +8,13 @@ import (
 )
 
 type Repo struct {
-	Name        string // Name in GitHub (e.g. magic-modules)
-	Title       string // Title for display (e.g. Magic Modules)
-	Branch      string // Branch to clone, optional
-	Owner       string // Owner of repo, optional
-	Path        string // local Path once cloned, including Name
-	Version     provider.Version
-	Cloned      bool
+	Name    string // Name in GitHub (e.g. magic-modules)
+	Title   string // Title for display (e.g. Magic Modules)
+	Branch  string // Branch to clone, optional
+	Owner   string // Owner of repo, optional
+	Path    string // local Path once cloned, including Name
+	Version provider.Version
+	Cloned  bool
 }
 
 type Controller struct {