From 8d1e479fc7ae87a26a048546e8d2050830f8e6f3 Mon Sep 17 00:00:00 2001 From: James Bardin Date: Wed, 17 Jan 2018 15:35:02 -0500 Subject: [PATCH] don't ignore partial containers in diffs Containers (maps, lists, sets) in an InstanceDiff need to be handled in their entirety. Unchanged values cannot be filtered out from diffs, as providers expect attribute containers to be complete. If a value in ignore_changes maps to a single key in an attribute container, and there are other changes present, that ignored value must be included in the diff as well. --- terraform/diff.go | 5 -- terraform/eval_diff.go | 27 ++++++--- terraform/eval_diff_test.go | 106 +++++++++++++++++++++++------------- 3 files changed, 87 insertions(+), 51 deletions(-) diff --git a/terraform/diff.go b/terraform/diff.go index b6651c0a8..d6dc55061 100644 --- a/terraform/diff.go +++ b/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 diff --git a/terraform/eval_diff.go b/terraform/eval_diff.go index c1def9166..26205ce51 100644 --- a/terraform/eval_diff.go +++ b/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 - } + 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 } } diff --git a/terraform/eval_diff_test.go b/terraform/eval_diff_test.go index 0da3b0ee6..b1f76ea3d 100644 --- a/terraform/eval_diff_test.go +++ b/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") - 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)) - } - - 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)) + 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) != 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) + } + } + }) } }