Further refactoring of magician VCR (GoogleCloudPlatform#11647)

Co-authored-by: Stephen Lewis (Burrows) <[email protected]>

.ci/gcb-pr-downstream-generation-and-test.yml

@@ -264,7 +264,7 @@ steps:
         - $COMMIT_SHA
         - $BUILD_ID
         - $PROJECT_ID
-        - "22"  # Build step
+        - "23"  # Build step
 
     - name: 'gcr.io/graphite-docker-images/go-plus'
       entrypoint: '/workspace/.ci/scripts/go-plus/magician/exec.sh'

.ci/magician/cmd/check_cassettes.go

@@ -64,7 +64,7 @@ var checkCassettesCmd = &cobra.Command{
 
 		ctlr := source.NewController(env["GOPATH"], "modular-magician", githubToken, rnr)
 
-		vt, err := vcr.NewTester(env, "vcr-check-cassettes", "ci-vcr-cassettes", rnr)
+		vt, err := vcr.NewTester(env, "ci-vcr-cassettes", "vcr-check-cassettes", rnr)
 		if err != nil {
 			return fmt.Errorf("error creating VCR tester: %w", err)
 		}

.ci/magician/cmd/generate_comment.go

@@ -504,9 +504,9 @@ func detectMissingTests(diffProcessorPath, tpgbLocalPath string, rnr ExecRunner)
 }
 
 func formatDiffComment(data diffCommentData) (string, error) {
-	tmpl, err := template.New("DIFF_COMMENT.md").Parse(diffComment)
+	tmpl, err := template.New("DIFF_COMMENT.md.tmpl").Parse(diffComment)
 	if err != nil {
-		panic(fmt.Sprintf("Unable to parse DIFF_COMMENT.md: %s", err))
+		return "", fmt.Errorf("unable to parse template DIFF_COMMENT.md.tmpl: %s", err)
 	}
 	sb := new(strings.Builder)
 	err = tmpl.Execute(sb, data)

.ci/magician/cmd/test_terraform_vcr.go

@@ -120,7 +120,7 @@ var testTerraformVCRCmd = &cobra.Command{
 		}
 		ctlr := source.NewController(env["GOPATH"], "modular-magician", env["GITHUB_TOKEN_DOWNSTREAMS"], rnr)
 
-		vt, err := vcr.NewTester(env, "ci-vcr-logs", "ci-vcr-cassettes", rnr)
+		vt, err := vcr.NewTester(env, "ci-vcr-cassettes", "ci-vcr-logs", rnr)
 		if err != nil {
 			return fmt.Errorf("error creating VCR tester: %w", err)
 		}
@@ -180,7 +180,7 @@ func execTestTerraformVCR(prNumber, mmCommitSha, buildID, projectID, buildStep,
 	}
 	fmt.Println("Running tests: Go files or test fixtures changed")
 
-	if err := vt.FetchCassettes(provider.Beta, baseBranch, prNumber); err != nil {
+	if err := vt.FetchCassettes(provider.Beta, baseBranch, newBranch); err != nil {
 		return fmt.Errorf("error fetching cassettes: %w", err)
 	}
 
@@ -196,10 +196,10 @@ func execTestTerraformVCR(prNumber, mmCommitSha, buildID, projectID, buildStep,
 	}
 
 	if err := vt.UploadLogs(vcr.UploadLogsOptions{
-		PRNumber: prNumber,
-		BuildID:  buildID,
-		Mode:     vcr.Replaying,
-		Version:  provider.Beta,
+		Head:    newBranch,
+		BuildID: buildID,
+		Mode:    vcr.Replaying,
+		Version: provider.Beta,
 	}); err != nil {
 		return fmt.Errorf("error uploading replaying logs: %w", err)
 	}
@@ -261,12 +261,12 @@ func execTestTerraformVCR(prNumber, mmCommitSha, buildID, projectID, buildStep,
 			testState = "success"
 		}
 
-		if err := vt.UploadCassettes(prNumber, provider.Beta); err != nil {
+		if err := vt.UploadCassettes(newBranch, provider.Beta); err != nil {
 			return fmt.Errorf("error uploading cassettes: %w", err)
 		}
 
 		if err := vt.UploadLogs(vcr.UploadLogsOptions{
-			PRNumber: prNumber,
+			Head:     newBranch,
 			BuildID:  buildID,
 			Parallel: true,
 			Mode:     vcr.Recording,
@@ -295,10 +295,10 @@ func execTestTerraformVCR(prNumber, mmCommitSha, buildID, projectID, buildStep,
 			}
 
 			if err := vt.UploadLogs(vcr.UploadLogsOptions{
-				PRNumber:       prNumber,
+				Head:           newBranch,
 				BuildID:        buildID,
-				Parallel:       true,
 				AfterRecording: true,
+				Parallel:       true,
 				Mode:           vcr.Replaying,
 				Version:        provider.Beta,
 			}); err != nil {
@@ -491,21 +491,21 @@ func formatComment(fileName string, tmplText string, data any) (string, error) {
 }
 
 func formatTestsAnalytics(data analytics) (string, error) {
-	return formatComment("test_terraform_vcr_test_analytics.tmpl", testsAnalyticsTmplText, data)
+	return formatComment("test_analytics.tmpl", testsAnalyticsTmplText, data)
 }
 
 func formatNonExercisedTests(data nonExercisedTests) (string, error) {
-	return formatComment("test_terraform_vcr_recording_mode_results.tmpl", nonExercisedTestsTmplText, data)
+	return formatComment("non_exercised_tests.tmpl", nonExercisedTestsTmplText, data)
 }
 
 func formatWithReplayFailedTests(data withReplayFailedTests) (string, error) {
-	return formatComment("test_terraform_vcr_with_replay_failed_tests.tmpl", withReplayFailedTestsTmplText, data)
+	return formatComment("with_replay_failed_tests.tmpl", withReplayFailedTestsTmplText, data)
 }
 
 func formatWithoutReplayFailedTests(data withoutReplayFailedTests) (string, error) {
-	return formatComment("test_terraform_vcr_without_replay_failed_tests.tmpl", withoutReplayFailedTestsTmplText, data)
+	return formatComment("without_replay_failed_tests.tmpl", withoutReplayFailedTestsTmplText, data)
 }
 
 func formatRecordReplay(data recordReplay) (string, error) {
-	return formatComment("test_terraform_vcr_record_replay.tmpl", recordReplayTmplText, data)
+	return formatComment("record_replay.tmpl", recordReplayTmplText, data)
 }

.ci/magician/cmd/vcr_cassette_update.go

@@ -90,7 +90,7 @@ var vcrCassetteUpdateCmd = &cobra.Command{
 		}
 		ctlr := source.NewController(env["GOPATH"], "hashicorp", env["GITHUB_TOKEN_CLASSIC"], rnr)
 
-		vt, err := vcr.NewTester(env, "", "ci-vcr-cassettes", rnr)
+		vt, err := vcr.NewTester(env, "ci-vcr-cassettes", "", rnr)
 		if err != nil {
 			return fmt.Errorf("error creating VCR tester: %w", err)
 		}

.ci/magician/cmd/vcr_cassette_update_test.go

@@ -401,7 +401,7 @@ func TestExecVCRCassetteUpdate(t *testing.T) {
 			ctlr := source.NewController("gopath", "hashicorp", "token", rnr)
 			vt, err := vcr.NewTester(map[string]string{
 				"SA_KEY": "sa_key",
-			}, "", "ci-vcr-cassettes", rnr)
+			}, "ci-vcr-cassettes", "", rnr)
 			if err != nil {
 				t.Fatalf("Failed to create new tester: %v", err)
 			}

.ci/magician/provider/version.go

@@ -6,16 +6,19 @@ const (
 	None Version = iota
 	GA
 	Beta
+	Alpha
 )
 
-const NumVersions = 2
+const NumVersions = 3
 
 func (v Version) String() string {
 	switch v {
 	case GA:
 		return "ga"
 	case Beta:
 		return "beta"
+	case Alpha:
+		return "alpha"
 	}
 	return "unknown"
 }
@@ -33,6 +36,8 @@ func (v Version) RepoName() string {
 		return "terraform-provider-google"
 	case Beta:
 		return "terraform-provider-google-beta"
+	case Alpha:
+		return "terraform-next"
 	}
 	return "unknown"
 }

.ci/magician/vcr/tester.go

@@ -50,8 +50,8 @@ type logKey struct {
 type Tester struct {
 	env            map[string]string           // shared environment variables for running tests
 	rnr            ExecRunner                  // for running commands and manipulating files
-	logBucket      string                      // GCS bucket name to store logs
-	cassetteBucket string                      // GCS bucket name to store cassettes
+	cassetteBucket string                      // name of GCS bucket to store cassettes
+	logBucket      string                      // name of GCS bucket to store logs
 	baseDir        string                      // the directory in which this tester was created
 	saKeyPath      string                      // where sa_key.json is relative to baseDir
 	cassettePaths  map[provider.Version]string // where cassettes are relative to baseDir by version
@@ -68,8 +68,46 @@ var testResultsExpression = regexp.MustCompile(`(?m:^--- (PASS|FAIL|SKIP): (Test
 
 var testPanicExpression = regexp.MustCompile(`^panic: .*`)
 
+var safeToLog = map[string]bool{
+	"ACCTEST_PARALLELISM":                        true,
+	"COMMIT_SHA":                                 true,
+	"GITHUB_TOKEN":                               false,
+	"GITHUB_TOKEN_CLASSIC":                       false,
+	"GITHUB_TOKEN_DOWNSTREAMS":                   false,
+	"GITHUB_TOKEN_MAGIC_MODULES":                 false,
+	"GOCACHE":                                    true,
+	"GOOGLE_APPLICATION_CREDENTIALS":             false,
+	"GOOGLE_BILLING_ACCOUNT":                     false,
+	"GOOGLE_CREDENTIALS":                         false,
+	"GOOGLE_CUST_ID":                             true,
+	"GOOGLE_IDENTITY_USER":                       true,
+	"GOOGLE_MASTER_BILLING_ACCOUNT":              false,
+	"GOOGLE_ORG":                                 true,
+	"GOOGLE_ORG_2":                               true,
+	"GOOGLE_ORG_DOMAIN":                          true,
+	"GOOGLE_PROJECT":                             true,
+	"GOOGLE_PROJECT_NUMBER":                      true,
+	"GOOGLE_PUBLIC_AVERTISED_PREFIX_DESCRIPTION": true,
+	"GOOGLE_REGION":                              true,
+	"GOOGLE_SERVICE_ACCOUNT":                     true,
+	"GOOGLE_TEST_DIRECTORY":                      true,
+	"GOOGLE_ZONE":                                true,
+	"GOPATH":                                     true,
+	"HOME":                                       true,
+	"PATH":                                       true,
+	"SA_KEY":                                     false,
+	"TF_ACC":                                     true,
+	"TF_LOG":                                     true,
+	"TF_LOG_PATH_MASK":                           true,
+	"TF_LOG_SDK_FRAMEWORK":                       true,
+	"TF_SCHEMA_PANIC_ON_ERROR":                   true,
+	"USER":                                       true,
+	"VCR_MODE":                                   true,
+	"VCR_PATH":                                   true,
+} // true if shown, false if hidden (default false)
+
 // Create a new tester in the current working directory and write the service account key file.
-func NewTester(env map[string]string, logBucket, cassetteBucket string, rnr ExecRunner) (*Tester, error) {
+func NewTester(env map[string]string, cassetteBucket, logBucket string, rnr ExecRunner) (*Tester, error) {
 	var saKeyPath string
 	if saKeyVal, ok := env["SA_KEY"]; ok {
 		saKeyPath = "sa_key.json"
@@ -80,8 +118,8 @@ func NewTester(env map[string]string, logBucket, cassetteBucket string, rnr Exec
 	return &Tester{
 		env:            env,
 		rnr:            rnr,
-		logBucket:      logBucket,
 		cassetteBucket: cassetteBucket,
+		logBucket:      logBucket,
 		baseDir:        rnr.GetCWD(),
 		saKeyPath:      saKeyPath,
 		cassettePaths:  make(map[provider.Version]string, provider.NumVersions),
@@ -96,7 +134,7 @@ func (vt *Tester) SetRepoPath(version provider.Version, repoPath string) {
 
 // Fetch the cassettes for the current version if not already fetched.
 // Should be run from the base dir.
-func (vt *Tester) FetchCassettes(version provider.Version, baseBranch, prNumber string) error {
+func (vt *Tester) FetchCassettes(version provider.Version, baseBranch, head string) error {
 	_, ok := vt.cassettePaths[version]
 	if ok {
 		return nil
@@ -116,8 +154,8 @@ func (vt *Tester) FetchCassettes(version provider.Version, baseBranch, prNumber
 			fmt.Println("Error fetching cassettes: ", err)
 		}
 	}
-	if prNumber != "" {
-		bucketPath := fmt.Sprintf("gs://%s/%srefs/heads/auto-pr-%s/fixtures/*", vt.cassetteBucket, version.BucketPath(), prNumber)
+	if head != "" {
+		bucketPath := fmt.Sprintf("gs://%s/%srefs/heads/%s/fixtures/*", vt.cassetteBucket, version.BucketPath(), head)
 		if err := vt.fetchBucketPath(bucketPath, cassettePath); err != nil {
 			fmt.Println("Error fetching cassettes: ", err)
 		}
@@ -131,7 +169,7 @@ func (vt *Tester) fetchBucketPath(bucketPath, cassettePath string) error {
 	args := []string{"-m", "-q", "cp", bucketPath, cassettePath}
 	fmt.Println("Fetching cassettes:\n", "gsutil", strings.Join(args, " "))
 	if _, err := vt.rnr.Run("gsutil", args, nil); err != nil {
-		return err
+		return fmt.Errorf("error running gsutil: %v", err)
 	}
 	return nil
 }
@@ -225,7 +263,7 @@ func (vt *Tester) Run(opt RunOptions) (Result, error) {
 	}
 	var printedEnv string
 	for ev, val := range env {
-		if ev == "SA_KEY" || ev == "GOOGLE_CREDENTIALS" || strings.HasPrefix(ev, "GITHUB_TOKEN") {
+		if !safeToLog[ev] {
 			val = "{hidden}"
 		}
 		printedEnv += fmt.Sprintf("%s=%s\n", ev, val)
@@ -403,21 +441,19 @@ func (vt *Tester) getLogPath(mode Mode, version provider.Version) (string, error
 	return logPath, nil
 }
 
-// UploadLogsOptions defines options for uploading logs.
 type UploadLogsOptions struct {
-	PRNumber       string
+	Head           string
 	BuildID        string
 	Parallel       bool
 	AfterRecording bool
 	Mode           Mode
 	Version        provider.Version
 }
 
-// UploadLogs uploads logs to Google Cloud Storage.
 func (vt *Tester) UploadLogs(opts UploadLogsOptions) error {
 	bucketPath := fmt.Sprintf("gs://%s/%s/", vt.logBucket, opts.Version)
-	if opts.PRNumber != "" {
-		bucketPath += fmt.Sprintf("refs/heads/auto-pr-%s/", opts.PRNumber)
+	if opts.Head != "" {
+		bucketPath += fmt.Sprintf("refs/heads/%s/", opts.Head)
 	}
 	if opts.BuildID != "" {
 		bucketPath += fmt.Sprintf("artifacts/%s/", opts.BuildID)
@@ -478,7 +514,7 @@ func (vt *Tester) UploadLogs(opts UploadLogsOptions) error {
 	return nil
 }
 
-func (vt *Tester) UploadCassettes(prNumber string, version provider.Version) error {
+func (vt *Tester) UploadCassettes(head string, version provider.Version) error {
 	cassettePath, ok := vt.cassettePaths[version]
 	if !ok {
 		return fmt.Errorf("no cassettes found for version %s", version)
@@ -488,7 +524,7 @@ func (vt *Tester) UploadCassettes(prNumber string, version provider.Version) err
 		"-q",
 		"cp",
 		filepath.Join(cassettePath, "*"),
-		fmt.Sprintf("gs://%s/%s/refs/heads/auto-pr-%s/fixtures/", vt.cassetteBucket, version, prNumber),
+		fmt.Sprintf("gs://%s/%s/refs/heads/%s/fixtures/", vt.cassetteBucket, version, head),
 	}
 	fmt.Println("Uploading cassettes:\n", "gsutil", strings.Join(args, " "))
 	if _, err := vt.rnr.Run("gsutil", args, nil); err != nil {