Merge branch 'GoogleCloudPlatform:main' into lv_dp

Author: ron-gal

.ci/infra/terraform/README.md

@@ -73,3 +73,4 @@ Quotas that will need to be adjusted to support all tests:
 - compute.googleapis.com/c2_cpus (us-central1)
 - compute.googleapis.com/n2_cpus (us-central1) to 36+
 - VMware Engine standard 72 vCPUs nodes per region - southamerica-east1 to 21
+- logging.googleapis.com/log_buckets_count to 200

.ci/magician/cmd/delete_branches_test.go

@@ -1,32 +1,43 @@
 package cmd
 
 import (
-	"magician/exec"
-	"magician/github"
-	"os"
 	"testing"
 )
 
 func TestFetchPRNumber(t *testing.T) {
-	rnr, err := exec.NewRunner()
-	if err != nil {
-		t.Errorf("error creating Runner: %s", err)
-	}
-
-	githubToken, ok := os.LookupEnv("GITHUB_TOKEN_CLASSIC")
-	if !ok {
-		t.Errorf("did not provide GITHUB_TOKEN_CLASSIC environment variable")
+	mr := NewMockRunner()
+	gh := &mockGithub{
+		calledMethods: make(map[string][][]any),
+		commitMessage: "Add `additional_group_keys` attribute to `google_cloud_identity_group` resource (#9217) (#6504)\n\n* Add `additional_group_keys` attribute to `google_cloud_identity_group` resource\n\n* Update acceptance test to check for attribute\n\n* Fix test check\n\n* Add `output: true` to nested properties in output field\n[upstream:49d3741f9d4d810a0a4768363bb8498afa21c688]\n\nSigned-off-by: Modular Magician <[email protected]>",
 	}
 
-	gh := github.NewClient(githubToken)
-
-	prNumber, err := fetchPRNumber("8c6e61bb62d52c950008340deafc1e2a2041898a", "main", rnr, gh)
-
+	// Call function with mocks
+	prNumber, err := fetchPRNumber("8c6e61bb62d52c950008340deafc1e2a2041898a", "main", mr, gh)
 	if err != nil {
 		t.Errorf("error fetching PR number: %s", err)
 	}
 
 	if prNumber != "6504" {
 		t.Errorf("PR number is %s, expected 6504", prNumber)
 	}
+
+	// Verify GitHub API was called
+	if calls, ok := gh.calledMethods["GetCommitMessage"]; !ok || len(calls) == 0 {
+		t.Errorf("Expected GetCommitMessage to be called")
+	} else {
+		args := calls[0]
+		if len(args) != 3 {
+			t.Errorf("Expected GetCommitMessage to be called with 3 arguments, got %d", len(args))
+		} else {
+			if args[0] != "hashicorp" {
+				t.Errorf("Expected owner to be 'hashicorp', got '%s'", args[0])
+			}
+			if args[1] != "terraform-provider-google-beta" {
+				t.Errorf("Expected repo to be 'terraform-provider-google-beta', got '%s'", args[1])
+			}
+			if args[2] != "8c6e61bb62d52c950008340deafc1e2a2041898a" {
+				t.Errorf("Expected SHA to be '8c6e61bb62d52c950008340deafc1e2a2041898a', got '%s'", args[2])
+			}
+		}
+	}
 }

.ci/magician/cmd/generate_comment.go

@@ -45,9 +45,11 @@ var (
 )
 
 type Diff struct {
-	Title     string
-	Repo      string
-	ShortStat string
+	Title        string
+	Repo         string
+	ShortStat    string
+	CommitSHA    string
+	OldCommitSHA string
 }
 
 type BreakingChange struct {
@@ -77,7 +79,6 @@ type Errors struct {
 }
 
 type diffCommentData struct {
-	PrNumber             int
 	Diffs                []Diff
 	BreakingChanges      []BreakingChange
 	MissingServiceLabels []string
@@ -214,9 +215,7 @@ func execGenerateComment(prNumber int, ghTokenMagicModules, buildId, buildStep,
 	}
 
 	// Initialize repos
-	data := diffCommentData{
-		PrNumber: prNumber,
-	}
+	data := diffCommentData{}
 	for _, repo := range []*source.Repo{&tpgRepo, &tpgbRepo, &tgcRepo, &tfoicsRepo} {
 		errors[repo.Title] = []string{}
 		repo.Branch = newBranch
@@ -262,10 +261,24 @@ func execGenerateComment(prNumber int, ghTokenMagicModules, buildId, buildStep,
 			errors[repo.Title] = append(errors[repo.Title], "Failed to compute repo diff shortstats")
 		}
 		if shortStat != "" {
+			variablePath := fmt.Sprintf("/workspace/commitSHA_modular-magician_%s.txt", repo.Name)
+			oldVariablePath := fmt.Sprintf("/workspace/commitSHA_modular-magician_%s-old.txt", repo.Name)
+			commitSHA, err := rnr.ReadFile(variablePath)
+			if err != nil {
+				errors[repo.Title] = append(errors[repo.Title], "Failed to read commit sha from file")
+				continue
+			}
+			oldCommitSHA, err := rnr.ReadFile(oldVariablePath)
+			if err != nil {
+				errors[repo.Title] = append(errors[repo.Title], "Failed to read old commit sha from file")
+				continue
+			}
 			diffs = append(diffs, Diff{
-				Title:     repo.Title,
-				Repo:      repo.Name,
-				ShortStat: shortStat,
+				Title:        repo.Title,
+				Repo:         repo.Name,
+				ShortStat:    shortStat,
+				CommitSHA:    commitSHA,
+				OldCommitSHA: oldCommitSHA,
 			})
 			repo.ChangedFiles, err = ctlr.DiffNameOnly(repo, oldBranch, newBranch)
 			if err != nil {

.ci/magician/cmd/generate_comment_test.go

@@ -16,6 +16,7 @@
 package cmd
 
 import (
+	"fmt"
 	"os"
 	"reflect"
 	"testing"
@@ -38,6 +39,22 @@ func TestExecGenerateComment(t *testing.T) {
 		"GOPATH":  os.Getenv("GOPATH"),
 		"HOME":    os.Getenv("HOME"),
 	}
+	for _, repo := range []string{
+		"terraform-provider-google",
+		"terraform-provider-google-beta",
+		"terraform-google-conversion",
+	} {
+		variablePathOld := fmt.Sprintf("/workspace/commitSHA_modular-magician_%s-old.txt", repo)
+		variablePath := fmt.Sprintf("/workspace/commitSHA_modular-magician_%s.txt", repo)
+		err := mr.WriteFile(variablePathOld, "1a2a3a4a")
+		if err != nil {
+			t.Errorf("Error writing file: %s", err)
+		}
+		err = mr.WriteFile(variablePath, "1a2a3a4b")
+		if err != nil {
+			t.Errorf("Error writing file: %s", err)
+		}
+	}
 	execGenerateComment(
 		123456,
 		"*******",
@@ -115,7 +132,7 @@ func TestExecGenerateComment(t *testing.T) {
 			{"123456", "terraform-provider-breaking-change-test", "success", "https://console.cloud.google.com/cloud-build/builds;region=global/build1;step=17?project=project1", "sha1"},
 			{"123456", "terraform-provider-missing-service-labels", "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\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\n\n"}},
+		"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/1a2a3a4a..1a2a3a4b) ( 2 files changed, 40 insertions(+))\n`google-beta` provider: [Diff](https://github.com/modular-magician/terraform-provider-google-beta/compare/1a2a3a4a..1a2a3a4b) ( 2 files changed, 40 insertions(+))\n`terraform-google-conversion`: [Diff](https://github.com/modular-magician/terraform-google-conversion/compare/1a2a3a4a..1a2a3a4b) ( 1 file changed, 10 insertions(+))\n\n\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\n\n"}},
 		"AddLabels":   {{"123456", []string{"service/alloydb"}}},
 	} {
 		if actualCalls, ok := gh.calledMethods[method]; !ok {
@@ -170,24 +187,27 @@ func TestFormatDiffComment(t *testing.T) {
 		},
 		"diffs are displayed": {
 			data: diffCommentData{
-				PrNumber: 1234567890,
 				Diffs: []Diff{
 					{
-						Title:     "Repo 1",
-						Repo:      "repo-1",
-						ShortStat: "+1 added, -1 removed",
+						Title:        "Repo 1",
+						Repo:         "repo-1",
+						ShortStat:    "+1 added, -1 removed",
+						CommitSHA:    "1a2a3a4b",
+						OldCommitSHA: "1a2a3a4a",
 					},
 					{
-						Title:     "Repo 2",
-						Repo:      "repo-2",
-						ShortStat: "+2 added, -2 removed",
+						Title:        "Repo 2",
+						Repo:         "repo-2",
+						ShortStat:    "+2 added, -2 removed",
+						CommitSHA:    "1a2a3a4d",
+						OldCommitSHA: "1a2a3a4c",
 					},
 				},
 			},
 			expectedStrings: []string{
 				"## Diff report",
 				"generated some diffs",
-				"Repo 1: [Diff](https://github.com/modular-magician/repo-1/compare/auto-pr-1234567890-old..auto-pr-1234567890) (+1 added, -1 removed)\nRepo 2: [Diff](https://github.com/modular-magician/repo-2/compare/auto-pr-1234567890-old..auto-pr-1234567890) (+2 added, -2 removed)",
+				"Repo 1: [Diff](https://github.com/modular-magician/repo-1/compare/1a2a3a4a..1a2a3a4b) (+1 added, -1 removed)\nRepo 2: [Diff](https://github.com/modular-magician/repo-2/compare/1a2a3a4c..1a2a3a4d) (+2 added, -2 removed)",
 			},
 			notExpectedStrings: []string{
 				"hasn't generated any diffs",

.ci/magician/cmd/generate_downstream.go

@@ -300,9 +300,11 @@ func runMake(downstreamRepo *source.Repo, command string, rnr ExecRunner) error
 			return err
 		}
 	case "terraform":
+		// --- legacy -- can be cleaned up after go/mm-pull/13722 is submitted
 		if _, err := rnr.Run("make", []string{"clean-provider", "OUTPUT_PATH=" + downstreamRepo.Path}, nil); err != nil {
 			return err
 		}
+		// -------------------------------------------------------------------
 		if _, err := rnr.Run("make", []string{"provider", "OUTPUT_PATH=" + downstreamRepo.Path, fmt.Sprintf("VERSION=%s", downstreamRepo.Version)}, nil); err != nil {
 			return err
 		}
@@ -338,8 +340,9 @@ func createCommit(scratchRepo *source.Repo, commitMessage string, rnr ExecRunner
 		return "", err
 	}
 
-	if _, err := rnr.Run("git", []string{"commit", "--signoff", "-m", commitMessage}, nil); err != nil {
-		return "", err
+	_, commitErr := rnr.Run("git", []string{"commit", "--signoff", "-m", commitMessage}, nil)
+	if commitErr != nil && !strings.Contains(commitErr.Error(), "nothing to commit") {
+		return "", commitErr
 	}
 
 	commitSha, err := rnr.Run("git", []string{"rev-parse", "HEAD"}, nil)
@@ -352,16 +355,21 @@ func createCommit(scratchRepo *source.Repo, commitMessage string, rnr ExecRunner
 
 	// auto-pr's use commitSHA_modular-magician_<repo>_.txt file to communicate commmit hash
 	// across cloudbuild steps. Used in test-tpg to execute unit tests for the HEAD commit
-	if strings.HasPrefix(scratchRepo.Branch, "auto-pr-") && !strings.HasSuffix(scratchRepo.Branch, "-old") {
-		variablePath := fmt.Sprintf("/workspace/commitSHA_modular-magician_%s.txt", scratchRepo.Name)
+	if strings.HasPrefix(scratchRepo.Branch, "auto-pr-") {
+		var variablePath string
+		if strings.HasSuffix(scratchRepo.Branch, "-old") {
+			variablePath = fmt.Sprintf("/workspace/commitSHA_modular-magician_%s-old.txt", scratchRepo.Name)
+		} else {
+			variablePath = fmt.Sprintf("/workspace/commitSHA_modular-magician_%s.txt", scratchRepo.Name)
+		}
 		fmt.Println("variablePath: ", variablePath)
 		err = rnr.WriteFile(variablePath, commitSha)
 		if err != nil {
 			fmt.Println("Error:", err)
 		}
 	}
 
-	return commitSha, err
+	return commitSha, commitErr
 }
 
 func addChangelogEntry(downstreamRepo *source.Repo, pullRequest *github.PullRequest, rnr ExecRunner) error {

.ci/magician/cmd/mock_github_test.go

@@ -29,6 +29,7 @@ type mockGithub struct {
 	pullRequestComments []github.PullRequestComment
 	teamMembers         map[string][]github.User
 	calledMethods       map[string][][]any
+	commitMessage       string
 }
 
 func (m *mockGithub) GetPullRequest(prNumber string) (github.PullRequest, error) {
@@ -63,7 +64,7 @@ func (m *mockGithub) GetPullRequestComments(prNumber string) ([]github.PullReque
 
 func (m *mockGithub) GetCommitMessage(owner, repo, sha string) (string, error) {
 	m.calledMethods["GetCommitMessage"] = append(m.calledMethods["GetCommitMessage"], []any{owner, repo, sha})
-	return "commit message", nil
+	return m.commitMessage, nil
 }
 
 func (m *mockGithub) GetTeamMembers(organization, team string) ([]github.User, error) {

.ci/magician/cmd/mock_runner_test.go

@@ -41,6 +41,7 @@ type mockRunner struct {
 	cwd           string
 	dirStack      *list.List
 	notifyError   bool
+	fileContents  map[string]string
 }
 
 func sortedEnvString(env map[string]string) string {
@@ -107,10 +108,14 @@ func (mr *mockRunner) Walk(root string, fn filepath.WalkFunc) error {
 }
 
 func (mr *mockRunner) ReadFile(name string) (string, error) {
-	return "", nil
+	return mr.fileContents[name], nil
 }
 
 func (mr *mockRunner) WriteFile(name, data string) error {
+	if mr.fileContents == nil {
+		mr.fileContents = make(map[string]string)
+	}
+	mr.fileContents[name] = data
 	return nil
 }
 

.ci/magician/cmd/templates/DIFF_COMMENT.md.tmpl

@@ -7,7 +7,7 @@ Your PR hasn't generated any diffs, but I'll let you know if a future commit doe
 Your PR generated some diffs in downstreams - here they are.
 
 {{range .Diffs -}}
-{{.Title}}: [Diff](https://github.com/modular-magician/{{.Repo}}/compare/auto-pr-{{$.PrNumber}}-old..auto-pr-{{$.PrNumber}}) ({{.ShortStat}})
+{{.Title}}: [Diff](https://github.com/modular-magician/{{.Repo}}/compare/{{.OldCommitSHA}}..{{.CommitSHA}}) ({{.ShortStat}})
 {{end -}}
 {{end -}}
 

.ci/magician/github/get.go

@@ -145,3 +145,28 @@ func (gh *Client) GetTeamMembers(organization, team string) ([]User, error) {
 	}
 	return members, nil
 }
+
+func (gh *Client) IsOrgMember(author, org string) bool {
+	url := fmt.Sprintf("https://api.github.com/orgs/%s/members/%s", org, author)
+	err := utils.RequestCallWithRetry(url, "GET", gh.token, nil, nil)
+	return err == nil
+}
+
+func (gh *Client) IsTeamMember(organization, teamSlug, username string) bool {
+	type TeamMembership struct {
+		URL   string `json:"url"`
+		Role  string `json:"role"`
+		State string `json:"state"`
+	}
+
+	url := fmt.Sprintf("https://api.github.com/orgs/%s/teams/%s/memberships/%s", organization, teamSlug, username)
+	var membership TeamMembership
+	err := utils.RequestCallWithRetry(url, "GET", gh.token, &membership, nil)
+
+	if err != nil {
+		return false
+	}
+
+	// User is considered a member if state is "active"
+	return membership.State == "active"
+}

.ci/magician/github/membership.go

@@ -50,12 +50,19 @@ func (gh *Client) GetUserType(user string) UserType {
 		return CoreContributorUserType
 	}
 
-	if isOrgMember(user, "GoogleCloudPlatform", gh.token) {
+	if gh.IsTeamMember("GoogleCloudPlatform", "terraform", user) {
+		fmt.Println("User is an active member of the 'terraform' team in 'GoogleCloudPlatform' organization")
+		return GooglerUserType
+	} else {
+		fmt.Printf("User '%s' is not an active member of the 'terraform' team in 'GoogleCloudPlatform' organization\n", user)
+	}
+
+	if gh.IsOrgMember(user, "GoogleCloudPlatform") {
 		fmt.Println("User is a GCP org member")
 		return GooglerUserType
 	}
 
-	if isOrgMember(user, "googlers", gh.token) {
+	if gh.IsOrgMember(user, "googlers") {
 		fmt.Println("User is a googlers org member")
 		return GooglerUserType
 	}
@@ -74,13 +81,6 @@ func IsCoreReviewer(user string) bool {
 	return isCoreReviewer
 }
 
-func isOrgMember(author, org, githubToken string) bool {
-	url := fmt.Sprintf("https://api.github.com/orgs/%s/members/%s", org, author)
-	err := utils.RequestCallWithRetry(url, "GET", githubToken, nil, nil)
-
-	return err == nil
-}
-
 // GetRandomReviewer returns a random available reviewer (optionally excluding some people from the reviewer pool)
 func GetRandomReviewer(excludedReviewers []string) string {
 	availableReviewers := AvailableReviewers(excludedReviewers)

.ci/magician/github/set.go

@@ -19,6 +19,7 @@ import (
 	"fmt"
 	utils "magician/utility"
 	"strings"
+	"time"
 )
 
 func (gh *Client) PostBuildStatus(prNumber, title, state, targetURL, commitSha string) error {
@@ -165,7 +166,10 @@ func (gh *Client) MergePullRequest(owner, repo, prNumber, commitSha string) erro
 		// Check if the error is "Merge already in progress" (405)
 		if strings.Contains(err.Error(), "Merge already in progress") {
 			fmt.Printf("Pull request %s is already being merged\n", prNumber)
-			return nil
+			// This status does not indicate that the Pull Request was merged
+			// Try again after 20s
+			time.Sleep(20 * time.Second)
+			return gh.MergePullRequest(owner, repo, prNumber, commitSha)
 		}
 		// Check if the PR is already merged (returns 405 Pull Request is not mergeable)
 		if strings.Contains(err.Error(), "Pull Request is not mergeable") {