Collapsed IAM resources + parent for multiple resources check (#15973)

Author: melinath

.ci/magician/cmd/generate_comment.go

@@ -89,7 +89,7 @@ type diffCommentData struct {
 	MissingServiceLabels []string
 	MissingTests         map[string]*MissingTestInfo
 	MissingDocs          *MissingDocsSummary
-	AddedResources       []string
+	MultipleResources    []string
 	Errors               []Errors
 }
 
@@ -377,7 +377,8 @@ func execGenerateComment(prNumber int, ghTokenMagicModules, buildId, buildStep,
 
 	// Check if multiple resources were added.
 	multipleResourcesState := "success"
-	if len(uniqueAddedResources) > 1 {
+	data.MultipleResources = multipleResources(maps.Keys(uniqueAddedResources))
+	if len(data.MultipleResources) > 1 {
 		multipleResourcesState = "failure"
 		for _, label := range pullRequest.Labels {
 			if label.Name == allowMultipleResourcesLabel {
@@ -391,8 +392,6 @@ func execGenerateComment(prNumber int, ghTokenMagicModules, buildId, buildStep,
 		fmt.Printf("Error posting terraform-provider-multiple-resources 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: "+multipleResourcesState)
 	}
-	data.AddedResources = maps.Keys(uniqueAddedResources)
-	slices.Sort(data.AddedResources)
 
 	// Compute affected resources based on changed files
 	changedFilesAffectedResources := map[string]struct{}{}
@@ -642,6 +641,38 @@ func formatDiffComment(data diffCommentData) (string, error) {
 	return sb.String(), nil
 }
 
+// addedMultipleResources returns a sorted slice of resource names that are considered "separate" resources.
+// In particular, IAM resources are merged with the parent resource as part of this check.
+func multipleResources(resources []string) []string {
+	if len(resources) == 0 {
+		return nil
+	}
+	iam := map[string]struct{}{}
+	final := map[string]struct{}{}
+
+	for _, r := range resources {
+		if k, found := strings.CutSuffix(r, "_iam_member"); found {
+			iam[k] = struct{}{}
+		} else if k, found := strings.CutSuffix(r, "_iam_binding"); found {
+			iam[k] = struct{}{}
+		} else if k, found := strings.CutSuffix(r, "_iam_policy"); found {
+			iam[k] = struct{}{}
+		} else {
+			final[k] = struct{}{}
+		}
+	}
+
+	for r, _ := range iam {
+		if _, ok := final[r]; !ok {
+			final[r+"_iam_*"] = struct{}{}
+		}
+	}
+
+	ret := maps.Keys(final)
+	slices.Sort(ret)
+	return ret
+}
+
 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`)
 

.ci/magician/cmd/generate_comment_test.go

@@ -246,7 +246,7 @@ func TestFormatDiffComment(t *testing.T) {
 		},
 		"multiple resources are displayed": {
 			data: diffCommentData{
-				AddedResources: []string{"google_redis_instance", "google_alloydb_cluster"},
+				MultipleResources: []string{"google_redis_instance", "google_alloydb_cluster"},
 			},
 			expectedStrings: []string{
 				"## Diff report",
@@ -352,6 +352,72 @@ func TestFormatDiffComment(t *testing.T) {
 	}
 }
 
+func TestMultipleResources(t *testing.T) {
+	cases := []struct {
+		name      string
+		resources []string
+		want      []string
+	}{
+		{
+			name: "no resources",
+		},
+		{
+			name:      "single non-iam",
+			resources: []string{"google_redis_instance"},
+			want:      []string{"google_redis_instance"},
+		},
+		{
+			name:      "multiple non-iam",
+			resources: []string{"google_redis_instance", "google_alloydb_cluster"},
+			want:      []string{"google_alloydb_cluster", "google_redis_instance"},
+		},
+		{
+			name:      "single iam only",
+			resources: []string{"google_redis_instance_iam_member", "google_redis_instance_iam_policy", "google_redis_instance_iam_binding"},
+			want:      []string{"google_redis_instance_iam_*"},
+		},
+		{
+			name:      "single iam with parent",
+			resources: []string{"google_redis_instance_iam_member", "google_redis_instance_iam_policy", "google_redis_instance_iam_binding", "google_redis_instance"},
+			want:      []string{"google_redis_instance"},
+		},
+		{
+			name: "multiple iam",
+			resources: []string{
+				"google_redis_instance_iam_member",
+				"google_redis_instance_iam_policy",
+				"google_redis_instance_iam_binding",
+				"google_alloydb_cluster_iam_member",
+				"google_alloydb_cluster_iam_policy",
+				"google_alloydb_cluster_iam_binding",
+			},
+			want: []string{"google_alloydb_cluster_iam_*", "google_redis_instance_iam_*"},
+		},
+		{
+			name: "multiple iam with parent",
+			resources: []string{
+				"google_redis_instance_iam_member",
+				"google_redis_instance_iam_policy",
+				"google_redis_instance_iam_binding",
+				"google_alloydb_cluster_iam_member",
+				"google_alloydb_cluster_iam_policy",
+				"google_alloydb_cluster_iam_binding",
+				"google_redis_instance",
+			},
+			want: []string{"google_alloydb_cluster_iam_*", "google_redis_instance"},
+		},
+	}
+
+	for _, tc := range cases {
+		t.Run(tc.name, func(t *testing.T) {
+			t.Parallel()
+
+			got := multipleResources(tc.resources)
+			assert.Equal(t, tc.want, got)
+		})
+	}
+}
+
 func TestFileToResource(t *testing.T) {
 	cases := map[string]struct {
 		path string

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

@@ -51,10 +51,10 @@ If you believe this detection to be incorrect please raise the concern with your
 An `override-missing-service-label` label can be added to allow merging.
 {{end}}
 
-{{- if gt (len .AddedResources) 1 }}
+{{- if gt (len .MultipleResources) 1 }}
 ## Multiple resources added
 
-This PR adds multiple new resources: {{range $i, $resource := .AddedResources}}{{ if gt $i 0}}, {{end}}`{{$resource}}`{{end}}. This makes review significantly more difficult. Please split it into multiple PRs, one per resource.
+This PR adds multiple new resources: {{range $i, $resource := .MultipleResources}}{{ if gt $i 0}}, {{end}}`{{$resource}}`{{end}}. This makes review significantly more difficult. Please split it into multiple PRs, one per resource.
 An `override-multiple-resources` label can be added to allow merging.
 {{end}}