From 68eec5a7ffc7485bc2c283a9953c1a2bd98fbdfd Mon Sep 17 00:00:00 2001 From: Stephen Lewis Date: Wed, 20 Mar 2024 11:08:48 -0700 Subject: [PATCH 01/14] Added computation of changed file list per repo --- .ci/magician/cmd/DIFF_COMMENT.md | 2 +- .ci/magician/cmd/generate_comment.go | 44 +++++++++++------------ .ci/magician/cmd/generate_comment_test.go | 15 ++++---- .ci/magician/source/repo.go | 32 +++++++++++------ 4 files changed, 53 insertions(+), 40 deletions(-) diff --git a/.ci/magician/cmd/DIFF_COMMENT.md b/.ci/magician/cmd/DIFF_COMMENT.md index b2c44b960823..5b569de4bff5 100644 --- a/.ci/magician/cmd/DIFF_COMMENT.md +++ b/.ci/magician/cmd/DIFF_COMMENT.md @@ -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}}) ({{.DiffStats}}) +{{.Title}}: [Diff](https://github.com/modular-magician/{{.Repo}}/compare/auto-pr-{{$.PrNumber}}-old..auto-pr-{{$.PrNumber}}) ({{.ShortStat}}) {{end -}} {{end -}} diff --git a/.ci/magician/cmd/generate_comment.go b/.ci/magician/cmd/generate_comment.go index 78dcbc7b1347..40e88f18af8f 100644 --- a/.ci/magician/cmd/generate_comment.go +++ b/.ci/magician/cmd/generate_comment.go @@ -44,7 +44,7 @@ var ( type Diff struct { Title string Repo string - DiffStats string + ShortStat string } type Errors struct { @@ -187,31 +187,41 @@ func execGenerateComment(prNumber int, ghTokenMagicModules, buildId, buildStep, for _, repo := range []*source.Repo{&tpgRepo, &tpgbRepo, &tgcRepo, &tfoicsRepo} { errors[repo.Title] = []string{} repo.Branch = newBranch + repo.Cloned = true if err := ctlr.Clone(repo); err != nil { - fmt.Println("Failed to clone repo: ", err) - errors[repo.Title] = append(errors[repo.Title], "Failed to clone repo") - } else { - repo.Cloned = true + fmt.Println("Failed to clone repo at new branch: ", err) + errors[repo.Title] = append(errors[repo.Title], "Failed to clone repo at new branch") + repo.Cloned = false + } + if err := ctlr.Fetch(repo, oldBranch); err != nil { + fmt.Println("Failed to fetch old branch: ", err) + errors[repo.Title] = append(errors[repo.Title], "Failed to clone repo at old branch") + repo.Cloned = false } } diffs := []Diff{} - for _, repo := range []source.Repo{tpgRepo, tpgbRepo, tgcRepo, tfoicsRepo} { + for _, repo := range []*source.Repo{&tpgRepo, &tpgbRepo, &tgcRepo, &tfoicsRepo} { if !repo.Cloned { fmt.Println("Skipping diff; repo failed to clone: ", repo.Name) continue } - diffStats, err := computeDiff(&repo, oldBranch, ctlr) + shortStat, err := ctlr.DiffShortStat(repo, oldBranch, newBranch) if err != nil { - fmt.Println("diffing repo: ", err) - errors[repo.Title] = append(errors[repo.Title], "Failed to compute repo diff stats") + fmt.Println("diffing repo shortstats: ", err) + errors[repo.Title] = append(errors[repo.Title], "Failed to compute repo diff short stats") } - if diffStats != "" { + if shortStat != "" { diffs = append(diffs, Diff{ Title: repo.Title, Repo: repo.Name, - DiffStats: diffStats, + ShortStat: shortStat, }) + repo.ChangedFiles, err = ctlr.DiffNameOnly(repo, oldBranch, newBranch) + if err != nil { + fmt.Println("diffing repo nameonly: ", err) + errors[repo.Title] = append(errors[repo.Title], "Failed to compute repo changed filenames") + } } } data.Diffs = diffs @@ -362,18 +372,6 @@ func execGenerateComment(prNumber int, ghTokenMagicModules, buildId, buildStep, } } -func computeDiff(repo *source.Repo, oldBranch string, ctlr *source.Controller) (string, error) { - if err := ctlr.Fetch(repo, oldBranch); err != nil { - return "", err - } - // Get shortstat summary of the diff - diff, err := ctlr.Diff(repo, oldBranch, repo.Branch) - if err != nil { - return "", err - } - return strings.TrimSuffix(diff, "\n"), nil -} - // Build the diff processor for tpg or tpgb func buildDiffProcessor(diffProcessorPath, providerLocalPath string, env map[string]string, rnr ExecRunner) error { if err := rnr.PushDir(diffProcessorPath); err != nil { diff --git a/.ci/magician/cmd/generate_comment_test.go b/.ci/magician/cmd/generate_comment_test.go index f8055e165b06..f70ad9c11f0f 100644 --- a/.ci/magician/cmd/generate_comment_test.go +++ b/.ci/magician/cmd/generate_comment_test.go @@ -67,16 +67,19 @@ func TestExecGenerateComment(t *testing.T) { }, "Run": { {"/mock/dir/magic-modules/.ci/magician", "git", []string{"clone", "-b", "auto-pr-123456", "https://modular-magician:*******@github.com/modular-magician/terraform-provider-google", "/mock/dir/tpg"}, map[string]string(nil)}, + {"/mock/dir/tpg", "git", []string{"fetch", "origin", "auto-pr-123456-old"}, map[string]string(nil)}, {"/mock/dir/magic-modules/.ci/magician", "git", []string{"clone", "-b", "auto-pr-123456", "https://modular-magician:*******@github.com/modular-magician/terraform-provider-google-beta", "/mock/dir/tpgb"}, map[string]string(nil)}, + {"/mock/dir/tpgb", "git", []string{"fetch", "origin", "auto-pr-123456-old"}, map[string]string(nil)}, {"/mock/dir/magic-modules/.ci/magician", "git", []string{"clone", "-b", "auto-pr-123456", "https://modular-magician:*******@github.com/modular-magician/terraform-google-conversion", "/mock/dir/tgc"}, map[string]string(nil)}, + {"/mock/dir/tgc", "git", []string{"fetch", "origin", "auto-pr-123456-old"}, map[string]string(nil)}, {"/mock/dir/magic-modules/.ci/magician", "git", []string{"clone", "-b", "auto-pr-123456", "https://modular-magician:*******@github.com/modular-magician/docs-examples", "/mock/dir/tfoics"}, map[string]string(nil)}, - {"/mock/dir/tpg", "git", []string{"fetch", "origin", "auto-pr-123456-old"}, map[string]string(nil)}, + {"/mock/dir/tfoics", "git", []string{"fetch", "origin", "auto-pr-123456-old"}, map[string]string(nil)}, {"/mock/dir/tpg", "git", []string{"diff", "origin/auto-pr-123456-old", "origin/auto-pr-123456", "--shortstat"}, map[string]string(nil)}, - {"/mock/dir/tpgb", "git", []string{"fetch", "origin", "auto-pr-123456-old"}, map[string]string(nil)}, + {"/mock/dir/tpg", "git", []string{"diff", "origin/auto-pr-123456-old", "origin/auto-pr-123456", "--name-only"}, map[string]string(nil)}, {"/mock/dir/tpgb", "git", []string{"diff", "origin/auto-pr-123456-old", "origin/auto-pr-123456", "--shortstat"}, map[string]string(nil)}, - {"/mock/dir/tgc", "git", []string{"fetch", "origin", "auto-pr-123456-old"}, map[string]string(nil)}, + {"/mock/dir/tpgb", "git", []string{"diff", "origin/auto-pr-123456-old", "origin/auto-pr-123456", "--name-only"}, map[string]string(nil)}, {"/mock/dir/tgc", "git", []string{"diff", "origin/auto-pr-123456-old", "origin/auto-pr-123456", "--shortstat"}, map[string]string(nil)}, - {"/mock/dir/tfoics", "git", []string{"fetch", "origin", "auto-pr-123456-old"}, map[string]string(nil)}, + {"/mock/dir/tgc", "git", []string{"diff", "origin/auto-pr-123456-old", "origin/auto-pr-123456", "--name-only"}, map[string]string(nil)}, {"/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)}, @@ -173,12 +176,12 @@ func TestFormatDiffComment(t *testing.T) { { Title: "Repo 1", Repo: "repo-1", - DiffStats: "+1 added, -1 removed", + ShortStat: "+1 added, -1 removed", }, { Title: "Repo 2", Repo: "repo-2", - DiffStats: "+2 added, -2 removed", + ShortStat: "+2 added, -2 removed", }, }, }, diff --git a/.ci/magician/source/repo.go b/.ci/magician/source/repo.go index 0c53cd40695c..a342f08ba7ed 100644 --- a/.ci/magician/source/repo.go +++ b/.ci/magician/source/repo.go @@ -8,13 +8,14 @@ import ( ) type Repo struct { - Name string // Name in GitHub (e.g. magic-modules) - Title string // Title for display (e.g. Magic Modules) - Branch string // Branch to clone, optional - Owner string // Owner of repo, optional - Path string // local Path once cloned, including Name - Version provider.Version - Cloned bool + Name string // Name in GitHub (e.g. magic-modules) + Title string // Title for display (e.g. Magic Modules) + Branch string // Branch to clone, optional + Owner string // Owner of repo, optional + Path string // local Path once cloned, including Name + Version provider.Version + Cloned bool + ChangedFiles []string } type Controller struct { @@ -91,15 +92,26 @@ func (gc Controller) Fetch(repo *Repo, branch string) error { return gc.rnr.PopDir() } -func (gc Controller) Diff(repo *Repo, oldBranch, newBranch string) (string, error) { +func (gc Controller) DiffShortStat(repo *Repo, oldBranch, newBranch string) (string, error) { if err := gc.rnr.PushDir(repo.Path); err != nil { return "", err } - diffs, err := gc.rnr.Run("git", []string{"diff", "origin/" + oldBranch, "origin/" + newBranch, "--shortstat"}, nil) + shortStat, err := gc.rnr.Run("git", []string{"diff", "origin/" + oldBranch, "origin/" + newBranch, "--shortstat"}, nil) if err != nil { return "", fmt.Errorf("error diffing %s and %s: %v", oldBranch, newBranch, err) } - return diffs, gc.rnr.PopDir() + return strings.TrimSuffix(shortStat, "\n"), gc.rnr.PopDir() +} + +func (gc Controller) DiffNameOnly(repo *Repo, oldBranch, newBranch string) ([]string, error) { + if err := gc.rnr.PushDir(repo.Path); err != nil { + return nil, err + } + nameOnly, err := gc.rnr.Run("git", []string{"diff", "origin/" + oldBranch, "origin/" + newBranch, "--name-only"}, nil) + if err != nil { + return nil, fmt.Errorf("error diffing %s and %s: %v", oldBranch, newBranch, err) + } + return strings.Split(strings.TrimSuffix(nameOnly, "\n"), "\n"), gc.rnr.PopDir() } func (gc Controller) Cleanup(repo *Repo) error { From 427c54d3dbeeb72f9dfffb10d057fbfa13da8428 Mon Sep 17 00:00:00 2001 From: Stephen Lewis Date: Wed, 20 Mar 2024 13:49:43 -0700 Subject: [PATCH 02/14] Switched running missing test unit tests to check changed paths for repo instead of making a shell call --- .ci/magician/cmd/generate_comment.go | 51 ++++++++++----------- .ci/magician/cmd/generate_comment_test.go | 55 ++++++++++++++++++++++- 2 files changed, 78 insertions(+), 28 deletions(-) diff --git a/.ci/magician/cmd/generate_comment.go b/.ci/magician/cmd/generate_comment.go index 40e88f18af8f..1c45d908c2ca 100644 --- a/.ci/magician/cmd/generate_comment.go +++ b/.ci/magician/cmd/generate_comment.go @@ -326,18 +326,21 @@ func execGenerateComment(prNumber int, ghTokenMagicModules, buildId, buildStep, data.MissingTests = missingTests } - // Run unit tests for missing test detector - if err = runMissingTestUnitTests( - mmLocalPath, - tpgbRepo.Path, - targetURL, - commitSha, - prNumber, - gh, - rnr, - ); err != nil { - fmt.Println("Error running missing test detector unit tests: ", err) - errors["Other"] = append(errors["Other"], "Missing test detector unit tests failed to run.") + // Run unit tests for missing test detector (currently only for beta) + if pathChanged("tools/missing-test-detector", tpgbRepo.ChangedFiles) { + fmt.Printf("Found diffs in missing test detector:\n%s\nRunning tests.\n", diffs) + if err = runMissingTestUnitTests( + mmLocalPath, + tpgbRepo.Path, + targetURL, + commitSha, + prNumber, + gh, + rnr, + ); err != nil { + fmt.Println("Error running missing test detector unit tests: ", err) + errors["Other"] = append(errors["Other"], "Missing test detector unit tests failed to run.") + } } // Add errors to data as an ordered list @@ -523,21 +526,6 @@ func updatePackageName(name, path string, rnr ExecRunner) error { // Run unit tests for the missing test detector. // Report results using Github API. func runMissingTestUnitTests(mmLocalPath, tpgbLocalPath, targetURL, commitSha string, prNumber int, gh GithubClient, rnr ExecRunner) error { - if err := rnr.PushDir(mmLocalPath); err != nil { - return err - } - - diffs, err := rnr.Run("git", []string{"diff", "HEAD", "origin/main", "tools/missing-test-detector"}, nil) - if err != nil { - return err - } - if diffs == "" { - // Short-circuit if there are no changes to the missing test detector - return rnr.PopDir() - } - - fmt.Printf("Found diffs in missing test detector:\n%s\nRunning tests.\n", diffs) - missingTestDetectorPath := filepath.Join(mmLocalPath, "tools", "missing-test-detector") rnr.PushDir(missingTestDetectorPath) if _, err := rnr.Run("go", []string{"mod", "tidy"}, nil); err != nil { @@ -573,6 +561,15 @@ func formatDiffComment(data diffCommentData) (string, error) { return sb.String(), nil } +func pathChanged(path string, changedFiles []string) bool { + for _, f := range changedFiles { + if strings.HasPrefix(f, path) { + return true + } + } + return false +} + func init() { rootCmd.AddCommand(generateCommentCmd) } diff --git a/.ci/magician/cmd/generate_comment_test.go b/.ci/magician/cmd/generate_comment_test.go index f70ad9c11f0f..9611e76e3c4f 100644 --- a/.ci/magician/cmd/generate_comment_test.go +++ b/.ci/magician/cmd/generate_comment_test.go @@ -98,7 +98,6 @@ func TestExecGenerateComment(t *testing.T) { {"/mock/dir/magic-modules/tools/missing-test-detector", "go", []string{"mod", "edit", "-replace", "google/provider/old=/mock/dir/tpgbold"}, map[string]string(nil)}, {"/mock/dir/magic-modules/tools/missing-test-detector", "go", []string{"mod", "tidy"}, map[string]string(nil)}, {"/mock/dir/magic-modules/tools/missing-test-detector", "go", []string{"run", ".", "-services-dir=/mock/dir/tpgb/google-beta/services"}, map[string]string(nil)}, - {"/mock/dir/magic-modules", "git", []string{"diff", "HEAD", "origin/main", "tools/missing-test-detector"}, map[string]string(nil)}, }, } { if actualCalls, ok := mr.Calls(method); !ok { @@ -251,3 +250,57 @@ func TestFormatDiffComment(t *testing.T) { }) } } + +func TestPathChanged(t *testing.T) { + cases := map[string]struct { + path string + changedFiles []string + want bool + }{ + "no changed files": { + path: "path/to/folder/file.go", + changedFiles: []string{}, + want: false, + }, + "path matches exactly": { + path: "path/to/folder/file.go", + changedFiles: []string{"path/to/folder/file.go"}, + want: true, + }, + "path matches files in a folder": { + path: "path/to/folder/", + changedFiles: []string{"path/to/folder/file.go"}, + want: true, + }, + "path matches partial folder name": { + path: "path/to/folder", + changedFiles: []string{"path/to/folder2/file.go"}, + want: true, + }, + "path matches second item in list": { + path: "path/to/folder/", + changedFiles: []string{"path/to/folder2/file.go", "path/to/folder/file.go"}, + want: true, + }, + "path doesn't match files in a different folder": { + path: "path/to/folder/", + changedFiles: []string{"path/to/folder2/file.go"}, + want: false, + }, + "path doesn't match multiple items": { + path: "path/to/folder/", + changedFiles: []string{"path/to/folder2/file.go", "path/to/folder3"}, + want: false, + }, + } + + for tn, tc := range cases { + tc := tc + t.Run(tn, func(t *testing.T) { + t.Parallel() + + got := pathChanged(tc.path, tc.changedFiles) + assert.Equal(t, tc.want, got) + }) + } +} From 9be3c650489907d91aa6424b23485e2bb9fbc8a5 Mon Sep 17 00:00:00 2001 From: Stephen Lewis Date: Wed, 20 Mar 2024 16:04:02 -0700 Subject: [PATCH 03/14] Added initial implementation and tests for fileToResource --- .ci/magician/cmd/generate_comment.go | 55 ++++++++- .ci/magician/cmd/generate_comment_test.go | 130 ++++++++++++++++++++++ 2 files changed, 184 insertions(+), 1 deletion(-) diff --git a/.ci/magician/cmd/generate_comment.go b/.ci/magician/cmd/generate_comment.go index 1c45d908c2ca..10e716d7b3cb 100644 --- a/.ci/magician/cmd/generate_comment.go +++ b/.ci/magician/cmd/generate_comment.go @@ -25,6 +25,7 @@ import ( "strings" "text/template" + "github.com/GoogleCloudPlatform/magic-modules/tools/issue-labeler/labeler" "magician/exec" "magician/github" "magician/provider" @@ -240,9 +241,12 @@ func execGenerateComment(prNumber int, ghTokenMagicModules, buildId, buildStep, } for _, repo := range []source.Repo{tpgRepo, tpgbRepo} { if !repo.Cloned { - fmt.Println("Skipping breaking changes; repo failed to clone: ", repo.Name) + fmt.Println("Skipping diff processor; repo failed to clone: ", repo.Name) continue } + if len(repo.ChangedFiles) == 0 { + fmt.Println("Skipping diff processor; no diff: ", repo.Name) + } err = buildDiffProcessor(diffProcessorPath, repo.Path, diffProcessorEnv, rnr) if err != nil { fmt.Println("building diff processor: ", err) @@ -283,6 +287,29 @@ func execGenerateComment(prNumber int, ghTokenMagicModules, buildId, buildStep, sort.Strings(breakingChangesSlice) data.BreakingChanges = breakingChangesSlice + // Compute additional service labels based on changed files + regexpLabels, err := labeler.BuildRegexLabels(labeler.EnrolledTeamsYaml) + if err != nil { + fmt.Println("error building regexp labels: ", err) + errors["Other"] = append(errors["Other"], "Failed to parse service label mapping") + } else { + for _, repo := range []source.Repo{tpgRepo, tpgbRepo} { + if !repo.Cloned { + fmt.Println("Skipping changed file service labels; repo failed to clone: ", repo.Name) + continue + } + affectedResources := map[string]struct{}{} + for _, path := range repo.ChangedFiles { + if r := fileToResource(path); r != "" { + affectedResources[r] = struct{}{} + } + } + for _, label := range labeler.ComputeLabels(maps.Keys(affectedResources), regexpLabels) { + uniqueServiceLabels[label] = struct{}{} + } + } + } + // Add service labels to PR if len(uniqueServiceLabels) > 0 { serviceLabelsSlice := maps.Keys(uniqueServiceLabels) @@ -561,6 +588,32 @@ func formatDiffComment(data diffCommentData) (string, error) { return sb.String(), nil } +func fileToResource(path string) string { + // Must be a Go file + path, isResource := strings.CutSuffix(path, ".go") + if !isResource { + return "" + } + // Only consider the end of the path + path = path[strings.LastIndex(path, "/")+1:] + // Must start with `resource_` or `iam_` + if !strings.HasPrefix(path, "resource_") && !strings.HasPrefix(path, "iam_") { + return "" + } + if !isResource { + return "" + } + // Remove prefixes for iam & resources + path, _ = strings.CutPrefix(path, "resource_") + path, _ = strings.CutPrefix(path, "iam_") + // Remove prefixes & suffixes for test files, iam, and sweepers + path, _ = strings.CutSuffix(path, "_generated_test") + path, _ = strings.CutSuffix(path, "_iam_test") + path, _ = strings.CutSuffix(path, "_test") + path, _ = strings.CutSuffix(path, "_sweeper") + return "google_" + path +} + func pathChanged(path string, changedFiles []string) bool { for _, f := range changedFiles { if strings.HasPrefix(f, path) { diff --git a/.ci/magician/cmd/generate_comment_test.go b/.ci/magician/cmd/generate_comment_test.go index 9611e76e3c4f..1e1b7631de92 100644 --- a/.ci/magician/cmd/generate_comment_test.go +++ b/.ci/magician/cmd/generate_comment_test.go @@ -251,6 +251,136 @@ func TestFormatDiffComment(t *testing.T) { } } +func TestFileToResource(t *testing.T) { + cases := map[string]struct { + path string + want string + }{ + // Resource go files + "files outside services directory are not resources": { + path: "/google-beta/tpgiamresource/resource_iam_binding.go", + want: "", + }, + "non-go files in service directories are not resources": { + path: "/google-beta/services/firebaserules/resource_firebaserules_release.html.markdown", + want: "", + }, + "resource file": { + path: "/google-beta/services/firebaserules/resource_firebaserules_release.go", + want: "google_firebaserules_release", + }, + "resource iam file": { + path: "/google/services/kms/iam_kms_crypto_key.go", + want: "google_kms_crypto_key", + }, + "resource generated test file": { + path: "/google-beta/services/containeraws/resource_container_aws_node_pool_generated_test.go", + want: "google_container_aws_node_pool", + }, + "resource handwritten test file": { + path: "/google-beta/services/oslogin/resource_os_login_ssh_public_key_test.go", + want: "google_os_login_ssh_public_key", + }, + "resource internal_test file": { + path: "/google/services/redis/resource_redis_instance_internal_test.go", + want: "google_redis_instance", + }, + "resource sweeper file": { + path: "/google-beta/services/sql/resource_sql_source_representation_instance_sweeper.go", + want: "google_sql_source_representation_instance", + }, + "resource iam handwritten test file": { + path: "/google-beta/services/bigtable/resource_bigtable_instance_iam_test.go", + want: "google_bigtable_instance", + }, + "resource iam generated test file": { + path: "/google-beta/services/privateca/iam_privateca_ca_pool_generated_test.go", + want: "google_privateca_ca_pool", + }, + "resource ignore google_ prefix": { + path: "/google-beta/services/resourcemanager/resource_google_project_sweeper.go", + want: "google_project", + }, + "resource starting with iam_": { + path: "/google-beta/services/iam2/resource_iam_access_boundary_policy.go", + want: "google_iam_access_boundary_policy", + }, + + // Datasource files + "datasource file": { + path: "/google/services/dns/data_source_dns_keys.go", + want: "google_dns_keys", + }, + "datasource handwritten test file": { + path: "/google-beta/services/monitoring/data_source_monitoring_service_test.go", + want: "google_monitoring_service", + }, + // Future-proofing + "datasource generated test file": { + path: "/google-beta/services/alloydb/data_source_alloydb_locations_generated_test.go", + want: "google_alloydb_locations", + }, + "datasource internal_test file": { + path: "/google/services/storage/data_source_storage_object_signed_url_internal_test.go", + want: "google_storage_object_signed_url", + }, + "datasource ignore google_ prefix": { + path: "/google-beta/services/certificatemanager/data_source_google_certificate_manager_certificate_map_test.go", + want: "google_certificate_manager_certificate_map", + }, + "datasource starting with iam_": { + path: "/google-beta/services/resourcemanager/data_source_iam_policy_test.go", + want: "google_iam_policy", + }, + + // Resource documentation + "files outside /r or /d directories are not resources": { + path: "/website/docs/guides/common_issues.html.markdown", + want: "", + }, + "non-markdown files are not resources": { + path: "/website/docs/r/access_context_manager_access_level.go", + want: "", + }, + "resource docs": { + path: "/website/docs/r/firestore_document.html.markdown", + want: "google_firestore_document", + }, + "resource docs ignore google_ prefix": { + path: "/website/docs/r/google_project_service.html.markdown", + want: "google_project_service", + }, + "resource docs starting with iam_": { + path: "/website/docs/r/iam_deny_policy.html.markdown", + want: "google_iam_deny_policy", + }, + + // Datasource documentation + "datasource docs": { + path: "/website/docs/d/beyondcorp_app_gateway.html.markdown", + want: "google_beyondcorp_app_gateway", + }, + "datasource docs ignore google_ prefix": { + path: "/website/docs/d/google_vertex_ai_index.html.markdown", + want: "google_vertex_ai_index", + }, + "datasource docs starting with iam_": { + path: "/website/docs/d/iam_role.html.markdown", + want: "google_iam_role", + }, + } + + for tn, tc := range cases { + tc := tc + t.Run(tn, func(t *testing.T) { + t.Parallel() + + got := fileToResource(tc.path) + assert.Equal(t, tc.want, got) + }) + } +} + func TestPathChanged(t *testing.T) { cases := map[string]struct { path string From 350379435e66cce83753a5e66f0de7e18b45cb0f Mon Sep 17 00:00:00 2001 From: Stephen Lewis Date: Thu, 21 Mar 2024 09:01:14 -0700 Subject: [PATCH 04/14] Set up regexes to properly handle resource name extraction from file path --- .ci/magician/cmd/generate_comment.go | 46 ++++++++++++++-------------- 1 file changed, 23 insertions(+), 23 deletions(-) diff --git a/.ci/magician/cmd/generate_comment.go b/.ci/magician/cmd/generate_comment.go index 10e716d7b3cb..ad2ebc240814 100644 --- a/.ci/magician/cmd/generate_comment.go +++ b/.ci/magician/cmd/generate_comment.go @@ -20,6 +20,7 @@ import ( "fmt" "os" "path/filepath" + "regexp" "sort" "strconv" "strings" @@ -289,6 +290,7 @@ func execGenerateComment(prNumber int, ghTokenMagicModules, buildId, buildStep, // Compute additional service labels based on changed files regexpLabels, err := labeler.BuildRegexLabels(labeler.EnrolledTeamsYaml) + affectedResources := map[string]struct{}{} if err != nil { fmt.Println("error building regexp labels: ", err) errors["Other"] = append(errors["Other"], "Failed to parse service label mapping") @@ -298,17 +300,16 @@ func execGenerateComment(prNumber int, ghTokenMagicModules, buildId, buildStep, fmt.Println("Skipping changed file service labels; repo failed to clone: ", repo.Name) continue } - affectedResources := map[string]struct{}{} for _, path := range repo.ChangedFiles { if r := fileToResource(path); r != "" { affectedResources[r] = struct{}{} } } - for _, label := range labeler.ComputeLabels(maps.Keys(affectedResources), regexpLabels) { - uniqueServiceLabels[label] = struct{}{} - } } } + for _, label := range labeler.ComputeLabels(maps.Keys(affectedResources), regexpLabels) { + uniqueServiceLabels[label] = struct{}{} + } // Add service labels to PR if len(uniqueServiceLabels) > 0 { @@ -588,30 +589,29 @@ func formatDiffComment(data diffCommentData) (string, error) { return sb.String(), nil } +var resourceFileRegexp = regexp.MustCompile(`^.*/services/[^/]+/(?:data_source_|resource_|iam_)(.*?)(?:_test|_sweeper|_iam_test|_generated_test|_internal_test)?.go`) +var resourceDocsRegexp = regexp.MustCompile(`^.*/website/docs/(?:r|d)/(.*).html.markdown`) + func fileToResource(path string) string { - // Must be a Go file - path, isResource := strings.CutSuffix(path, ".go") - if !isResource { - return "" + var submatches []string + if strings.HasSuffix(path, ".go") { + submatches = resourceFileRegexp.FindStringSubmatch(path) + } else if strings.HasSuffix(path, ".html.markdown") { + submatches = resourceDocsRegexp.FindStringSubmatch(path) } - // Only consider the end of the path - path = path[strings.LastIndex(path, "/")+1:] - // Must start with `resource_` or `iam_` - if !strings.HasPrefix(path, "resource_") && !strings.HasPrefix(path, "iam_") { + + if len(submatches) == 0 { return "" } - if !isResource { - return "" + + // The regexes will each return the resource name as the first + // submatch, stripping any prefixes or suffixes. + resource := submatches[1] + + if !strings.HasPrefix(resource, "google_") { + resource = "google_" + resource } - // Remove prefixes for iam & resources - path, _ = strings.CutPrefix(path, "resource_") - path, _ = strings.CutPrefix(path, "iam_") - // Remove prefixes & suffixes for test files, iam, and sweepers - path, _ = strings.CutSuffix(path, "_generated_test") - path, _ = strings.CutSuffix(path, "_iam_test") - path, _ = strings.CutSuffix(path, "_test") - path, _ = strings.CutSuffix(path, "_sweeper") - return "google_" + path + return resource } func pathChanged(path string, changedFiles []string) bool { From a16bfb295190411e60f74eb1a594ef3f3103c4ed Mon Sep 17 00:00:00 2001 From: Stephen Lewis Date: Thu, 21 Mar 2024 09:03:27 -0700 Subject: [PATCH 05/14] Docs-only change to demonstrate new detection capability --- .../terraform/website/docs/d/cloudbuild_trigger.html.markdown | 1 + 1 file changed, 1 insertion(+) diff --git a/mmv1/third_party/terraform/website/docs/d/cloudbuild_trigger.html.markdown b/mmv1/third_party/terraform/website/docs/d/cloudbuild_trigger.html.markdown index c03e45ecfb15..0c57b53645b5 100644 --- a/mmv1/third_party/terraform/website/docs/d/cloudbuild_trigger.html.markdown +++ b/mmv1/third_party/terraform/website/docs/d/cloudbuild_trigger.html.markdown @@ -6,6 +6,7 @@ description: |- # google\_cloudbuild\_trigger +UPDATE To get more information about Cloudbuild Trigger, see: * [API documentation](https://cloud.google.com/build/docs/api/reference/rest/v1/projects.triggers) From 7f09fad9f6a5dcc6cc4851b3b7bf43f5defcabaf Mon Sep 17 00:00:00 2001 From: Stephen Lewis Date: Thu, 21 Mar 2024 11:18:49 -0700 Subject: [PATCH 06/14] Updated diff-processor README --- tools/diff-processor/README.md | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/tools/diff-processor/README.md b/tools/diff-processor/README.md index 3bb107cae9d1..5391278344c9 100644 --- a/tools/diff-processor/README.md +++ b/tools/diff-processor/README.md @@ -14,9 +14,8 @@ make build OLD_REF=branch_or_commit NEW_REF=branch_or_commit # Run breaking change detection on the difference between OLD_REF and NEW_REF bin/diff-processor breaking-changes -# Add labels to a PR based on the resources changed between OLD_REF and NEW_REF -# The token used must have write access to issues -GITHUB_TOKEN_MAGIC_MODULES=github_token bin/diff-processor add-labels PR_ID [--dry-run] +# Compute service labels to add bsaed on the resources changed between OLD_REF and NEW_REF +bin/diff-processor changed-schema-labels ``` ## Test From f51bdc002b704daed044fdf1bbac32d8e1e048c2 Mon Sep 17 00:00:00 2001 From: Stephen Lewis Date: Thu, 21 Mar 2024 11:27:42 -0700 Subject: [PATCH 07/14] Added additional logging related to changed files -> resources --- .ci/magician/cmd/generate_comment.go | 3 +++ 1 file changed, 3 insertions(+) diff --git a/.ci/magician/cmd/generate_comment.go b/.ci/magician/cmd/generate_comment.go index ad2ebc240814..47e35d422ff0 100644 --- a/.ci/magician/cmd/generate_comment.go +++ b/.ci/magician/cmd/generate_comment.go @@ -301,12 +301,15 @@ func execGenerateComment(prNumber int, ghTokenMagicModules, buildId, buildStep, continue } for _, path := range repo.ChangedFiles { + fmt.Println("Checking file: ", path) if r := fileToResource(path); r != "" { affectedResources[r] = struct{}{} + fmt.Println("-- Resource: ", r) } } } } + fmt.Printf("affected resources based on changed files: %v\n", maps.Keys(affectedResources)) for _, label := range labeler.ComputeLabels(maps.Keys(affectedResources), regexpLabels) { uniqueServiceLabels[label] = struct{}{} } From c1bff9deb8b912e3a1394f719c1981f2089fad99 Mon Sep 17 00:00:00 2001 From: Stephen Lewis Date: Thu, 21 Mar 2024 11:50:29 -0700 Subject: [PATCH 08/14] Fixed handling of website file paths coming from changed files --- .ci/magician/cmd/generate_comment.go | 2 +- .ci/magician/cmd/generate_comment_test.go | 16 ++++++++++++++++ 2 files changed, 17 insertions(+), 1 deletion(-) diff --git a/.ci/magician/cmd/generate_comment.go b/.ci/magician/cmd/generate_comment.go index 47e35d422ff0..63babb57975d 100644 --- a/.ci/magician/cmd/generate_comment.go +++ b/.ci/magician/cmd/generate_comment.go @@ -593,7 +593,7 @@ func formatDiffComment(data diffCommentData) (string, error) { } var resourceFileRegexp = regexp.MustCompile(`^.*/services/[^/]+/(?:data_source_|resource_|iam_)(.*?)(?:_test|_sweeper|_iam_test|_generated_test|_internal_test)?.go`) -var resourceDocsRegexp = regexp.MustCompile(`^.*/website/docs/(?:r|d)/(.*).html.markdown`) +var resourceDocsRegexp = regexp.MustCompile(`^.*website/docs/(?:r|d)/(.*).html.markdown`) func fileToResource(path string) string { var submatches []string diff --git a/.ci/magician/cmd/generate_comment_test.go b/.ci/magician/cmd/generate_comment_test.go index 1e1b7631de92..39e2871af366 100644 --- a/.ci/magician/cmd/generate_comment_test.go +++ b/.ci/magician/cmd/generate_comment_test.go @@ -305,6 +305,10 @@ func TestFileToResource(t *testing.T) { path: "/google-beta/services/iam2/resource_iam_access_boundary_policy.go", want: "google_iam_access_boundary_policy", }, + "resource file without starting slash": { + path: "google-beta/services/firebaserules/resource_firebaserules_release.go", + want: "google_firebaserules_release", + }, // Datasource files "datasource file": { @@ -332,6 +336,10 @@ func TestFileToResource(t *testing.T) { path: "/google-beta/services/resourcemanager/data_source_iam_policy_test.go", want: "google_iam_policy", }, + "datasource file without starting slash": { + path: "google/services/dns/data_source_dns_keys.go", + want: "google_dns_keys", + }, // Resource documentation "files outside /r or /d directories are not resources": { @@ -354,6 +362,10 @@ func TestFileToResource(t *testing.T) { path: "/website/docs/r/iam_deny_policy.html.markdown", want: "google_iam_deny_policy", }, + "resource docs without starting slash": { + path: "website/docs/d/cloudbuild_trigger.html.markdown", + want: "google_cloudbuild_trigger", + }, // Datasource documentation "datasource docs": { @@ -368,6 +380,10 @@ func TestFileToResource(t *testing.T) { path: "/website/docs/d/iam_role.html.markdown", want: "google_iam_role", }, + "datasource docs without starting slash": { + path: "website/docs/d/beyondcorp_app_gateway.html.markdown", + want: "google_beyondcorp_app_gateway", + }, } for tn, tc := range cases { From 6a8cb8c1733e10c24b52f061bd952159054b939c Mon Sep 17 00:00:00 2001 From: Stephen Lewis Date: Thu, 21 Mar 2024 11:51:08 -0700 Subject: [PATCH 09/14] Removed unnecessary logging --- .ci/magician/cmd/generate_comment.go | 2 -- 1 file changed, 2 deletions(-) diff --git a/.ci/magician/cmd/generate_comment.go b/.ci/magician/cmd/generate_comment.go index 63babb57975d..9e89eff48299 100644 --- a/.ci/magician/cmd/generate_comment.go +++ b/.ci/magician/cmd/generate_comment.go @@ -301,10 +301,8 @@ func execGenerateComment(prNumber int, ghTokenMagicModules, buildId, buildStep, continue } for _, path := range repo.ChangedFiles { - fmt.Println("Checking file: ", path) if r := fileToResource(path); r != "" { affectedResources[r] = struct{}{} - fmt.Println("-- Resource: ", r) } } } From da58617288ea18d58aa09c3a6029458eab124343 Mon Sep 17 00:00:00 2001 From: Stephen Lewis Date: Thu, 21 Mar 2024 13:42:45 -0700 Subject: [PATCH 10/14] Revert "Docs-only change to demonstrate new detection capability" This reverts commit a16bfb295190411e60f74eb1a594ef3f3103c4ed. --- .../terraform/website/docs/d/cloudbuild_trigger.html.markdown | 1 - 1 file changed, 1 deletion(-) diff --git a/mmv1/third_party/terraform/website/docs/d/cloudbuild_trigger.html.markdown b/mmv1/third_party/terraform/website/docs/d/cloudbuild_trigger.html.markdown index 0c57b53645b5..c03e45ecfb15 100644 --- a/mmv1/third_party/terraform/website/docs/d/cloudbuild_trigger.html.markdown +++ b/mmv1/third_party/terraform/website/docs/d/cloudbuild_trigger.html.markdown @@ -6,7 +6,6 @@ description: |- # google\_cloudbuild\_trigger -UPDATE To get more information about Cloudbuild Trigger, see: * [API documentation](https://cloud.google.com/build/docs/api/reference/rest/v1/projects.triggers) From fef063e033e1952d276a1e6320dbd7266e4676d7 Mon Sep 17 00:00:00 2001 From: Stephen Lewis Date: Wed, 27 Mar 2024 09:23:29 -0700 Subject: [PATCH 11/14] Tweaked error messages --- .ci/magician/cmd/generate_comment.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/.ci/magician/cmd/generate_comment.go b/.ci/magician/cmd/generate_comment.go index c623bd5350ef..27c9c43f7013 100644 --- a/.ci/magician/cmd/generate_comment.go +++ b/.ci/magician/cmd/generate_comment.go @@ -210,8 +210,8 @@ func execGenerateComment(prNumber int, ghTokenMagicModules, buildId, buildStep, } shortStat, err := ctlr.DiffShortStat(repo, oldBranch, newBranch) if err != nil { - fmt.Println("diffing repo shortstats: ", err) - errors[repo.Title] = append(errors[repo.Title], "Failed to compute repo diff short stats") + fmt.Println("Failed to compute repo diff --shortstat: ", err) + errors[repo.Title] = append(errors[repo.Title], "Failed to compute repo diff shortstats") } if shortStat != "" { diffs = append(diffs, Diff{ @@ -221,7 +221,7 @@ func execGenerateComment(prNumber int, ghTokenMagicModules, buildId, buildStep, }) repo.ChangedFiles, err = ctlr.DiffNameOnly(repo, oldBranch, newBranch) if err != nil { - fmt.Println("diffing repo nameonly: ", err) + fmt.Println("Failed to compute repo diff --name-only: ", err) errors[repo.Title] = append(errors[repo.Title], "Failed to compute repo changed filenames") } } From 19df793171c5a8e76743d9cf6d1cadabb4046738 Mon Sep 17 00:00:00 2001 From: Stephen Lewis Date: Wed, 27 Mar 2024 09:23:45 -0700 Subject: [PATCH 12/14] Added missing continue --- .ci/magician/cmd/generate_comment.go | 1 + 1 file changed, 1 insertion(+) diff --git a/.ci/magician/cmd/generate_comment.go b/.ci/magician/cmd/generate_comment.go index 27c9c43f7013..506f37f43b1f 100644 --- a/.ci/magician/cmd/generate_comment.go +++ b/.ci/magician/cmd/generate_comment.go @@ -247,6 +247,7 @@ func execGenerateComment(prNumber int, ghTokenMagicModules, buildId, buildStep, } if len(repo.ChangedFiles) == 0 { fmt.Println("Skipping diff processor; no diff: ", repo.Name) + continue } err = buildDiffProcessor(diffProcessorPath, repo.Path, diffProcessorEnv, rnr) if err != nil { From 4323821d8978dabeb2e39961abb5cf6cd255c871 Mon Sep 17 00:00:00 2001 From: Stephen Lewis Date: Wed, 27 Mar 2024 09:25:28 -0700 Subject: [PATCH 13/14] Made logic around computing affected resources and corresponding service labels easier to read --- .ci/magician/cmd/generate_comment.go | 34 +++++++++++++++------------- 1 file changed, 18 insertions(+), 16 deletions(-) diff --git a/.ci/magician/cmd/generate_comment.go b/.ci/magician/cmd/generate_comment.go index 506f37f43b1f..d561c8c35aa6 100644 --- a/.ci/magician/cmd/generate_comment.go +++ b/.ci/magician/cmd/generate_comment.go @@ -283,29 +283,31 @@ func execGenerateComment(prNumber int, ghTokenMagicModules, buildId, buildStep, sort.Strings(breakingChangesSlice) data.BreakingChanges = breakingChangesSlice - // Compute additional service labels based on changed files - regexpLabels, err := labeler.BuildRegexLabels(labeler.EnrolledTeamsYaml) + // Compute affected resources based on changed files affectedResources := map[string]struct{}{} + for _, repo := range []source.Repo{tpgRepo, tpgbRepo} { + if !repo.Cloned { + fmt.Println("Skipping changed file service labels; repo failed to clone: ", repo.Name) + continue + } + for _, path := range repo.ChangedFiles { + if r := fileToResource(path); r != "" { + affectedResources[r] = struct{}{} + } + } + } + fmt.Printf("affected resources based on changed files: %v\n", maps.Keys(affectedResources)) + + // Compute additional service labels based on affected resources + regexpLabels, err := labeler.BuildRegexLabels(labeler.EnrolledTeamsYaml) if err != nil { fmt.Println("error building regexp labels: ", err) errors["Other"] = append(errors["Other"], "Failed to parse service label mapping") } else { - for _, repo := range []source.Repo{tpgRepo, tpgbRepo} { - if !repo.Cloned { - fmt.Println("Skipping changed file service labels; repo failed to clone: ", repo.Name) - continue - } - for _, path := range repo.ChangedFiles { - if r := fileToResource(path); r != "" { - affectedResources[r] = struct{}{} - } - } + for _, label := range labeler.ComputeLabels(maps.Keys(affectedResources), regexpLabels) { + uniqueServiceLabels[label] = struct{}{} } } - fmt.Printf("affected resources based on changed files: %v\n", maps.Keys(affectedResources)) - for _, label := range labeler.ComputeLabels(maps.Keys(affectedResources), regexpLabels) { - uniqueServiceLabels[label] = struct{}{} - } // Add service labels to PR if len(uniqueServiceLabels) > 0 { From f8c31cb8cbccee24239f03f5a4cb5c02fd5dd8e0 Mon Sep 17 00:00:00 2001 From: Stephen Lewis Date: Wed, 27 Mar 2024 09:31:54 -0700 Subject: [PATCH 14/14] Explicitly check len regexpLabels for clarity --- .ci/magician/cmd/generate_comment.go | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/.ci/magician/cmd/generate_comment.go b/.ci/magician/cmd/generate_comment.go index d561c8c35aa6..c89696d9a52a 100644 --- a/.ci/magician/cmd/generate_comment.go +++ b/.ci/magician/cmd/generate_comment.go @@ -303,7 +303,8 @@ func execGenerateComment(prNumber int, ghTokenMagicModules, buildId, buildStep, if err != nil { fmt.Println("error building regexp labels: ", err) errors["Other"] = append(errors["Other"], "Failed to parse service label mapping") - } else { + } + if len(regexpLabels) > 0 { for _, label := range labeler.ComputeLabels(maps.Keys(affectedResources), regexpLabels) { uniqueServiceLabels[label] = struct{}{} }