Skip to content
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.

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

Changed google_compute_instance_group_manager target_size default to 0. #65

Merged
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
35 changes: 15 additions & 20 deletions google/resource_compute_instance_group_manager.go
Original file line number Diff line number Diff line change
Expand Up @@ -132,18 +132,18 @@ func resourceComputeInstanceGroupManagerCreate(d *schema.ResourceData, meta inte
return err
}

// Get group size, default to 1 if not given
var target_size int64 = 1
var targetSize int64 = 0
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

targetSize := 0

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

if v, ok := d.GetOk("target_size"); ok {
target_size = int64(v.(int))
targetSize = int64(v.(int))
}

// 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"},
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

}

// Set optional fields
Expand Down Expand Up @@ -256,7 +256,6 @@ func resourceComputeInstanceGroupManagerRead(d *schema.ResourceData, meta interf
d.Set("named_port", flattenNamedPorts(manager.NamedPorts))
d.Set("fingerprint", manager.Fingerprint)
d.Set("instance_group", manager.InstanceGroup)
d.Set("target_size", manager.TargetSize)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

boooo github

d.Set("self_link", manager.SelfLink)
update_strategy, ok := d.GetOk("update_strategy")
if !ok {
Expand Down Expand Up @@ -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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed.

if d.HasChange("target_size") {
if v, ok := d.GetOk("target_size"); ok {
// Only do anything if the new size is set
target_size := int64(v.(int))

op, err := config.clientCompute.InstanceGroupManagers.Resize(
project, d.Get("zone").(string), d.Id(), target_size).Do()
if err != nil {
return fmt.Errorf("Error updating InstanceGroupManager: %s", err)
}
target_size := int64(d.Get("target_size").(int))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

targetSize

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

op, err := config.clientCompute.InstanceGroupManagers.Resize(
project, d.Get("zone").(string), d.Id(), target_size).Do()
if err != nil {
return fmt.Errorf("Error updating InstanceGroupManager: %s", err)
}

// Wait for the operation to complete
err = computeOperationWaitZone(config, op, project, d.Get("zone").(string), "Updating InstanceGroupManager")
if err != nil {
return err
}
// Wait for the operation to complete
err = computeOperationWaitZone(config, op, project, d.Get("zone").(string), "Updating InstanceGroupManager")
if err != nil {
return err
}

d.SetPartial("target_size")
Expand Down
63 changes: 63 additions & 0 deletions google/resource_compute_instance_group_manager_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,32 @@ func TestAccInstanceGroupManager_basic(t *testing.T) {
})
}

func TestAccInstanceGroupManager_targetSizeZero(t *testing.T) {
var manager compute.InstanceGroupManager

template := fmt.Sprintf("igm-test-%s", acctest.RandString(10))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

optional nit: templateName, igmName

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

igm := fmt.Sprintf("igm-test-%s", acctest.RandString(10))

resource.Test(t, resource.TestCase{
PreCheck: func() { testAccPreCheck(t) },
Providers: testAccProviders,
CheckDestroy: testAccCheckInstanceGroupManagerDestroy,
Steps: []resource.TestStep{
resource.TestStep{
Config: testAccInstanceGroupManager_targetSizeZero(template, igm),
Check: resource.ComposeTestCheckFunc(
testAccCheckInstanceGroupManagerExists(
"google_compute_instance_group_manager.igm-basic", &manager),
),
},
},
})

if manager.TargetSize != 0 {
t.Errorf("Expected target_size to be 0, got %d", manager.TargetSize)
}
}

func TestAccInstanceGroupManager_update(t *testing.T) {
var manager compute.InstanceGroupManager

Expand Down Expand Up @@ -388,6 +414,43 @@ func testAccInstanceGroupManager_basic(template, target, igm1, igm2 string) stri
`, template, target, igm1, igm2)
}

func testAccInstanceGroupManager_targetSizeZero(template, igm string) string {
return fmt.Sprintf(`
resource "google_compute_instance_template" "igm-basic" {
name = "%s"
machine_type = "n1-standard-1"
can_ip_forward = false
tags = ["foo", "bar"]

disk {
source_image = "debian-cloud/debian-8-jessie-v20160803"
auto_delete = true
boot = true
}

network_interface {
network = "default"
}

metadata {
foo = "bar"
}

service_account {
scopes = ["userinfo-email", "compute-ro", "storage-ro"]
}
}

resource "google_compute_instance_group_manager" "igm-basic" {
description = "Terraform test instance group manager"
name = "%s"
instance_template = "${google_compute_instance_template.igm-basic.self_link}"
base_instance_name = "igm-basic"
zone = "us-central1-c"
}
`, template, igm)
}

func testAccInstanceGroupManager_update(template, target, igm string) string {
return fmt.Sprintf(`
resource "google_compute_instance_template" "igm-update" {
Expand Down
6 changes: 3 additions & 3 deletions website/docs/r/compute_instance_group_manager.html.markdown
Original file line number Diff line number Diff line change
Expand Up @@ -74,9 +74,9 @@ The following arguments are supported:
restart all of the instances at once. In the future, as the GCE API matures
we will support `"ROLLING_UPDATE"` as well.

* `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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

instance group. This value should always be explicitly set unless this resource is attached to
an autoscaler, in which case it should never be set.

* `target_pools` - (Optional) The full URL of all target pools to which new
instances in the group are added. Updating the target pools attribute does
Expand Down