Merge branch 'GoogleCloudPlatform:main' into main

.ci/README.md

@@ -42,18 +42,17 @@ Don't panic - this is all quite safe and we have fixed it before.  We store the
 
 It's possible for a job to be cancelled or fail in the middle of pushing downstreams in a transient way.  The sorts of failures that happen at scale - lightning strikes a datacenter (ours or GitHub's!) or some other unlikely misfortune happens.  This has a chance to cause a hiccup in the downstream history, but isn't dangerous.  If that happens, the sync tags may need to be manually updated to sit at the same commit, just before the commit which needs to be generated, or some failed tasks might need to be run by hand.
 
-Updating the sync tags is done like this:
-First, check their state: `git fetch origin && git rev-parse origin/tpg-sync origin/tpgb-sync origin/tf-oics-sync origin/tgc-sync` will list the commits for each of the sync tags.
-(If you have changed the name of the `googlecloudplatform/magic-modules` remote from `origin`, substitute that name instead)
+First, check their state: `git fetch origin && git rev-parse origin/tpg-sync origin/tpgb-sync origin/tf-oics-sync origin/tgc-sync` will list the commits for each of the sync tags. (If you have changed the name of the `GoogleCloudPlatform/magic-modules` remote from `origin`, substitute that name instead, such as `git fetch upstream && git rev-parse upstream/tpg-sync upstream/tpgb-sync upstream/tf-oics-sync upstream/tgc-sync`)
+
 In normal, steady-state operation, these tags will all be identical.  When a failure occurs, some of them may be one commit ahead of the others.  It is rare for any of them to be 2 or more commits ahead of any other.  If some of them are one commit ahead of the others, and there is no pusher task currently running, this means you need to reset them by hand and rerun the failed jobs.  If they diverge by more than one commit, or a pusher task is currently running, you will need to manually run missing tasks.
 
 ### Divergence by zero commits
 
-Just click retry on the failed job in Cloud Build.  Yay!
+Just click retry on the failed job in Cloud Build. This is fairly rare, as most failures involve a step failing after another has already succeeded.
 
 ### Divergence by exactly one commit.
 
-Find which commit caused the error.  This will usually be easy - cloud build lists the commit which triggered a build, so you can probably just use that one.  You need to set all the sync tags to the parent of that commit.  Say the commit which caused the error is `12345abc`.  You can find the parent of that commit with `git rev-parse 12345abc~` (note the `~` suffix).  Some of the sync tags are likely set to this value already.  For the remainder, simply perform a git push.  Assuming that the parent commit is `98765fed`, that would be, e.g. `git push origin 98765fed:tf-validator-sync`.
+Find which commit caused the error.  This will usually be easy - cloud build lists the commit which triggered a build, so you can probably just use that one.  You need to set all the sync tags to the parent of that commit.  Say the commit which caused the error is `12345abc`.  You can find the parent of that commit with `git rev-parse 12345abc~` (note the `~` suffix).  Some of the sync tags are likely set to this value already.  For the remainder, simply perform a git push.  Assuming that the parent commit is `98765fed`, that would be, e.g. `git push -f origin 98765fed:tf-validator-sync`.
 
 If you are unlucky, there may be open PRs - this only happens if the failure occurred during the ~5 second period surrounding the merging of one of the downstreams.  Close those PRs before proceeding to the final step.
 
@@ -103,6 +102,9 @@ The best approach is
 * Build the `downstream-generator` container locally, with the new Gemfile and Gemfile.lock.  This will involve hand-modifying the Dockerfile to use the local Gemfile/Gemfile.lock instead of wget from this repo's `main` branch.  You don't need to check in those changes.
 * When that container is built, and while nothing else is running in GCB (wait, if you need to), push the container to GCR, and as soon as possible afterwards, merge the dependency-changing PR.
 
+## Changes to cloud build yaml:
+If changes are made to `gcb-contributor-membership-checker.yml` or `gcb-community-checker.yml` they will not be reflected in presubmit runs for existing PRs without a rebase. This is because these build triggers are linked to pull request creation and not pushes to the PR branch. If changes are needed to these build files they will need to be made in a backwards-compatible manner. Note that changes to other files used by these triggers will be immediately reflected in all PRs, leading to a possible disconnect between the yaml files and the rest of the CI code.
+
 ## Historical Note: Design choices & tradeoffs
 * The downstream push doesn't wait for checks on its PRs against downstreams.  This may inconvenience some existing workflows which rely on the downstream PR checks.  This ensures that merge conflicts never come into play, since the downstreams never have dangling PRs, but it requires some up-front work to get those checks into the differ.  If a new check is introduced into the downstream Travis, we will need to introduce it into the terraform-tester container.
 * The downstream push is disconnected from the output of the differ (but runs the same code).  This means that the diff which is approved isn't guaranteed to be applied *exactly*, if for instance magic modules' behavior changes on main between diff generation and downstream push.  This is also intended to avoid merge conflicts by, effectively, rebasing each commit on top of main before final generation is done.

.ci/gcb-community-checker.yml

@@ -72,6 +72,7 @@ steps:
         - $_HEAD_BRANCH
         - $_BASE_BRANCH
 
+logsBucket: 'gs://cloudbuild-community-checker-logs'
 availableSecrets:
   secretManager:
     - versionName: projects/673497134629/secrets/github-magician-token-generate-diffs-magic-modules/versions/latest

.ci/gcb-contributor-membership-checker.yml

@@ -69,6 +69,7 @@ steps:
       - $_PR_NUMBER
       - $COMMIT_SHA
 
+logsBucket: 'gs://cloudbuild-membership-checker-logs'
 availableSecrets:
   secretManager:
     - versionName: projects/673497134629/secrets/github-magician-token-generate-diffs-magic-modules/versions/latest

.ci/gcb-generate-diffs-new.yml

@@ -208,7 +208,7 @@ steps:
 
     - name: 'gcr.io/graphite-docker-images/go-plus'
       id: tgc-test-integration
-      entrypoint: '/workspace/.ci/scripts/go-plus/tgc-tester-integration/test_tgc_integration.sh'
+      entrypoint: '/workspace/.ci/scripts/go-plus/magician/exec.sh'
       allowFailure: true
       secretEnv: ["GITHUB_TOKEN_MAGIC_MODULES"]
       waitFor: ["tpgb-head", "tpgb-base", "tgc-head", "tgc-base"]
@@ -218,12 +218,13 @@ steps:
         - TEST_ANCESTRY=$_VALIDATOR_TEST_ANCESTRY
         - TEST_ORG_ID=$_VALIDATOR_TEST_ORG
       args:
-          - $_PR_NUMBER
-          - $COMMIT_SHA
-          - $BUILD_ID
-          - $PROJECT_ID
-          - "18"  # Build step
-          - terraform-google-conversion
+        - 'test-tgc-integration'
+        - $_PR_NUMBER
+        - $COMMIT_SHA
+        - $BUILD_ID
+        - $PROJECT_ID
+        - "18"  # Build step
+        - terraform-google-conversion
 
     - name: 'gcr.io/graphite-docker-images/go-plus'
       id: tpgb-test
@@ -282,6 +283,7 @@ timeout: 20000s
 options:
     machineType: 'N1_HIGHCPU_32'
 
+logsBucket: 'gs://cloudbuild-generate-diffs-logs'
 availableSecrets:
   secretManager:
     - versionName: projects/673497134629/secrets/github-magician-token-generate-diffs-downstreams/versions/latest

.ci/infra/terraform/main.tf

@@ -135,6 +135,12 @@ resource "google_organization_iam_member" "sa_storage_admin" {
   member = google_service_account.sa.member
 }
 
+resource "google_organization_iam_member" "apphub_admin" {
+  org_id = data.google_organization.org.org_id
+  role               = "roles/apphub.admin"
+  member             = google_service_account.sa.member
+}
+
 resource "google_billing_account_iam_member" "sa_master_billing_admin" {
   billing_account_id = data.google_billing_account.master_acct.id
   role               = "roles/billing.admin"

.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,39 +249,43 @@ 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.")
 		}
-		err = cleanDiffProcessor(diffProcessorPath, rnr)
-		if err != nil {
-			fmt.Println("cleaning up diff processor: ", err)
-			errors[repo.Title] = append(errors[repo.Title], "The diff processor failed to clean up properly.")
+		for _, serviceLabel := range serviceLabels {
+			uniqueServiceLabels[serviceLabel] = struct{}{}
 		}
 	}
 	breakingChangesSlice := maps.Keys(uniqueBreakingChanges)
 	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
 			}
 		}
 	}
@@ -357,6 +370,11 @@ func computeDiff(repo *source.Repo, oldBranch string, ctlr *source.Controller) (
 
 // Build the diff processor for tpg or tpgb
 func buildDiffProcessor(diffProcessorPath, providerLocalPath string, env map[string]string, rnr ExecRunner) error {
+	for _, path := range []string{"old", "new", "bin"} {
+		if err := rnr.RemoveAll(filepath.Join(diffProcessorPath, path)); err != nil {
+			return err
+		}
+	}
 	if err := rnr.PushDir(diffProcessorPath); err != nil {
 		return err
 	}
@@ -387,25 +405,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
+		}
+	}
+	if hasServiceLabels {
+		return nil, nil
 	}
-	output, err := rnr.Run("bin/diff-processor", []string{"add-labels", strconv.Itoa(prNumber)}, env)
-	fmt.Println(output)
+
+	output, err := rnr.Run("bin/diff-processor", []string{"changed-schema-labels"}, nil)
 	if err != nil {
-		return err
+		return nil, err
 	}
-	return rnr.PopDir()
-}
 
-func cleanDiffProcessor(diffProcessorPath string, rnr ExecRunner) error {
-	for _, path := range []string{"old", "new", "bin"} {
-		if err := rnr.RemoveAll(filepath.Join(diffProcessorPath, path)); err != nil {
-			return err
-		}
+	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 nil
+	return labels, nil
 }
 
 // Run the missing test detector and return the results.

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