New data source: compute region instance group #851

nat-henderson · 2017-12-13T02:23:13Z

This closes #765... I think! :) This is my first data source - let me know if I've missed something, please! The timeout logic I wrote in the manager resource could use some special attention since I'm relatively new to Go.

…ager.

rosbo · 2017-12-13T18:07:51Z

Vet found suspicious constructs. Please check the reported constructs
and fix them if necessary before submitting the code for review.
make: *** [vet] Error 1
The command "make vet" exited with 2.

rosbo · 2017-12-13T18:08:24Z

google/data_source_google_compute_region_instance_group_manager.go

+func dataSourceComputeRegionInstanceGroupRead(d *schema.ResourceData, meta interface{}) error {
+	config := meta.(*Config)
+	var project, region, name string
+	// add self_link parsing here.


It looks like you've already added self_link parsing below

Ha, true, removed.

rosbo · 2017-12-13T18:09:33Z

google/data_source_google_compute_region_instance_group_manager.go

+				Computed: true,
+			},
+
+			"items": {


We tend to not pluralize field names (no always followed). Would that make more sense to call it instance instead of item?

No strong feelings - I thought it would be best to stick to exactly the field names that come back from the /listInstances call. Do you prefer instance to items given that items is what's used here?
https://cloud.google.com/compute/docs/reference/latest/regionInstanceGroups/listInstances

FWIW I agree that "items" is a lousy name! It's probably the least specific you can be about what's in the list.

instance is definitely better. I think actually the pluralized version would be better since it is read only.

people will use it for interpolation in their data source this way:

instance = "${data.google_compute_region_instance_group_manager.instances.0.self_link}"

Agreed, and done.

rosbo · 2017-12-13T18:13:19Z

google/resource_compute_region_instance_group_manager.go

@@ -110,6 +112,12 @@ func resourceComputeRegionInstanceGroupManager() *schema.Resource {
 				Optional: true,
 			},

+			"wait_for_instances": &schema.Schema{


Documentation for this

rosbo · 2017-12-13T18:15:40Z

google/resource_compute_region_instance_group_manager.go

+			}
+		}
+	}()
+	select {


why using goroutine if you are blocking the main thread anyway here?

In this case, because there are two goroutines - time.After() returns a channel that sends "true" after the duration specified. This serves as an (apparently) idiomatic timeout mechanism. I like it, but willing to reformulate it if you prefer. Definitely need to comment it.

You could probably rely on state.WaitForState() and wait until the creating + creatingwhitoutretries is equal to 0.

It would be more consistent with the rest of the waiting logic in Terraform

Aha, yes, state.WaitForState is a much better version of what I've written here. Will do!

…m in resourceComputeRegionInstanceGroupManagerCreate.

danisla · 2017-12-13T19:22:54Z

I really like the addition of the wait_for_instances argument! Can we get that on the google_compute_instance_group_manager resource too?

nat-henderson · 2017-12-13T21:42:00Z

I think so! I've rewritten it to be extensible. I'll do that in a followup PR.

…coded ones.

nat-henderson · 2017-12-14T01:11:41Z

==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./google -v -run=TestAccDataSourceRegionInstanceGroup* -timeout 120m
=== RUN   TestAccDataSourceRegionInstanceGroupManager
--- PASS: TestAccDataSourceRegionInstanceGroupManager (159.80s)
PASS
ok      github.com/terraform-providers/terraform-provider-google/google 159.811s

rosbo

Just missing the documentation for this data source

nat-henderson · 2017-12-14T01:25:52Z

Added docs - how can I preview them to make sure they look right?

Also,

==> Checking that code complies with gofmt requirements...                     
TF_ACC=1 go test ./google -v -run=TestAccRegionInstanceGroup* -timeout 120m    
=== RUN   TestAccRegionInstanceGroupManager_basic                              
=== RUN   TestAccRegionInstanceGroupManager_targetSizeZero                     
=== RUN   TestAccRegionInstanceGroupManager_update                             
=== RUN   TestAccRegionInstanceGroupManager_updateLifecycle                    
=== RUN   TestAccRegionInstanceGroupManager_separateRegions                    
=== RUN   TestAccRegionInstanceGroupManager_autoHealingPolicies                
--- PASS: TestAccRegionInstanceGroupManager_update (180.35s)                   
--- PASS: TestAccRegionInstanceGroupManager_autoHealingPolicies (243.64s)      
--- PASS: TestAccRegionInstanceGroupManager_basic (243.75s)                    
--- PASS: TestAccRegionInstanceGroupManager_targetSizeZero (47.80s)            
--- PASS: TestAccRegionInstanceGroupManager_separateRegions (305.04s)          
--- PASS: TestAccRegionInstanceGroupManager_updateLifecycle (156.64s)          
PASS                                   
ok      github.com/terraform-providers/terraform-provider-google/google 337.010s

rosbo · 2017-12-14T01:29:44Z

You can follow these instructions:
https://github.com/hashicorp/terraform-website#running-the-site-locally

rosbo · 2017-12-14T01:31:02Z

website/docs/d/datasource_compute_region_instance_group.html.markdown

+page_title: "Google: google_compute_instance_group"
+sidebar_current: "docs-google-datasource-compute-instance-group"
+description: |-
+  Get a Compute Region Instance Group within GCE.


The big feature is being able to list the instances inside the instance group. We should make that clear here.

Done! Took a lot of tiny commits to test the website - sorry about the mess, of course I'll squash before I merge.

* Add new data source: compute region instance group manager's groups. * Add documentation for wait_for_instances and for the timeout mechanism in resourceComputeRegionInstanceGroupManagerCreate.

Signed-off-by: Modular Magician <magic-modules@google.com>

ghost · 2020-03-30T14:03: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 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 👉 hashibot-feedback@hashicorp.com. Thanks!

Initial commit for new data source: compute region instance group man…

18f23c7

…ager.

nat-henderson requested a review from rosbo December 13, 2017 02:23

update for more generic ID. :)

1a9ecf4

rosbo suggested changes Dec 13, 2017

View reviewed changes

nat-henderson changed the title ~~Initial commit for new data source: compute region instance group~~ New data source: compute region instance group Dec 13, 2017

nat-henderson added 2 commits December 13, 2017 18:37

Remove note to self. :)

374954a

Add documentation for wait_for_instances and for the timeout mechanis…

ed217db

…m in resourceComputeRegionInstanceGroupManagerCreate.

nat-henderson added 3 commits December 13, 2017 21:42

Update to use WaitforState instead of homebrewed version.

6711ff1

Replace 'items' with 'instances'.

9f8c2ed

Update group manager to include configurable timeouts instead of hard…

dc60b64

…coded ones.

rosbo approved these changes Dec 14, 2017

View reviewed changes

rosbo suggested changes Dec 14, 2017

View reviewed changes

Add docs for region instance group.

cf5d566

rosbo approved these changes Dec 14, 2017

View reviewed changes

rosbo reviewed Dec 14, 2017

View reviewed changes

nat-henderson added 3 commits December 14, 2017 02:15

Tweak docs page for region instance group.

12f3a4a

Update docs locations

7299144

Add more information to docs about use of instance group data source

c23ad4b

nat-henderson merged commit d2611d4 into hashicorp:master Dec 14, 2017

nat-henderson deleted the region_instance_data_source branch December 14, 2017 21:35

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

Allow count=0 for guest_accelerators (hashicorp#851)

1e6799e

Signed-off-by: Modular Magician <magic-modules@google.com>

ghost locked and limited conversation to collaborators Mar 30, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

New data source: compute region instance group #851

New data source: compute region instance group #851

nat-henderson commented Dec 13, 2017

rosbo commented Dec 13, 2017

rosbo Dec 13, 2017

nat-henderson Dec 13, 2017

rosbo Dec 13, 2017

nat-henderson Dec 13, 2017

rosbo Dec 13, 2017

nat-henderson Dec 13, 2017

rosbo Dec 13, 2017

nat-henderson Dec 13, 2017

rosbo Dec 13, 2017

nat-henderson Dec 13, 2017

rosbo Dec 13, 2017

nat-henderson Dec 13, 2017

danisla commented Dec 13, 2017

nat-henderson commented Dec 13, 2017

nat-henderson commented Dec 14, 2017

rosbo left a comment

nat-henderson commented Dec 14, 2017

rosbo commented Dec 14, 2017

rosbo Dec 14, 2017

nat-henderson Dec 14, 2017

ghost commented Mar 30, 2020

New data source: compute region instance group #851

New data source: compute region instance group #851

Conversation

nat-henderson commented Dec 13, 2017

rosbo commented Dec 13, 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

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

danisla commented Dec 13, 2017

nat-henderson commented Dec 13, 2017

nat-henderson commented Dec 14, 2017

rosbo left a comment

Choose a reason for hiding this comment

nat-henderson commented Dec 14, 2017

rosbo commented Dec 14, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghost commented Mar 30, 2020