Merge pull request #10139 from hashicorp/b-schema-diff-destroy

helper/schema: mark diff as forcenew if element is computed

helper/schema/schema.go

@@ -925,6 +925,13 @@ func (m schemaMap) diffSet(
 	oldStr := strconv.Itoa(oldLen)
 	newStr := strconv.Itoa(newLen)
 
+	// Build a schema for our count
+	countSchema := &Schema{
+		Type:     TypeInt,
+		Computed: schema.Computed,
+		ForceNew: schema.ForceNew,
+	}
+
 	// If the set computed then say that the # is computed
 	if computedSet || schema.Computed && !nSet {
 		// If # already exists, equals 0 and no new set is supplied, there
@@ -941,22 +948,16 @@ func (m schemaMap) diffSet(
 			countStr = ""
 		}
 
-		diff.Attributes[k+".#"] = &terraform.ResourceAttrDiff{
+		diff.Attributes[k+".#"] = countSchema.finalizeDiff(&terraform.ResourceAttrDiff{
 			Old:         countStr,
 			NewComputed: true,
-		}
+		})
 		return nil
 	}
 
 	// If the counts are not the same, then record that diff
 	changed := oldLen != newLen
 	if changed || all {
-		countSchema := &Schema{
-			Type:     TypeInt,
-			Computed: schema.Computed,
-			ForceNew: schema.ForceNew,
-		}
-
 		diff.Attributes[k+".#"] = countSchema.finalizeDiff(&terraform.ResourceAttrDiff{
 			Old: oldStr,
 			New: newStr,

helper/schema/schema_test.go

@@ -2608,6 +2608,49 @@ func TestSchemaMap_Diff(t *testing.T) {
 
 			Err: false,
 		},
+
+		{
+			Name: "Set ForceNew marks count as ForceNew if computed",
+			Schema: map[string]*Schema{
+				"ports": &Schema{
+					Type:     TypeSet,
+					Required: true,
+					ForceNew: true,
+					Elem:     &Schema{Type: TypeInt},
+					Set: func(a interface{}) int {
+						return a.(int)
+					},
+				},
+			},
+
+			State: &terraform.InstanceState{
+				Attributes: map[string]string{
+					"ports.#": "3",
+					"ports.1": "1",
+					"ports.2": "2",
+					"ports.4": "4",
+				},
+			},
+
+			Config: map[string]interface{}{
+				"ports": []interface{}{"${var.foo}", 2, 1},
+			},
+
+			ConfigVariables: map[string]ast.Variable{
+				"var.foo": interfaceToVariableSwallowError(config.UnknownVariableValue),
+			},
+
+			Diff: &terraform.InstanceDiff{
+				Attributes: map[string]*terraform.ResourceAttrDiff{
+					"ports.#": &terraform.ResourceAttrDiff{
+						Old:         "3",
+						New:         "",
+						NewComputed: true,
+						RequiresNew: true,
+					},
+				},
+			},
+		},
 	}
 
 	for i, tc := range cases {

terraform/diff.go

@@ -606,13 +606,36 @@ func (d *InstanceDiff) Same(d2 *InstanceDiff) (bool, string) {
 	d.mu.Lock()
 	defer d.mu.Unlock()
 
-	if d.Destroy != d2.GetDestroy() {
+	// If we're going from requiring new to NOT requiring new, then we have
+	// to see if all required news were computed. If so, it is allowed since
+	// computed may also mean "same value and therefore not new".
+	oldNew := d.requiresNew()
+	newNew := d2.RequiresNew()
+	if oldNew && !newNew {
+		oldNew = false
+		for _, rd := range d.Attributes {
+			// 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.
+			if rd != nil && rd.RequiresNew && !rd.NewComputed {
+				oldNew = true
+				break
+			}
+		}
+	}
+
+	if oldNew != newNew {
 		return false, fmt.Sprintf(
-			"diff: Destroy; old: %t, new: %t", d.Destroy, d2.GetDestroy())
+			"diff RequiresNew; old: %t, new: %t", oldNew, newNew)
 	}
-	if d.requiresNew() != d2.RequiresNew() {
+
+	// Verify that destroy matches. The second boolean here allows us to
+	// have mismatching Destroy if we're moving from RequiresNew true
+	// to false above. Therefore, the second boolean will only pass if
+	// we're moving from Destroy: true to false as well.
+	if d.Destroy != d2.GetDestroy() && d.requiresNew() == oldNew {
 		return false, fmt.Sprintf(
-			"diff RequiresNew; old: %t, new: %t", d.requiresNew(), d2.RequiresNew())
+			"diff: Destroy; old: %t, new: %t", d.Destroy, d2.GetDestroy())
 	}
 
 	// Go through the old diff and make sure the new diff has all the

terraform/diff_test.go

@@ -750,6 +750,76 @@ func TestInstanceDiffSame(t *testing.T) {
 			"",
 		},
 
+		// Computed can change RequiresNew by removal, and that's okay
+		{
+			&InstanceDiff{
+				Attributes: map[string]*ResourceAttrDiff{
+					"foo.#": &ResourceAttrDiff{
+						Old:         "0",
+						NewComputed: true,
+						RequiresNew: true,
+					},
+				},
+			},
+			&InstanceDiff{
+				Attributes: map[string]*ResourceAttrDiff{},
+			},
+			true,
+			"",
+		},
+
+		// Computed can change Destroy by removal, and that's okay
+		{
+			&InstanceDiff{
+				Attributes: map[string]*ResourceAttrDiff{
+					"foo.#": &ResourceAttrDiff{
+						Old:         "0",
+						NewComputed: true,
+						RequiresNew: true,
+					},
+				},
+
+				Destroy: true,
+			},
+			&InstanceDiff{
+				Attributes: map[string]*ResourceAttrDiff{},
+			},
+			true,
+			"",
+		},
+
+		// Computed can change Destroy by elements
+		{
+			&InstanceDiff{
+				Attributes: map[string]*ResourceAttrDiff{
+					"foo.#": &ResourceAttrDiff{
+						Old:         "0",
+						NewComputed: true,
+						RequiresNew: true,
+					},
+				},
+
+				Destroy: true,
+			},
+			&InstanceDiff{
+				Attributes: map[string]*ResourceAttrDiff{
+					"foo.#": &ResourceAttrDiff{
+						Old: "1",
+						New: "1",
+					},
+					"foo.12": &ResourceAttrDiff{
+						Old:         "4",
+						New:         "12",
+						RequiresNew: true,
+					},
+				},
+
+				Destroy: true,
+			},
+			true,
+			"",
+		},
+
 		// Computed sets may not contain all fields in the original diff, and
 		// because multiple entries for the same set can compute to the same
 		// hash before the values are computed or interpolated, the overall