From 39542898b0009bc9db1263ef7b331013a15799eb Mon Sep 17 00:00:00 2001 From: Mitchell Hashimoto Date: Tue, 15 Nov 2016 11:02:14 -0800 Subject: [PATCH 1/2] helper/schema: mark diff as forcenew if element is computed Fixes #10125 If the elements are computed and the field is ForceNew, then we should mark the computed count as potentially forcing a new operation. Example, assuming `groups` forces new... **Step 1:** groups = ["1", "2", "3"] At this point, the resource isn't create, so this should result in a diff like: CREATE resource: groups: "" => ["1", "2", "3"] **Step 2:** groups = ["${computedvar}"] The OLD behavior was: UPDATE resource groups.#: "3" => "computed" This would cause a diff mismatch because if `${computedvar}` was different then it should force new. The NEW behavior is: DESTROY/CREATE resource: groups.#: "3" => "computed" (forces new) --- helper/schema/schema.go | 17 +++++++------- helper/schema/schema_test.go | 43 ++++++++++++++++++++++++++++++++++++ 2 files changed, 52 insertions(+), 8 deletions(-) diff --git a/helper/schema/schema.go b/helper/schema/schema.go index 4a2fd1714..07bdae680 100644 --- a/helper/schema/schema.go +++ b/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, diff --git a/helper/schema/schema_test.go b/helper/schema/schema_test.go index b07892867..94b6b4dab 100644 --- a/helper/schema/schema_test.go +++ b/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 { From 55ef93f0f98f1b831d4326eb4540d79d73a38468 Mon Sep 17 00:00:00 2001 From: Mitchell Hashimoto Date: Tue, 15 Nov 2016 11:26:42 -0800 Subject: [PATCH 2/2] terraform: Diff.Same should understand that Destroy might go false --- terraform/diff.go | 33 +++++++++++++++++--- terraform/diff_test.go | 70 ++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 98 insertions(+), 5 deletions(-) diff --git a/terraform/diff.go b/terraform/diff.go index a474efad1..002f72921 100644 --- a/terraform/diff.go +++ b/terraform/diff.go @@ -606,14 +606,37 @@ 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 RequiresNew; old: %t, new: %t", oldNew, newNew) + } + + // 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: Destroy; old: %t, new: %t", d.Destroy, d2.GetDestroy()) } - if d.requiresNew() != d2.RequiresNew() { - return false, fmt.Sprintf( - "diff RequiresNew; old: %t, new: %t", d.requiresNew(), d2.RequiresNew()) - } // Go through the old diff and make sure the new diff has all the // same attributes. To start, build up the check map to be all the keys. diff --git a/terraform/diff_test.go b/terraform/diff_test.go index 655b165f9..ae5ce6da4 100644 --- a/terraform/diff_test.go +++ b/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