Merge pull request #19988 from hashicorp/jbardin/refresh-shims

Don't add empty blocks to config, and fix ReadResource

builtin/providers/test/provider.go

@@ -26,6 +26,7 @@ func Provider() terraform.ResourceProvider {
 			"test_resource_nested":           testResourceNested(),
 			"test_resource_nested_set":       testResourceNestedSet(),
 			"test_resource_state_func":       testResourceStateFunc(),
+			"test_resource_deprecated":       testResourceDeprecated(),
 		},
 		DataSourcesMap: map[string]*schema.Resource{
 			"test_data_source":    testDataSource(),

builtin/providers/test/resource_deprecated.go

@@ -0,0 +1,119 @@
+package test
+
+import (
+	"github.com/hashicorp/terraform/helper/schema"
+)
+
+func testResourceDeprecated() *schema.Resource {
+	return &schema.Resource{
+		Create: testResourceDeprecatedCreate,
+		Read:   testResourceDeprecatedRead,
+		Update: testResourceDeprecatedUpdate,
+		Delete: testResourceDeprecatedDelete,
+
+		Schema: map[string]*schema.Schema{
+			"map_deprecated": {
+				Type:       schema.TypeMap,
+				Optional:   true,
+				Deprecated: "deprecated",
+			},
+			"map_removed": {
+				Type:     schema.TypeMap,
+				Optional: true,
+				Removed:  "removed",
+			},
+			"set_block_deprecated": {
+				Type:       schema.TypeSet,
+				Optional:   true,
+				MaxItems:   1,
+				Deprecated: "deprecated",
+				Elem: &schema.Resource{
+					Schema: map[string]*schema.Schema{
+						"value": {
+							Type:       schema.TypeString,
+							Required:   true,
+							Deprecated: "deprecated",
+						},
+						"optional": {
+							Type:       schema.TypeString,
+							ForceNew:   true,
+							Optional:   true,
+							Deprecated: "deprecated",
+						},
+					},
+				},
+			},
+			"set_block_removed": {
+				Type:     schema.TypeSet,
+				Optional: true,
+				MaxItems: 1,
+				Removed:  "Removed",
+				Elem: &schema.Resource{
+					Schema: map[string]*schema.Schema{
+						"optional": {
+							Type:     schema.TypeString,
+							ForceNew: true,
+							Optional: true,
+							Computed: true,
+							Removed:  "removed",
+						},
+					},
+				},
+			},
+			"list_block_deprecated": {
+				Type:       schema.TypeList,
+				Optional:   true,
+				Deprecated: "deprecated",
+				Elem: &schema.Resource{
+					Schema: map[string]*schema.Schema{
+						"value": {
+							Type:       schema.TypeString,
+							Required:   true,
+							Deprecated: "deprecated",
+						},
+						"optional": {
+							Type:       schema.TypeString,
+							ForceNew:   true,
+							Optional:   true,
+							Deprecated: "deprecated",
+						},
+					},
+				},
+			},
+			"list_block_removed": {
+				Type:     schema.TypeList,
+				Optional: true,
+				Removed:  "removed",
+				Elem: &schema.Resource{
+					Schema: map[string]*schema.Schema{
+						"optional": {
+							Type:     schema.TypeString,
+							ForceNew: true,
+							Optional: true,
+							Removed:  "removed",
+						},
+					},
+				},
+			},
+		},
+	}
+}
+
+func testResourceDeprecatedCreate(d *schema.ResourceData, meta interface{}) error {
+	d.SetId("testId")
+	return nil
+}
+
+func testResourceDeprecatedRead(d *schema.ResourceData, meta interface{}) error {
+
+	return nil
+}
+
+func testResourceDeprecatedUpdate(d *schema.ResourceData, meta interface{}) error {
+	return nil
+}
+
+func testResourceDeprecatedDelete(d *schema.ResourceData, meta interface{}) error {
+	d.SetId("")
+	return nil
+}

builtin/providers/test/resource_deprecated_test.go

@@ -0,0 +1,71 @@
+package test
+
+import (
+	"regexp"
+	"strings"
+	"testing"
+
+	"github.com/hashicorp/terraform/helper/resource"
+)
+
+// an empty config should be ok, because no deprecated/removed fields are set.
+func TestResourceDeprecated_empty(t *testing.T) {
+	resource.UnitTest(t, resource.TestCase{
+		Providers:    testAccProviders,
+		CheckDestroy: testAccCheckResourceDestroy,
+		Steps: []resource.TestStep{
+			resource.TestStep{
+				Config: strings.TrimSpace(`
+resource "test_resource_deprecated" "foo" {
+}
+				`),
+			},
+		},
+	})
+}
+
+// Deprecated fields should still work
+func TestResourceDeprecated_deprecatedOK(t *testing.T) {
+	resource.UnitTest(t, resource.TestCase{
+		Providers:    testAccProviders,
+		CheckDestroy: testAccCheckResourceDestroy,
+		Steps: []resource.TestStep{
+			resource.TestStep{
+				Config: strings.TrimSpace(`
+resource "test_resource_deprecated" "foo" {
+	map_deprecated = {
+		"a" = "b",
+	}
+	set_block_deprecated {
+		value = "1"
+	}
+	list_block_deprecated {
+		value = "2"
+	}
+}
+				`),
+			},
+		},
+	})
+}
+
+// Declaring an empty block should trigger the error
+func TestResourceDeprecated_removedBlocks(t *testing.T) {
+	resource.UnitTest(t, resource.TestCase{
+		Providers:    testAccProviders,
+		CheckDestroy: testAccCheckResourceDestroy,
+		Steps: []resource.TestStep{
+			resource.TestStep{
+				Config: strings.TrimSpace(`
+resource "test_resource_deprecated" "foo" {
+	set_block_removed {
+	}
+	list_block_removed {
+	}
+}
+				`),
+				ExpectError: regexp.MustCompile("REMOVED"),
+			},
+		},
+	})
+}

config/hcl2shim/values.go

@@ -80,6 +80,11 @@ func ConfigValueFromHCL2Block(v cty.Value, schema *configschema.Block) map[strin
 
 		case configschema.NestingList, configschema.NestingSet:
 			l := bv.LengthInt()
+			if l == 0 {
+				// skip empty collections to better mimic how HCL1 would behave
+				continue
+			}
+
 			elems := make([]interface{}, 0, l)
 			for it := bv.ElementIterator(); it.Next(); {
 				_, ev := it.Element()
@@ -92,6 +97,11 @@ func ConfigValueFromHCL2Block(v cty.Value, schema *configschema.Block) map[strin
 			ret[name] = elems
 
 		case configschema.NestingMap:
+			if bv.LengthInt() == 0 {
+				// skip empty collections to better mimic how HCL1 would behave
+				continue
+			}
+
 			elems := make(map[string]interface{})
 			for it := bv.ElementIterator(); it.Next(); {
 				ek, ev := it.Element()

config/hcl2shim/values_test.go

@@ -151,7 +151,7 @@ func TestConfigValueFromHCL2Block(t *testing.T) {
 		},
 		{
 			cty.ObjectVal(map[string]cty.Value{
-				"address": cty.ListValEmpty(cty.EmptyObject),
+				"address": cty.ListValEmpty(cty.EmptyObject), // should be omitted altogether in result
 			}),
 			&configschema.Block{
 				BlockTypes: map[string]*configschema.NestedBlock{
@@ -161,9 +161,7 @@ func TestConfigValueFromHCL2Block(t *testing.T) {
 					},
 				},
 			},
-			map[string]interface{}{
-				"address": []interface{}{},
-			},
+			map[string]interface{}{},
 		},
 		{
 			cty.ObjectVal(map[string]cty.Value{
@@ -195,9 +193,7 @@ func TestConfigValueFromHCL2Block(t *testing.T) {
 					},
 				},
 			},
-			map[string]interface{}{
-				"address": []interface{}{},
-			},
+			map[string]interface{}{},
 		},
 		{
 			cty.ObjectVal(map[string]cty.Value{
@@ -229,9 +225,7 @@ func TestConfigValueFromHCL2Block(t *testing.T) {
 					},
 				},
 			},
-			map[string]interface{}{
-				"address": map[string]interface{}{},
-			},
+			map[string]interface{}{},
 		},
 		{
 			cty.NullVal(cty.EmptyObject),
@@ -240,8 +234,8 @@ func TestConfigValueFromHCL2Block(t *testing.T) {
 		},
 	}
 
-	for i, test := range tests {
-		t.Run(fmt.Sprintf("%d-%#v", i, test.Input), func(t *testing.T) {
+	for _, test := range tests {
+		t.Run(fmt.Sprintf("%#v", test.Input), func(t *testing.T) {
 			got := ConfigValueFromHCL2Block(test.Input, test.Schema)
 			if !reflect.DeepEqual(got, test.Want) {
 				t.Errorf("wrong result\ninput: %#v\ngot:   %#v\nwant:  %#v", test.Input, got, test.Want)

helper/plugin/grpc_provider.go

@@ -421,6 +421,13 @@ func (s *GRPCProviderServer) ReadResource(_ context.Context, req *proto.ReadReso
 		return resp, nil
 	}
 
+	if newInstanceState != nil {
+		// here we use the prior state to check for unknown/zero containers values
+		// when normalizing the flatmap.
+		stateAttrs := hcl2shim.FlatmapValueFromHCL2(stateVal)
+		newInstanceState.Attributes = normalizeFlatmapContainers(stateAttrs, newInstanceState.Attributes, true)
+	}
+
 	if newInstanceState == nil || newInstanceState.ID == "" {
 		// The old provider API used an empty id to signal that the remote
 		// object appears to have been deleted, but our new protocol expects
@@ -445,6 +452,7 @@ func (s *GRPCProviderServer) ReadResource(_ context.Context, req *proto.ReadReso
 	}
 
 	newStateVal = copyTimeoutValues(newStateVal, stateVal)
+	newStateVal = copyMissingValues(newStateVal, stateVal)
 
 	newStateMP, err := msgpack.Marshal(newStateVal, block.ImpliedType())
 	if err != nil {
@@ -521,7 +529,7 @@ func (s *GRPCProviderServer) PlanResourceChange(_ context.Context, req *proto.Pl
 
 	// strip out non-diffs
 	for k, v := range diff.Attributes {
-		if v.New == v.Old && !v.NewComputed && !v.NewRemoved {
+		if v.New == v.Old && !v.NewComputed {
 			delete(diff.Attributes, k)
 		}
 	}
@@ -705,7 +713,7 @@ func (s *GRPCProviderServer) ApplyResourceChange(_ context.Context, req *proto.A
 
 	// strip out non-diffs
 	for k, v := range diff.Attributes {
-		if v.New == v.Old && !v.NewComputed && !v.NewRemoved && v.NewExtra == "" {
+		if v.New == v.Old && !v.NewComputed && v.NewExtra == "" {
 			delete(diff.Attributes, k)
 		}
 	}

helper/resource/state_shim.go

@@ -49,7 +49,7 @@ func shimNewState(newState *states.State, providers map[string]terraform.Resourc
 			resType := res.Addr.Type
 			providerType := res.ProviderConfig.ProviderConfig.Type
 
-			resource := getResource(providers, providerType, resType)
+			resource := getResource(providers, providerType, res.Addr)
 
 			for key, i := range res.Instances {
 				flatmap, err := shimmedAttributes(i.Current, resource)
@@ -116,7 +116,7 @@ func shimNewState(newState *states.State, providers map[string]terraform.Resourc
 	return state, nil
 }
 
-func getResource(providers map[string]terraform.ResourceProvider, providerName, resourceType string) *schema.Resource {
+func getResource(providers map[string]terraform.ResourceProvider, providerName string, addr addrs.Resource) *schema.Resource {
 	p := providers[providerName]
 	if p == nil {
 		panic(fmt.Sprintf("provider %q not found in test step", providerName))
@@ -125,23 +125,24 @@ func getResource(providers map[string]terraform.ResourceProvider, providerName,
 	// this is only for tests, so should only see schema.Providers
 	provider := p.(*schema.Provider)
 
-	resource := provider.ResourcesMap[resourceType]
-	if resource != nil {
-		return resource
-
+	switch addr.Mode {
+	case addrs.ManagedResourceMode:
+		resource := provider.ResourcesMap[addr.Type]
+		if resource != nil {
+			return resource
+		}
+	case addrs.DataResourceMode:
+		resource := provider.DataSourcesMap[addr.Type]
+		if resource != nil {
+			return resource
+		}
 	}
 
-	resource = provider.DataSourcesMap[resourceType]
-	if resource != nil {
-		return resource
-	}
-
-	panic(fmt.Sprintf("resource %s not found in test step", resourceType))
+	panic(fmt.Sprintf("resource %s not found in test step", addr.Type))
 }
 
 func shimmedAttributes(instance *states.ResourceInstanceObjectSrc, res *schema.Resource) (map[string]string, error) {
 	flatmap := instance.AttrsFlat
-
 	if flatmap != nil {
 		return flatmap, nil
 	}