From b4df304b47ab6d9caf7fb6b1209de68db51981ef Mon Sep 17 00:00:00 2001 From: Paul Hinze Date: Thu, 5 May 2016 09:00:58 -0500 Subject: [PATCH] helper/schema: Normalize bools to "true"/"false" in diffs For a long time now, the diff logic has relied on the behavior of `mapstructure.WeakDecode` to determine how various primitives are converted into strings. The `schema.DiffString` function is used for all primitive field types: TypeBool, TypeInt, TypeFloat, and TypeString. The `mapstructure` library's string representation of booleans is "0" and "1", which differs from `strconv.FormatBool`'s "false" and "true" (which is used in writing out boolean fields to the state). Because of this difference, diffs have long had the potential for cosmetically odd but semantically neutral output like: "true" => "1" "false" => "0" So long as `mapstructure.Decode` or `strconv.ParseBool` are used to interpret these strings, there's no functional problem. We had our first clear functional problem with #6005 and friends, where users noticed diffs like the above showing up unexpectedly and causing troubles when `ignore_changes` was in play. This particular bug occurs down in Terraform core's EvalIgnoreChanges. There, the diff is modified to account for ignored attributes, and special logic attempts to handle properly the situation where the ignored attribute was going to trigger a resource replacement. That logic relies on the string representations of the Old and New fields in the diff to be the same so that it filters properly. So therefore, we now get a bug when a diff includes `Old: "0", New: "false"` since the strings do not match, and `ignore_changes` is not properly handled. Here, we introduce `TypeBool`-specific normalizing into `finalizeDiff`. I spiked out a full `diffBool` function, but figuring out which pieces of `diffString` to duplicate there got hairy. This seemed like a simpler and more direct solution. Fixes #6005 (and potentially others!) --- builtin/providers/test/resource.go | 4 ++ builtin/providers/test/resource_test.go | 41 +++++++++++++++++++ helper/schema/schema.go | 14 +++++++ helper/schema/schema_test.go | 52 ++++++++++++++++++++++++- 4 files changed, 110 insertions(+), 1 deletion(-) diff --git a/builtin/providers/test/resource.go b/builtin/providers/test/resource.go index 9df02bc15..549f7271c 100644 --- a/builtin/providers/test/resource.go +++ b/builtin/providers/test/resource.go @@ -21,6 +21,10 @@ func testResource() *schema.Resource { Type: schema.TypeString, Optional: true, }, + "optional_bool": &schema.Schema{ + Type: schema.TypeBool, + Optional: true, + }, "optional_force_new": &schema.Schema{ Type: schema.TypeString, Optional: true, diff --git a/builtin/providers/test/resource_test.go b/builtin/providers/test/resource_test.go index 6b12227f9..59b4c4785 100644 --- a/builtin/providers/test/resource_test.go +++ b/builtin/providers/test/resource_test.go @@ -124,6 +124,47 @@ resource "test_resource" "foo" { }) } +// Covers specific scenario in #6005, handled by normalizing boolean strings in +// helper/schema +func TestResource_ignoreChangesForceNewBoolean(t *testing.T) { + resource.UnitTest(t, resource.TestCase{ + Providers: testAccProviders, + CheckDestroy: testAccCheckResourceDestroy, + Steps: []resource.TestStep{ + resource.TestStep{ + Config: strings.TrimSpace(` +resource "test_resource" "foo" { + required = "yep" + optional_force_new = "one" + optional_bool = true + lifecycle { + ignore_changes = ["optional_force_new"] + } +} + `), + Check: func(s *terraform.State) error { + return nil + }, + }, + resource.TestStep{ + Config: strings.TrimSpace(` +resource "test_resource" "foo" { + required = "yep" + optional_force_new = "two" + optional_bool = true + lifecycle { + ignore_changes = ["optional_force_new"] + } +} + `), + Check: func(s *terraform.State) error { + return nil + }, + }, + }, + }) +} + func TestResource_ignoreChangesMap(t *testing.T) { resource.UnitTest(t, resource.TestCase{ Providers: testAccProviders, diff --git a/helper/schema/schema.go b/helper/schema/schema.go index cfe3c6a33..43b5cc21a 100644 --- a/helper/schema/schema.go +++ b/helper/schema/schema.go @@ -243,6 +243,20 @@ func (s *Schema) finalizeDiff( return d } + if s.Type == TypeBool { + normalizeBoolString := func(s string) string { + switch s { + case "0": + return "false" + case "1": + return "true" + } + return s + } + d.Old = normalizeBoolString(d.Old) + d.New = normalizeBoolString(d.New) + } + if d.NewRemoved { return d } diff --git a/helper/schema/schema_test.go b/helper/schema/schema_test.go index 94c9c57ff..dba10ee34 100644 --- a/helper/schema/schema_test.go +++ b/helper/schema/schema_test.go @@ -491,7 +491,7 @@ func TestSchemaMap_Diff(t *testing.T) { Attributes: map[string]*terraform.ResourceAttrDiff{ "port": &terraform.ResourceAttrDiff{ Old: "", - New: "0", + New: "false", RequiresNew: true, }, }, @@ -2344,6 +2344,56 @@ func TestSchemaMap_Diff(t *testing.T) { Err: false, }, + + "Bools can be set with 0/1 in config, still get true/false": { + Schema: map[string]*Schema{ + "one": &Schema{ + Type: TypeBool, + Optional: true, + }, + "two": &Schema{ + Type: TypeBool, + Optional: true, + }, + "three": &Schema{ + Type: TypeBool, + Optional: true, + }, + }, + + State: &terraform.InstanceState{ + Attributes: map[string]string{ + "one": "false", + "two": "true", + "three": "true", + }, + }, + + Config: map[string]interface{}{ + "one": "1", + "two": "0", + }, + + Diff: &terraform.InstanceDiff{ + Attributes: map[string]*terraform.ResourceAttrDiff{ + "one": &terraform.ResourceAttrDiff{ + Old: "false", + New: "true", + }, + "two": &terraform.ResourceAttrDiff{ + Old: "true", + New: "false", + }, + "three": &terraform.ResourceAttrDiff{ + Old: "true", + New: "false", + NewRemoved: true, + }, + }, + }, + + Err: false, + }, } for tn, tc := range cases {