Added missing service label handling to generate_comment (#13143)

Author: melinath

.ci/magician/cmd/generate_comment.go

@@ -66,18 +66,20 @@ type Errors struct {
 }
 
 type diffCommentData struct {
-	PrNumber        int
-	Diffs           []Diff
-	BreakingChanges []BreakingChange
-	MissingTests    map[string]*MissingTestInfo
-	Errors          []Errors
+	PrNumber             int
+	Diffs                []Diff
+	BreakingChanges      []BreakingChange
+	MissingServiceLabels []string
+	MissingTests         map[string]*MissingTestInfo
+	Errors               []Errors
 }
 
 type simpleSchemaDiff struct {
 	AddedResources, ModifiedResources, RemovedResources []string
 }
 
 const allowBreakingChangesLabel = "override-breaking-change"
+const allowMissingServiceLabelsLabel = "override-missing-service-labels"
 
 var gcEnvironmentVariables = [...]string{
 	"BUILD_ID",
@@ -263,6 +265,7 @@ func execGenerateComment(prNumber int, ghTokenMagicModules, buildId, buildStep,
 	data.Diffs = diffs
 
 	// The breaking changes are unique across both provider versions
+	uniqueAddedResources := map[string]struct{}{}
 	uniqueAffectedResources := map[string]struct{}{}
 	uniqueBreakingChanges := map[string]BreakingChange{}
 	diffProcessorPath := filepath.Join(mmLocalPath, "tools", "diff-processor")
@@ -309,12 +312,16 @@ func execGenerateComment(prNumber int, ghTokenMagicModules, buildId, buildStep,
 			data.MissingTests = missingTests
 		}
 
-		affectedResources, err := computeAffectedResources(diffProcessorPath, rnr, repo)
+		simpleDiff, err := computeAffectedResources(diffProcessorPath, rnr, repo)
 		if err != nil {
 			fmt.Println("computing changed resource schemas: ", err)
 			errors[repo.Title] = append(errors[repo.Title], "The diff processor crashed while computing changed resource schemas.")
 		}
-		for _, resource := range affectedResources {
+		for _, resource := range simpleDiff.AddedResources {
+			uniqueAddedResources[resource] = struct{}{}
+			uniqueAffectedResources[resource] = struct{}{}
+		}
+		for _, resource := range append(simpleDiff.ModifiedResources, simpleDiff.RemovedResources...) {
 			uniqueAffectedResources[resource] = struct{}{}
 		}
 	}
@@ -390,10 +397,43 @@ func execGenerateComment(prNumber int, ghTokenMagicModules, buildId, buildStep,
 	}
 	targetURL := fmt.Sprintf("https://console.cloud.google.com/cloud-build/builds;region=global/%s;step=%s?project=%s", buildId, buildStep, projectId)
 	if err = gh.PostBuildStatus(strconv.Itoa(prNumber), "terraform-provider-breaking-change-test", breakingState, targetURL, commitSha); err != nil {
-		fmt.Printf("Error posting build status for pr %d commit %s: %v\n", prNumber, commitSha, err)
+		fmt.Printf("Error posting terraform-provider-breaking-change-test build status for pr %d commit %s: %v\n", prNumber, commitSha, err)
 		errors["Other"] = append(errors["Other"], "Failed to update breaking-change status check with state: "+breakingState)
 	}
 
+	// Flag missing service labels for added resources
+	missingServiceLabels := []string{}
+	if len(regexpLabels) > 0 {
+		for resource, _ := range uniqueAddedResources {
+			found := false
+			for _, rl := range regexpLabels {
+				if rl.Regexp.MatchString(resource) {
+					found = true
+					break
+				}
+			}
+			if !found {
+				missingServiceLabels = append(missingServiceLabels, resource)
+			}
+		}
+		missingServiceLabelsState := "success"
+		if len(missingServiceLabels) > 0 {
+			missingServiceLabelsState = "failure"
+			for _, label := range pullRequest.Labels {
+				if label.Name == allowMissingServiceLabelsLabel {
+					missingServiceLabelsState = "success"
+					break
+				}
+			}
+		}
+		if err = gh.PostBuildStatus(strconv.Itoa(prNumber), "terraform-provider-missing-service-labels", missingServiceLabelsState, targetURL, commitSha); err != nil {
+			fmt.Printf("Error posting terraform-provider-missing-service-labels build status for pr %d commit %s: %v\n", prNumber, commitSha, err)
+			errors["Other"] = append(errors["Other"], "Failed to update missing-service-labels status check with state: "+missingServiceLabelsState)
+		}
+	}
+	sort.Strings(missingServiceLabels)
+	data.MissingServiceLabels = missingServiceLabels
+
 	// Add errors to data as an ordered list
 	errorsList := []Errors{}
 	for _, repo := range []source.Repo{tpgRepo, tpgbRepo, tgcRepo, tfoicsRepo} {
@@ -466,31 +506,27 @@ func computeBreakingChanges(diffProcessorPath string, rnr ExecRunner) ([]Breakin
 	return changes, rnr.PopDir()
 }
 
-func computeAffectedResources(diffProcessorPath string, rnr ExecRunner, repo source.Repo) ([]string, error) {
+func computeAffectedResources(diffProcessorPath string, rnr ExecRunner, repo source.Repo) (simpleSchemaDiff, error) {
 	if err := rnr.PushDir(diffProcessorPath); err != nil {
-		return nil, err
+		return simpleSchemaDiff{}, err
 	}
 
 	output, err := rnr.Run("bin/diff-processor", []string{"schema-diff"}, nil)
 	if err != nil {
-		return nil, err
+		return simpleSchemaDiff{}, err
 	}
 
 	fmt.Printf("Schema diff for %q: %s\n", repo.Name, output)
 
 	var simpleDiff simpleSchemaDiff
 	if err = json.Unmarshal([]byte(output), &simpleDiff); err != nil {
-		return nil, err
+		return simpleSchemaDiff{}, err
 	}
 
 	if err = rnr.PopDir(); err != nil {
-		return nil, err
+		return simpleSchemaDiff{}, err
 	}
-	var resources []string
-	resources = append(resources, simpleDiff.AddedResources...)
-	resources = append(resources, simpleDiff.ModifiedResources...)
-	resources = append(resources, simpleDiff.RemovedResources...)
-	return resources, nil
+	return simpleDiff, nil
 }
 
 // Run the missing test detector and return the results.

.ci/magician/cmd/generate_comment_test.go

@@ -110,9 +110,12 @@ func TestExecGenerateComment(t *testing.T) {
 	}
 
 	for method, expectedCalls := range map[string][][]any{
-		"PostBuildStatus": {{"123456", "terraform-provider-breaking-change-test", "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"}},
-		"AddLabels":       {{"123456", []string{"service/alloydb"}}},
+		"PostBuildStatus": {
+			{"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"}},
+		"AddLabels":   {{"123456", []string{"service/alloydb"}}},
 	} {
 		if actualCalls, ok := gh.calledMethods[method]; !ok {
 			t.Fatalf("Found no calls for %s", method)

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

@@ -11,8 +11,7 @@ Your PR generated some diffs in downstreams - here they are.
 {{end -}}
 {{end -}}
 
-{{- $breakingChangesLength := len .BreakingChanges }}
-{{- if gt $breakingChangesLength 0}}
+{{if gt (len .BreakingChanges) 0}}
 ## Breaking Change(s) Detected
 
 The following breaking change(s) were detected within your pull request.
@@ -40,6 +39,18 @@ Please add an acceptance test which includes these fields. The test should inclu
 {{- end }}
 {{end}}
 
+{{if gt (len .MissingServiceLabels) 0}}
+## Missing service labels
+
+The following new resources do not have corresponding service labels:
+
+{{- range .MissingServiceLabels}}
+- {{.}}{{end}}
+
+If you believe this detection to be incorrect please raise the concern with your reviewer.
+An `override-missing-service-label` label can be added to allow merging.
+{{end}}
+
 {{- $errorsLength := len .Errors}}
 {{- if gt $errorsLength 0}}
 ## Errors