From 75602df5ef5f1bf98b7784385fc30531c157455d Mon Sep 17 00:00:00 2001 From: James Bardin Date: Fri, 28 Jun 2019 11:51:52 -0400 Subject: [PATCH 1/2] Revert "Remove removed attribute from applied state" This reverts commit 2e2a3630528d00c9b6899145f34c9ed04a390300. --- builtin/providers/test/resource.go | 7 +--- builtin/providers/test/resource_test.go | 49 ------------------------- helper/plugin/grpc_provider.go | 19 +--------- 3 files changed, 2 insertions(+), 73 deletions(-) diff --git a/builtin/providers/test/resource.go b/builtin/providers/test/resource.go index 28c5c79a14..09732d421d 100644 --- a/builtin/providers/test/resource.go +++ b/builtin/providers/test/resource.go @@ -202,13 +202,8 @@ func testResourceRead(d *schema.ResourceData, meta interface{}) error { d.Set("set", []interface{}{}) } - _, ok := d.GetOk("optional_computed") - if !ok { - d.Set("optional_computed", "computed") - } - // This should not show as set unless it's set in the config - _, ok = d.GetOkExists("get_ok_exists_false") + _, ok := d.GetOkExists("get_ok_exists_false") if ok { return errors.New("get_ok_exists_false should not be set") } diff --git a/builtin/providers/test/resource_test.go b/builtin/providers/test/resource_test.go index afcbf7967a..8b78bb4155 100644 --- a/builtin/providers/test/resource_test.go +++ b/builtin/providers/test/resource_test.go @@ -36,55 +36,6 @@ resource "test_resource" "foo" { }) } -func TestResource_removedOptional(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" - required_map = { - key = "value" - } - int = 1 - optional = "string" - optional_computed = "not computed" - optional_bool = true -} - `), - }, - resource.TestStep{ - Config: strings.TrimSpace(` -resource "test_resource" "foo" { - required = "yep" - required_map = { - key = "value" - } -} - `), - Check: resource.ComposeTestCheckFunc( - resource.TestCheckNoResourceAttr( - "test_resource.foo", "int", - ), - resource.TestCheckNoResourceAttr( - "test_resource.foo", "string", - ), - resource.TestCheckNoResourceAttr( - "test_resource.foo", "optional_bool", - ), - // while this isn't optimal, the prior config value being - // left for optional+computed is expected - resource.TestCheckResourceAttr( - "test_resource.foo", "optional_computed", "not computed", - ), - ), - }, - }, - }) -} - func TestResource_changedList(t *testing.T) { resource.UnitTest(t, resource.TestCase{ Providers: testAccProviders, diff --git a/helper/plugin/grpc_provider.go b/helper/plugin/grpc_provider.go index 9584beef1b..104c8f5f47 100644 --- a/helper/plugin/grpc_provider.go +++ b/helper/plugin/grpc_provider.go @@ -5,7 +5,6 @@ import ( "fmt" "log" "strconv" - "strings" "github.com/zclconf/go-cty/cty" ctyconvert "github.com/zclconf/go-cty/cty/convert" @@ -868,7 +867,6 @@ func (s *GRPCProviderServer) ApplyResourceChange(_ context.Context, req *proto.A diff.Meta = private } - var newRemoved []string for k, d := range diff.Attributes { // We need to turn off any RequiresNew. There could be attributes // without changes in here inserted by helper/schema, but if they have @@ -876,10 +874,8 @@ func (s *GRPCProviderServer) ApplyResourceChange(_ context.Context, req *proto.A d.RequiresNew = false // Check that any "removed" attributes that don't actually exist in the - // prior state, or helper/schema will confuse itself, and record them - // to make sure they are actually removed from the state. + // prior state, or helper/schema will confuse itself if d.NewRemoved { - newRemoved = append(newRemoved, k) if _, ok := priorState.Attributes[k]; !ok { delete(diff.Attributes, k) } @@ -908,19 +904,6 @@ func (s *GRPCProviderServer) ApplyResourceChange(_ context.Context, req *proto.A return resp, nil } - // Now remove any primitive zero values that were left from NewRemoved - // attributes. Any attempt to reconcile more complex structures to the best - // of our abilities happens in normalizeNullValues. - for _, r := range newRemoved { - if strings.HasSuffix(r, ".#") || strings.HasSuffix(r, ".%") { - continue - } - switch newInstanceState.Attributes[r] { - case "", "0", "false": - delete(newInstanceState.Attributes, r) - } - } - // We keep the null val if we destroyed the resource, otherwise build the // entire object, even if the new state was nil. newStateVal, err = schema.StateValueFromInstanceState(newInstanceState, schemaBlock.ImpliedType()) From 0d2363a058a0b567d2e7d4971df7bfe5238d8654 Mon Sep 17 00:00:00 2001 From: James Bardin Date: Fri, 28 Jun 2019 12:09:50 -0400 Subject: [PATCH 2/2] add tests to preserve existing Set nil behavior While it may not be intuitive, providers expect that setting a `nil` value will appear as an empty string in state. --- builtin/providers/test/resource.go | 8 ++++++ builtin/providers/test/resource_test.go | 36 +++++++++++++++++++++++++ 2 files changed, 44 insertions(+) diff --git a/builtin/providers/test/resource.go b/builtin/providers/test/resource.go index 09732d421d..b05fcc681e 100644 --- a/builtin/providers/test/resource.go +++ b/builtin/providers/test/resource.go @@ -202,6 +202,14 @@ func testResourceRead(d *schema.ResourceData, meta interface{}) error { d.Set("set", []interface{}{}) } + // This mimics many providers always setting a *string value. + // The existing behavior is that this will appear in the state as an empty + // string, which we have to maintain. + o := d.Get("optional") + if o == "" { + d.Set("optional", nil) + } + // This should not show as set unless it's set in the config _, ok := d.GetOkExists("get_ok_exists_false") if ok { diff --git a/builtin/providers/test/resource_test.go b/builtin/providers/test/resource_test.go index 8b78bb4155..9277e9f629 100644 --- a/builtin/providers/test/resource_test.go +++ b/builtin/providers/test/resource_test.go @@ -1050,3 +1050,39 @@ resource "test_resource" "foo" { }, }) } + +func TestResource_unsetNil(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" + required_map = { + key = "value" + } + optional = "a" +} + `), + Check: resource.ComposeTestCheckFunc( + resource.TestCheckResourceAttr("test_resource.foo", "optional", "a"), + ), + }, + resource.TestStep{ + Config: strings.TrimSpace(` +resource "test_resource" "foo" { + required = "yep" + required_map = { + key = "value" + } +} + `), + Check: resource.ComposeTestCheckFunc( + resource.TestCheckResourceAttr("test_resource.foo", "optional", ""), + ), + }, + }, + }) +}