Import for compute_address now requires region/name. Changed the ID format. #378

rosbo · 2017-08-31T23:24:50Z

Fixes #374

Also performed the migration to the new ID format for backward compatibility.

make testacc TESTARGS="-run TestAccComputeAddress_basic" TEST="./google"
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./google -v -run TestAccComputeAddress_basic -timeout 120m
=== RUN   TestAccComputeAddress_basic
--- PASS: TestAccComputeAddress_basic (23.69s)
PASS
ok  	github.com/terraform-providers/terraform-provider-google/google	23.831s

make testacc TESTARGS="-run TestAccComputeAddress_importBasic" TEST="./google"
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./google -v -run TestAccComputeAddress_importBasic -timeout 120m
=== RUN   TestAccComputeAddress_importBasic
--- PASS: TestAccComputeAddress_importBasic (23.08s)
PASS
ok  	github.com/terraform-providers/terraform-provider-google/google	23.217s

@paddycarver We use this PR to discuss also including the project in the import string and the the relative part of the self link (anything right of the version) as the resource id.

…ormat. Also performed the migration to the new ID format for backward compatibility.

selmanj · 2017-09-01T20:41:36Z

@rosbo What do you think about using the self_link here as the import id instead? Is that more/less confusing?

rosbo · 2017-09-05T16:32:54Z

I think having the user type `projects/MY-PROJECT/region/MY-REGION/address/MY-ADDRESS is very confusing for the user because it is different for the import for all the other resources. It also requires more typing.

selmanj · 2017-09-05T16:55:54Z

Right, but let's say a user wants to import a resource that's not within their provider's default project. Wouldn't they have the same problem as above re: zone?

I think we all discussed the idea of using self-link as id here but I can't remember where we landed. Pinging @paddycarver for comments

rosbo · 2017-09-05T17:08:15Z

All resources are associated with a project. Right now, we don't support import from a project different than the default project. If we change it to support project. I think we should do it in one swoop to avoid having different resources working in different ways.

We discussed using the relative part of the self-link as the resource ID which is a good idea I think. I am not sure if we should use the same for the import ID. i.e.:

terraform import my-resource.a-name projects/MY-PROJECT/region/MY-REGION/address/MY-ADDRESS
Or
terraform import my-resource.a-name MY-PROJECT/MY-REGION.MY-ADDRESS

selmanj · 2017-09-05T17:22:15Z

We discussed using the relative part of the self-link as the resource ID which is a good idea I think. I am not sure if we should use the same for the import ID. i.e.:

Aren't these the same? Can you distinguish between the two during import?

rosbo · 2017-09-05T17:41:53Z

Yes you can distinguish between the two during import. You just need to write your own import function instead of using a pass through. See:
https://github.com/terraform-providers/terraform-provider-google/blob/master/google/resource_compute_subnetwork.go#L319

Note: You can call SetId in this function to construct the self-link from the different parts received in the import ID.

selmanj · 2017-09-05T20:48:56Z

Thanks, I didn't know that!

I think that if Id is the self-link, we should allow importing from the self-link as well; naturally the first thing a person will do when checking to see what the id is is look at their state file and copy/paste.

Given what you've pasted above, maybe we could support importing fragments of the self link? E.g. if the id has no '/' symbols then use the default provider region/project, if it contains region/foo/address/bar then use the provided region, etc...

What do you think?

rosbo · 2017-09-06T17:21:19Z

Good idea.

I will create a generic ID class we can reuse for all the resources. Each resource will instantiate this class by specifying the parts of its id. It will then deal with the various import id format parsing, generating the id and so on. I suggest to do it in a separate PR.

4 import id format can be supported:

project/name
name
relative self-link
full self-link

rosbo · 2017-09-06T17:21:53Z

I added the project to the id for this PR to avoid having to do another id migration.

rosbo · 2017-09-06T17:32:04Z

make testacc TESTARGS="-run TestAccComputeAddress_" TEST="./google"
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./google -v -run TestAccComputeAddress_ -timeout 120m
=== RUN TestAccComputeAddress_importBasic
--- PASS: TestAccComputeAddress_importBasic (24.35s)
=== RUN TestAccComputeAddress_basic
--- PASS: TestAccComputeAddress_basic (23.96s)
PASS
ok github.com/terraform-providers/terraform-provider-google/google 48.438s

selmanj · 2017-09-06T23:42:44Z

I love the import idea.

Just one last thing; the id here is not the self-link (or even a fragment of the self link as it's missing the projects, regions and addresses fragments). Wouldn't it be cleaner to just use the self-link? Customers can already see the self-link in gcloud which would make it easy to copy and paste.

rosbo · 2017-09-07T00:49:48Z

I had planned to support the 4 cases listed above in a different PRs but finally decided to do it in this one to avoid having to do multiple id migration. (this addresses your last comment about supporting self_link).

I will generalize the computeAddressId struct and the parseComputeAddressId to be able to reuse it for all resources in my next PR.

rosbo · 2017-09-07T00:50:33Z

TF_ACC=1 go test ./google -v -run TestAccComputeAddress_ -timeout 120m
=== RUN   TestAccComputeAddress_importBasic
--- PASS: TestAccComputeAddress_importBasic (23.62s)
=== RUN   TestAccComputeAddress_basic
--- PASS: TestAccComputeAddress_basic (23.54s)
PASS
ok  	github.com/terraform-providers/terraform-provider-google/google	47.321s

go test -i ./google || exit 1
echo ./google | \
		xargs -t -n4 go test -run TestComputeAddressIdParsing -timeout=30s -parallel=4
go test -run TestComputeAddressIdParsing -timeout=30s -parallel=4 ./google
ok  	github.com/terraform-providers/terraform-provider-google/google	0.122s
/Users/rosbo/go/src/github.com/terraform-providers/terraform-provider-google

selmanj · 2017-09-07T15:13:28Z

google/resource_compute_address.go

+
+var (
+	computeAddressIdTemplate = "projects/%s/regions/%s/addresses/%s"
+	computeAddressLinkRegex  = regexp.MustCompile("projects/(.*)/regions/(.*)/addresses/(.*)$")


Minor quibble: using .+ is preferred to .* as it excludes a few invalid compute regexes that match (that probably won't ever occur but oh well)

Totally agree, fixed.

selmanj · 2017-09-07T15:14:01Z

google/resource_compute_address.go

-	if err != nil {
-		return err
-	}
+	log.Println("[rosbo] resourceComputeAddressRead", addressId)


Oops this guy snuck in

Oupps. Removed.

selmanj · 2017-09-07T15:16:51Z

google/resource_compute_address.go

+			Region:  parts[0],
+			Name:    parts[1],
+		}, nil
+	}


Missing one more case (only specifying name, which is the current behavior), or was this not included on purpose?

It was on purpose but now that I think about it, I see no good reason for not supporting this case. Support added.

selmanj

Left some minor comments, but it looks great!

I think this will make importing a lot easier 👍

This allows to import address from region and project different than the default project.

[Terraform] Gcloud scheduler

ghost · 2020-03-31T13:55: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!

Import for compute_address now requires region/name. Changed the ID f…

2ce3872

…ormat. Also performed the migration to the new ID format for backward compatibility.

rosbo requested review from paddycarver and selmanj August 31, 2017 23:24

rosbo added 2 commits September 6, 2017 10:14

Add project to compute address id

98c1197

Fix migrate state project

cc9e336

Support multiple import id format

5e70850

rosbo added 2 commits September 6, 2017 18:30

Update documentation

900c90a

Fix TestComputeAddressMigrateState

98614ec

selmanj reviewed Sep 7, 2017

View reviewed changes

selmanj approved these changes Sep 7, 2017

View reviewed changes

rosbo added 2 commits September 7, 2017 09:40

Add import by name only

122ee50

Fix docs

d56c327

rosbo merged commit 5be9d28 into hashicorp:master Sep 7, 2017

rosbo deleted the fix-import-compute-address branch September 7, 2017 17:38

paddycarver mentioned this pull request Sep 8, 2017

Add new resource region_instance_group_manager #394

Merged

danawillow mentioned this pull request Sep 13, 2017

Tests that rely on google_compute_address are failing #418

Closed

rosbo mentioned this pull request Sep 13, 2017

Save region name-only instead of the self-link in compute_address #422

Merged

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

Import for compute_address supports multiple id formats. (hashicorp#378)

9fc8c0c

This allows to import address from region and project different than the default project.

danawillow mentioned this pull request Oct 25, 2017

Adds support for creating KMS KeyRing resources #518

Merged

3 tasks

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

Import for compute_address supports multiple id formats. (hashicorp#378)

95b1c4e

This allows to import address from region and project different than the default project.

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

Merge pull request hashicorp#378 from modular-magician/codegen-pr-1102

15e2437

[Terraform] Gcloud scheduler

ghost locked and limited conversation to collaborators Mar 31, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Import for compute_address now requires region/name. Changed the ID format. #378

Import for compute_address now requires region/name. Changed the ID format. #378

rosbo commented Aug 31, 2017

selmanj commented Sep 1, 2017

rosbo commented Sep 5, 2017

selmanj commented Sep 5, 2017

rosbo commented Sep 5, 2017

selmanj commented Sep 5, 2017

rosbo commented Sep 5, 2017

selmanj commented Sep 5, 2017

rosbo commented Sep 6, 2017

rosbo commented Sep 6, 2017 •

edited

Loading

rosbo commented Sep 6, 2017

selmanj commented Sep 6, 2017

rosbo commented Sep 7, 2017

rosbo commented Sep 7, 2017

selmanj Sep 7, 2017

rosbo Sep 7, 2017

selmanj Sep 7, 2017

rosbo Sep 7, 2017

selmanj Sep 7, 2017

rosbo Sep 7, 2017

selmanj left a comment

ghost commented Mar 31, 2020

Import for compute_address now requires region/name. Changed the ID format. #378

Import for compute_address now requires region/name. Changed the ID format. #378

Conversation

rosbo commented Aug 31, 2017

selmanj commented Sep 1, 2017

rosbo commented Sep 5, 2017

selmanj commented Sep 5, 2017

rosbo commented Sep 5, 2017

selmanj commented Sep 5, 2017

rosbo commented Sep 5, 2017

selmanj commented Sep 5, 2017

rosbo commented Sep 6, 2017

rosbo commented Sep 6, 2017 • edited Loading

rosbo commented Sep 6, 2017

selmanj commented Sep 6, 2017

rosbo commented Sep 7, 2017

rosbo commented Sep 7, 2017

selmanj Sep 7, 2017

Choose a reason for hiding this comment

rosbo Sep 7, 2017

Choose a reason for hiding this comment

selmanj Sep 7, 2017

Choose a reason for hiding this comment

rosbo Sep 7, 2017

Choose a reason for hiding this comment

selmanj Sep 7, 2017

Choose a reason for hiding this comment

rosbo Sep 7, 2017

Choose a reason for hiding this comment

selmanj left a comment

Choose a reason for hiding this comment

ghost commented Mar 31, 2020

rosbo commented Sep 6, 2017 •

edited

Loading