Skip to content

Commit fe32f7b

Browse files
mitchellhmitchellh
committed
terraform: ignore RequiresNew for collection removal in diff.Same
Fixes hashicorp#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.
1 parent 640faf1 commit fe32f7b

2 files changed

Lines changed: 96 additions & 10 deletions

File tree

‎terraform/diff.go‎

Lines changed: 39 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -644,7 +644,45 @@ func (d *InstanceDiff) Same(d2 *InstanceDiff) (bool, string) {
644644
newNew := d2.RequiresNew()
645645
if oldNew && !newNew {
646646
oldNew = false
647-
for _, rd := range d.Attributes {
647+
648+
// This section builds a list of ignorable attributes for requiresNew
649+
// by removing off any elements of collections going to zero elements.
650+
// For collections going to zero, they may not exist at all in the
651+
// new diff (and hence RequiresNew == false).
652+
ignoreAttrs := make(map[string]struct{})
653+
for k, diffOld := range d.Attributes {
654+
if !strings.HasSuffix(k, ".%") && !strings.HasSuffix(k, ".#") {
655+
continue
656+
}
657+
658+
// This case is in here as a protection measure. The bug that this
659+
// code originally fixed (GH-11349) didn't have to deal with computed
660+
// so I'm not 100% sure what the correct behavior is. Best to leave
661+
// the old behavior.
662+
if diffOld.NewComputed {
663+
continue
664+
}
665+
666+
// We're looking for the case a map goes to exactly 0.
667+
if diffOld.New != "0" {
668+
continue
669+
}
670+
671+
// Found it! Ignore all of these. The prefix here is stripping
672+
// off the "%" so it is just "k."
673+
prefix := k[:len(k)-1]
674+
for k2, _ := range d.Attributes {
675+
if strings.HasPrefix(k2, prefix) {
676+
ignoreAttrs[k2] = struct{}{}
677+
}
678+
}
679+
}
680+
681+
for k, rd := range d.Attributes {
682+
if _, ok := ignoreAttrs[k]; ok {
683+
continue
684+
}
685+
648686
// If the field is requires new and NOT computed, then what
649687
// we have is a diff mismatch for sure. We set that the old
650688
// diff does REQUIRE a ForceNew.

‎terraform/diff_test.go‎

Lines changed: 57 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,7 @@
11
package terraform
22

33
import (
4+
"fmt"
45
"reflect"
56
"strings"
67
"testing"
@@ -1085,18 +1086,65 @@ func TestInstanceDiffSame(t *testing.T) {
10851086
true,
10861087
"",
10871088
},
1089+
1090+
// When removing all collection items, the diff is allowed to contain
1091+
// nothing when re-creating the resource. This should be the "Same"
1092+
// since we said we were going from 1 to 0.
1093+
{
1094+
&InstanceDiff{
1095+
Attributes: map[string]*ResourceAttrDiff{
1096+
"foo.%": &ResourceAttrDiff{
1097+
Old: "1",
1098+
New: "0",
1099+
RequiresNew: true,
1100+
},
1101+
"foo.bar": &ResourceAttrDiff{
1102+
Old: "baz",
1103+
New: "",
1104+
NewRemoved: true,
1105+
RequiresNew: true,
1106+
},
1107+
},
1108+
},
1109+
&InstanceDiff{},
1110+
true,
1111+
"",
1112+
},
1113+
1114+
{
1115+
&InstanceDiff{
1116+
Attributes: map[string]*ResourceAttrDiff{
1117+
"foo.#": &ResourceAttrDiff{
1118+
Old: "1",
1119+
New: "0",
1120+
RequiresNew: true,
1121+
},
1122+
"foo.0": &ResourceAttrDiff{
1123+
Old: "baz",
1124+
New: "",
1125+
NewRemoved: true,
1126+
RequiresNew: true,
1127+
},
1128+
},
1129+
},
1130+
&InstanceDiff{},
1131+
true,
1132+
"",
1133+
},
10881134
}
10891135

10901136
for i, tc := range cases {
1091-
same, reason := tc.One.Same(tc.Two)
1092-
if same != tc.Same {
1093-
t.Fatalf("%d: expected same: %t, got %t (%s)\n\n one: %#v\n\ntwo: %#v",
1094-
i, tc.Same, same, reason, tc.One, tc.Two)
1095-
}
1096-
if reason != tc.Reason {
1097-
t.Fatalf(
1098-
"%d: bad reason\n\nexpected: %#v\n\ngot: %#v", i, tc.Reason, reason)
1099-
}
1137+
t.Run(fmt.Sprintf("%d", i), func(t *testing.T) {
1138+
same, reason := tc.One.Same(tc.Two)
1139+
if same != tc.Same {
1140+
t.Fatalf("%d: expected same: %t, got %t (%s)\n\n one: %#v\n\ntwo: %#v",
1141+
i, tc.Same, same, reason, tc.One, tc.Two)
1142+
}
1143+
if reason != tc.Reason {
1144+
t.Fatalf(
1145+
"%d: bad reason\n\nexpected: %#v\n\ngot: %#v", i, tc.Reason, reason)
1146+
}
1147+
})
11001148
}
11011149
}
11021150

0 commit comments

Comments
 (0)