Support import for google_compute_instance_group #201

rosbo · 2017-07-14T23:19:34Z

issue #182

rileykarson · 2017-07-14T23:38:29Z

website/docs/r/compute_instance_group.html.markdown

+
+## Import
+
+Instance group can be imported using the `zone` and `name`, e.g.


Just wondering if you and @danawillow have talked about what we want to do for region and zone across all importable resources - we've checked against the provider region, or against all the provider region's zones in the past to find objects by Id alone.

I like this approach a lot - it means we don't clutter read with parsing logic by using an import func, we don't need to write migration functions, and it lets users import resources from more than 1 region/1 set of zones inside a single provider. Are we going to use the same approach across all importable resources going forward and do we have a plan to bring it to older ones?

I decided to import using zone/name because it is a valid case to have multiple instance groups with the same name across different zones.

I will sync up with @danawillow to ensure consistency on our approach for all importable resources.

After talking with @danawillow and Evan, we decided that import method should:

Always supports the fully specified resource identifier (implemented in this PR).

For convenience, we can add support for partial specification by name only if guessing the other parameters is feasible. The logic for guessing should be in the ImportFunc and not the read method to avoid polluting the read method's logic. If more than one resource match the name (e.g. two instance groups in two different zones), fail with an error message to avoid surprising behavior.

We will go ahead and submit this PR as is. I will add support for the convenient import by name only in a separate PR.

We should update the import method for all the other resources to support the fully specified resource. Otherwise, in the case where two resources have the same name in two different zones or regions, the user cannot import the resource.

SGTM! 👍

Thanks for taking the time to flesh out a concrete plan for this.

danawillow

You'll also want to add a migration fn in resource_compute_instance_group_migrate.go to change the ids of resources that already exist. The way things are right now aren't a huge deal since it looks like nothing is reading the id anymore, but just in case someone decides to in the future we should probably canonicalize all existing ids.

danawillow · 2017-07-18T18:37:08Z

make testacc TEST=./google TESTARGS='-run=TestAccComputeInstanceGroup_'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./google -v -run=TestAccComputeInstanceGroup_ -timeout 120m
=== RUN   TestAccComputeInstanceGroup_import
--- PASS: TestAccComputeInstanceGroup_import (136.04s)
=== RUN   TestAccComputeInstanceGroup_basic
--- PASS: TestAccComputeInstanceGroup_basic (92.69s)
=== RUN   TestAccComputeInstanceGroup_update
--- PASS: TestAccComputeInstanceGroup_update (134.13s)
=== RUN   TestAccComputeInstanceGroup_outOfOrderInstances
--- PASS: TestAccComputeInstanceGroup_outOfOrderInstances (93.11s)
=== RUN   TestAccComputeInstanceGroup_network
--- PASS: TestAccComputeInstanceGroup_network (154.72s)
PASS
ok  	github.com/terraform-providers/terraform-provider-google/google	610.832s

rosbo · 2017-07-18T22:14:46Z

Added ID state migration.

There was already a MigrateState function from version 0 to 1 but it wasn't properly setup. I have wired it properly. Let me know if you have any concerns about this.

danawillow · 2017-07-19T17:09:38Z

google/resource_compute_instance_group_migrate_test.go

+		}
+
+		for k, v := range tc.ExpectedAttributes {
+			if k == "id" {


empty if statement?

Oupps. Removed. The id assertions is done above.

danawillow · 2017-07-19T17:10:37Z

google/resource_compute_instance_group_migrate_test.go

 	}{
-		"change instances from list to set": {
+		"v1 to v2": {


It doesn't look like this test is actually running:

=== RUN TestComputeInstanceGroupMigrateState 2017/07/19 10:00:58 [INFO] Found Compute Instance Group State v0; migrating to v1 2017/07/19 10:00:58 [DEBUG] Attributes before migration: map[string]string{"instances.#":"1", "instances.0":"https://www.googleapis.com/compute/v1/projects/project_name/zones/zone_name/instances/instancegroup-test-1", "instances.1":"https://www.googleapis.com/compute/v1/projects/project_name/zones/zone_name/instances/instancegroup-test-0"} 2017/07/19 10:00:58 [DEBUG] Attributes after migration: map[string]string{"instances.#":"1", "instances.1519187872":"https://www.googleapis.com/compute/v1/projects/project_name/zones/zone_name/instances/instancegroup-test-0", "instances.764135222":"https://www.googleapis.com/compute/v1/projects/project_name/zones/zone_name/instances/instancegroup-test-1"} --- PASS: TestComputeInstanceGroupMigrateState (0.00s)

It's not showing anything migrating to v2

wait just kidding, I forgot to actually pull your latest commit. Nothing to see here.

danawillow · 2017-07-19T20:16:08Z

google/resource_compute_instance_group_migrate_test.go

@@ -108,6 +104,10 @@ func TestComputeInstanceGroupMigrateState_empty(t *testing.T) {
 		is = &terraform.InstanceState{}
 		is, err = resourceComputeInstanceGroupMigrateState(tc.StateVersion, is, meta)

+		// should handle non-nil but empty


I think these few lines can be removed (they're the same as what's directly above them)

danawillow · 2017-07-20T16:19:20Z

👍 , merge at will

@chrisst

/cc @chrisst

ghost · 2020-03-31T14:10:47Z

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!

rileykarson reviewed Jul 14, 2017

View reviewed changes

Support import for google_compute_instance_group

576d4d5

rosbo force-pushed the import_compute_instance_group branch from a26feea to 576d4d5 Compare July 17, 2017 17:17

danawillow reviewed Jul 18, 2017

View reviewed changes

Add ID state migration.

2633edb

danawillow reviewed Jul 19, 2017

View reviewed changes

Remove unused if block.

6f083ba

rosbo force-pushed the import_compute_instance_group branch from cca2b3e to 6f083ba Compare July 19, 2017 17:19

danawillow reviewed Jul 19, 2017

View reviewed changes

removed duplicate block in migrate test

3be3c63

rosbo merged commit 076d31d into hashicorp:master Jul 20, 2017

rosbo deleted the import_compute_instance_group branch July 20, 2017 16:21

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

Support import for google_compute_instance_group (hashicorp#201)

0c60ce0

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

Support import for google_compute_instance_group (hashicorp#201)

d05780c

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

Add tpuIpv4CidrBlock for container clusters (hashicorp#201)

00cc6cc

 /cc @chrisst

ghost locked and limited conversation to collaborators Mar 31, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support import for google_compute_instance_group #201

Support import for google_compute_instance_group #201

rosbo commented Jul 14, 2017 •

edited

Loading

rileykarson Jul 14, 2017

rosbo Jul 17, 2017

rosbo Jul 18, 2017

rileykarson Jul 18, 2017

danawillow left a comment

danawillow commented Jul 18, 2017

rosbo commented Jul 18, 2017

danawillow Jul 19, 2017

rosbo Jul 19, 2017

danawillow Jul 19, 2017

danawillow Jul 19, 2017

danawillow Jul 19, 2017

danawillow commented Jul 20, 2017

ghost commented Mar 31, 2020


		## Import

		Instance group can be imported using the `zone` and `name`, e.g.

Support import for google_compute_instance_group #201

Support import for google_compute_instance_group #201

Conversation

rosbo commented Jul 14, 2017 • edited Loading

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

danawillow left a comment

Choose a reason for hiding this comment

danawillow commented Jul 18, 2017

rosbo commented Jul 18, 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

danawillow commented Jul 20, 2017

ghost commented Mar 31, 2020

rosbo commented Jul 14, 2017 •

edited

Loading