diff --git a/docs/content/develop/breaking-changes/breaking-changes.md b/docs/content/develop/breaking-changes/breaking-changes.md index b972fb023216..906f7f3b7ea7 100644 --- a/docs/content/develop/breaking-changes/breaking-changes.md +++ b/docs/content/develop/breaking-changes/breaking-changes.md @@ -84,10 +84,7 @@ For more information, see * For handwritten resources, removing `DiffSuppressFunc` from a field. * Adding a subfield to a SchemaConfigModeAttr field. - * For MMv1 resources, adding a subfield to a field that has - SchemaConfigModeAttr. - * For handwritten resources, adding a subfield to a field that has - SchemaConfigModeAttr. + * Subfields of SchemaConfigModeAttr fields are treated as required even if the schema says they are optional. * Removing update support from a field. ### Making validation more strict diff --git a/tools/diff-processor/constants/constants.go b/tools/diff-processor/constants/constants.go deleted file mode 100644 index e41faa4a3a57..000000000000 --- a/tools/diff-processor/constants/constants.go +++ /dev/null @@ -1,10 +0,0 @@ -package constants - -const BreakingChangeRelativeLocation = "develop/" -const BreakingChangeFileName = "breaking-changes" - -var docsite = "https://googlecloudplatform.github.io/magic-modules/" - -func GetFileUrl(identifier string) string { - return docsite + BreakingChangeRelativeLocation + BreakingChangeFileName + "#" + identifier -} diff --git a/tools/diff-processor/rules/breaking_changes.go b/tools/diff-processor/rules/breaking_changes.go index 354f5db073ca..53153a65dfa0 100644 --- a/tools/diff-processor/rules/breaking_changes.go +++ b/tools/diff-processor/rules/breaking_changes.go @@ -1,6 +1,8 @@ package rules import ( + "fmt" + "github.com/GoogleCloudPlatform/magic-modules/tools/diff-processor/diff" ) @@ -9,56 +11,45 @@ type BreakingChange struct { Field string Message string DocumentationReference string - RuleTemplate string - RuleDefinition string RuleName string } -func ComputeBreakingChanges(schemaDiff diff.SchemaDiff) []*BreakingChange { - var messages []*BreakingChange +const breakingChangesPath = "develop/breaking-changes/breaking-changes" + +func NewBreakingChange(message, identifier string) BreakingChange { + return BreakingChange{ + Message: message, + DocumentationReference: fmt.Sprintf("https://googlecloudplatform.github.io/magic-modules/%s#%s", breakingChangesPath, identifier), + } +} + +func ComputeBreakingChanges(schemaDiff diff.SchemaDiff) []BreakingChange { + var breakingChanges []BreakingChange for resource, resourceDiff := range schemaDiff { - for _, rule := range ResourceInventoryRules { - if rule.isRuleBreak(resourceDiff.ResourceConfig.Old, resourceDiff.ResourceConfig.New) { - messages = append(messages, rule.Message(resource)) + for _, rule := range ResourceConfigDiffRules { + for _, message := range rule.Messages(resource, resourceDiff.ResourceConfig.Old, resourceDiff.ResourceConfig.New) { + breakingChanges = append(breakingChanges, NewBreakingChange(message, rule.Identifier)) } } - // If the resource was added or removed, don't check field schema diffs. + // If the resource was added or removed, don't check rules that include field information. if resourceDiff.ResourceConfig.Old == nil || resourceDiff.ResourceConfig.New == nil { continue } - // TODO: Move field removal to field_rules and merge resource schema / resource inventory rules - // b/300124253 - for _, rule := range ResourceSchemaRules { - violatingFields := rule.IsRuleBreak(resourceDiff) - if len(violatingFields) > 0 { - for _, field := range violatingFields { - newMessage := rule.Message(resource, field) - messages = append(messages, newMessage) - } + for _, rule := range ResourceDiffRules { + for _, message := range rule.Messages(resource, resourceDiff) { + breakingChanges = append(breakingChanges, NewBreakingChange(message, rule.Identifier)) } } for field, fieldDiff := range resourceDiff.Fields { - for _, rule := range FieldRules { - // TODO: refactor rules to use interface-based implementation that separates checking whether - // a rule broke from composing a message for a rule break. - breakageMessage := rule.IsRuleBreak( - fieldDiff.Old, - fieldDiff.New, - MessageContext{ - Resource: resource, - Field: field, - definition: rule.definition, - name: rule.name, - }, - ) - if breakageMessage != nil { - messages = append(messages, breakageMessage) + for _, rule := range FieldDiffRules { + for _, message := range rule.Messages(resource, field, fieldDiff.Old, fieldDiff.New) { + breakingChanges = append(breakingChanges, NewBreakingChange(message, rule.Identifier)) } } } } - return messages + return breakingChanges } diff --git a/tools/diff-processor/rules/breaking_changes_test.go b/tools/diff-processor/rules/breaking_changes_test.go index 81d68b01c84e..d72e32b9801a 100644 --- a/tools/diff-processor/rules/breaking_changes_test.go +++ b/tools/diff-processor/rules/breaking_changes_test.go @@ -15,7 +15,7 @@ func TestComputeBreakingChanges(t *testing.T) { name string oldResourceMap map[string]*schema.Resource newResourceMap map[string]*schema.Resource - wantViolations []*BreakingChange + wantViolations []BreakingChange }{ { name: "control", @@ -91,14 +91,10 @@ func TestComputeBreakingChanges(t *testing.T) { }, }, newResourceMap: map[string]*schema.Resource{}, - wantViolations: []*BreakingChange{ + wantViolations: []BreakingChange{ { - Resource: "google-x", Message: "Resource `google-x` was either removed or renamed", - DocumentationReference: "https://googlecloudplatform.github.io/magic-modules/develop/breaking-changes#resource-map-resource-removal-or-rename", - RuleTemplate: "Resource {{resource}} was either removed or renamed", - RuleDefinition: "In terraform resources should be retained whenever possible. A removable of an resource will result in a configuration breakage wherever a dependency on that resource exists. Renaming or Removing a resources are functionally equivalent in terms of configuration breakages.", - RuleName: "Removing or Renaming an Resource", + DocumentationReference: "https://googlecloudplatform.github.io/magic-modules/develop/breaking-changes/breaking-changes#resource-map-resource-removal-or-rename", }, }, }, @@ -119,15 +115,10 @@ func TestComputeBreakingChanges(t *testing.T) { }, }, }, - wantViolations: []*BreakingChange{ + wantViolations: []BreakingChange{ { - Resource: "google-x", - Field: "field-b", Message: "Field `field-b` within resource `google-x` was either removed or renamed", - DocumentationReference: "https://googlecloudplatform.github.io/magic-modules/develop/breaking-changes#resource-schema-field-removal-or-rename", - RuleTemplate: "Field {{field}} within resource {{resource}} was either removed or renamed", - RuleDefinition: "In terraform fields should be retained whenever possible. A removable of an field will result in a configuration breakage wherever a dependency on that field exists. Renaming or Removing a field are functionally equivalent in terms of configuration breakages.", - RuleName: "Removing or Renaming an field", + DocumentationReference: "https://googlecloudplatform.github.io/magic-modules/develop/breaking-changes/breaking-changes#resource-schema-field-removal-or-rename", }, }, }, @@ -149,15 +140,10 @@ func TestComputeBreakingChanges(t *testing.T) { }, }, }, - wantViolations: []*BreakingChange{ + wantViolations: []BreakingChange{ { - Resource: "google-x", - Field: "field-a", Message: "Field `field-a` changed from optional to required on `google-x`", - DocumentationReference: "https://googlecloudplatform.github.io/magic-modules/develop/breaking-changes#field-optional-to-required", - RuleTemplate: "Field {{field}} changed from optional to required on {{resource}}", - RuleName: "Field becoming Required Field", - RuleDefinition: "A field cannot become required as existing configs may not have this field defined. Thus, breaking configs in sequential plan or applies. If you are adding Required to a field so a block won't remain empty, this can cause two issues. First if it's a singular nested field the block may gain more fields later and it's not clear whether the field is actually required so it may be misinterpreted by future contributors. Second if users are defining empty blocks in existing configurations this change will break them. Consider these points in admittance of this type of change.", + DocumentationReference: "https://googlecloudplatform.github.io/magic-modules/develop/breaking-changes/breaking-changes#field-optional-to-required", }, }, }, @@ -178,24 +164,14 @@ func TestComputeBreakingChanges(t *testing.T) { }, }, }, - wantViolations: []*BreakingChange{ + wantViolations: []BreakingChange{ { - Resource: "google-x", - Field: "field-a", Message: "Field `field-a` changed from optional to required on `google-x`", - DocumentationReference: "https://googlecloudplatform.github.io/magic-modules/develop/breaking-changes#field-optional-to-required", - RuleTemplate: "Field {{field}} changed from optional to required on {{resource}}", - RuleName: "Field becoming Required Field", - RuleDefinition: "A field cannot become required as existing configs may not have this field defined. Thus, breaking configs in sequential plan or applies. If you are adding Required to a field so a block won't remain empty, this can cause two issues. First if it's a singular nested field the block may gain more fields later and it's not clear whether the field is actually required so it may be misinterpreted by future contributors. Second if users are defining empty blocks in existing configurations this change will break them. Consider these points in admittance of this type of change.", + DocumentationReference: "https://googlecloudplatform.github.io/magic-modules/develop/breaking-changes/breaking-changes#field-optional-to-required", }, { - Resource: "google-x", - Field: "field-b", Message: "Field `field-b` within resource `google-x` was either removed or renamed", - DocumentationReference: "https://googlecloudplatform.github.io/magic-modules/develop/breaking-changes#resource-schema-field-removal-or-rename", - RuleTemplate: "Field {{field}} within resource {{resource}} was either removed or renamed", - RuleDefinition: "In terraform fields should be retained whenever possible. A removable of an field will result in a configuration breakage wherever a dependency on that field exists. Renaming or Removing a field are functionally equivalent in terms of configuration breakages.", - RuleName: "Removing or Renaming an field", + DocumentationReference: "https://googlecloudplatform.github.io/magic-modules/develop/breaking-changes/breaking-changes#resource-schema-field-removal-or-rename", }, }, }, @@ -221,32 +197,18 @@ func TestComputeBreakingChanges(t *testing.T) { }, }, }, - wantViolations: []*BreakingChange{ + wantViolations: []BreakingChange{ { - Resource: "google-x", - Field: "field-a", Message: "Field `field-a` changed from optional to required on `google-x`", - DocumentationReference: "https://googlecloudplatform.github.io/magic-modules/develop/breaking-changes#field-optional-to-required", - RuleTemplate: "Field {{field}} changed from optional to required on {{resource}}", - RuleName: "Field becoming Required Field", - RuleDefinition: "A field cannot become required as existing configs may not have this field defined. Thus, breaking configs in sequential plan or applies. If you are adding Required to a field so a block won't remain empty, this can cause two issues. First if it's a singular nested field the block may gain more fields later and it's not clear whether the field is actually required so it may be misinterpreted by future contributors. Second if users are defining empty blocks in existing configurations this change will break them. Consider these points in admittance of this type of change.", + DocumentationReference: "https://googlecloudplatform.github.io/magic-modules/develop/breaking-changes/breaking-changes#field-optional-to-required", }, { - Resource: "google-x", - Field: "field-b", Message: "Field `field-b` within resource `google-x` was either removed or renamed", - DocumentationReference: "https://googlecloudplatform.github.io/magic-modules/develop/breaking-changes#resource-schema-field-removal-or-rename", - RuleTemplate: "Field {{field}} within resource {{resource}} was either removed or renamed", - RuleDefinition: "In terraform fields should be retained whenever possible. A removable of an field will result in a configuration breakage wherever a dependency on that field exists. Renaming or Removing a field are functionally equivalent in terms of configuration breakages.", - RuleName: "Removing or Renaming an field", + DocumentationReference: "https://googlecloudplatform.github.io/magic-modules/develop/breaking-changes/breaking-changes#resource-schema-field-removal-or-rename", }, { - Resource: "google-y", Message: "Resource `google-y` was either removed or renamed", - DocumentationReference: "https://googlecloudplatform.github.io/magic-modules/develop/breaking-changes#resource-map-resource-removal-or-rename", - RuleTemplate: "Resource {{resource}} was either removed or renamed", - RuleDefinition: "In terraform resources should be retained whenever possible. A removable of an resource will result in a configuration breakage wherever a dependency on that resource exists. Renaming or Removing a resources are functionally equivalent in terms of configuration breakages.", - RuleName: "Removing or Renaming an Resource", + DocumentationReference: "https://googlecloudplatform.github.io/magic-modules/develop/breaking-changes/breaking-changes#resource-map-resource-removal-or-rename", }, }, }, @@ -289,15 +251,10 @@ func TestComputeBreakingChanges(t *testing.T) { }, }, }, - wantViolations: []*BreakingChange{ + wantViolations: []BreakingChange{ { - Resource: "google-x", - Field: "field-a.sub-field-2", Message: "Field `field-a.sub-field-2` within resource `google-x` was either removed or renamed", - DocumentationReference: "https://googlecloudplatform.github.io/magic-modules/develop/breaking-changes#resource-schema-field-removal-or-rename", - RuleTemplate: "Field {{field}} within resource {{resource}} was either removed or renamed", - RuleDefinition: "In terraform fields should be retained whenever possible. A removable of an field will result in a configuration breakage wherever a dependency on that field exists. Renaming or Removing a field are functionally equivalent in terms of configuration breakages.", - RuleName: "Removing or Renaming an field", + DocumentationReference: "https://googlecloudplatform.github.io/magic-modules/develop/breaking-changes/breaking-changes#resource-schema-field-removal-or-rename", }, }, }, @@ -339,15 +296,10 @@ func TestComputeBreakingChanges(t *testing.T) { }, }, }, - wantViolations: []*BreakingChange{ + wantViolations: []BreakingChange{ { - Resource: "google-x", - Field: "field-a.sub-field-1", Message: "Field `field-a.sub-field-1` MinItems went from 100 to 25 on `google-x`", - DocumentationReference: "https://googlecloudplatform.github.io/magic-modules/develop/breaking-changes#field-shrinking-max", - RuleTemplate: "Field {{field}} MinItems went from 100 to 25 on {{resource}}", - RuleName: "Shrinking Maximum Items", - RuleDefinition: "MaxItems cannot shrink. Otherwise existing terraform configurations that don't satisfy this rule will break.", + DocumentationReference: "https://googlecloudplatform.github.io/magic-modules/develop/breaking-changes/breaking-changes#field-shrinking-max", }, }, }, @@ -389,15 +341,10 @@ func TestComputeBreakingChanges(t *testing.T) { }, }, }, - wantViolations: []*BreakingChange{ + wantViolations: []BreakingChange{ { - Resource: "google-x", - Field: "field-a.sub-field-1", Message: "Field `field-a.sub-field-1` MinItems went from 100 to 25 on `google-x`", - DocumentationReference: "https://googlecloudplatform.github.io/magic-modules/develop/breaking-changes#field-shrinking-max", - RuleTemplate: "Field {{field}} MinItems went from 100 to 25 on {{resource}}", - RuleName: "Shrinking Maximum Items", - RuleDefinition: "MaxItems cannot shrink. Otherwise existing terraform configurations that don't satisfy this rule will break.", + DocumentationReference: "https://googlecloudplatform.github.io/magic-modules/develop/breaking-changes/breaking-changes#field-shrinking-max", }, }, }, @@ -425,15 +372,10 @@ func TestComputeBreakingChanges(t *testing.T) { }, }, }, - wantViolations: []*BreakingChange{ + wantViolations: []BreakingChange{ { - Resource: "google-x", - Field: "field-a", Message: "Field `field-a` MinItems went from 1 to 4 on `google-x`", - DocumentationReference: "https://googlecloudplatform.github.io/magic-modules/develop/breaking-changes#field-growing-min", - RuleTemplate: "Field {{field}} MinItems went from 1 to 4 on {{resource}}", - RuleName: "Growing Minimum Items", - RuleDefinition: "MinItems cannot grow. Otherwise existing terraform configurations that don't satisfy this rule will break.", + DocumentationReference: "https://googlecloudplatform.github.io/magic-modules/develop/breaking-changes/breaking-changes#field-growing-min", }, }, }, diff --git a/tools/diff-processor/rules/message_context.go b/tools/diff-processor/rules/message_context.go deleted file mode 100644 index bb83df6a0267..000000000000 --- a/tools/diff-processor/rules/message_context.go +++ /dev/null @@ -1,36 +0,0 @@ -package rules - -import ( - "fmt" - "strings" - - "github.com/GoogleCloudPlatform/magic-modules/tools/diff-processor/constants" -) - -// MessageContext - is an envelope for additional -// metadata about context around the rule breakage -type MessageContext struct { - Resource string - Field string - definition string - name string - identifier string - message string -} - -func populateMessageContext(message string, mc MessageContext) *BreakingChange { - template := message - 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 &BreakingChange{ - Resource: mc.Resource, - Field: mc.Field, - Message: message, - RuleTemplate: template, - DocumentationReference: constants.GetFileUrl(mc.identifier), - RuleDefinition: mc.definition, - RuleName: mc.name, - } -} diff --git a/tools/diff-processor/rules/rule.go b/tools/diff-processor/rules/rule.go deleted file mode 100644 index fd5d6d0ba649..000000000000 --- a/tools/diff-processor/rules/rule.go +++ /dev/null @@ -1,56 +0,0 @@ -package rules - -// Rule is an interface that all Rule -// types implement. This give consistency -// for the potential documentation generation. -type Rule interface { - Name() string - Definition() string - Identifier() string - Undetectable() bool -} - -// RuleCategory holds documentation and -// inventory for each of the rule types -type RuleCategory struct { - Name string - Definition string - Rules []Rule -} - -// Rules is a type to hold all existing -// rules. Exposed for potential documentation generator -type Rules struct { - Categories []RuleCategory -} - -// GetRules returns a list of all rules for -// potential documentation generation -func GetRules() *Rules { - categories := []RuleCategory{ - { - Name: "Provider Configuration Level Breakages", - Definition: "Top level behavior such as provider configuration and authentication changes.", - Rules: providerConfigRulesToRuleArray(ProviderConfigRules), - }, - { - Name: "Resource List Level Breakages", - Definition: "Resource/datasource naming conventions and entry differences.", - Rules: resourceInventoryRulesToRuleArray(ResourceInventoryRules), - }, - { - Name: "Resource Level Breakages", - Definition: "Individual resource breakages like field entry removals or behavior within a resource.", - Rules: resourceSchemaRulesToRuleArray(ResourceSchemaRules), - }, - { - Name: "Field Level Breakages", - Definition: "Field level conventions like attribute changes and naming conventions.", - Rules: fieldRulesToRuleArray(FieldRules), - }, - } - - return &Rules{ - Categories: categories, - } -} diff --git a/tools/diff-processor/rules/rule_test.go b/tools/diff-processor/rules/rule_test.go index 2ed61fd00c9a..5f19f064190e 100644 --- a/tools/diff-processor/rules/rule_test.go +++ b/tools/diff-processor/rules/rule_test.go @@ -23,7 +23,7 @@ func TestUniqueRuleIdentifiers(t *testing.T) { func TestMarkdownIdentifiers(t *testing.T) { // Define the Markdown file path relative to the importer - mdFilePath := "../../../docs/content/develop/breaking-changes/breaking-changes.md" + mdFilePath := fmt.Sprintf("../../../docs/content/%s.md", breakingChangesPath) // Read the Markdown file mdContent, err := ioutil.ReadFile(mdFilePath) @@ -50,17 +50,19 @@ func TestMarkdownIdentifiers(t *testing.T) { } func getArrayOfIdentifiers() []string { - var output []string - ruleCats := GetRules() - var rules []Rule - for _, rc := range ruleCats.Categories { - rules = append(rules, rc.Rules...) + var identifiers []string + + for _, r := range ResourceDiffRules { + identifiers = append(identifiers, r.Identifier) + } + + for _, r := range ResourceConfigDiffRules { + identifiers = append(identifiers, r.Identifier) } - for _, r := range rules { - identifier := r.Identifier() - output = append(output, identifier) + for _, r := range FieldDiffRules { + identifiers = append(identifiers, r.Identifier) } - return output + return identifiers } diff --git a/tools/diff-processor/rules/rules_field.go b/tools/diff-processor/rules/rules_field.go index 34c32958d305..93eb51106f1c 100644 --- a/tools/diff-processor/rules/rules_field.go +++ b/tools/diff-processor/rules/rules_field.go @@ -2,62 +2,48 @@ package rules import ( "fmt" - "strings" + "strconv" "github.com/hashicorp/terraform-plugin-sdk/v2/helper/schema" ) -// FieldRule provides structure for rules +// FieldDiffRule provides structure for rules // regarding field attribute changes -type FieldRule struct { - name string - definition string - message string - identifier string - isRuleBreak func(old, new *schema.Schema, mc MessageContext) *BreakingChange +type FieldDiffRule struct { + Identifier string + // TODO: change signature to take FieldDiff instead of old, new. + Messages func(resource, field string, old, new *schema.Schema) []string } -// FieldRules is a list of FieldRule +// FieldDiffRules is a list of FieldDiffRule // guarding against provider breaking changes -var FieldRules = []FieldRule{ - fieldRule_ChangingType, - fieldRule_BecomingRequired, - fieldRule_BecomingComputedOnly, - fieldRule_OptionalComputedToOptional, - fieldRule_DefaultModification, - fieldRule_GrowingMin, - fieldRule_ShrinkingMax, - fieldRule_RemovingDiffSuppress, - fieldRule_AddingSubfieldToConfigModeAttr, - fieldRule_ChangingFieldDataFormat, +var FieldDiffRules = []FieldDiffRule{ + FieldChangingType, + FieldBecomingRequired, + FieldBecomingComputedOnly, + FieldOptionalComputedToOptional, + FieldDefaultModification, + FieldGrowingMin, + FieldShrinkingMax, + FieldRemovingDiffSuppress, + FieldAddingSubfieldToConfigModeAttr, } -var fieldRule_ChangingFieldDataFormat = FieldRule{ - name: "Changing field data format", - definition: "Modification of the data format (either by the API or manually) will cause a diff in subsequent plans if that field is not Computed. This results in a breakage. API breaking changes are out of scope with respect to provider responsibility but we may make changes in response to API breakages in some instances to provide more customer stability.", - identifier: "field-changing-data-format", +var FieldChangingType = FieldDiffRule{ + Identifier: "field-changing-type", + Messages: FieldChangingTypeMessages, } -var fieldRule_ChangingType = FieldRule{ - name: "Changing Field Type", - definition: "While certain Field Type migrations may be supported at a technical level, it's a practice that we highly discourage. We see little value for these transitions vs the risk they impose.", - message: "Field {{field}} changed from {{oldType}} to {{newType}} on {{resource}}", - identifier: "field-changing-type", - isRuleBreak: fieldRule_ChangingType_func, -} - -func fieldRule_ChangingType_func(old, new *schema.Schema, mc MessageContext) *BreakingChange { +func FieldChangingTypeMessages(resource, field string, old, new *schema.Schema) []string { // Type change doesn't matter for added / removed fields if old == nil || new == nil { return nil } - message := mc.message + tmpl := "Field `%s` changed from %s to %s on `%s`" if old.Type != new.Type { oldType := getValueType(old.Type) newType := getValueType(new.Type) - message = strings.ReplaceAll(message, "{{oldType}}", oldType) - message = strings.ReplaceAll(message, "{{newType}}", newType) - return populateMessageContext(message, mc) + return []string{fmt.Sprintf(tmpl, field, oldType, newType, resource)} } oldCasted, _ := old.Elem.(*schema.Schema) @@ -65,188 +51,158 @@ func fieldRule_ChangingType_func(old, new *schema.Schema, mc MessageContext) *Br if oldCasted != nil && newCasted != nil && oldCasted.Type != newCasted.Type { oldType := getValueType(old.Type) + "." + getValueType(oldCasted.Type) newType := getValueType(new.Type) + "." + getValueType(newCasted.Type) - message = strings.ReplaceAll(message, "{{oldType}}", oldType) - message = strings.ReplaceAll(message, "{{newType}}", newType) - return populateMessageContext(message, mc) + return []string{fmt.Sprintf(tmpl, field, oldType, newType, resource)} } return nil } -var fieldRule_BecomingRequired = FieldRule{ - name: "Field becoming Required Field", - definition: "A field cannot become required as existing configs may not have this field defined. Thus, breaking configs in sequential plan or applies. If you are adding Required to a field so a block won't remain empty, this can cause two issues. First if it's a singular nested field the block may gain more fields later and it's not clear whether the field is actually required so it may be misinterpreted by future contributors. Second if users are defining empty blocks in existing configurations this change will break them. Consider these points in admittance of this type of change.", - message: "Field {{field}} changed from optional to required on {{resource}}", - identifier: "field-optional-to-required", - isRuleBreak: fieldRule_BecomingRequired_func, +var FieldBecomingRequired = FieldDiffRule{ + Identifier: "field-optional-to-required", + Messages: FieldBecomingRequiredMessages, } -func fieldRule_BecomingRequired_func(old, new *schema.Schema, mc MessageContext) *BreakingChange { +func FieldBecomingRequiredMessages(resource, field string, old, new *schema.Schema) []string { // Ignore for added / removed fields if old == nil || new == nil { return nil } - message := mc.message + tmpl := "Field `%s` changed from optional to required on `%s`" if !old.Required && new.Required { - return populateMessageContext(message, mc) + return []string{fmt.Sprintf(tmpl, field, resource)} } return nil } -var fieldRule_BecomingComputedOnly = FieldRule{ - name: "Becoming a Computed only Field", - definition: "While a field can go from Optional to Optional+Computed it cannot go from Required or Optional to only Computed. This transition would effectively make the field read-only thus breaking configs in sequential plan or applies where this field is defined in a configuration.", - message: "Field {{field}} became Computed only on {{resource}}", - identifier: "field-becoming-computed", - isRuleBreak: fieldRule_BecomingComputedOnly_func, +var FieldBecomingComputedOnly = FieldDiffRule{ + Identifier: "field-becoming-computed", + Messages: FieldBecomingComputedOnlyMessages, } -func fieldRule_BecomingComputedOnly_func(old, new *schema.Schema, mc MessageContext) *BreakingChange { +func FieldBecomingComputedOnlyMessages(resource, field string, old, new *schema.Schema) []string { // ignore for added / removed fields if old == nil || new == nil { return nil } - message := mc.message // if the field is computed only already // this rule doesn't apply if old.Computed && !old.Optional { return nil } + tmpl := "Field `%s` became Computed only on `%s`" if new.Computed && !new.Optional { - return populateMessageContext(message, mc) + return []string{fmt.Sprintf(tmpl, field, resource)} } return nil } -var fieldRule_OptionalComputedToOptional = FieldRule{ - name: "Optional and Computed to Optional", - definition: "A field cannot go from Computed + Optional to Optional. On a sequential `apply` the terraform state will have the previously computed value. The value won't be present in the config, thus ultimately causing a diff.", - message: "Field {{field}} transitioned from optional+computed to optional {{resource}}", - identifier: "field-oc-to-c", - isRuleBreak: fieldRule_OptionalComputedToOptional_func, +var FieldOptionalComputedToOptional = FieldDiffRule{ + Identifier: "field-oc-to-c", + Messages: FieldOptionalComputedToOptionalMessages, } -func fieldRule_OptionalComputedToOptional_func(old, new *schema.Schema, mc MessageContext) *BreakingChange { +func FieldOptionalComputedToOptionalMessages(resource, field string, old, new *schema.Schema) []string { // ignore for added / removed fields if old == nil || new == nil { return nil } - message := mc.message + tmpl := "Field `%s` transitioned from optional+computed to optional `%s`" if (old.Computed && old.Optional) && (new.Optional && !new.Computed) { - return populateMessageContext(message, mc) + return []string{fmt.Sprintf(tmpl, field, resource)} } return nil } -var fieldRule_DefaultModification = FieldRule{ - name: "Adding or Changing a Default Value", - definition: "Adding a default value where one was not previously declared can work in a very limited subset of scenarios but is an all around 'not good' practice to engage in. Changing a default value will absolutely cause a breakage. The mechanism of break for both scenarios is current terraform deployments now gain a diff with sequential applies where the diff is the new or changed default value.", - message: "Field {{field}} default value changed from {{oldDefault}} to {{newDefault}} on {{resource}}", - identifier: "field-changing-default-value", - isRuleBreak: fieldRule_DefaultModification_func, +var FieldDefaultModification = FieldDiffRule{ + Identifier: "field-changing-default-value", + Messages: FieldDefaultModificationMessages, } -func fieldRule_DefaultModification_func(old, new *schema.Schema, mc MessageContext) *BreakingChange { +func FieldDefaultModificationMessages(resource, field string, old, new *schema.Schema) []string { // ignore for added / removed fields if old == nil || new == nil { return nil } - message := mc.message + tmpl := "Field `%s` default value changed from %s to %s on `%s`" if old.Default != new.Default { oldDefault := fmt.Sprintf("%v", old.Default) newDefault := fmt.Sprintf("%v", new.Default) - message = strings.ReplaceAll(message, "{{oldDefault}}", oldDefault) - message = strings.ReplaceAll(message, "{{newDefault}}", newDefault) - return populateMessageContext(message, mc) + return []string{fmt.Sprintf(tmpl, field, oldDefault, newDefault, resource)} } return nil } -var fieldRule_GrowingMin = FieldRule{ - name: "Growing Minimum Items", - definition: "MinItems cannot grow. Otherwise existing terraform configurations that don't satisfy this rule will break.", - message: "Field {{field}} MinItems went from {{oldMin}} to {{newMin}} on {{resource}}", - identifier: "field-growing-min", - isRuleBreak: fieldRule_GrowingMin_func, +var FieldGrowingMin = FieldDiffRule{ + Identifier: "field-growing-min", + Messages: FieldGrowingMinMessages, } -func fieldRule_GrowingMin_func(old, new *schema.Schema, mc MessageContext) *BreakingChange { +func FieldGrowingMinMessages(resource, field string, old, new *schema.Schema) []string { // ignore for added / removed fields if old == nil || new == nil { return nil } - message := mc.message + tmpl := "Field `%s` MinItems went from %s to %s on `%s`" if old.MinItems < new.MinItems || old.MinItems == 0 && new.MinItems > 0 { - oldMin := fmt.Sprint(old.MinItems) + oldMin := strconv.Itoa(old.MinItems) if old.MinItems == 0 { oldMin = "unset" } - newMin := fmt.Sprint(new.MinItems) - message = strings.ReplaceAll(message, "{{oldMin}}", oldMin) - message = strings.ReplaceAll(message, "{{newMin}}", newMin) - return populateMessageContext(message, mc) + newMin := strconv.Itoa(new.MinItems) + return []string{fmt.Sprintf(tmpl, field, oldMin, newMin, resource)} } return nil } -var fieldRule_ShrinkingMax = FieldRule{ - name: "Shrinking Maximum Items", - definition: "MaxItems cannot shrink. Otherwise existing terraform configurations that don't satisfy this rule will break.", - message: "Field {{field}} MinItems went from {{oldMax}} to {{newMax}} on {{resource}}", - identifier: "field-shrinking-max", - isRuleBreak: fieldRule_ShrinkingMax_func, +var FieldShrinkingMax = FieldDiffRule{ + Identifier: "field-shrinking-max", + Messages: FieldShrinkingMaxMessages, } -func fieldRule_ShrinkingMax_func(old, new *schema.Schema, mc MessageContext) *BreakingChange { +func FieldShrinkingMaxMessages(resource, field string, old, new *schema.Schema) []string { // ignore for added / removed fields if old == nil || new == nil { return nil } - message := mc.message + tmpl := "Field `%s` MinItems went from %s to %s on `%s`" if old.MaxItems > new.MaxItems || old.MaxItems == 0 && new.MaxItems > 0 { - oldMax := fmt.Sprint(old.MaxItems) + oldMax := strconv.Itoa(old.MaxItems) if old.MaxItems == 0 { oldMax = "unset" } - newMax := fmt.Sprint(new.MaxItems) - message = strings.ReplaceAll(message, "{{oldMax}}", oldMax) - message = strings.ReplaceAll(message, "{{newMax}}", newMax) - return populateMessageContext(message, mc) + newMax := strconv.Itoa(new.MaxItems) + return []string{fmt.Sprintf(tmpl, field, oldMax, newMax, resource)} } return nil } -var fieldRule_RemovingDiffSuppress = FieldRule{ - name: "Removing Diff Suppress Function", - definition: "Diff suppress functions cannot be removed. Otherwise terraform configurations that previously had no diffs would show diffs.", - message: "Field {{field}} lost its diff suppress function", - identifier: "field-removing-diff-suppress", - isRuleBreak: fieldRule_RemovingDiffSuppress_func, +var FieldRemovingDiffSuppress = FieldDiffRule{ + Identifier: "field-removing-diff-suppress", + Messages: FieldRemovingDiffSuppressMessages, } -func fieldRule_RemovingDiffSuppress_func(old, new *schema.Schema, mc MessageContext) *BreakingChange { +func FieldRemovingDiffSuppressMessages(resource, field string, old, new *schema.Schema) []string { // ignore for added / removed fields if old == nil || new == nil { return nil } + // TODO: Add resource to this message + tmpl := "Field `%s` lost its diff suppress function" if old.DiffSuppressFunc != nil && new.DiffSuppressFunc == nil { - return populateMessageContext(mc.message, mc) + return []string{fmt.Sprintf(tmpl, field)} } return nil } -var fieldRule_AddingSubfieldToConfigModeAttr = FieldRule{ - name: "Adding a subfield to a SchemaConfigModeAttr field", - definition: "Subfields cannot be added to fields with SchemaConfigModeAttr because they will be treated as required even if optional.", - message: "Field {{field}} gained a subfield {{subfield}} when it has SchemaConfigModeAttr", - identifier: "field-adding-subfield-to-config-mode-attr", - isRuleBreak: fieldRule_AddingSubfieldToConfigModeAttr_func, +var FieldAddingSubfieldToConfigModeAttr = FieldDiffRule{ + Identifier: "field-adding-subfield-to-config-mode-attr", + Messages: FieldAddingSubfieldToConfigModeAttrMessages, } -func fieldRule_AddingSubfieldToConfigModeAttr_func(old, new *schema.Schema, mc MessageContext) *BreakingChange { +func FieldAddingSubfieldToConfigModeAttrMessages(resource, field string, old, new *schema.Schema) []string { if old == nil || new == nil { return nil } @@ -259,53 +215,13 @@ func fieldRule_AddingSubfieldToConfigModeAttr_func(old, new *schema.Schema, mc M if !ok { return nil } - message := mc.message - for fieldName := range newObj.Schema { - if _, ok := oldObj.Schema[fieldName]; !ok { - message = strings.ReplaceAll(message, "{{subfield}}", fieldName) - return populateMessageContext(message, mc) + // TODO: Add resource to this message + tmpl := "Field `%s` gained a subfield `%s` when it has SchemaConfigModeAttr" + for subfield := range newObj.Schema { + if _, ok := oldObj.Schema[subfield]; !ok { + return []string{fmt.Sprintf(tmpl, field, subfield)} } } } return nil } - -func fieldRulesToRuleArray(frs []FieldRule) []Rule { - var rules []Rule - for _, fr := range frs { - rules = append(rules, fr) - } - return rules -} - -// Name - a human readable name for the rule -func (fr FieldRule) Name() string { - return fr.name -} - -// Definition - a definition for the rule -func (fr FieldRule) Definition() string { - return fr.definition -} - -// Identifier - a navigation oriented name for the rule -func (fr FieldRule) Identifier() string { - return fr.identifier -} - -// IsRuleBreak - compares the fields and returns -// a string defining the rule breakage if detected -func (fr FieldRule) IsRuleBreak(old, new *schema.Schema, mc MessageContext) *BreakingChange { - if fr.isRuleBreak == nil { - return nil - } - mc.identifier = fr.identifier - mc.message = fr.message - return fr.isRuleBreak(old, new, mc) -} - -// Undetectable - informs if there are functions in place -// to detect this rule. -func (fr FieldRule) Undetectable() bool { - return fr.isRuleBreak == nil -} diff --git a/tools/diff-processor/rules/rules_field_test.go b/tools/diff-processor/rules/rules_field_test.go index f68847dab6bb..e4ad39664309 100644 --- a/tools/diff-processor/rules/rules_field_test.go +++ b/tools/diff-processor/rules/rules_field_test.go @@ -1,7 +1,6 @@ package rules import ( - "strings" "testing" "github.com/hashicorp/terraform-plugin-sdk/v2/helper/schema" @@ -14,13 +13,13 @@ type fieldTestCase struct { expectedViolation bool } -func TestFieldRule_BecomingRequired(t *testing.T) { - for _, tc := range fieldRule_BecomingRequiredTestCases { - tc.check(fieldRule_BecomingRequired, t) +func TestFieldBecomingRequired(t *testing.T) { + for _, tc := range FieldBecomingRequiredTestCases { + tc.check(FieldBecomingRequired, t) } } -var fieldRule_BecomingRequiredTestCases = []fieldTestCase{ +var FieldBecomingRequiredTestCases = []fieldTestCase{ { name: "control", oldField: &schema.Schema{ @@ -93,15 +92,15 @@ var fieldRule_BecomingRequiredTestCases = []fieldTestCase{ } // !! min max ? -// isRuleBreak: fieldRule_OptionalComputedToOptional_func, +// isRuleBreak: FieldOptionalComputedToOptional_func, -func TestFieldRule_ChangingType(t *testing.T) { - for _, tc := range fieldRule_ChangingTypeTestCases { - tc.check(fieldRule_ChangingType, t) +func TestFieldChangingType(t *testing.T) { + for _, tc := range FieldChangingTypeTestCases { + tc.check(FieldChangingType, t) } } -var fieldRule_ChangingTypeTestCases = []fieldTestCase{ +var FieldChangingTypeTestCases = []fieldTestCase{ { name: "control", oldField: &schema.Schema{ @@ -174,13 +173,13 @@ var fieldRule_ChangingTypeTestCases = []fieldTestCase{ }, } -func TestFieldRule_DefaultModification(t *testing.T) { - for _, tc := range fieldRule_DefaultModificationTestCases { - tc.check(fieldRule_DefaultModification, t) +func TestFieldDefaultModification(t *testing.T) { + for _, tc := range FieldDefaultModificationTestCases { + tc.check(FieldDefaultModification, t) } } -var fieldRule_DefaultModificationTestCases = []fieldTestCase{ +var FieldDefaultModificationTestCases = []fieldTestCase{ { name: "control", oldField: &schema.Schema{ @@ -275,13 +274,13 @@ var fieldRule_DefaultModificationTestCases = []fieldTestCase{ }, } -func TestFieldRule_BecomingComputedOnly(t *testing.T) { - for _, tc := range fieldRule_BecomingComputedOnlyTestCases { - tc.check(fieldRule_BecomingComputedOnly, t) +func TestFieldBecomingComputedOnly(t *testing.T) { + for _, tc := range FieldBecomingComputedOnlyTestCases { + tc.check(FieldBecomingComputedOnly, t) } } -var fieldRule_BecomingComputedOnlyTestCases = []fieldTestCase{ +var FieldBecomingComputedOnlyTestCases = []fieldTestCase{ { name: "control - already computed", oldField: &schema.Schema{ @@ -363,13 +362,13 @@ var fieldRule_BecomingComputedOnlyTestCases = []fieldTestCase{ }, } -func TestFieldRule_OptionalComputedToOptional(t *testing.T) { - for _, tc := range fieldRule_OptionalComputedToOptionalTestCases { - tc.check(fieldRule_OptionalComputedToOptional, t) +func TestFieldOptionalComputedToOptional(t *testing.T) { + for _, tc := range FieldOptionalComputedToOptionalTestCases { + tc.check(FieldOptionalComputedToOptional, t) } } -var fieldRule_OptionalComputedToOptionalTestCases = []fieldTestCase{ +var FieldOptionalComputedToOptionalTestCases = []fieldTestCase{ { name: "control - static", oldField: &schema.Schema{ @@ -422,13 +421,13 @@ var fieldRule_OptionalComputedToOptionalTestCases = []fieldTestCase{ }, } -func TestFieldRule_GrowingMin(t *testing.T) { - for _, tc := range fieldRule_GrowingMinTestCases { - tc.check(fieldRule_GrowingMin, t) +func TestFieldGrowingMin(t *testing.T) { + for _, tc := range FieldGrowingMinTestCases { + tc.check(FieldGrowingMin, t) } } -var fieldRule_GrowingMinTestCases = []fieldTestCase{ +var FieldGrowingMinTestCases = []fieldTestCase{ { name: "control:min - static", oldField: &schema.Schema{ @@ -491,13 +490,13 @@ var fieldRule_GrowingMinTestCases = []fieldTestCase{ }, } -func TestFieldRule_ShrinkingMax(t *testing.T) { - for _, tc := range fieldRule_ShrinkingMaxTestCases { - tc.check(fieldRule_ShrinkingMax, t) +func TestFieldShrinkingMax(t *testing.T) { + for _, tc := range FieldShrinkingMaxTestCases { + tc.check(FieldShrinkingMax, t) } } -var fieldRule_ShrinkingMaxTestCases = []fieldTestCase{ +var FieldShrinkingMaxTestCases = []fieldTestCase{ { name: "control:max - static", oldField: &schema.Schema{ @@ -560,13 +559,13 @@ var fieldRule_ShrinkingMaxTestCases = []fieldTestCase{ }, } -func TestFieldRule_AddingSubfieldToConfigModeAttr(t *testing.T) { - for _, tc := range fieldRule_AddingSubfieldToConfigModeAttrTestCases { - tc.check(fieldRule_AddingSubfieldToConfigModeAttr, t) +func TestFieldAddingSubfieldToConfigModeAttr(t *testing.T) { + for _, tc := range FieldAddingSubfieldToConfigModeAttrTestCases { + tc.check(FieldAddingSubfieldToConfigModeAttr, t) } } -var fieldRule_AddingSubfieldToConfigModeAttrTestCases = []fieldTestCase{ +var FieldAddingSubfieldToConfigModeAttrTestCases = []fieldTestCase{ { name: "no new subfields", oldField: &schema.Schema{ @@ -635,35 +634,11 @@ var fieldRule_AddingSubfieldToConfigModeAttrTestCases = []fieldTestCase{ }, } -func (tc *fieldTestCase) check(rule FieldRule, t *testing.T) { - breakage := rule.isRuleBreak(tc.oldField, tc.newField, MessageContext{}) +func (tc *fieldTestCase) check(rule FieldDiffRule, t *testing.T) { + messages := rule.Messages("resource", "field", tc.oldField, tc.newField) - violation := breakage != nil - if breakage != nil && strings.Contains(breakage.Message, "{{") { - t.Errorf("Test `%s` failed: replacements for `{{}}` not successful ", tc.name) - } + violation := len(messages) > 0 if tc.expectedViolation != violation { t.Errorf("Test `%s` failed: expected %v violations, got %v", tc.name, tc.expectedViolation, violation) } } - -func TestBreakingMessage(t *testing.T) { - breakageMessage := fieldRule_OptionalComputedToOptional.IsRuleBreak( - &schema.Schema{ - Optional: true, - Computed: true, - }, - &schema.Schema{ - Optional: true, - }, - MessageContext{ - Resource: "a", - Field: "b", - }, - ) - - if !strings.Contains(breakageMessage.Message, "Field `b` transitioned from optional+computed to optional `a`") { - t.Errorf("Test `%s` failed: replacements for `{{}}` not successful ", "TestBreakingMessage") - } - -} diff --git a/tools/diff-processor/rules/rules_provider.go b/tools/diff-processor/rules/rules_provider.go deleted file mode 100644 index ca0df309686a..000000000000 --- a/tools/diff-processor/rules/rules_provider.go +++ /dev/null @@ -1,75 +0,0 @@ -package rules - -import ( - "fmt" - "strings" - - "github.com/hashicorp/terraform-plugin-sdk/v2/helper/schema" -) - -// ProviderConfigRule provides -// structure for rules regarding resource -// inventory changes -type ProviderConfigRule struct { - name string - definition string - message string - identifier string - isRuleBreak func(old, new map[string]*schema.Resource) []string -} - -// ProviderConfigRules is a list of ProviderConfigRule -// guarding against provider breaking changes -var ProviderConfigRules = []ProviderConfigRule{providerConfigRule_ConfigurationChanges} - -var providerConfigRule_ConfigurationChanges = ProviderConfigRule{ - name: "Changing fundamental provider behavior", - definition: "Including, but not limited to modification of: authentication, environment variable usage, and constricting retry behavior.", - identifier: "provider-config-fundamental", - isRuleBreak: nil, -} - -func providerConfigRulesToRuleArray(pcrs []ProviderConfigRule) []Rule { - var rules []Rule - for _, prc := range pcrs { - rules = append(rules, prc) - } - return rules -} - -// Name - a human readable name for the rule -func (prc ProviderConfigRule) Name() string { - return prc.name -} - -// Definition - a definition for the rule -func (prc ProviderConfigRule) Definition() string { - return prc.definition -} - -// Identifier - a navigation oriented name for the rule -func (prc ProviderConfigRule) Identifier() string { - return prc.identifier -} - -// Message - a message to to inform the user -// of a breakage. -func (prc ProviderConfigRule) Message(resource string) string { - msg := prc.message - resource = fmt.Sprintf("`%s`", resource) - msg = strings.ReplaceAll(msg, "{{resource}}", resource) - return msg + documentationReference(prc.identifier) -} - -// IsRuleBreak - compares resource entries and returns -// a list of resources violating the rule -func (prc ProviderConfigRule) IsRuleBreak(old, new map[string]*schema.Resource) []string { - if prc.isRuleBreak == nil { - return []string{} - } - return prc.isRuleBreak(old, new) -} - -func (prc ProviderConfigRule) Undetectable() bool { - return prc.isRuleBreak == nil -} diff --git a/tools/diff-processor/rules/rules_resource_inventory.go b/tools/diff-processor/rules/rules_resource_inventory.go index e97e3dd54c4f..0f179300f94d 100644 --- a/tools/diff-processor/rules/rules_resource_inventory.go +++ b/tools/diff-processor/rules/rules_resource_inventory.go @@ -2,79 +2,32 @@ package rules import ( "fmt" - "strings" - "github.com/GoogleCloudPlatform/magic-modules/tools/diff-processor/constants" "github.com/hashicorp/terraform-plugin-sdk/v2/helper/schema" ) // ResourceInventoryRule provides // structure for rules regarding resource // inventory changes -type ResourceInventoryRule struct { - name string - definition string - message string - identifier string - isRuleBreak func(old, new *schema.Resource) bool +type ResourceConfigDiffRule struct { + Identifier string + // TODO: Make this take a ResourceConfigDiff instead of old, new. + Messages func(resource string, old, new *schema.Resource) []string } // ResourceInventoryRules is a list of ResourceInventoryRule // guarding against provider breaking changes -var ResourceInventoryRules = []ResourceInventoryRule{resourceInventoryRule_RemovingAResource} +var ResourceConfigDiffRules = []ResourceConfigDiffRule{ResourceConfigRemovingAResource} -var resourceInventoryRule_RemovingAResource = ResourceInventoryRule{ - name: "Removing or Renaming an Resource", - definition: "In terraform resources should be retained whenever possible. A removable of an resource will result in a configuration breakage wherever a dependency on that resource exists. Renaming or Removing a resources are functionally equivalent in terms of configuration breakages.", - message: "Resource {{resource}} was either removed or renamed", - identifier: "resource-map-resource-removal-or-rename", - isRuleBreak: resourceInventoryRule_RemovingAResource_func, +var ResourceConfigRemovingAResource = ResourceConfigDiffRule{ + Identifier: "resource-map-resource-removal-or-rename", + Messages: ResourceConfigRemovingAResourceMessages, } -func resourceInventoryRule_RemovingAResource_func(old, new *schema.Resource) bool { - return new == nil && old != nil -} - -func resourceInventoryRulesToRuleArray(rms []ResourceInventoryRule) []Rule { - var rules []Rule - for _, rm := range rms { - rules = append(rules, rm) +func ResourceConfigRemovingAResourceMessages(resource string, old, new *schema.Resource) []string { + if new == nil && old != nil { + tmpl := "Resource `%s` was either removed or renamed" + return []string{fmt.Sprintf(tmpl, resource)} } - return rules -} - -// Name - a human readable name for the rule -func (rm ResourceInventoryRule) Name() string { - return rm.name -} - -// Definition - a definition for the rule -func (rm ResourceInventoryRule) Definition() string { - return rm.definition -} - -// Identifier - a navigation oriented name for the rule -func (rm ResourceInventoryRule) Identifier() string { - return rm.identifier -} - -// Message - a message to to inform the user -// of a breakage. -func (rm ResourceInventoryRule) Message(resource string) *BreakingChange { - msg := rm.message - msg = strings.ReplaceAll(msg, "{{resource}}", fmt.Sprintf("`%s`", resource)) - return &BreakingChange{ - RuleTemplate: rm.message, - Resource: resource, - Message: msg, - DocumentationReference: constants.GetFileUrl(rm.identifier), - RuleDefinition: rm.definition, - RuleName: rm.name, - } -} - -// Undetectable - informs if there are functions in place -// to detect this rule. -func (rm ResourceInventoryRule) Undetectable() bool { - return rm.isRuleBreak == nil + return nil } diff --git a/tools/diff-processor/rules/rules_resource_inventory_test.go b/tools/diff-processor/rules/rules_resource_inventory_test.go index 2bc309bfdda4..6c77a2a0a27d 100644 --- a/tools/diff-processor/rules/rules_resource_inventory_test.go +++ b/tools/diff-processor/rules/rules_resource_inventory_test.go @@ -7,42 +7,39 @@ import ( ) type resourceInventoryTestCase struct { - name string - old *schema.Resource - new *schema.Resource - expected bool + name string + old *schema.Resource + new *schema.Resource + wantViolations bool } func TestResourceInventoryRule_RemovingAResource(t *testing.T) { - for _, tc := range resourceInventoryRule_RemovingAResourceTestCases { - tc.check(resourceInventoryRule_RemovingAResource, t) + for _, tc := range resourceConfigRemovingAResourceTestCases { + got := ResourceConfigRemovingAResource.Messages("resource", tc.old, tc.new) + gotViolations := len(got) > 0 + if tc.wantViolations != gotViolations { + t.Errorf("ResourceConfigRemovingAResource.Messages(%v) violations not expected. Got %v, want %v", tc.name, gotViolations, tc.wantViolations) + } } } -var resourceInventoryRule_RemovingAResourceTestCases = []resourceInventoryTestCase{ +var resourceConfigRemovingAResourceTestCases = []resourceInventoryTestCase{ { - name: "control", - old: &schema.Resource{}, - new: &schema.Resource{}, - expected: false, + name: "control", + old: &schema.Resource{}, + new: &schema.Resource{}, + wantViolations: false, }, { - name: "resource added", - old: nil, - new: &schema.Resource{}, - expected: false, + name: "resource added", + old: nil, + new: &schema.Resource{}, + wantViolations: false, }, { - name: "resource removed", - old: &schema.Resource{}, - new: nil, - expected: true, + name: "resource removed", + old: &schema.Resource{}, + new: nil, + wantViolations: true, }, } - -func (tc *resourceInventoryTestCase) check(rule ResourceInventoryRule, t *testing.T) { - got := rule.isRuleBreak(tc.old, tc.new) - if tc.expected != got { - t.Errorf("Test `%s` failed: want %t, got %t", tc.name, tc.expected, got) - } -} diff --git a/tools/diff-processor/rules/rules_resource_schema.go b/tools/diff-processor/rules/rules_resource_schema.go index 5b06453ec988..a6fa0a6f0116 100644 --- a/tools/diff-processor/rules/rules_resource_schema.go +++ b/tools/diff-processor/rules/rules_resource_schema.go @@ -2,107 +2,37 @@ package rules import ( "fmt" - "strings" - "github.com/GoogleCloudPlatform/magic-modules/tools/diff-processor/constants" "github.com/GoogleCloudPlatform/magic-modules/tools/diff-processor/diff" ) -// ResourceSchemaRule provides structure for -// rules regarding resource attribute changes -type ResourceSchemaRule struct { - name string - definition string - message string - identifier string - isRuleBreak func(resourceDiff diff.ResourceDiff) []string +// ResourceDiffRule is a rule that operates on an entire ResourceDiff +type ResourceDiffRule struct { + Identifier string + Messages func(resource string, resourceDiff diff.ResourceDiff) []string } -// ResourceSchemaRules is a list of ResourceInventoryRule -// guarding against provider breaking changes -var ResourceSchemaRules = []ResourceSchemaRule{resourceSchemaRule_RemovingAField, resourceSchemaRule_ChangingResourceIDFormat, resourceSchemaRule_ChangingImportIDFormat} +// ResourceDiffRules is a list of all ResourceDiff rules +var ResourceDiffRules = []ResourceDiffRule{RemovingAField} -var resourceSchemaRule_ChangingResourceIDFormat = ResourceSchemaRule{ - name: "Changing resource ID format", - definition: "Terraform uses resource ID to read resource state from the api. Modification of the ID format will break the ability to parse the IDs from any deployments.", - identifier: "resource-id", +var RemovingAField = ResourceDiffRule{ + Identifier: "resource-schema-field-removal-or-rename", + Messages: RemovingAFieldMessages, } -var resourceSchemaRule_ChangingImportIDFormat = ResourceSchemaRule{ - name: "Changing resource ID import format", - definition: "Automation external to our provider may rely on importing resources with a certain format. Removal or modification of existing formats will break this automation.", - identifier: "resource-import-format", -} - -var resourceSchemaRule_RemovingAField = ResourceSchemaRule{ - name: "Removing or Renaming an field", - definition: "In terraform fields should be retained whenever possible. A removable of an field will result in a configuration breakage wherever a dependency on that field exists. Renaming or Removing a field are functionally equivalent in terms of configuration breakages.", - message: "Field {{field}} within resource {{resource}} was either removed or renamed", - identifier: "resource-schema-field-removal-or-rename", - isRuleBreak: resourceSchemaRule_RemovingAField_func, -} - -func resourceSchemaRule_RemovingAField_func(resourceDiff diff.ResourceDiff) []string { +// TODO: Make field removal a FieldDiffRule b/300124253 +func RemovingAFieldMessages(resource string, resourceDiff diff.ResourceDiff) []string { fieldsRemoved := []string{} for field, fieldDiff := range resourceDiff.Fields { if fieldDiff.Old != nil && fieldDiff.New == nil { fieldsRemoved = append(fieldsRemoved, field) } } - return fieldsRemoved -} -func resourceSchemaRulesToRuleArray(rss []ResourceSchemaRule) []Rule { - var rules []Rule - for _, rs := range rss { - rules = append(rules, rs) + tmpl := "Field `%s` within resource `%s` was either removed or renamed" + var messages []string + for _, field := range fieldsRemoved { + messages = append(messages, fmt.Sprintf(tmpl, field, resource)) } - return rules -} - -// Name - a human readable name for the rule -func (rs ResourceSchemaRule) Name() string { - return rs.name -} - -// Definition - a definition for the rule -func (rs ResourceSchemaRule) Definition() string { - return rs.definition -} - -// Identifier - a navigation oriented name for the rule -func (rs ResourceSchemaRule) Identifier() string { - return rs.identifier -} - -// Message - a message to to inform the user -// of a breakage. -func (rs ResourceSchemaRule) Message(resource, field string) *BreakingChange { - msg := rs.message - msg = strings.ReplaceAll(msg, "{{resource}}", fmt.Sprintf("`%s`", resource)) - msg = strings.ReplaceAll(msg, "{{field}}", fmt.Sprintf("`%s`", field)) - return &BreakingChange{ - Resource: resource, - Field: field, - Message: msg, - DocumentationReference: constants.GetFileUrl(rs.identifier), - RuleTemplate: rs.message, - RuleDefinition: rs.definition, - RuleName: rs.name, - } -} - -// IsRuleBreak - compares the field entries and returns -// a list of fields violating the rule -func (rs ResourceSchemaRule) IsRuleBreak(resourceDiff diff.ResourceDiff) []string { - if rs.isRuleBreak == nil { - return []string{} - } - return rs.isRuleBreak(resourceDiff) -} - -// Undetectable - informs if there are functions in place -// to detect this rule. -func (rs ResourceSchemaRule) Undetectable() bool { - return rs.isRuleBreak == nil + return messages } diff --git a/tools/diff-processor/rules/rules_resource_schema_test.go b/tools/diff-processor/rules/rules_resource_schema_test.go index da8b488bf69e..e80b67e71b57 100644 --- a/tools/diff-processor/rules/rules_resource_schema_test.go +++ b/tools/diff-processor/rules/rules_resource_schema_test.go @@ -1,17 +1,30 @@ package rules import ( + "sort" + "strings" "testing" "github.com/GoogleCloudPlatform/magic-modules/tools/diff-processor/diff" - "github.com/google/go-cmp/cmp" - "github.com/google/go-cmp/cmp/cmpopts" "github.com/hashicorp/terraform-plugin-sdk/v2/helper/schema" ) -func TestResourceSchemaRule_RemovingAField(t *testing.T) { +func TestRemovingAFieldMessages(t *testing.T) { for _, tc := range resourceSchemaRule_RemovingAField_TestCases { - tc.check(resourceSchemaRule_RemovingAField, t) + gotMessages := RemovingAFieldMessages("resource", tc.resourceDiff) + + if len(gotMessages) != len(tc.expectedFields) { + t.Errorf("RemovingAFieldMessages(%v) got %d messages; want %d", tc.name, len(gotMessages), len(tc.expectedFields)) + continue + } + wantFields := tc.expectedFields + sort.Strings(wantFields) + sort.Strings(gotMessages) + for i, field := range wantFields { + if !strings.Contains(gotMessages[i], field) { + t.Errorf("RemovingAFieldMessages(%v) got message %q; want field %q", tc.name, gotMessages[i], field) + } + } } } @@ -75,11 +88,3 @@ var resourceSchemaRule_RemovingAField_TestCases = []resourceSchemaTestCase{ expectedFields: []string{"field-a", "field-b"}, }, } - -func (tc *resourceSchemaTestCase) check(rule ResourceSchemaRule, t *testing.T) { - fields := rule.IsRuleBreak(tc.resourceDiff) - less := func(a, b string) bool { return a < b } - if !cmp.Equal(fields, tc.expectedFields, cmpopts.SortSlices(less)) { - t.Errorf("Test `%s` failed: wanted %v , got %v", tc.name, tc.expectedFields, fields) - } -} diff --git a/tools/diff-processor/rules/rules_undetectable.go b/tools/diff-processor/rules/rules_undetectable.go deleted file mode 100644 index ea3cb452569c..000000000000 --- a/tools/diff-processor/rules/rules_undetectable.go +++ /dev/null @@ -1,10 +0,0 @@ -package rules - -/* -undetectable - Changing fundamental provider behaviors (e.g. authentication or configuration precedence) - - *? Changing resource import ID format - Changing resource ID format - -*/ diff --git a/tools/diff-processor/rules/utility.go b/tools/diff-processor/rules/utility.go index 78e82b63fcb8..f2f3642a7a26 100644 --- a/tools/diff-processor/rules/utility.go +++ b/tools/diff-processor/rules/utility.go @@ -1,16 +1,9 @@ package rules import ( - "fmt" - - "github.com/GoogleCloudPlatform/magic-modules/tools/diff-processor/constants" "github.com/hashicorp/terraform-plugin-sdk/v2/helper/schema" ) -func documentationReference(identifier string) string { - return fmt.Sprintf(" - [reference](%s)", constants.GetFileUrl(identifier)) -} - func getValueType(valueType schema.ValueType) string { switch valueType { case schema.TypeBool: