Merge pull request #17127 from hashicorp/jbardin/ignore_changes

don't ignore partial containers in diffs

terraform/diff.go

@@ -396,11 +396,6 @@ type ResourceAttrDiff struct {
 	Type        DiffAttrType
 }
 
-// Modified returns the inequality of Old and New for this attr
-func (d *ResourceAttrDiff) Modified() bool {
-	return d.Old != d.New
-}
-
 // Empty returns true if the diff for this attr is neutral
 func (d *ResourceAttrDiff) Empty() bool {
 	return d.Old == d.New && !d.NewComputed && !d.NewRemoved

terraform/eval_diff.go

@@ -256,13 +256,15 @@ func (n *EvalDiff) processIgnoreChanges(diff *InstanceDiff) error {
 		containers := groupContainers(diff)
 		keep := map[string]bool{}
 		for _, v := range containers {
-			if v.keepDiff() {
+			if v.keepDiff(ignorableAttrKeys) {
 				// At least one key has changes, so list all the sibling keys
-				// to keep in the diff if any values have changed
+				// to keep in the diff
 				for k := range v {
-					if v[k].Modified() {
 					keep[k] = true
-					}
+					// this key may have been added by the user to ignore, but
+					// if it's a subkey in a container, we need to un-ignore it
+					// to keep the complete containter.
+					delete(ignorableAttrKeys, k)
 				}
 			}
 		}
@@ -294,10 +296,17 @@ func (n *EvalDiff) processIgnoreChanges(diff *InstanceDiff) error {
 // a group of key-*ResourceAttrDiff pairs from the same flatmapped container
 type flatAttrDiff map[string]*ResourceAttrDiff
 
-// we need to keep all keys if any of them have a diff
-func (f flatAttrDiff) keepDiff() bool {
-	for _, v := range f {
-		if !v.Empty() && !v.NewComputed {
+// we need to keep all keys if any of them have a diff that's not ignored
+func (f flatAttrDiff) keepDiff(ignoreChanges map[string]bool) bool {
+	for k, v := range f {
+		ignore := false
+		for attr := range ignoreChanges {
+			if strings.HasPrefix(k, attr) {
+				ignore = true
+			}
+		}
+
+		if !v.Empty() && !v.NewComputed && !ignore {
 			return true
 		}
 	}

terraform/eval_diff_test.go

@@ -1,6 +1,7 @@
 package terraform
 
 import (
+	"fmt"
 	"reflect"
 	"testing"
 
@@ -79,7 +80,7 @@ func TestEvalFilterDiff(t *testing.T) {
 	}
 }
 
-func TestProcessIgnoreChangesOnResourceIgnoredWithRequiresNew(t *testing.T) {
+func TestProcessIgnoreChanges(t *testing.T) {
 	var evalDiff *EvalDiff
 	var instanceDiff *InstanceDiff
 
@@ -94,53 +95,84 @@ func TestProcessIgnoreChangesOnResourceIgnoredWithRequiresNew(t *testing.T) {
 			&InstanceDiff{
 				Destroy: true,
 				Attributes: map[string]*ResourceAttrDiff{
+					"resource.%": {
+						Old: "3",
+						New: "3",
+					},
 					"resource.changed": {
 						RequiresNew: true,
 						Type:        DiffAttrInput,
 						Old:         "old",
 						New:         "new",
 					},
-					"resource.unchanged": {
-						Old: "unchanged",
+					"resource.maybe": {
+						Old: "",
 						New: newAttribute,
 					},
+					"resource.same": {
+						Old: "same",
+						New: "same",
+					},
 				},
 			}
 	}
 
-	evalDiff, instanceDiff = testDiffs([]string{"resource.changed"}, "unchanged")
+	for i, tc := range []struct {
+		ignore    []string
+		newAttr   string
+		attrDiffs int
+	}{
+		// attr diffs should be all (4), or nothing
+		{
+			ignore:    []string{"resource.changed"},
+			attrDiffs: 0,
+		},
+		{
+			ignore:    []string{"resource.changed"},
+			newAttr:   "new",
+			attrDiffs: 4,
+		},
+		{
+			attrDiffs: 4,
+		},
+		{
+			ignore:    []string{"resource.maybe"},
+			newAttr:   "new",
+			attrDiffs: 4,
+		},
+		{
+			newAttr:   "new",
+			attrDiffs: 4,
+		},
+		{
+			ignore:    []string{"resource"},
+			newAttr:   "new",
+			attrDiffs: 0,
+		},
+		// extra ignored values shouldn't effect the diff
+		{
+			ignore:    []string{"resource.missing", "resource.maybe"},
+			newAttr:   "new",
+			attrDiffs: 4,
+		},
+		// this isn't useful, but make sure it doesn't break
+		{
+			ignore:    []string{"resource.%"},
+			attrDiffs: 4,
+		},
+	} {
+		t.Run(fmt.Sprintf("%d", i), func(t *testing.T) {
+			evalDiff, instanceDiff = testDiffs(tc.ignore, tc.newAttr)
 			err := evalDiff.processIgnoreChanges(instanceDiff)
 			if err != nil {
 				t.Fatalf("err: %s", err)
 			}
-	if len(instanceDiff.Attributes) > 0 {
-		t.Fatalf("Expected all resources to be ignored, found %d", len(instanceDiff.Attributes))
+			if len(instanceDiff.Attributes) != tc.attrDiffs {
+				t.Errorf("expected %d diffs, found %d", tc.attrDiffs, len(instanceDiff.Attributes))
+				for k, attr := range instanceDiff.Attributes {
+					fmt.Printf("  %s:%#v\n", k, attr)
 				}
-
-	evalDiff, instanceDiff = testDiffs([]string{}, "unchanged")
-	err = evalDiff.processIgnoreChanges(instanceDiff)
-	if err != nil {
-		t.Fatalf("err: %s", err)
 			}
-	if len(instanceDiff.Attributes) != 2 {
-		t.Fatalf("Expected 2 resources to be found, found %d", len(instanceDiff.Attributes))
-	}
-
-	evalDiff, instanceDiff = testDiffs([]string{"resource.changed"}, "changed")
-	err = evalDiff.processIgnoreChanges(instanceDiff)
-	if err != nil {
-		t.Fatalf("err: %s", err)
-	}
-	if len(instanceDiff.Attributes) != 1 {
-		t.Fatalf("Expected 1 resource to be found, found %d", len(instanceDiff.Attributes))
-	}
-
-	evalDiff, instanceDiff = testDiffs([]string{}, "changed")
-	err = evalDiff.processIgnoreChanges(instanceDiff)
-	if err != nil {
-		t.Fatalf("err: %s", err)
-	}
-	if len(instanceDiff.Attributes) != 2 {
-		t.Fatalf("Expected 2 resource to be found, found %d", len(instanceDiff.Attributes))
+		})
 	}
 }

terraform/terraform_test.go

@@ -1675,7 +1675,10 @@ aws_instance.foo:
 const testTFPlanDiffIgnoreChangesWithFlatmaps = `
 UPDATE: aws_instance.foo
   lst.#:   "1" => "2"
+  lst.0:   "j" => "j"
   lst.1:   "" => "k"
+  set.#:   "1" => "1"
+  set.0.a: "1" => "1"
   set.0.b: "" => "2"
   type:    "" => "aws_instance"
 `