implements region_instance_group_manager #10907

zachgersh · 2016-12-22T18:41:18Z

This fixes #8866

Some items of note:

changed the instance size / count on the tests due to flakey deletes. It is super hard to get regional instance group managers to go away quickly.
delete code was slightly tweaked

zachgersh · 2017-01-05T15:48:50Z

This should be good to go now cc @evandbrown

greysond · 2017-01-24T17:43:16Z

Any timeline on merging this?

baskaran-md · 2017-02-01T00:51:42Z

Can this be merged?

FYI - @evandbrown

zachgersh · 2017-02-02T15:36:55Z

I can also fix this merge conflict that it now has 💃

zachgersh · 2017-02-12T17:03:49Z

@evandbrown any input here?

farioas · 2017-02-27T08:14:09Z

any updates?

zachgersh · 2017-02-27T16:13:01Z

hey @paddyforan would you be able to review this as well for me? I have a merge conflict to resolve which I can do. I am unsure of the approach on this.

vorlock · 2017-03-28T09:08:34Z

it'd be nice to have it merged

paddycarver · 2017-03-28T17:54:46Z

Hey @zachgersh! Sorry for the delay on reviewing this. I'm queuing it up. I have a couple in line ahead of it (sorry) but I'm hoping to get to it shortly. It's been on my radar for a bit now, and just keeps slipping. I know you have a couple open that need some attention. :) Thanks for contributing to Terraform, and sorry for the lengthy review timelines. Trying to do better about that.

zachgersh · 2017-03-29T00:11:34Z

@paddyforan not a problem - I am planning on putting more PRs in and am glad to be here helping out (we use terraform a bunch at Pivotal). We all have things come up and I know you all are super slammed so THANK YOU for all the work you do on terraform!

I am going to fix the conflict here and push again so it works.

paddycarver

A few comments. Overall, this looks great! I just have some questions about a few specific portions, shouldn't be anything drastic.

Thanks for your patience, and sorry for the lengthy wait on the review.

paddycarver · 2017-04-13T18:16:51Z

builtin/providers/google/resource_compute_region_instance_group_manager.go

+		Delete: resourceComputeRegionInstanceGroupManagerDelete,
+		Importer: &schema.ResourceImporter{
+			State: schema.ImportStatePassthrough,
+		},


I'd love to see an Exists function here, too, just for completeness. Though kudos on the Import support right out the gate!

paddycarver · 2017-04-13T18:32:13Z

builtin/providers/google/resource_compute_region_instance_group_manager.go

+	d.Set("instance_group", service.InstanceGroup)
+	d.Set("target_size", service.TargetSize)
+	d.Set("self_link", service.SelfLink)
+	d.Set("update_strategy", "RESTART") //this field doesn't match the manager api, set to default value


This would cause a perpetual diff if the user set update_strategy to anything else, I imagine. Why not try to read it out of d and set it to the default only if d doesn't have a value for it?

great idea!

paddycarver · 2017-04-13T18:36:30Z

builtin/providers/google/resource_compute_region_instance_group_manager.go

+
+	currentSize := int64(d.Get("target_size").(int))
+
+	for err != nil && currentSize > 0 {


Can we comment this to better describe what's going on? Why are we backing off on error? Is it possible to use a StateChangeConf to wait for this instead of custom backoff code?

lets try it and see how it goes when I run the tests.

paddycarver · 2017-04-13T18:41:51Z

builtin/providers/google/resource_compute_region_instance_group_manager_test.go

+		PreCheck:     func() { testAccPreCheck(t) },
+		Providers:    testAccProviders,
+		CheckDestroy: testAccCheckRegionInstanceGroupManagerDestroy,
+		Steps: []resource.TestStep{


I may be reading this wrong, but with only one step, I'm not sure this is actually updating anything--I think it's just creating a new resource with the desired config.

yeah, don't remember what I was thinking here but I agree this test looks incorrect - let me make sure we get an update in here.

paddycarver · 2017-04-13T18:43:08Z

website/source/docs/providers/google/r/compute_instance_group_manager.html.markdown

-page_title: "Google: google_compute_instance_group_manager"
-sidebar_current: "docs-google-compute-instance-group-manager"
+page_title: "Google: google_compute_region_instance_group_manager"
+sidebar_current: "docs-google-compute-region-instance-group-manager"


This looks like you just updated the compute_instance_group_manager docs instead of copying them and adding a new resource.

benpollard · 2017-06-01T15:01:20Z

@zachgersh I imagine you're really busy but any ideas when you are going to have time to action the review points on the PR?

zachgersh · 2017-06-01T15:09:44Z

Hey @benpollard - targeting the end of the week :D I spoke to @paddycarver about this recently. Really appreciate you pinging me as well.

bjaroszewski · 2017-06-19T07:08:20Z

Hey @benpollard, @zachgersh,
Any chances to process this PR in any foreseeable future? :)

danielcompton · 2017-06-19T22:19:18Z

@bjaroszewski this PR probably will need to move over to the new Google Terraform provider repository.

matti · 2017-07-06T19:52:41Z

@danielcompton how will that move happen? I couldn't find the issue there (https://github.com/terraform-providers/terraform-provider-google/issues)

zachgersh · 2017-07-06T19:54:29Z

I will have to move it. Cheers, Zach

…

On Jul 6, 2017, 12:52 PM -0700, Matti Paksula ***@***.***>, wrote: @danielcompton how will that move happen? I couldn't find the issue there (https://github.com/terraform-providers/terraform-provider-google/issues) — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

apparentlymart · 2017-07-06T22:02:11Z

Hi @zachgersh,

In case it's helpful, I rebased your change into the separated provider repo here:
https://github.com/terraform-providers/terraform-provider-google/compare/zachgersh-region-instance-group-manager

I didn't open a PR for it since I think it'd be better for you to copy these changes into your own fork and open a PR from there, since that way you can make any further changes you want to make in a repository where you have write access.

This was an automatic rebase with some manual conflict resolution, so I apologize if I messed anything up along the way.

mattes · 2017-08-31T03:53:19Z

It's really hard to track what the status of this is. There is a lot of duplicate tickets open. Maybe someone can close the dups.

rosbo · 2017-09-06T22:20:44Z

@paddycarver Can you close this one. I have a PR open in the new repo for the same thing:
hashicorp/terraform-provider-google#394

paddycarver · 2017-09-06T23:04:11Z

Closing this one out in favour of hashicorp/terraform-provider-google#394. Thanks for all the hard work on this, @zachgersh!

ghost · 2020-04-07T02:15:48Z

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 have found a problem that seems similar to this, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

zachgersh added 2 commits December 22, 2016 13:38

implements region_instance_group_manager

2f433e1

adds docs

1f82658

zachgersh changed the title ~~WIP: implements region_instance_group_manager~~ implements region_instance_group_manager Jan 5, 2017

radeksimko added new-resource provider/google-cloud labels Jan 18, 2017

CoryPulm mentioned this pull request Jan 26, 2017

[WIP] Feature - GCP region level autoscaler #11439

Closed

CoryPulm mentioned this pull request Mar 21, 2017

Feature gcp region autoscaler #12928

Closed

paddycarver suggested changes Apr 13, 2017

View reviewed changes

paddycarver mentioned this pull request May 18, 2017

Feature Request: support regional google instance group managers #8866

Closed

mattes mentioned this pull request Aug 31, 2017

Feature Request: support regional google instance group managers hashicorp/terraform-provider-google#45

Closed

enxebre mentioned this pull request Sep 6, 2017

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

Open

paddycarver closed this Sep 6, 2017

enxebre mentioned this pull request Sep 8, 2017

platforms/google: support for multiple Kube-masters enxebre/tectonic-installer#16

Merged

ghost locked and limited conversation to collaborators Apr 7, 2020

zachgersh deleted the region-instance-group-manager branch April 7, 2020 14:57

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

implements region_instance_group_manager #10907

implements region_instance_group_manager #10907

zachgersh commented Dec 22, 2016 •

edited

Loading

zachgersh commented Jan 5, 2017

greysond commented Jan 24, 2017

baskaran-md commented Feb 1, 2017

zachgersh commented Feb 2, 2017

zachgersh commented Feb 12, 2017

farioas commented Feb 27, 2017 •

edited

Loading

zachgersh commented Feb 27, 2017

vorlock commented Mar 28, 2017

paddycarver commented Mar 28, 2017

zachgersh commented Mar 29, 2017

paddycarver left a comment

paddycarver Apr 13, 2017

paddycarver Apr 13, 2017

zachgersh Apr 14, 2017

paddycarver Apr 13, 2017

zachgersh Apr 14, 2017

paddycarver Apr 13, 2017

zachgersh Apr 14, 2017

paddycarver Apr 13, 2017

benpollard commented Jun 1, 2017

zachgersh commented Jun 1, 2017

bjaroszewski commented Jun 19, 2017

danielcompton commented Jun 19, 2017

matti commented Jul 6, 2017

zachgersh commented Jul 6, 2017 via email

apparentlymart commented Jul 6, 2017

mattes commented Aug 31, 2017

rosbo commented Sep 6, 2017

paddycarver commented Sep 6, 2017

ghost commented Apr 7, 2020


		currentSize := int64(d.Get("target_size").(int))

		for err != nil && currentSize > 0 {

implements region_instance_group_manager #10907

implements region_instance_group_manager #10907

Conversation

zachgersh commented Dec 22, 2016 • edited Loading

zachgersh commented Jan 5, 2017

greysond commented Jan 24, 2017

baskaran-md commented Feb 1, 2017

zachgersh commented Feb 2, 2017

zachgersh commented Feb 12, 2017

farioas commented Feb 27, 2017 • edited Loading

zachgersh commented Feb 27, 2017

vorlock commented Mar 28, 2017

paddycarver commented Mar 28, 2017

zachgersh commented Mar 29, 2017

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

benpollard commented Jun 1, 2017

zachgersh commented Jun 1, 2017

bjaroszewski commented Jun 19, 2017

danielcompton commented Jun 19, 2017

matti commented Jul 6, 2017

zachgersh commented Jul 6, 2017 via email

apparentlymart commented Jul 6, 2017

mattes commented Aug 31, 2017

rosbo commented Sep 6, 2017

paddycarver commented Sep 6, 2017

ghost commented Apr 7, 2020

zachgersh commented Dec 22, 2016 •

edited

Loading

farioas commented Feb 27, 2017 •

edited

Loading