Skip to content
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.

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

Detect service labels for PRs based on changed files #10248

Merged
merged 15 commits into from
Mar 27, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
2 changes: 1 addition & 1 deletion .ci/magician/cmd/DIFF_COMMENT.md
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}}) ({{.DiffStats}})
{{.Title}}: [Diff](https://github.com/modular-magician/{{.Repo}}/compare/auto-pr-{{$.PrNumber}}-old..auto-pr-{{$.PrNumber}}) ({{.ShortStat}})
{{end -}}
{{end -}}

Expand Down
155 changes: 104 additions & 51 deletions .ci/magician/cmd/generate_comment.go
Original file line number Diff line number Diff line change
Expand Up @@ -20,11 +20,13 @@ import (
"fmt"
"os"
"path/filepath"
"regexp"
"sort"
"strconv"
"strings"
"text/template"

"github.com/GoogleCloudPlatform/magic-modules/tools/issue-labeler/labeler"
"magician/exec"
"magician/github"
"magician/provider"
Expand All @@ -44,7 +46,7 @@ var (
type Diff struct {
Title string
Repo string
DiffStats string
ShortStat string
}

type Errors struct {
Expand Down Expand Up @@ -187,31 +189,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("Failed to compute repo diff --shortstat: ", err)
errors[repo.Title] = append(errors[repo.Title], "Failed to compute repo diff shortstats")
}
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("Failed to compute repo diff --name-only: ", err)
errors[repo.Title] = append(errors[repo.Title], "Failed to compute repo changed filenames")
}
}
}
data.Diffs = diffs
Expand All @@ -230,7 +242,11 @@ 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)
continue
}
err = buildDiffProcessor(diffProcessorPath, repo.Path, diffProcessorEnv, rnr)
Expand Down Expand Up @@ -267,6 +283,33 @@ func execGenerateComment(prNumber int, ghTokenMagicModules, buildId, buildStep,
sort.Strings(breakingChangesSlice)
data.BreakingChanges = breakingChangesSlice

// 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")
}
if len(regexpLabels) > 0 {
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)
Expand Down Expand Up @@ -310,18 +353,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
Expand Down Expand Up @@ -356,18 +402,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 {
for _, path := range []string{"old", "new", "bin"} {
Expand Down Expand Up @@ -515,21 +549,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 {
Expand Down Expand Up @@ -565,6 +584,40 @@ 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 {
var submatches []string
if strings.HasSuffix(path, ".go") {
submatches = resourceFileRegexp.FindStringSubmatch(path)
} else if strings.HasSuffix(path, ".html.markdown") {
submatches = resourceDocsRegexp.FindStringSubmatch(path)
}

if len(submatches) == 0 {
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
}
return resource
}

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)
}
Loading
Loading