Add new resource region_instance_group_manager #394

rosbo · 2017-09-06T20:26:39Z

Fixes #45

I decided not to implement the client-side update_strategy like we did for the single zone instance_group_manager resource. The reason is that there is now a new rolling update feature in beta. I will add the beta update feature to both instance_group_manager and region_instance_group_manager in my next PR (see #51).

cc/ @paddycarver @selmanj

paddycarver

Quick, minor feedback, and this should be good to go!

paddycarver · 2017-09-08T06:19:20Z

google/resource_compute_region_instance_group_manager.go

+			"target_pools": &schema.Schema{
+				Type:             schema.TypeSet,
+				Optional:         true,
+				DiffSuppressFunc: compareSelfLinkRelativePaths,


I think DiffSuppressFunc works weirdly with Sets and Lists, IIRC. :/

Removed, the selfLinkRelativePathHash is enough since a target pool must be a self_link (API doesn't support name only). I added tests to make sure we don't regress on this. I will update the single zone resource in its own PR to do the same.

paddycarver · 2017-09-08T06:19:40Z

google/resource_compute_region_instance_group_manager.go

+						"health_check": &schema.Schema{
+							Type:             schema.TypeString,
+							Required:         true,
+							DiffSuppressFunc: compareSelfLinkRelativePaths,


I think DiffSuppressFunc works weirdly with Sets and Lists, IIRC. :/

DiffSuppressFunc works well on nested string. At least we use it in many places and I just did some manual testing and works as expected.

Apologies, my information must be out of date. :) Glad it works! I remember something about the count getting off or something, but it must have been fixed since then.

paddycarver · 2017-09-08T06:24:01Z

google/resource_compute_region_instance_group_manager.go

+	if err != nil {
+		if gerr, ok := err.(*googleapi.Error); ok && gerr.Code == 404 {
+			return false, nil
+		}


Looks like an error is being dropped here. What happens if it's not a 404?

We can't say for sure that the resource doesn't exist, it could be a transient error from the server side. I added a comment.

This is totally undocumented and unclear, but in the case where an error is encountered checking for existence, Exists should return true with the error.

(The idea being that false with the error would remove the resource from state, which is not what we want. true will have no impact except returning the error, AFAIK.)

Returning true and the error now. I agree, it makes way more sense this way.

rosbo · 2017-09-08T17:11:51Z

TF_ACC=1 go test ./google -v -run TestAccRegionInstanceGroupManager_update -timeout 120m
=== RUN   TestAccRegionInstanceGroupManager_update
--- PASS: TestAccRegionInstanceGroupManager_update (151.99s)
=== RUN   TestAccRegionInstanceGroupManager_updateLifecycle
--- PASS: TestAccRegionInstanceGroupManager_updateLifecycle (118.69s)
PASS
ok  	github.com/terraform-providers/terraform-provider-google/google	270.807s

paddycarver · 2017-09-08T23:40:20Z

google/resource_compute_region_instance_group_manager.go

+		TargetPools:         convertStringSet(d.Get("target_pools").(*schema.Set)),
+		AutoHealingPolicies: expandAutoHealingPolicies(d.Get("auto_healing_policies").([]interface{})),
+		// Force send TargetSize to allow size of 0.
+		ForceSendFields: []string{"TargetSize"},


TargetSize is Optional, and has no default. So if users don't set it, that will create a group with target size of 0 by default. Is that what we want? (I don't have a preference either way, just want to ask the question and get the answer recorded for posterity :D)

TargetSize of zero is the default behavior for our single zone resource: resource_compute_instance_group_manager. If user don't specify, I think it is better if we play it safe and don't create an arbitrary number of instances that our users might not want.

Sounds sane and reasonable and like the best choice.

paddycarver · 2017-09-08T23:42:12Z

google/resource_compute_region_instance_group_manager.go

+			return err
+		}
+		managerV1.ForceSendFields = manager.ForceSendFields
+		op, err = config.clientCompute.RegionInstanceGroupManagers.Insert(project, d.Get("region").(string), managerV1).Do()


Note to future-Paddy:

I thought about asking to use getRegion here, but region is required, and it seems reasonable to require the user to specify which region they want their regional resource in, instead of just defaulting to the Provider-configured region.

paddycarver · 2017-09-08T23:43:07Z

google/resource_compute_region_instance_group_manager.go

+		return fmt.Errorf("Error creating RegionInstanceGroupManager: %s", err)
+	}
+
+	d.SetId(manager.Name)


Couldn't we hypothetically have two IGMs in different regions with the same name? What if we use the "canonical link" that @selmanj used for #378?

Name is unique globally. FYI: I was the one who actually wrote #378 ;)

I am working on a RegionalId and ZonalId class to standardize all of this. I left it to simply the name for now to mimic the behavior of the single zone resource (resource_compute_instance_group_manager). Once I am done with the reusable id classes, I will change all our resources to have consistent and more user-friendly import behavior.

Name is unique globally.

Then ignore me entirely. :D

FYI: I was the one who actually wrote #378 ;)

Well, this is mortifying. Apologies. :(

I am working on a RegionalId and ZonalId class to standardize all of this. I left it to simply the name for now to mimic the behavior of the single zone resource (resource_compute_instance_group_manager). Once I am done with the reusable id classes, I will change all our resources to have consistent and more user-friendly import behavior.

Sounds good to me!

…ts (hashicorp#394) Signed-off-by: Modular Magician <[email protected]>

ghost · 2020-03-31T13:54:39Z

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!

rosbo and others added 2 commits September 6, 2017 13:10

Add new resource region_instance_group_manager.

fb3444d

Add documentation for the new resource

b8052a6

rosbo requested a review from paddycarver September 6, 2017 20:26

rosbo changed the title ~~Add new resource region_instance_group_manager.~~ Add new resource region_instance_group_manager Sep 6, 2017

rosbo requested a review from selmanj September 6, 2017 21:10

rosbo mentioned this pull request Sep 6, 2017

implements region_instance_group_manager hashicorp/terraform#10907

Closed

enxebre mentioned this pull request Sep 7, 2017

Explore regional instance group manager enxebre/tectonic-installer#4

Open

paddycarver suggested changes Sep 8, 2017

View reviewed changes

Fix target pools and exists method

bc741ad

rosbo assigned paddycarver Sep 8, 2017

Propagating api error in exists method

7b3b472

paddycarver suggested changes Sep 8, 2017

View reviewed changes

paddycarver approved these changes Sep 9, 2017

View reviewed changes

rosbo merged commit c751a32 into hashicorp:master Sep 9, 2017

rosbo deleted the region_instance_manager branch September 9, 2017 00:02

enxebre mentioned this pull request Sep 22, 2017

gcp basis coreos/tectonic-installer#1918

Merged

negz pushed a commit to negz/terraform-provider-google that referenced this pull request Oct 17, 2017

Add new resource region_instance_group_manager (hashicorp#394)

8ca8b41

dghubble mentioned this pull request Nov 4, 2017

Google Cloud regional workers and improvements poseidon/typhoon#49

Merged

chrisst pushed a commit to chrisst/terraform-provider-google that referenced this pull request Nov 9, 2018

Add new resource region_instance_group_manager (hashicorp#394)

de25051

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

Add fields to InterconnectAttachment to allow for PARTNER interconnec…

a38fb51

…ts (hashicorp#394) Signed-off-by: Modular Magician <[email protected]>

ghost locked and limited conversation to collaborators Mar 31, 2020

paultyng unassigned paddycarver Aug 6, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add new resource region_instance_group_manager #394

Add new resource region_instance_group_manager #394

rosbo commented Sep 6, 2017 •

edited

Loading

paddycarver left a comment

paddycarver Sep 8, 2017

rosbo Sep 8, 2017

paddycarver Sep 8, 2017

rosbo Sep 8, 2017

paddycarver Sep 8, 2017

paddycarver Sep 8, 2017

rosbo Sep 8, 2017

paddycarver Sep 8, 2017

paddycarver Sep 8, 2017

rosbo Sep 8, 2017

rosbo commented Sep 8, 2017

paddycarver Sep 8, 2017

rosbo Sep 8, 2017

paddycarver Sep 9, 2017

paddycarver Sep 8, 2017

paddycarver Sep 8, 2017

rosbo Sep 8, 2017 •

edited

Loading

paddycarver Sep 9, 2017

ghost commented Mar 31, 2020

Add new resource region_instance_group_manager #394

Add new resource region_instance_group_manager #394

Conversation

rosbo commented Sep 6, 2017 • edited Loading

paddycarver left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rosbo commented Sep 8, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rosbo Sep 8, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghost commented Mar 31, 2020

rosbo commented Sep 6, 2017 •

edited

Loading

rosbo Sep 8, 2017 •

edited

Loading