Skip to content

Commit

Permalink
Simplified breaking changes logic (GoogleCloudPlatform#11307)
Browse files Browse the repository at this point in the history
  • Loading branch information
@melinath @BBBmau
melinath authored and BBBmau committed Aug 21, 2024
1 parent 7cc92a2 commit e6b3051
Show file tree
Hide file tree
Showing 16 changed files with 244 additions and 730 deletions.
5 changes: 1 addition & 4 deletions docs/content/develop/breaking-changes/breaking-changes.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -84,10 +84,7 @@ For more information, see
* For handwritten resources, removing `DiffSuppressFunc` from a field.
* <a name="field-adding-subfield-to-config-mode-attr"></a> 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
Expand Down
10 changes: 0 additions & 10 deletions tools/diff-processor/constants/constants.go
View file

This file was deleted.

57 changes: 24 additions & 33 deletions tools/diff-processor/rules/breaking_changes.go
View file
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
package rules

import (
"fmt"

"github.com/GoogleCloudPlatform/magic-modules/tools/diff-processor/diff"
)

Expand All @@ -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
}
102 changes: 22 additions & 80 deletions tools/diff-processor/rules/breaking_changes_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand Down Expand Up @@ -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",
},
},
},
Expand All @@ -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",
},
},
},
Expand All @@ -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",
},
},
},
Expand All @@ -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",
},
},
},
Expand All @@ -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",
},
},
},
Expand Down Expand Up @@ -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",
},
},
},
Expand Down Expand Up @@ -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",
},
},
},
Expand Down Expand Up @@ -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",
},
},
},
Expand Down Expand Up @@ -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",
},
},
},
Expand Down
36 changes: 0 additions & 36 deletions tools/diff-processor/rules/message_context.go
View file

This file was deleted.

Loading

0 comments on commit e6b3051

Please sign in to comment.