Skip to content

Commit 526394d

Browse files
trodgeBBBmau
trodge
authored and
BBBmau
committed
Use commit shas instead of branch names in diff comment (GoogleCloudPlatform#13732)
1 parent 678c548 commit 526394d

File tree

5 files changed

+69
-24
lines changed

5 files changed

+69
-24
lines changed

‎.ci/magician/cmd/generate_comment.go

+23-10
Original file line numberDiff line numberDiff line change
@@ -45,9 +45,11 @@ var (
4545
)
4646

4747
type Diff struct {
48-
Title string
49-
Repo string
50-
ShortStat string
48+
Title string
49+
Repo string
50+
ShortStat string
51+
CommitSHA string
52+
OldCommitSHA string
5153
}
5254

5355
type BreakingChange struct {
@@ -77,7 +79,6 @@ type Errors struct {
7779
}
7880

7981
type diffCommentData struct {
80-
PrNumber int
8182
Diffs []Diff
8283
BreakingChanges []BreakingChange
8384
MissingServiceLabels []string
@@ -214,9 +215,7 @@ func execGenerateComment(prNumber int, ghTokenMagicModules, buildId, buildStep,
214215
}
215216

216217
// Initialize repos
217-
data := diffCommentData{
218-
PrNumber: prNumber,
219-
}
218+
data := diffCommentData{}
220219
for _, repo := range []*source.Repo{&tpgRepo, &tpgbRepo, &tgcRepo, &tfoicsRepo} {
221220
errors[repo.Title] = []string{}
222221
repo.Branch = newBranch
@@ -262,10 +261,24 @@ func execGenerateComment(prNumber int, ghTokenMagicModules, buildId, buildStep,
262261
errors[repo.Title] = append(errors[repo.Title], "Failed to compute repo diff shortstats")
263262
}
264263
if shortStat != "" {
264+
variablePath := fmt.Sprintf("/workspace/commitSHA_modular-magician_%s.txt", repo.Name)
265+
oldVariablePath := fmt.Sprintf("/workspace/commitSHA_modular-magician_%s-old.txt", repo.Name)
266+
commitSHA, err := rnr.ReadFile(variablePath)
267+
if err != nil {
268+
errors[repo.Title] = append(errors[repo.Title], "Failed to read commit sha from file")
269+
continue
270+
}
271+
oldCommitSHA, err := rnr.ReadFile(oldVariablePath)
272+
if err != nil {
273+
errors[repo.Title] = append(errors[repo.Title], "Failed to read old commit sha from file")
274+
continue
275+
}
265276
diffs = append(diffs, Diff{
266-
Title: repo.Title,
267-
Repo: repo.Name,
268-
ShortStat: shortStat,
277+
Title: repo.Title,
278+
Repo: repo.Name,
279+
ShortStat: shortStat,
280+
CommitSHA: commitSHA,
281+
OldCommitSHA: oldCommitSHA,
269282
})
270283
repo.ChangedFiles, err = ctlr.DiffNameOnly(repo, oldBranch, newBranch)
271284
if err != nil {

‎.ci/magician/cmd/generate_comment_test.go

+29-9
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,7 @@
1616
package cmd
1717

1818
import (
19+
"fmt"
1920
"os"
2021
"reflect"
2122
"testing"
@@ -38,6 +39,22 @@ func TestExecGenerateComment(t *testing.T) {
3839
"GOPATH": os.Getenv("GOPATH"),
3940
"HOME": os.Getenv("HOME"),
4041
}
42+
for _, repo := range []string{
43+
"terraform-provider-google",
44+
"terraform-provider-google-beta",
45+
"terraform-google-conversion",
46+
} {
47+
variablePathOld := fmt.Sprintf("/workspace/commitSHA_modular-magician_%s-old.txt", repo)
48+
variablePath := fmt.Sprintf("/workspace/commitSHA_modular-magician_%s.txt", repo)
49+
err := mr.WriteFile(variablePathOld, "1a2a3a4a")
50+
if err != nil {
51+
t.Errorf("Error writing file: %s", err)
52+
}
53+
err = mr.WriteFile(variablePath, "1a2a3a4b")
54+
if err != nil {
55+
t.Errorf("Error writing file: %s", err)
56+
}
57+
}
4158
execGenerateComment(
4259
123456,
4360
"*******",
@@ -115,7 +132,7 @@ func TestExecGenerateComment(t *testing.T) {
115132
{"123456", "terraform-provider-breaking-change-test", "success", "https://console.cloud.google.com/cloud-build/builds;region=global/build1;step=17?project=project1", "sha1"},
116133
{"123456", "terraform-provider-missing-service-labels", "success", "https://console.cloud.google.com/cloud-build/builds;region=global/build1;step=17?project=project1", "sha1"},
117134
},
118-
"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"}},
135+
"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"}},
119136
"AddLabels": {{"123456", []string{"service/alloydb"}}},
120137
} {
121138
if actualCalls, ok := gh.calledMethods[method]; !ok {
@@ -170,24 +187,27 @@ func TestFormatDiffComment(t *testing.T) {
170187
},
171188
"diffs are displayed": {
172189
data: diffCommentData{
173-
PrNumber: 1234567890,
174190
Diffs: []Diff{
175191
{
176-
Title: "Repo 1",
177-
Repo: "repo-1",
178-
ShortStat: "+1 added, -1 removed",
192+
Title: "Repo 1",
193+
Repo: "repo-1",
194+
ShortStat: "+1 added, -1 removed",
195+
CommitSHA: "1a2a3a4b",
196+
OldCommitSHA: "1a2a3a4a",
179197
},
180198
{
181-
Title: "Repo 2",
182-
Repo: "repo-2",
183-
ShortStat: "+2 added, -2 removed",
199+
Title: "Repo 2",
200+
Repo: "repo-2",
201+
ShortStat: "+2 added, -2 removed",
202+
CommitSHA: "1a2a3a4d",
203+
OldCommitSHA: "1a2a3a4c",
184204
},
185205
},
186206
},
187207
expectedStrings: []string{
188208
"## Diff report",
189209
"generated some diffs",
190-
"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)",
210+
"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)",
191211
},
192212
notExpectedStrings: []string{
193213
"hasn't generated any diffs",

‎.ci/magician/cmd/generate_downstream.go

+10-3
Original file line numberDiff line numberDiff line change
@@ -339,7 +339,9 @@ func createCommit(scratchRepo *source.Repo, commitMessage string, rnr ExecRunner
339339
}
340340

341341
if _, err := rnr.Run("git", []string{"commit", "--signoff", "-m", commitMessage}, nil); err != nil {
342-
return "", err
342+
if !strings.Contains(err.Error(), "nothing to commit") {
343+
return "", err
344+
}
343345
}
344346

345347
commitSha, err := rnr.Run("git", []string{"rev-parse", "HEAD"}, nil)
@@ -352,8 +354,13 @@ func createCommit(scratchRepo *source.Repo, commitMessage string, rnr ExecRunner
352354

353355
// auto-pr's use commitSHA_modular-magician_<repo>_.txt file to communicate commmit hash
354356
// across cloudbuild steps. Used in test-tpg to execute unit tests for the HEAD commit
355-
if strings.HasPrefix(scratchRepo.Branch, "auto-pr-") && !strings.HasSuffix(scratchRepo.Branch, "-old") {
356-
variablePath := fmt.Sprintf("/workspace/commitSHA_modular-magician_%s.txt", scratchRepo.Name)
357+
if strings.HasPrefix(scratchRepo.Branch, "auto-pr-") {
358+
var variablePath string
359+
if strings.HasSuffix(scratchRepo.Branch, "-old") {
360+
variablePath = fmt.Sprintf("/workspace/commitSHA_modular-magician_%s-old.txt", scratchRepo.Name)
361+
} else {
362+
variablePath = fmt.Sprintf("/workspace/commitSHA_modular-magician_%s.txt", scratchRepo.Name)
363+
}
357364
fmt.Println("variablePath: ", variablePath)
358365
err = rnr.WriteFile(variablePath, commitSha)
359366
if err != nil {

‎.ci/magician/cmd/mock_runner_test.go

+6-1
Original file line numberDiff line numberDiff line change
@@ -41,6 +41,7 @@ type mockRunner struct {
4141
cwd string
4242
dirStack *list.List
4343
notifyError bool
44+
fileContents map[string]string
4445
}
4546

4647
func sortedEnvString(env map[string]string) string {
@@ -107,10 +108,14 @@ func (mr *mockRunner) Walk(root string, fn filepath.WalkFunc) error {
107108
}
108109

109110
func (mr *mockRunner) ReadFile(name string) (string, error) {
110-
return "", nil
111+
return mr.fileContents[name], nil
111112
}
112113

113114
func (mr *mockRunner) WriteFile(name, data string) error {
115+
if mr.fileContents == nil {
116+
mr.fileContents = make(map[string]string)
117+
}
118+
mr.fileContents[name] = data
114119
return nil
115120
}
116121

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

+1-1
Original file line numberDiff line numberDiff line change
@@ -7,7 +7,7 @@ Your PR hasn't generated any diffs, but I'll let you know if a future commit doe
77
Your PR generated some diffs in downstreams - here they are.
88

99
{{range .Diffs -}}
10-
{{.Title}}: [Diff](https://github.com/modular-magician/{{.Repo}}/compare/auto-pr-{{$.PrNumber}}-old..auto-pr-{{$.PrNumber}}) ({{.ShortStat}})
10+
{{.Title}}: [Diff](https://github.com/modular-magician/{{.Repo}}/compare/{{.OldCommitSHA}}..{{.CommitSHA}}) ({{.ShortStat}})
1111
{{end -}}
1212
{{end -}}
1313

0 commit comments

Comments
 (0)