Fix scheduling automatic restart, remove deprecated options (#248)

* Fix bug where scheduling.automatic_restart false is never used

* Remove deprecated automatic_restart value in favor of scheduling.automatic_restart

* Remove deprecated on_host_maintenance

* Correct bad var name

* Re-add removed schema values and marked as Removed

* Fix var to snake case

* Migrate empty scheduling blocks in compute_instance_template

* Shorten error message

* Use only one return value instead of two

google/resource_compute_instance.go

@@ -1063,7 +1063,7 @@ func resourceComputeInstanceRead(d *schema.ResourceData, meta interface{}) error
 	d.Set("attached_disk", attachedDisks)
 	d.Set("scratch_disk", scratchDisks)
 
-	scheduling, _ := flattenScheduling(instance.Scheduling)
+	scheduling := flattenScheduling(instance.Scheduling)
 	d.Set("scheduling", scheduling)
 
 	d.Set("self_link", instance.SelfLink)

google/resource_compute_instance_template.go

@@ -18,6 +18,8 @@ func resourceComputeInstanceTemplate() *schema.Resource {
 		Importer: &schema.ResourceImporter{
 			State: schema.ImportStatePassthrough,
 		},
+		SchemaVersion: 1,
+		MigrateState:  resourceComputeInstanceTemplateMigrateState,
 
 		Schema: map[string]*schema.Schema{
 			"name": &schema.Schema{
@@ -44,6 +46,7 @@ func resourceComputeInstanceTemplate() *schema.Resource {
 					return
 				},
 			},
+
 			"disk": &schema.Schema{
 				Type:     schema.TypeList,
 				Required: true,
@@ -132,11 +135,11 @@ func resourceComputeInstanceTemplate() *schema.Resource {
 			},
 
 			"automatic_restart": &schema.Schema{
-				Type:       schema.TypeBool,
-				Optional:   true,
-				Default:    true,
-				ForceNew:   true,
-				Deprecated: "Please use `scheduling.automatic_restart` instead",
+				Type:     schema.TypeBool,
+				Optional: true,
+				Default:  true,
+				ForceNew: true,
+				Removed:  "Use 'scheduling.automatic_restart' instead.",
 			},
 
 			"can_ip_forward": &schema.Schema{
@@ -226,10 +229,10 @@ func resourceComputeInstanceTemplate() *schema.Resource {
 			},
 
 			"on_host_maintenance": &schema.Schema{
-				Type:       schema.TypeString,
-				Optional:   true,
-				ForceNew:   true,
-				Deprecated: "Please use `scheduling.on_host_maintenance` instead",
+				Type:     schema.TypeString,
+				Optional: true,
+				ForceNew: true,
+				Removed:  "Use 'scheduling.on_host_maintenance' instead.",
 			},
 
 			"project": &schema.Schema{
@@ -510,15 +513,6 @@ func resourceComputeInstanceTemplateCreate(d *schema.ResourceData, meta interfac
 	instanceProperties.Scheduling = &compute.Scheduling{}
 	instanceProperties.Scheduling.OnHostMaintenance = "MIGRATE"
 
-	// Depreciated fields
-	if v, ok := d.GetOk("automatic_restart"); ok {
-		instanceProperties.Scheduling.AutomaticRestart = googleapi.Bool(v.(bool))
-	}
-
-	if v, ok := d.GetOk("on_host_maintenance"); ok {
-		instanceProperties.Scheduling.OnHostMaintenance = v.(string)
-	}
-
 	forceSendFieldsScheduling := make([]string, 0, 3)
 	var hasSendMaintenance bool
 	hasSendMaintenance = false
@@ -529,10 +523,10 @@ func resourceComputeInstanceTemplateCreate(d *schema.ResourceData, meta interfac
 		}
 		_scheduling := _schedulings[0].(map[string]interface{})
 
-		if vp, okp := _scheduling["automatic_restart"]; okp {
-			instanceProperties.Scheduling.AutomaticRestart = googleapi.Bool(vp.(bool))
-			forceSendFieldsScheduling = append(forceSendFieldsScheduling, "AutomaticRestart")
-		}
+		// "automatic_restart" has a default value and is always safe to dereference
+		automaticRestart := _scheduling["automatic_restart"].(bool)
+		instanceProperties.Scheduling.AutomaticRestart = googleapi.Bool(automaticRestart)
+		forceSendFieldsScheduling = append(forceSendFieldsScheduling, "AutomaticRestart")
 
 		if vp, okp := _scheduling["on_host_maintenance"]; okp {
 			instanceProperties.Scheduling.OnHostMaintenance = vp.(string)
@@ -672,7 +666,7 @@ func flattenNetworkInterfaces(networkInterfaces []*compute.NetworkInterface) ([]
 	return result, region
 }
 
-func flattenScheduling(scheduling *compute.Scheduling) ([]map[string]interface{}, *bool) {
+func flattenScheduling(scheduling *compute.Scheduling) []map[string]interface{} {
 	result := make([]map[string]interface{}, 0, 1)
 	schedulingMap := make(map[string]interface{})
 	if scheduling.AutomaticRestart != nil {
@@ -681,8 +675,7 @@ func flattenScheduling(scheduling *compute.Scheduling) ([]map[string]interface{}
 	schedulingMap["on_host_maintenance"] = scheduling.OnHostMaintenance
 	schedulingMap["preemptible"] = scheduling.Preemptible
 	result = append(result, schedulingMap)
-	// TODO(selmanj) No need to return two values as automatic restart is captured in map
-	return result, scheduling.AutomaticRestart
+	return result
 }
 
 func flattenServiceAccounts(serviceAccounts []*compute.ServiceAccount) []map[string]interface{} {
@@ -786,13 +779,10 @@ func resourceComputeInstanceTemplateRead(d *schema.ResourceData, meta interface{
 		}
 	}
 	if instanceTemplate.Properties.Scheduling != nil {
-		scheduling, autoRestart := flattenScheduling(instanceTemplate.Properties.Scheduling)
+		scheduling := flattenScheduling(instanceTemplate.Properties.Scheduling)
 		if err = d.Set("scheduling", scheduling); err != nil {
 			return fmt.Errorf("Error setting scheduling: %s", err)
 		}
-		if err = d.Set("automatic_restart", autoRestart); err != nil {
-			return fmt.Errorf("Error setting automatic_restart: %s", err)
-		}
 	}
 	if instanceTemplate.Properties.Tags != nil {
 		if err = d.Set("tags", instanceTemplate.Properties.Tags.Items); err != nil {

google/resource_compute_instance_template_migrate.go

@@ -0,0 +1,58 @@
+package google
+
+import (
+	"fmt"
+	"log"
+
+	"github.com/hashicorp/terraform/terraform"
+)
+
+func resourceComputeInstanceTemplateMigrateState(
+	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 Template State v0; migrating to v1")
+		return migrateComputeInstanceTemplateStateV0toV1(is)
+	default:
+		return is, fmt.Errorf("Unexpected schema version: %d", v)
+	}
+}
+
+func migrateComputeInstanceTemplateStateV0toV1(is *terraform.InstanceState) (*terraform.InstanceState, error) {
+	log.Printf("[DEBUG] Attributes before migration: %#v", is.Attributes)
+
+	// automatic_restart is stored in two places. The top-level automatic_restart value is deprecated, so let's delete
+	// it from the state map for now. For paranoia's sake, we compare it to the value stored in scheduling as well.
+	ar := is.Attributes["automatic_restart"]
+	delete(is.Attributes, "automatic_restart")
+
+	schedulingCount, ok := is.Attributes["scheduling.#"]
+	if ok && schedulingCount != "0" && schedulingCount != "1" {
+		return nil, fmt.Errorf("Found multiple scheduling blocks when there should only be one")
+	}
+
+	if !ok || schedulingCount == "0" {
+		// Either scheduling is missing or empty; go ahead and add
+		is.Attributes["scheduling.#"] = "1"
+		is.Attributes["scheduling.0.automatic_restart"] = ar
+	}
+
+	schedAr := is.Attributes["scheduling.0.automatic_restart"]
+	if ar != schedAr {
+		// Here we could try to choose one value over the other, but in reality they should never be out of sync; error
+		// for now
+		return nil, fmt.Errorf("Found differing values for automatic_restart in state, unsure how to proceed. automatic_restart = %#v, scheduling.0.automatic_restart = %#v", ar, schedAr)
+	}
+
+	// We also nuke "on_host_maintenance" as it's been deprecated as well. Here we don't check the current value though
+	// as the authoritative value has always been maintained in the scheduling block.
+	delete(is.Attributes, "on_host_maintenance")
+
+	log.Printf("[DEBUG] Attributes after migration: %#v", is.Attributes)
+	return is, nil
+}

google/resource_compute_instance_template_migrate_test.go

@@ -0,0 +1,137 @@
+package google
+
+import (
+	"testing"
+
+	"reflect"
+	"strings"
+
+	"github.com/hashicorp/terraform/terraform"
+)
+
+func TestComputeInstanceTemplateMigrateState(t *testing.T) {
+	cases := map[string]struct {
+		StateVersion        int
+		BeforeAttributes    map[string]string
+		AfterAttributes     map[string]string
+		ErrorStringExpected string
+		Meta                interface{}
+	}{
+		"invalid state version": {
+			StateVersion:        -1,
+			ErrorStringExpected: "Unexpected schema version: -1",
+		},
+		"simple automatic_restart case removal": {
+			StateVersion: 0,
+			BeforeAttributes: map[string]string{
+				"automatic_restart":              "true",
+				"scheduling.#":                   "1",
+				"scheduling.0.automatic_restart": "true",
+			},
+			AfterAttributes: map[string]string{
+				"scheduling.#":                   "1",
+				"scheduling.0.automatic_restart": "true",
+			},
+		},
+		"simple on_host_maintenance removal": {
+			StateVersion: 0,
+			BeforeAttributes: map[string]string{
+				"on_host_maintenance":            "MIGRATE",
+				"automatic_restart":              "true",
+				"scheduling.#":                   "1",
+				"scheduling.0.automatic_restart": "true",
+			},
+			AfterAttributes: map[string]string{
+				"scheduling.#":                   "1",
+				"scheduling.0.automatic_restart": "true",
+			},
+		},
+		"missing scheduling block": {
+			StateVersion: 0,
+			BeforeAttributes: map[string]string{
+				"automatic_restart": "true",
+			},
+			AfterAttributes: map[string]string{
+				"scheduling.#":                   "1",
+				"scheduling.0.automatic_restart": "true",
+			},
+		},
+		"empty scheduling block": {
+			StateVersion: 0,
+			BeforeAttributes: map[string]string{
+				"automatic_restart": "true",
+				"scheduling.#":      "0",
+			},
+			AfterAttributes: map[string]string{
+				"scheduling.#":                   "1",
+				"scheduling.0.automatic_restart": "true",
+			},
+		},
+		"error upon multiple scheduling block": {
+			StateVersion: 0,
+			BeforeAttributes: map[string]string{
+				"automatic_restart":              "true",
+				"scheduling.#":                   "2",
+				"scheduling.0.automatic_restart": "true",
+				"scheduling.1.automatic_restart": "true",
+			},
+			ErrorStringExpected: "Found multiple scheduling blocks when there should only be one",
+		},
+		"error upon differing automatic_restart values": {
+			StateVersion: 0,
+			BeforeAttributes: map[string]string{
+				"automatic_restart":              "true",
+				"scheduling.#":                   "1",
+				"scheduling.0.automatic_restart": "false",
+			},
+			ErrorStringExpected: "Found differing values for automatic_restart in state, unsure how to proceed.",
+		},
+	}
+
+	for tn, tc := range cases {
+		is := &terraform.InstanceState{
+			ID:         "i-abc123",
+			Attributes: tc.BeforeAttributes,
+		}
+		is, err := resourceComputeInstanceTemplateMigrateState(
+			tc.StateVersion, is, tc.Meta)
+
+		if err != nil {
+			if tc.ErrorStringExpected == "" {
+				t.Fatalf("bad: %s, err: %#v", tn, err)
+			} else if !strings.Contains(err.Error(), tc.ErrorStringExpected) {
+				t.Fatalf("Expected error containing string %s, instead found %#v", tc.ErrorStringExpected, err)
+			} else {
+				continue
+			}
+		}
+
+		// Compare both maps for identity
+		if !reflect.DeepEqual(is.Attributes, tc.AfterAttributes) {
+			t.Fatalf("Expected attributes %#v, got attributes %#v", tc.AfterAttributes, is.Attributes)
+		}
+	}
+}
+
+func TestComputeInstanceTemplateMigrateState_empty(t *testing.T) {
+	var is *terraform.InstanceState
+	var meta interface{}
+
+	// should handle nil
+	is, err := resourceComputeInstanceTemplateMigrateState(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 = resourceComputeInstanceTemplateMigrateState(0, is, meta)
+
+	if err != nil {
+		t.Fatalf("err: %#v", err)
+	}
+}

google/resource_compute_instance_template_test.go

@@ -34,6 +34,27 @@ func TestAccComputeInstanceTemplate_basic(t *testing.T) {
 	})
 }
 
+func TestAccComputeInstanceTemplate_preemptible(t *testing.T) {
+	var instanceTemplate compute.InstanceTemplate
+
+	resource.Test(t, resource.TestCase{
+		PreCheck:     func() { testAccPreCheck(t) },
+		Providers:    testAccProviders,
+		CheckDestroy: testAccCheckComputeInstanceTemplateDestroy,
+		Steps: []resource.TestStep{
+			resource.TestStep{
+				Config: testAccComputeInstanceTemplate_preemptible,
+				Check: resource.ComposeTestCheckFunc(
+					testAccCheckComputeInstanceTemplateExists(
+						"google_compute_instance_template.foobar", &instanceTemplate),
+					testAccCheckComputeInstanceTemplateAutomaticRestart(&instanceTemplate, false),
+					testAccCheckComputeInstanceTemplatePreemptible(&instanceTemplate, true),
+				),
+			},
+		},
+	})
+}
+
 func TestAccComputeInstanceTemplate_IP(t *testing.T) {
 	var instanceTemplate compute.InstanceTemplate
 
@@ -333,6 +354,28 @@ func testAccCheckComputeInstanceTemplateTag(instanceTemplate *compute.InstanceTe
 	}
 }
 
+func testAccCheckComputeInstanceTemplatePreemptible(instanceTemplate *compute.InstanceTemplate, preemptible bool) resource.TestCheckFunc {
+	return func(s *terraform.State) error {
+		if instanceTemplate.Properties.Scheduling.Preemptible != preemptible {
+			return fmt.Errorf("Expected preemptible value %v, got %v", preemptible, instanceTemplate.Properties.Scheduling.Preemptible)
+		}
+		return nil
+	}
+}
+
+func testAccCheckComputeInstanceTemplateAutomaticRestart(instanceTemplate *compute.InstanceTemplate, automaticRestart bool) resource.TestCheckFunc {
+	return func(s *terraform.State) error {
+		ar := instanceTemplate.Properties.Scheduling.AutomaticRestart
+		if ar == nil {
+			return fmt.Errorf("Expected to see a value for AutomaticRestart, but got nil")
+		}
+		if *ar != automaticRestart {
+			return fmt.Errorf("Expected automatic restart value %v, got %v", automaticRestart, ar)
+		}
+		return nil
+	}
+}
+
 func testAccCheckComputeInstanceTemplateStartupScript(instanceTemplate *compute.InstanceTemplate, n string) resource.TestCheckFunc {
 	return func(s *terraform.State) error {
 		if instanceTemplate.Properties.Metadata == nil && n == "" {
@@ -400,6 +443,37 @@ resource "google_compute_instance_template" "foobar" {
 	}
 }`, acctest.RandString(10))
 
+var testAccComputeInstanceTemplate_preemptible = fmt.Sprintf(`
+resource "google_compute_instance_template" "foobar" {
+	name = "instancet-test-%s"
+	machine_type = "n1-standard-1"
+	can_ip_forward = false
+	tags = ["foo", "bar"]
+
+	disk {
+		source_image = "debian-8-jessie-v20160803"
+		auto_delete = true
+		boot = true
+	}
+
+	network_interface {
+		network = "default"
+	}
+
+	scheduling {
+		preemptible = true
+		automatic_restart = false
+	}
+
+	metadata {
+		foo = "bar"
+	}
+
+	service_account {
+		scopes = ["userinfo-email", "compute-ro", "storage-ro"]
+	}
+}`, acctest.RandString(10))
+
 var testAccComputeInstanceTemplate_ip = fmt.Sprintf(`
 resource "google_compute_address" "foo" {
 	name = "instancet-test-%s"