New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Changed google_compute_instance_group_manager target_size default to 0. #65

Merged

danawillow merged 3 commits into hashicorp:master from rileykarson:13-instance-group-manager-target-size

Jun 16, 2017

Collaborator

rileykarson commented Jun 13, 2017

Set the default target_size to 0 so that target sizes of 0 don't get set as the old default of 1. Existing configurations won't change as a result of changing the default, since the field is computed.

This is not a breaking change for existing configurations; resources with no value set will stay at their current setting. Scripts that rely on the old default of 1 at creation time will need to be updated.

Fixes #13
@danawillow


          Changed google_compute_instance_group_manager target_size default to 0.

78b0879

Collaborator Author

rileykarson commented Jun 13, 2017

Related: hashicorp/terraform#5694 tracks the ability to both have a default of 1 + allow 0 values for this field.

rileykarson added the bug label

danawillow suggested changes

View reviewed changes

google/resource_compute_instance_group_manager.go

               	d.Set("named_port", flattenNamedPorts(manager.NamedPorts))
               	d.Set("fingerprint", manager.Fingerprint)
               	d.Set("instance_group", manager.InstanceGroup)
-              	d.Set("target_size", manager.TargetSize)

Contributor

danawillow Jun 16, 2017

This was removed because it could change with the autoscaler, right? If so mind adding a comment of that nature?

Collaborator Author

rileykarson Jun 16, 2017

It was getting set twice before, the other d.Set is just out of view here.

Contributor

danawillow Jun 16, 2017

boooo github

google/resource_compute_instance_group_manager.go Outdated

               		return err
               	}
-              	// Get group size, default to 1 if not given
-              	var target_size int64 = 1
+              	var targetSize int64 = 0

Contributor

danawillow Jun 16, 2017

targetSize := 0

Collaborator Author

rileykarson Jun 16, 2017

Go is convinced this is an int, not int64 with that syntax; using targetSize := int64(0)

google/resource_compute_instance_group_manager.go Outdated

               	}
               	// Build the parameter
               	manager := &compute.InstanceGroupManager{
               		Name:             d.Get("name").(string),
               		BaseInstanceName: d.Get("base_instance_name").(string),
               		InstanceTemplate: d.Get("instance_template").(string),
-              		TargetSize:       target_size,
+              		TargetSize:       targetSize,
+              		ForceSendFields:  []string{"TargetSize"},

Contributor

danawillow Jun 16, 2017

might be nice to have a comment here like "Force send TargetSize to allow it to have a value of 0"

Collaborator Author

rileykarson Jun 16, 2017

Done.

google/resource_compute_instance_group_manager.go Outdated

    
            @@ -382,23 +381,19 @@ func resourceComputeInstanceGroupManagerUpdate(d *schema.ResourceData, meta inte
          
              		d.SetPartial("named_port")

              	}

              	// If size changes trigger a resize

              	// We won't ever see changes if target_size is unset

Contributor

danawillow Jun 16, 2017

I'm not quite sure what this comment is trying to tell me

Collaborator Author

rileykarson Jun 16, 2017

Removed.

google/resource_compute_instance_group_manager.go Outdated

-              			if err != nil {
-              				return fmt.Errorf("Error updating InstanceGroupManager: %s", err)
-              			}
+              		target_size := int64(d.Get("target_size").(int))

Contributor

danawillow Jun 16, 2017

targetSize

Collaborator Author

rileykarson Jun 16, 2017

Done.

google/resource_compute_instance_group_manager_test.go Outdated

+              func TestAccInstanceGroupManager_targetSizeZero(t *testing.T) {
+              	var manager compute.InstanceGroupManager
+              	template := fmt.Sprintf("igm-test-%s", acctest.RandString(10))

Contributor

danawillow Jun 16, 2017

optional nit: templateName, igmName

Collaborator Author

rileykarson Jun 16, 2017

Done.

website/docs/r/compute_instance_group_manager.html.markdown Outdated

-              * `target_size` - (Optional) If not given at creation time, this defaults to 1.
-                  Do not specify this if you are managing the group with an autoscaler, as
-                  this will cause fighting.
+              * `target_size` - (Optional, Default `0`) The target number of running instances for this managed

Contributor

danawillow Jun 16, 2017

Do we normally put the default in that spot? I don't necessarily have a problem with it, but I think normally we just put it in the description.

Collaborator Author

rileykarson Jun 16, 2017

We use that style in a few places, I think I happened to have seen one of those right before writing this. Updated.

rileykarson added 2 commits

June 16, 2017 15:39


          Updated variable names and docs.

3f26ea6


          go fmt

de9b9a0

danawillow approved these changes

View reviewed changes

Contributor

danawillow commented Jun 16, 2017

TF_ACC=1 go test ./google -v -run=TestAccInstanceGroupManager -timeout 120m
=== RUN   TestAccInstanceGroupManager_importBasic
--- PASS: TestAccInstanceGroupManager_importBasic (253.35s)
=== RUN   TestAccInstanceGroupManager_importUpdate
--- PASS: TestAccInstanceGroupManager_importUpdate (263.10s)
=== RUN   TestAccInstanceGroupManager_basic
--- PASS: TestAccInstanceGroupManager_basic (222.92s)
=== RUN   TestAccInstanceGroupManager_targetSizeZero
--- PASS: TestAccInstanceGroupManager_targetSizeZero (46.14s)
=== RUN   TestAccInstanceGroupManager_update
--- PASS: TestAccInstanceGroupManager_update (288.82s)
=== RUN   TestAccInstanceGroupManager_updateLifecycle
--- PASS: TestAccInstanceGroupManager_updateLifecycle (114.07s)
=== RUN   TestAccInstanceGroupManager_updateStrategy
--- PASS: TestAccInstanceGroupManager_updateStrategy (231.41s)
=== RUN   TestAccInstanceGroupManager_separateRegions
--- PASS: TestAccInstanceGroupManager_separateRegions (231.78s)
PASS

danawillow merged commit fc3d41e into hashicorp:master

pdecat pushed a commit to pdecat/terraform-provider-google that referenced this pull request


          Merge pull request hashicorp#65 from rileykarson/13-instance-group-ma…

0dfc87f

…nager-target-size

Changed google_compute_instance_group_manager target_size default to 0.

danisla mentioned this pull request

"Unset" value for google_compute_region_instance_group_manager target_size argument? #667

Closed

chrisst pushed a commit to chrisst/terraform-provider-google that referenced this pull request


          Merge pull request hashicorp#65 from rileykarson/13-instance-group-ma…

6a0229b

…nager-target-size

Changed google_compute_instance_group_manager target_size default to 0.

luis-silva pushed a commit to luis-silva/terraform-provider-google that referenced this pull request


          Merge pull request hashicorp#65 from modular-magician/codegen-pr-650

f9760a7

Fix typo in resource name, same as TPG hashicorp#2365

ghost commented Mar 31, 2020

I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.

If you feel this issue should be reopened, we encourage creating a new issue linking back to this one for added context. If you feel I made an error 🤖 🙉 , please reach out to my human friends 👉 [email protected]. Thanks!

ghost locked and limited conversation to collaborators

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

bug