terraform: ignore RequiresNew for collection removal in diff.Same

Fixes #11349

I tracked this bug back to the early 0.7 days so this has been around a
really long time. I wanted to confirm that this wasn't introduced by any
new graph changes and it appears to predate all of that. I couldn't find
a single 0.7.x release where this worked, and I didn't want to go back
to 0.6.x since it was pre-vendoring.

The test case shows the logic the best, but the basic idea is: for
collections that go to zero elements, the "RequiresNew" sameness check
should be ignored, since the new diff can choose to not have that at all
in the diff.
This commit is contained in:
Mitchell Hashimoto 2017-02-06 17:40:44 -08:00
parent 640faf18c3
commit fe32f7b189
No known key found for this signature in database
GPG Key ID: 744E147AA52F5B0A
2 changed files with 96 additions and 10 deletions

View File

@ -644,7 +644,45 @@ func (d *InstanceDiff) Same(d2 *InstanceDiff) (bool, string) {
newNew := d2.RequiresNew()
if oldNew && !newNew {
oldNew = false
for _, rd := range d.Attributes {
// This section builds a list of ignorable attributes for requiresNew
// by removing off any elements of collections going to zero elements.
// For collections going to zero, they may not exist at all in the
// new diff (and hence RequiresNew == false).
ignoreAttrs := make(map[string]struct{})
for k, diffOld := range d.Attributes {
if !strings.HasSuffix(k, ".%") && !strings.HasSuffix(k, ".#") {
continue
}
// This case is in here as a protection measure. The bug that this
// code originally fixed (GH-11349) didn't have to deal with computed
// so I'm not 100% sure what the correct behavior is. Best to leave
// the old behavior.
if diffOld.NewComputed {
continue
}
// We're looking for the case a map goes to exactly 0.
if diffOld.New != "0" {
continue
}
// Found it! Ignore all of these. The prefix here is stripping
// off the "%" so it is just "k."
prefix := k[:len(k)-1]
for k2, _ := range d.Attributes {
if strings.HasPrefix(k2, prefix) {
ignoreAttrs[k2] = struct{}{}
}
}
}
for k, rd := range d.Attributes {
if _, ok := ignoreAttrs[k]; ok {
continue
}
// If the field is requires new and NOT computed, then what
// we have is a diff mismatch for sure. We set that the old
// diff does REQUIRE a ForceNew.

View File

@ -1,6 +1,7 @@
package terraform
import (
"fmt"
"reflect"
"strings"
"testing"
@ -1085,18 +1086,65 @@ func TestInstanceDiffSame(t *testing.T) {
true,
"",
},
// When removing all collection items, the diff is allowed to contain
// nothing when re-creating the resource. This should be the "Same"
// since we said we were going from 1 to 0.
{
&InstanceDiff{
Attributes: map[string]*ResourceAttrDiff{
"foo.%": &ResourceAttrDiff{
Old: "1",
New: "0",
RequiresNew: true,
},
"foo.bar": &ResourceAttrDiff{
Old: "baz",
New: "",
NewRemoved: true,
RequiresNew: true,
},
},
},
&InstanceDiff{},
true,
"",
},
{
&InstanceDiff{
Attributes: map[string]*ResourceAttrDiff{
"foo.#": &ResourceAttrDiff{
Old: "1",
New: "0",
RequiresNew: true,
},
"foo.0": &ResourceAttrDiff{
Old: "baz",
New: "",
NewRemoved: true,
RequiresNew: true,
},
},
},
&InstanceDiff{},
true,
"",
},
}
for i, tc := range cases {
same, reason := tc.One.Same(tc.Two)
if same != tc.Same {
t.Fatalf("%d: expected same: %t, got %t (%s)\n\n one: %#v\n\ntwo: %#v",
i, tc.Same, same, reason, tc.One, tc.Two)
}
if reason != tc.Reason {
t.Fatalf(
"%d: bad reason\n\nexpected: %#v\n\ngot: %#v", i, tc.Reason, reason)
}
t.Run(fmt.Sprintf("%d", i), func(t *testing.T) {
same, reason := tc.One.Same(tc.Two)
if same != tc.Same {
t.Fatalf("%d: expected same: %t, got %t (%s)\n\n one: %#v\n\ntwo: %#v",
i, tc.Same, same, reason, tc.One, tc.Two)
}
if reason != tc.Reason {
t.Fatalf(
"%d: bad reason\n\nexpected: %#v\n\ngot: %#v", i, tc.Reason, reason)
}
})
}
}