Support `distributionPolicy` when creating regional instance group managers. #1092

tobz · 2018-02-15T18:57:05Z

This addresses #549.

This one is sort of a doozy because it's both an un-updatable attribute and beta feature. The format required for the zones to be specified in was also a little tricky. Definitely looking for thoughts on how my approach could be better. I chose what I thought was the best user experience -- users specifying the "naked" zone -- and augmenting it behind-the-scenes.

TF_ACC=1 go test ./google -v -run=TestAccRegionInstanceGroupManager -timeout 120m
=== RUN   TestAccRegionInstanceGroupManager_basic
=== RUN   TestAccRegionInstanceGroupManager_targetSizeZero
=== RUN   TestAccRegionInstanceGroupManager_update
=== RUN   TestAccRegionInstanceGroupManager_updateLifecycle
=== RUN   TestAccRegionInstanceGroupManager_separateRegions
=== RUN   TestAccRegionInstanceGroupManager_autoHealingPolicies
=== RUN   TestAccRegionInstanceGroupManager_distributionPolicy
--- PASS: TestAccRegionInstanceGroupManager_targetSizeZero (49.02s)
--- PASS: TestAccRegionInstanceGroupManager_updateLifecycle (218.13s)
--- PASS: TestAccRegionInstanceGroupManager_update (241.82s)
--- PASS: TestAccRegionInstanceGroupManager_distributionPolicy (244.57s)
--- PASS: TestAccRegionInstanceGroupManager_autoHealingPolicies (244.72s)
--- PASS: TestAccRegionInstanceGroupManager_separateRegions (255.48s)
--- PASS: TestAccRegionInstanceGroupManager_basic (306.53s)
PASS
ok  	github.com/terraform-providers/terraform-provider-google/google	306.793s

…nagers.

tobz · 2018-03-01T18:18:15Z

@rosbo @danawillow @ndmckinley @paddycarver

How do PRs get assigned to someone for review? There doesn't seem be any documentation explaining the process, and no activity at all for two weeks is sadface. :(

danawillow · 2018-03-01T18:43:16Z

Sorry @tobz, between the flu that's been going around, some travel, bugs we've needed to fix quickly, and other project work, this one seems to have slipped through the cracks. I'll take a look at it this afternoon :)

danawillow · 2018-03-02T03:11:49Z

google/resource_compute_region_instance_group_manager.go

@@ -145,6 +149,18 @@ func resourceComputeRegionInstanceGroupManager() *schema.Resource {
 					},
 				},
 			},
+
+			"distribution_policy": &schema.Schema{


The thing that I'm not totally sold on for this approach is that with a name like "distribution_policy", it should really be a list of policies rather than a list of zones.

As much as the nesting is weird for just one field, it does make the resource more maintainable in the future (especially once we start generating resources). If the API maintainers decide to add another field to that block for example, we'd then have to deprecate this field and make it into a nested block, which is a bit of a pain.

Happy to hear a rebuttal though :)

I'm not against such a change.

Like I mentioned in the opening comments, I made the choices I did to present a good UX to the end-user: ultimately, this attribute is simply for saying which zones in a region should be used.

I guess I'd be curious about how easily users could splat out a list of zones into such a list of nested blocks. I'm not sure I've seen that or done it myself.

Alright, so I ended up putting you suggestion into action. Now it's a list of objects, matching the structure of the API.

Sorry to go back and forth on this- your comment about being able to splat is a good one. I asked around a bit and the suggestion I got that I like is to just have a top-level list of strings called distribution_policy_zones. That leaves room to deprecate it later in favor of a straight distribution_policy object in case the API adds more things, but makes a good UX for the time being.

You're killin' me, smalls! :)

Alright, I'll go with that approach.

danawillow · 2018-03-02T03:12:34Z

google/resource_compute_region_instance_group_manager.go

@@ -227,6 +245,7 @@ func getManager(d *schema.ResourceData, meta interface{}) (*computeBeta.Instance

 	if err != nil {
 		handleNotFoundError(err, d, fmt.Sprintf("Region Instance Manager %q", d.Get("name").(string)))
+		return nil, err


Good catch! This should just be return handleNotFoundError(...)

So, the signature doesn't actually support that specific code snippet.

What's curious, though, is that we'd be returning nil, nil in the case of a not found error if we used the output of handleNotFoundError. This feels totally wrong, but I'm willing to deal if that's simply the way things shook out in the provider.

So what we want overall is to make sure that if the IGM isn't found when we try to read it, that we remove it from state. This is a totally valid case and shouldn't cause Terraform to error. So we don't want to return the actual error here, because then a not found will cause our terraform command to fail. Here's how we handle it for resource_compute_instance:
https://github.com/terraform-providers/terraform-provider-google/blob/ab385ff5740b7a53ca6302d6c6214c273e178aa0/google/resource_compute_instance.go#L773-L776
https://github.com/terraform-providers/terraform-provider-google/blob/ab385ff5740b7a53ca6302d6c6214c273e178aa0/google/resource_compute_instance.go#L588-L603

Makes sense. I'll take care of it.

danawillow · 2018-03-02T03:30:22Z

google/resource_compute_region_instance_group_manager.go

+				Type:     schema.TypeSet,
+				Optional: true,
+				ForceNew: true,
+				Computed: true,


I don't think this needs to be computed, does it?

I'd have to go back and test with/without it, but I believe it needs to be computed because while the API will let you pass in, say, us-central1-a, whenever you retrieve the info for the rIGM, you will get back the full zone URL i.e. https://googlecloud.com/ur_project/zones/us-central1-a, and then TF would freak out thinking there needed to be an update.

Let me try to retest that one. I'm also open to ideas on generating the full one URL behind-the-scenes, but it seemed like a bunch of extra work at the time, and I wasn't really familiar with a way to easily nab the current project, etc, to build it.

Nah, that's what the diffsuppress is for. Computed means that if it isn't set, the API will provide a value (so don't show a diff when the config doesn't have a value for that field).

So, I went back. This is actually required.

If we use the beta API version, the API actually fills in the distribution policy for you, based on -- what appears to be -- the zones the rIGM seeds itself with during creation.

danawillow · 2018-03-02T03:46:09Z

google/resource_compute_region_instance_group_manager.go

+		}
+	}
+
+	return schema.NewSet(schema.HashSchema(&schema.Schema{


I think just return zones will work (and then update the return type)

FWIW, I tried this, to no avail.

I just uploaded a commit where this seems to work. Let me know what you think!

tobz · 2018-03-08T13:28:06Z

@danawillow I think this is ready for another pass.

This approach lets us more simply allow a list of zones to use, while providing a deprecation path for implementing the distribution policy field more holistically, avoiding backwards-incompatible changes.

tobz · 2018-03-09T15:26:20Z

@danawillow Alright, I think we've arrived. :P

danawillow · 2018-03-09T18:35:03Z

FYI I just pushed a typo fix and the change in flatten that I was talking about. If that seems all right to you, I'm ready to merge this :)

tobz · 2018-03-09T18:47:50Z

Thanks for the fix-ups! I now see what you meant by changing the flatten method. :)

Tests pass; LGTM.

tobz · 2018-03-09T19:08:35Z

For my own edification, when are releases usually cut?

danawillow · 2018-03-09T19:19:49Z

We aim for every 2 weeks, but sometimes things come up and it stretches a bit. If all goes according to plan, we'll do a release on Monday, but no promises.

tobz · 2018-03-12T19:33:46Z

Late response: yeah, not in a rush, just curious. :)

Thanks again!

…nagers. (hashicorp#1092) * Support `distributionPolicy` when creating regional instance group managers. * Better match the API structure of distributionPolicy. * Switch to "distribution_policy_zones". This approach lets us more simply allow a list of zones to use, while providing a deprecation path for implementing the distribution policy field more holistically, avoiding backwards-incompatible changes. * fix typo * use slice instead of Set for flattenDP

Signed-off-by: Modular Magician <[email protected]>

ghost · 2020-03-29T14:20:38Z

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!

Support distributionPolicy when creating regional instance group ma…

8d79582

…nagers.

danawillow self-requested a review March 1, 2018 18:43

danawillow self-assigned this Mar 1, 2018

danawillow reviewed Mar 2, 2018

View reviewed changes

Better match the API structure of distributionPolicy.

1860f21

Switch to "distribution_policy_zones".

210eaa9

This approach lets us more simply allow a list of zones to use, while providing a deprecation path for implementing the distribution policy field more holistically, avoiding backwards-incompatible changes.

danawillow added 2 commits March 9, 2018 10:30

fix typo

bfc09c9

use slice instead of Set for flattenDP

8b41ec0

danawillow approved these changes Mar 9, 2018

View reviewed changes

danawillow merged commit 08e81f5 into hashicorp:master Mar 9, 2018

danawillow mentioned this pull request Mar 9, 2018

Add support for providing zones in google_compute_region_instance_group_manager #549

Closed

tobz deleted the tlawrence_add_dist_policy_to_region_igm branch March 9, 2018 19:06

modular-magician added a commit to modular-magician/terraform-provider-google that referenced this pull request Sep 27, 2019

Fix IAM policy behavior regression for empty members (hashicorp#1092)

4447526

Signed-off-by: Modular Magician <[email protected]>

ghost locked and limited conversation to collaborators Mar 29, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support `distributionPolicy` when creating regional instance group managers. #1092

Support `distributionPolicy` when creating regional instance group managers. #1092

tobz commented Feb 15, 2018

tobz commented Mar 1, 2018

danawillow commented Mar 1, 2018

danawillow Mar 2, 2018

tobz Mar 2, 2018

tobz Mar 6, 2018

danawillow Mar 8, 2018

tobz Mar 8, 2018

danawillow Mar 2, 2018

tobz Mar 6, 2018

danawillow Mar 8, 2018

tobz Mar 8, 2018

danawillow Mar 2, 2018

tobz Mar 2, 2018

danawillow Mar 2, 2018

tobz Mar 6, 2018

danawillow Mar 2, 2018

tobz Mar 9, 2018

danawillow Mar 9, 2018

tobz commented Mar 8, 2018

tobz commented Mar 9, 2018

danawillow commented Mar 9, 2018

tobz commented Mar 9, 2018

tobz commented Mar 9, 2018

danawillow commented Mar 9, 2018

tobz commented Mar 12, 2018

ghost commented Mar 29, 2020

Support distributionPolicy when creating regional instance group managers. #1092

Support distributionPolicy when creating regional instance group managers. #1092

Conversation

tobz commented Feb 15, 2018

tobz commented Mar 1, 2018

danawillow commented Mar 1, 2018

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

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

tobz commented Mar 8, 2018

tobz commented Mar 9, 2018

danawillow commented Mar 9, 2018

tobz commented Mar 9, 2018

tobz commented Mar 9, 2018

danawillow commented Mar 9, 2018

tobz commented Mar 12, 2018

ghost commented Mar 29, 2020

Support `distributionPolicy` when creating regional instance group managers. #1092

Support `distributionPolicy` when creating regional instance group managers. #1092