From ef2e2441084317c7f69b0e97ec8df4fd2599554e Mon Sep 17 00:00:00 2001 From: Thomas Rodgers Date: Thu, 17 Apr 2025 17:01:05 -0700 Subject: [PATCH 1/4] Use commit shas instead of branch names in diff comment --- .ci/magician/cmd/generate_comment.go | 33 +++++++++++----- .ci/magician/cmd/generate_comment_test.go | 38 ++++++++++++++----- .ci/magician/cmd/generate_downstream.go | 9 ++++- .ci/magician/cmd/mock_runner_test.go | 7 +++- .../cmd/templates/DIFF_COMMENT.md.tmpl | 2 +- 5 files changed, 66 insertions(+), 23 deletions(-) diff --git a/.ci/magician/cmd/generate_comment.go b/.ci/magician/cmd/generate_comment.go index 9c526cc51e1b..ac689fdc8db7 100644 --- a/.ci/magician/cmd/generate_comment.go +++ b/.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 { diff --git a/.ci/magician/cmd/generate_comment_test.go b/.ci/magician/cmd/generate_comment_test.go index 25d91879db76..8b08fb59a3b9 100644 --- a/.ci/magician/cmd/generate_comment_test.go +++ b/.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", diff --git a/.ci/magician/cmd/generate_downstream.go b/.ci/magician/cmd/generate_downstream.go index 402bf704e2dc..2ffcc5091f71 100644 --- a/.ci/magician/cmd/generate_downstream.go +++ b/.ci/magician/cmd/generate_downstream.go @@ -352,8 +352,13 @@ func createCommit(scratchRepo *source.Repo, commitMessage string, rnr ExecRunner // auto-pr's use commitSHA_modular-magician__.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 { diff --git a/.ci/magician/cmd/mock_runner_test.go b/.ci/magician/cmd/mock_runner_test.go index 79bc206f2c43..742ca0617dba 100644 --- a/.ci/magician/cmd/mock_runner_test.go +++ b/.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 } diff --git a/.ci/magician/cmd/templates/DIFF_COMMENT.md.tmpl b/.ci/magician/cmd/templates/DIFF_COMMENT.md.tmpl index f50fdee4626a..7f22c8073e3a 100644 --- a/.ci/magician/cmd/templates/DIFF_COMMENT.md.tmpl +++ b/.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 -}} From b159aaebc83c6498ceb3dcaad0786e37593a0a48 Mon Sep 17 00:00:00 2001 From: Thomas Rodgers Date: Wed, 23 Apr 2025 15:39:01 -0700 Subject: [PATCH 2/4] Make a test change --- .../services/compute/resource_compute_instance.go.tmpl | 3 +++ 1 file changed, 3 insertions(+) diff --git a/mmv1/third_party/terraform/services/compute/resource_compute_instance.go.tmpl b/mmv1/third_party/terraform/services/compute/resource_compute_instance.go.tmpl index d85a74e24e3f..fbbc58e4cedf 100644 --- a/mmv1/third_party/terraform/services/compute/resource_compute_instance.go.tmpl +++ b/mmv1/third_party/terraform/services/compute/resource_compute_instance.go.tmpl @@ -1,5 +1,8 @@ package compute + +// Test change. + import ( "context" "crypto/sha256" From 3be438de070334f449b143506982e7104aad72c2 Mon Sep 17 00:00:00 2001 From: Thomas Rodgers Date: Mon, 28 Apr 2025 14:27:33 -0700 Subject: [PATCH 3/4] Ignore nothing to commit error --- .ci/magician/cmd/generate_downstream.go | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/.ci/magician/cmd/generate_downstream.go b/.ci/magician/cmd/generate_downstream.go index 2ffcc5091f71..9f8713b1a6ed 100644 --- a/.ci/magician/cmd/generate_downstream.go +++ b/.ci/magician/cmd/generate_downstream.go @@ -339,7 +339,9 @@ func createCommit(scratchRepo *source.Repo, commitMessage string, rnr ExecRunner } if _, err := rnr.Run("git", []string{"commit", "--signoff", "-m", commitMessage}, nil); err != nil { - return "", err + if !strings.Contains(err.Error(), "nothing to commit") { + return "", err + } } commitSha, err := rnr.Run("git", []string{"rev-parse", "HEAD"}, nil) From e6c2ff97db81c9bb4b642f7dd6639f4c2fbf4003 Mon Sep 17 00:00:00 2001 From: Thomas Rodgers Date: Mon, 28 Apr 2025 16:00:08 -0700 Subject: [PATCH 4/4] Revert "Make a test change" This reverts commit b159aaebc83c6498ceb3dcaad0786e37593a0a48. --- .../services/compute/resource_compute_instance.go.tmpl | 3 --- 1 file changed, 3 deletions(-) diff --git a/mmv1/third_party/terraform/services/compute/resource_compute_instance.go.tmpl b/mmv1/third_party/terraform/services/compute/resource_compute_instance.go.tmpl index fbbc58e4cedf..d85a74e24e3f 100644 --- a/mmv1/third_party/terraform/services/compute/resource_compute_instance.go.tmpl +++ b/mmv1/third_party/terraform/services/compute/resource_compute_instance.go.tmpl @@ -1,8 +1,5 @@ package compute - -// Test change. - import ( "context" "crypto/sha256"