Skip to content

Use commit shas instead of branch names in diff comment #13732

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 4 commits into from
Apr 28, 2025
Merged

Use commit shas instead of branch names in diff comment #13732

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
ef2e244
Use commit shas instead of branch names in diff comment
trodge Apr 18, 2025
b159aae
Make a test change
trodge Apr 23, 2025
3be438d
Ignore nothing to commit error
trodge Apr 28, 2025
e6c2ff9
Revert "Make a test change"
trodge Apr 28, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
33 changes: 23 additions & 10 deletions .ci/magician/cmd/generate_comment.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down Expand Up @@ -77,7 +79,6 @@ type Errors struct {
}

type diffCommentData struct {
PrNumber int
Diffs []Diff
BreakingChanges []BreakingChange
MissingServiceLabels []string
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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 {
Expand Down
38 changes: 29 additions & 9 deletions .ci/magician/cmd/generate_comment_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@
package cmd

import (
"fmt"
"os"
"reflect"
"testing"
Expand All @@ -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,
"*******",
Expand Down Expand Up @@ -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 {
Expand Down Expand Up @@ -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",
Expand Down
9 changes: 7 additions & 2 deletions .ci/magician/cmd/generate_downstream.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -352,8 +352,13 @@ 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 {
Expand Down
7 changes: 6 additions & 1 deletion .ci/magician/cmd/mock_runner_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down Expand Up @@ -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
}

Expand Down
2 changes: 1 addition & 1 deletion .ci/magician/cmd/templates/DIFF_COMMENT.md.tmpl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 -}}

Expand Down
Loading