google: simplify instance metadata schema

It doesn't need to be a List of Maps, it can just be a Map.

We're also safe to remove a previous workaround I stuck in there.

The config parsing is equivalent between a list of maps and a plain map,
so we just need a state migration to make this backwards compatible.

builtin/providers/google/resource_compute_instance.go

@@ -18,6 +18,9 @@ func resourceComputeInstance() *schema.Resource {
 		Update: resourceComputeInstanceUpdate,
 		Delete: resourceComputeInstanceDelete,
 
+		SchemaVersion: 1,
+		MigrateState:  resourceComputeInstanceMigrateState,
+
 		Schema: map[string]*schema.Schema{
 			"name": &schema.Schema{
 				Type:     schema.TypeString,
@@ -168,11 +171,9 @@ func resourceComputeInstance() *schema.Resource {
 			},
 
 			"metadata": &schema.Schema{
-				Type:     schema.TypeList,
+				Type:     schema.TypeMap,
 				Optional: true,
-				Elem: &schema.Schema{
-					Type: schema.TypeMap,
-				},
+				Elem:     schema.TypeString,
 			},
 
 			"service_account": &schema.Schema{
@@ -735,6 +736,7 @@ func resourceComputeInstanceDelete(d *schema.ResourceData, meta interface{}) err
 	config := meta.(*Config)
 
 	zone := d.Get("zone").(string)
+	log.Printf("[INFO] Requesting instance deletion: %s", d.Id())
 	op, err := config.clientCompute.Instances.Delete(config.Project, zone, d.Id()).Do()
 	if err != nil {
 		return fmt.Errorf("Error deleting instance: %s", err)
@@ -751,32 +753,22 @@ func resourceComputeInstanceDelete(d *schema.ResourceData, meta interface{}) err
 }
 
 func resourceInstanceMetadata(d *schema.ResourceData) *compute.Metadata {
-	var metadata *compute.Metadata
-	if metadataList := d.Get("metadata").([]interface{}); len(metadataList) > 0 {
-		m := new(compute.Metadata)
-		m.Items = make([]*compute.MetadataItems, 0, len(metadataList))
-		for _, metadataMap := range metadataList {
-			for key, val := range metadataMap.(map[string]interface{}) {
-				// TODO: fix https://github.com/hashicorp/terraform/issues/883
-				//       and remove this workaround <3 phinze
-				if key == "#" {
-					continue
-				}
-				m.Items = append(m.Items, &compute.MetadataItems{
-					Key:   key,
-					Value: val.(string),
-				})
-			}
+	m := &compute.Metadata{}
+	if mdMap := d.Get("metadata").(map[string]interface{}); len(mdMap) > 0 {
+		m.Items = make([]*compute.MetadataItems, 0, len(mdMap))
+		for key, val := range mdMap {
+			m.Items = append(m.Items, &compute.MetadataItems{
+				Key:   key,
+				Value: val.(string),
+			})
 		}
 
 		// Set the fingerprint. If the metadata has never been set before
 		// then this will just be blank.
 		m.Fingerprint = d.Get("metadata_fingerprint").(string)
-
-		metadata = m
 	}
 
-	return metadata
+	return m
 }
 
 func resourceInstanceTags(d *schema.ResourceData) *compute.Tags {

builtin/providers/google/resource_compute_instance_migrate.go

@@ -0,0 +1,72 @@
+package google
+
+import (
+	"fmt"
+	"log"
+	"strconv"
+	"strings"
+
+	"github.com/hashicorp/terraform/terraform"
+)
+
+func resourceComputeInstanceMigrateState(
+	v int, is *terraform.InstanceState, meta interface{}) (*terraform.InstanceState, error) {
+	if is.Empty() {
+		log.Println("[DEBUG] Empty InstanceState; nothing to migrate.")
+		return is, nil
+	}
+
+	switch v {
+	case 0:
+		log.Println("[INFO] Found Compute Instance State v0; migrating to v1")
+		return migrateStateV0toV1(is)
+	default:
+		return is, fmt.Errorf("Unexpected schema version: %d", v)
+	}
+}
+
+func migrateStateV0toV1(is *terraform.InstanceState) (*terraform.InstanceState, error) {
+	log.Printf("[DEBUG] Attributes before migration: %#v", is.Attributes)
+
+	// Delete old count
+	delete(is.Attributes, "metadata.#")
+
+	newMetadata := make(map[string]string)
+
+	for k, v := range is.Attributes {
+		if !strings.HasPrefix(k, "metadata.") {
+			continue
+		}
+
+		// We have a key that looks like "metadata.*" and we know it's not
+		// metadata.# because we deleted it above, so it must be metadata.<N>.<key>
+		// from the List of Maps. Just need to convert it to a single Map by
+		// ditching the '<N>' field.
+		kParts := strings.SplitN(k, ".", 3)
+
+		// Sanity check: all three parts should be there and <N> should be a number
+		badFormat := false
+		if len(kParts) != 3 {
+			badFormat = true
+		} else if _, err := strconv.Atoi(kParts[1]); err != nil {
+			badFormat = true
+		}
+
+		if badFormat {
+			return is, fmt.Errorf(
+				"migration error: found metadata key in unexpected format: %s", k)
+		}
+
+		// Rejoin as "metadata.<key>"
+		newK := strings.Join([]string{kParts[0], kParts[2]}, ".")
+		newMetadata[newK] = v
+		delete(is.Attributes, k)
+	}
+
+	for k, v := range newMetadata {
+		is.Attributes[k] = v
+	}
+
+	log.Printf("[DEBUG] Attributes after migration: %#v", is.Attributes)
+	return is, nil
+}

builtin/providers/google/resource_compute_instance_migrate_test.go

@@ -0,0 +1,75 @@
+package google
+
+import (
+	"testing"
+
+	"github.com/hashicorp/terraform/terraform"
+)
+
+func TestComputeInstanceMigrateState(t *testing.T) {
+	cases := map[string]struct {
+		StateVersion int
+		Attributes   map[string]string
+		Expected     map[string]string
+		Meta         interface{}
+	}{
+		"v0.4.2 and earlier": {
+			StateVersion: 0,
+			Attributes: map[string]string{
+				"metadata.#":           "2",
+				"metadata.0.foo":       "bar",
+				"metadata.1.baz":       "qux",
+				"metadata.2.with.dots": "should.work",
+			},
+			Expected: map[string]string{
+				"metadata.foo":       "bar",
+				"metadata.baz":       "qux",
+				"metadata.with.dots": "should.work",
+			},
+		},
+	}
+
+	for tn, tc := range cases {
+		is := &terraform.InstanceState{
+			ID:         "i-abc123",
+			Attributes: tc.Attributes,
+		}
+		is, err := resourceComputeInstanceMigrateState(
+			tc.StateVersion, is, tc.Meta)
+
+		if err != nil {
+			t.Fatalf("bad: %s, err: %#v", tn, err)
+		}
+
+		for k, v := range tc.Expected {
+			if is.Attributes[k] != v {
+				t.Fatalf(
+					"bad: %s\n\n expected: %#v -> %#v\n got: %#v -> %#v\n in: %#v",
+					tn, k, v, k, is.Attributes[k], is.Attributes)
+			}
+		}
+	}
+}
+
+func TestComputeInstanceMigrateState_empty(t *testing.T) {
+	var is *terraform.InstanceState
+	var meta interface{}
+
+	// should handle nil
+	is, err := resourceComputeInstanceMigrateState(0, is, meta)
+
+	if err != nil {
+		t.Fatalf("err: %#v", err)
+	}
+	if is != nil {
+		t.Fatalf("expected nil instancestate, got: %#v", is)
+	}
+
+	// should handle non-nil but empty
+	is = &terraform.InstanceState{}
+	is, err = resourceComputeInstanceMigrateState(0, is, meta)
+
+	if err != nil {
+		t.Fatalf("err: %#v", err)
+	}
+}

builtin/providers/google/resource_compute_instance_test.go

@@ -47,6 +47,7 @@ func TestAccComputeInstance_basic(t *testing.T) {
 						"google_compute_instance.foobar", &instance),
 					testAccCheckComputeInstanceTag(&instance, "foo"),
 					testAccCheckComputeInstanceMetadata(&instance, "foo", "bar"),
+					testAccCheckComputeInstanceMetadata(&instance, "baz", "qux"),
 					testAccCheckComputeInstanceDisk(&instance, "terraform-test", true, true),
 				),
 			},
@@ -387,6 +388,9 @@ resource "google_compute_instance" "foobar" {
 	metadata {
 		foo = "bar"
 	}
+	metadata {
+		baz = "qux"
+	}
 }`
 
 const testAccComputeInstance_basic2 = `