Add more rules to the breaking change detector (#6692)

Co-authored-by: Cameron Thornton <[email protected]>
GoogleCloudPlatform · Oct 14, 2022 · 4f1df0a · 4f1df0a
1 parent 52783b9
commit 4f1df0a
Show file tree

Hide file tree

Showing 11 changed files with 812 additions and 31 deletions.
diff --git a/tools/breaking-change-detector/comparison.go b/tools/breaking-change-detector/comparison.go
@@ -70,16 +70,22 @@ func compareResourceSchema(resourceName string, old, new map[string]*schema.Sche
 
 func compareField(resourceName, fieldName string, old, new *schema.Schema) []string {
 	messages := []string{}
-	rules := rules.FieldRules
-
-	for _, rule := range rules {
-		isBreakage := rule.IsRuleBreak(old, new)
-		if isBreakage {
-			newMessage := rule.Message(*providerVersion, resourceName, fieldName)
-			messages = append(messages, newMessage)
+	fieldRules := rules.FieldRules
+
+	for _, rule := range fieldRules {
+		breakageMessage := rule.IsRuleBreak(
+			old,
+			new,
+			rules.MessageContext{
+				Resource: resourceName,
+				Field:    fieldName,
+				Version:  *providerVersion,
+			},
+		)
+		if breakageMessage != "" {
+			messages = append(messages, breakageMessage)
 		}
 	}
-
 	return messages
 }
 

diff --git a/tools/breaking-change-detector/comparison_test.go b/tools/breaking-change-detector/comparison_test.go
@@ -1,6 +1,7 @@
 package main
 
 import (
+	"strings"
 	"testing"
 
 	"github.com/hashicorp/terraform-plugin-sdk/v2/helper/schema"
@@ -222,10 +223,121 @@ var comparisonEngineTestCases = []comparisonEngineTestCase{
 		},
 		expectedViolations: 1,
 	},
+	{
+		name: "subfield max shrinking",
+		oldResourceMap: map[string]*schema.Resource{
+			"google-x": {
+				Schema: map[string]*schema.Schema{
+					"field-a": {
+						Description: "beep",
+						Optional:    true,
+						Type:        schema.TypeList,
+						MaxItems:    1,
+						Elem: &schema.Resource{
+							Schema: map[string]*schema.Schema{
+								"sub-field-1": {Description: "beep", Optional: true, MaxItems: 100},
+							},
+						},
+					},
+					"field-b": {Description: "beep", Optional: true},
+				},
+			},
+		},
+		newResourceMap: map[string]*schema.Resource{
+			"google-x": {
+				Schema: map[string]*schema.Schema{
+					"field-a": {
+						Description: "beep",
+						Optional:    true,
+						Type:        schema.TypeList,
+						MaxItems:    1,
+						Elem: &schema.Resource{
+							Schema: map[string]*schema.Schema{
+								"sub-field-1": {Description: "beep", Optional: true, MaxItems: 25},
+							},
+						},
+					},
+					"field-b": {Description: "beep", Optional: true},
+				},
+			},
+		},
+		expectedViolations: 1,
+	},
+	{
+		name: "subfield max shrinking",
+		oldResourceMap: map[string]*schema.Resource{
+			"google-x": {
+				Schema: map[string]*schema.Schema{
+					"field-a": {
+						Description: "beep",
+						Optional:    true,
+						Type:        schema.TypeList,
+						MaxItems:    1,
+						Elem: &schema.Resource{
+							Schema: map[string]*schema.Schema{
+								"sub-field-1": {Description: "beep", Optional: true, MaxItems: 100},
+							},
+						},
+					},
+					"field-b": {Description: "beep", Optional: true},
+				},
+			},
+		},
+		newResourceMap: map[string]*schema.Resource{
+			"google-x": {
+				Schema: map[string]*schema.Schema{
+					"field-a": {
+						Description: "beep",
+						Optional:    true,
+						Type:        schema.TypeList,
+						MaxItems:    1,
+						Elem: &schema.Resource{
+							Schema: map[string]*schema.Schema{
+								"sub-field-1": {Description: "beep", Optional: true, MaxItems: 25},
+							},
+						},
+					},
+					"field-b": {Description: "beep", Optional: true},
+				},
+			},
+		},
+		expectedViolations: 1,
+	},
+	{
+		name: "min growing",
+		oldResourceMap: map[string]*schema.Resource{
+			"google-x": {
+				Schema: map[string]*schema.Schema{
+					"field-a": {
+						Description: "beep",
+						Optional:    true,
+						MinItems:    1,
+					},
+				},
+			},
+		},
+		newResourceMap: map[string]*schema.Resource{
+			"google-x": {
+				Schema: map[string]*schema.Schema{
+					"field-a": {
+						Description: "beep",
+						Optional:    true,
+						MinItems:    4,
+					},
+				},
+			},
+		},
+		expectedViolations: 1,
+	},
 }
 
 func (tc *comparisonEngineTestCase) check(t *testing.T) {
 	violations := compareResourceMaps(tc.oldResourceMap, tc.newResourceMap)
+	for _, v := range violations {
+		if strings.Contains(v, "{{") || strings.Contains(v, "}}") {
+			t.Errorf("Test `%s` failed: found unreplaced characters in string - %s", tc.name, v)
+		}
+	}
 	if tc.expectedViolations != len(violations) {
 		t.Errorf("Test `%s` failed: expected %d violations, got %d", tc.name, tc.expectedViolations, len(violations))
 	}

diff --git a/tools/breaking-change-detector/docs/breaking-changes.md.tmpl b/tools/breaking-change-detector/docs/breaking-changes.md.tmpl
@@ -59,8 +59,9 @@ go into the four categories and rules therein.
 
 {{ range $r := $c.Rules -}}
 
-<h4 id="{{$r.Identifier}}"> {{ $r.Name }} </h4>
-{{ $r.Definition }}
+<h4 id="{{$r.Identifier}}"> {{ $r.Name }} {{if $r.Undetectable}}(Undetectable){{ end }} </h4>
+
+* {{ $r.Definition }}
 
 {{ end }}
 {{- end }}

diff --git a/tools/breaking-change-detector/rules/message_context.go b/tools/breaking-change-detector/rules/message_context.go
@@ -0,0 +1,24 @@
+package rules
+
+import (
+	"fmt"
+	"strings"
+)
+
+// MessageContext - is an envelope for additional
+// metadata about context around the rule breakage
+type MessageContext struct {
+	Resource   string
+	Field      string
+	Version    string
+	identifier string
+	message    string
+}
+
+func populateMessageContext(message string, mc MessageContext) string {
+	resource := fmt.Sprintf("`%s`", mc.Resource)
+	field := fmt.Sprintf("`%s`", mc.Field)
+	message = strings.ReplaceAll(message, "{{resource}}", resource)
+	message = strings.ReplaceAll(message, "{{field}}", field)
+	return message + documentationReference(mc.Version, mc.identifier)
+}
diff --git a/tools/breaking-change-detector/rules/rule.go b/tools/breaking-change-detector/rules/rule.go
@@ -7,6 +7,7 @@ type Rule interface {
 	Name() string
 	Definition() string
 	Identifier() string
+	Undetectable() bool
 }
 
 // RuleCategory holds documentation and
@@ -23,22 +24,17 @@ type Rules struct {
 	Categories []RuleCategory
 }
 
-var fieldRules RuleCategory = RuleCategory{
-	Name:       "",
-	Definition: "",
-}
-
 // GetRules returns a list of all rules for
 // documentation generation
 func GetRules() *Rules {
 	categories := []RuleCategory{
 		{
-			Name:       "Provider Level Breakages",
-			Definition: "Top level behavior such as provider configuration changes and authentication.",
-			Rules:      nil,
+			Name:       "Provider Configuration Level Breakages",
+			Definition: "Top level behavior such as provider configuration and authentication changes.",
+			Rules:      providerConfigRulesToRuleArray(ProviderConfigRules),
 		},
 		{
-			Name:       "Resource Inventory Level Breakages",
+			Name:       "Resource List Level Breakages",
 			Definition: "Resource/datasource naming conventions and entry differences.",
 			Rules:      resourceInventoryRulesToRuleArray(ResourceInventoryRules),
 		},