From 78772d951466a55c502ce7bd69d976e0407274b1 Mon Sep 17 00:00:00 2001 From: "Stephen Lewis (Burrows)" Date: Tue, 18 Jul 2023 11:54:45 -0700 Subject: [PATCH] Changed order of updates for min_cpu_platform and machine_type (#8249) * Changed order of updates for min_cpu_platform and machine_type Resolved https://github.com/hashicorp/terraform-provider-google/issues/14945 * Added allow_stopping_for_update = true * Added allow_stopping_for_update to ImportStateVerifyIgnore * Added diff suppress and clarified how to 'unset' the field --- .../compute/resource_compute_instance.go.erb | 51 +++++---- .../resource_compute_instance_test.go.erb | 107 +++++++++++++++++- .../docs/d/compute_instance.html.markdown | 2 +- 3 files changed, 134 insertions(+), 26 deletions(-) diff --git a/mmv1/third_party/terraform/services/compute/resource_compute_instance.go.erb b/mmv1/third_party/terraform/services/compute/resource_compute_instance.go.erb index 1458c1fedada..775ab9e42d71 100644 --- a/mmv1/third_party/terraform/services/compute/resource_compute_instance.go.erb +++ b/mmv1/third_party/terraform/services/compute/resource_compute_instance.go.erb @@ -102,6 +102,14 @@ func forceNewIfNetworkIPNotUpdatableFunc(d tpgresource.TerraformResourceDiff) er return nil } +// User may specify AUTOMATIC using any case; the API will accept it and return an empty string. +func ComputeInstanceMinCpuPlatformEmptyOrAutomaticDiffSuppress(k, old, new string, d *schema.ResourceData) bool { + old = strings.ToLower(old) + new = strings.ToLower(new) + defaultVal := "automatic" + return (old == "" && new == defaultVal) || (new == "" && old == defaultVal) +} + func ResourceComputeInstance() *schema.Resource { return &schema.Resource{ Create: resourceComputeInstanceCreate, @@ -596,10 +604,11 @@ func ResourceComputeInstance() *schema.Resource { }, "min_cpu_platform": { - Type: schema.TypeString, - Optional: true, - Computed: true, - Description: `The minimum CPU platform specified for the VM instance.`, + Type: schema.TypeString, + Optional: true, + Computed: true, + Description: `The minimum CPU platform specified for the VM instance.`, + DiffSuppressFunc: ComputeInstanceMinCpuPlatformEmptyOrAutomaticDiffSuppress, }, "project": { @@ -2020,40 +2029,34 @@ func resourceComputeInstanceUpdate(d *schema.ResourceData, meta interface{}) err } } - if d.HasChange("machine_type") { - mt, err := tpgresource.ParseMachineTypesFieldValue(d.Get("machine_type").(string), d, config) - if err != nil { - return err - } - req := &compute.InstancesSetMachineTypeRequest{ - MachineType: mt.RelativeLink(), + if d.HasChange("min_cpu_platform") { + minCpuPlatform := d.Get("min_cpu_platform") + req := &compute.InstancesSetMinCpuPlatformRequest{ + MinCpuPlatform: minCpuPlatform.(string), } - op, err := config.NewComputeClient(userAgent).Instances.SetMachineType(project, zone, instance.Name, req).Do() + op, err := config.NewComputeClient(userAgent).Instances.SetMinCpuPlatform(project, zone, instance.Name, req).Do() if err != nil { return err } - opErr := ComputeOperationWaitTime(config, op, project, "updating machinetype", userAgent, d.Timeout(schema.TimeoutUpdate)) + opErr := ComputeOperationWaitTime(config, op, project, "updating min cpu platform", userAgent, d.Timeout(schema.TimeoutUpdate)) if opErr != nil { return opErr } } - if d.HasChange("min_cpu_platform") { - minCpuPlatform, ok := d.GetOk("min_cpu_platform") - // Even though you don't have to set minCpuPlatform on create, you do have to set it to an - // actual value on update. "Automatic" is the default. This will be read back from the API as empty, - // so we don't need to worry about diffs. - if !ok { - minCpuPlatform = "Automatic" + if d.HasChange("machine_type") { + mt, err := tpgresource.ParseMachineTypesFieldValue(d.Get("machine_type").(string), d, config) + if err != nil { + return err } - req := &compute.InstancesSetMinCpuPlatformRequest{ - MinCpuPlatform: minCpuPlatform.(string), + req := &compute.InstancesSetMachineTypeRequest{ + MachineType: mt.RelativeLink(), } - op, err := config.NewComputeClient(userAgent).Instances.SetMinCpuPlatform(project, zone, instance.Name, req).Do() + op, err := config.NewComputeClient(userAgent).Instances.SetMachineType(project, zone, instance.Name, req).Do() if err != nil { return err } - opErr := ComputeOperationWaitTime(config, op, project, "updating min cpu platform", userAgent, d.Timeout(schema.TimeoutUpdate)) + opErr := ComputeOperationWaitTime(config, op, project, "updating machinetype", userAgent, d.Timeout(schema.TimeoutUpdate)) if opErr != nil { return opErr } diff --git a/mmv1/third_party/terraform/tests/resource_compute_instance_test.go.erb b/mmv1/third_party/terraform/tests/resource_compute_instance_test.go.erb index 97869a3e359f..3617d08fd662 100644 --- a/mmv1/third_party/terraform/tests/resource_compute_instance_test.go.erb +++ b/mmv1/third_party/terraform/tests/resource_compute_instance_test.go.erb @@ -28,6 +28,74 @@ import ( <% end -%> ) +func TestMinCpuPlatformDiffSuppress(t *testing.T) { + cases := map[string]struct { + Old, New string + ExpectDiffSuppress bool + }{ + "state: empty, conf: AUTOMATIC": { + Old: "", + New: "AUTOMATIC", + ExpectDiffSuppress: true, + }, + "state: empty, conf: automatic": { + Old: "", + New: "automatic", + ExpectDiffSuppress: true, + }, + "state: empty, conf: AuToMaTiC": { + Old: "", + New: "AuToMaTiC", + ExpectDiffSuppress: true, + }, + "state: empty, conf: Intel Haswell": { + Old: "", + New: "Intel Haswell", + ExpectDiffSuppress: false, + }, + // This case should never happen due to the field being + // Optional + Computed; however, including for completeness. + "state: Intel Haswell, conf: empty": { + Old: "Intel Haswell", + New: "", + ExpectDiffSuppress: false, + }, + // These cases should never happen given current API behavior; testing + // in case API behavior changes in the future. + "state: AUTOMATIC, conf: Intel Haswell": { + Old: "AUTOMATIC", + New: "Intel Haswell", + ExpectDiffSuppress: false, + }, + "state: Intel Haswell, conf: AUTOMATIC": { + Old: "Intel Haswell", + New: "AUTOMATIC", + ExpectDiffSuppress: false, + }, + "state: AUTOMATIC, conf: empty": { + Old: "AUTOMATIC", + New: "", + ExpectDiffSuppress: true, + }, + "state: automatic, conf: empty": { + Old: "automatic", + New: "", + ExpectDiffSuppress: true, + }, + "state: AuToMaTiC, conf: empty": { + Old: "AuToMaTiC", + New: "", + ExpectDiffSuppress: true, + }, + } + + for tn, tc := range cases { + if tpgcompute.ComputeInstanceMinCpuPlatformEmptyOrAutomaticDiffSuppress("min_cpu_platform", tc.Old, tc.New, nil) != tc.ExpectDiffSuppress { + t.Errorf("bad: %s, %q => %q expect DiffSuppress to return %t", tn, tc.Old, tc.New, tc.ExpectDiffSuppress) + } + } +} + func computeInstanceImportStep(zone, instanceName string, additionalImportIgnores []string) resource.TestStep { // metadata is only read into state if set in the config // importing doesn't know whether metadata.startup_script vs metadata_startup_script is set in the config, @@ -1436,7 +1504,15 @@ func TestAccComputeInstance_minCpuPlatform(t *testing.T) { testAccCheckComputeInstanceHasMinCpuPlatform(&instance, "Intel Haswell"), ), }, - computeInstanceImportStep("us-east1-d", instanceName, []string{}), + computeInstanceImportStep("us-east1-d", instanceName, []string{"allow_stopping_for_update"}), + { + Config: testAccComputeInstance_minCpuPlatform_remove(instanceName), + Check: resource.ComposeTestCheckFunc( + testAccCheckComputeInstanceExists(t, "google_compute_instance.foobar", &instance), + testAccCheckComputeInstanceHasMinCpuPlatform(&instance, ""), + ), + }, + computeInstanceImportStep("us-east1-d", instanceName, []string{"allow_stopping_for_update"}), }, }) } @@ -5306,6 +5382,35 @@ resource "google_compute_instance" "foobar" { } min_cpu_platform = "Intel Haswell" + allow_stopping_for_update = true +} +`, instance) +} + +func testAccComputeInstance_minCpuPlatform_remove(instance string) string { + return fmt.Sprintf(` +data "google_compute_image" "my_image" { + family = "debian-11" + project = "debian-cloud" +} + +resource "google_compute_instance" "foobar" { + name = "%s" + machine_type = "e2-micro" + zone = "us-east1-d" + + boot_disk { + initialize_params { + image = data.google_compute_image.my_image.self_link + } + } + + network_interface { + network = "default" + } + + min_cpu_platform = "AuToMaTiC" + allow_stopping_for_update = true } `, instance) } diff --git a/mmv1/third_party/terraform/website/docs/d/compute_instance.html.markdown b/mmv1/third_party/terraform/website/docs/d/compute_instance.html.markdown index 2a3e1d03f404..d478a3eca5ae 100644 --- a/mmv1/third_party/terraform/website/docs/d/compute_instance.html.markdown +++ b/mmv1/third_party/terraform/website/docs/d/compute_instance.html.markdown @@ -61,7 +61,7 @@ The following arguments are supported: * `metadata` - Metadata key/value pairs made available within the instance. -* `min_cpu_platform` - The minimum CPU platform specified for the VM instance. +* `min_cpu_platform` - The minimum CPU platform specified for the VM instance. Set to "AUTOMATIC" to remove a previously-set value. * `scheduling` - The scheduling strategy being used by the instance. Structure is [documented below](#nested_scheduling)