provider/google: enable use of URI as snapshot name when creating a disk

[tpoindessous]

Hi

It's possible to create a disk in a Google project A from a snapshot made in project B. But to do this, you need to use URI of the snapshot in project B.

This PR enable this usage.

I tested it :

• I made a snapshot of a disk in project A (named "snapshot-1")
• I used terraform to create a disk in project B, using the snapshot.

resource "google_compute_disk" "default" {
  name  = "test-disk-gbook"
  type  = "pd-ssd"
  zone  = "europe-west1-d"
  snapshot = "https://www.googleapis.com/compute/v1/projects/sk5-formations-thomas-build/global/snapshots/snapshot-1"
}

Thanks.

Thomas

[tpoindessous]

Maybe I should test against ^https://www.googleapis.com/compute

This version of the PR doesn't work if the snapshot's name begins by ^http

[paddycarver]

Hey @tpoindessous, thanks for the PR! I'm on board with the general idea, but I'd love to see a test case added that checks this new functionality. Feel free to let me know if you need help with that!

[tpoindessous]

Hi @paddyforan,

just to be sure of what I need to do : you want a new test case in builtin/providers/google/resource_compute_disk_test.go or you want a full .tf which prove that my PR really works in the real world (I tested that) 😄 ?

I think you ask for a new test case in resource_compute_disk_test.go, so I will look how to add a new one.

Thanks.

[paddycarver]

Hey @tpoindessous!

We like to update the tests associated with a resource (builtin/providers/google/resource_compute_disk_test.go in this case) whenever we add a new set of functionality or fix a bug. We run these tests every night, and they let us know if we've accidentally broken anything, or if the APIs have shifted underneath us. They're important to ensuring the health and stability of the project.

In this case, I'd suggest a test based on TestAccComputeDisk_basic but that has a snapshot attribute set, to a name and to a URL, to ensure both still work. This can get complicated around the test setup because it involves multiple projects, but I can help you with that, as necessary. :)

Or if you get stymied writing tests, that's ok! It's a tricky test case to write. I'm always happy to help write them--the easiest way to do that is to give me push access to the branch.

However you'd like to proceed, I'm here to help, and appreciate the time you've invested in this.

[tpoindessous]

Ok, I read the doc and I found that I needed to code a test acceptance ( make testacc TEST=./builtin/providers/google )

I coded a new test but it needs a snapshot's URI.

As a first attempt to submit this PR, I will add a new mandatory environnement variable (like in provider_test.go).

Then I will have a look to issue #11690, and then I will be able to create a new disk, snapshot it and use this snapshot to create another disk. And my test acceptance will be good.

Thanks

[tpoindessous]

Hi

I added a new test acceptance.

GOOGLE_COMPUTE_DISK_SNAPSHOT_URI must be set to a valid snapshot's uri like one of the output of
gcloud compute snapshots list --uri

GOOGLE_COMPUTE_DISK_SNAPSHOT_URI should be replaced by a proper snapshot made by TF (#11690)

Thanks.

[tpoindessous]

I'm waiting for inclusion of #12482 and then I will re-write the test acceptance and use auto-created snapshot.

Thanks.

[paddycarver]

Hey @tpoindessous. Wow, talk about going above and beyond the call of duty. Thanks for this and #12482, and sorry for the delay on these. Going to strive for a faster turnaround time on future responses.

I'd love to wait on this one for #12482 (which looks pretty near merge-ready) and use the new snapshot resource in conjunction with the google_project resource to create the snapshot in a different project in the test. Then we know this functionality works and can keep testing it for regressions and changes in the API.

Thanks for all the time you've put into this!

[paddycarver]

Hey @tpoindessous, now that #12482 is merged, are you interested in adding the test case for this? Or would you like me to go ahead and write the test and shepherd this through to merging? Up to you, either way. Thanks for all the work!

[tpoindessous]

Hi @paddycarver

Yes, I will update this PR to add corrects tests.

Thanks !

[tpoindessous]

Hi

I found time to change the tests :

$ make testacc TEST=./builtin/providers/google TESTARGS='-run=TestAccComputeDisk_from_snapshot_uri'
==> Checking that code complies with gofmt requirements...
go generate $(go list ./... | grep -v /terraform/vendor/)
2017/05/22 23:45:36 Generated command/internal_plugin_list.go
TF_ACC=1 go test ./builtin/providers/google -v -run=TestAccComputeDisk_from_snapshot_uri -timeout 120m
=== RUN   TestAccComputeDisk_from_snapshot_uri
--- PASS: TestAccComputeDisk_from_snapshot_uri (121.13s)
PASS
ok  	github.com/hashicorp/terraform/builtin/providers/google	121.152s

it's working !

Thanks.

[tpoindessous]

Oh God ... I did a rebase and now, my PR is huge :(

Sorry about that ... I can try to rollback this rebase or I can do a new branch from terraform/master and re-apply my changes.

As you want ! (I added you to my repo, only if you know a quick method to correct this mess and you want to do it yourself, but this is very optional)

Thanks

[paddycarver]

Ack @tpoindessous. Sorry for the closure. I figured a force-push back to the appropriate point would fix it, but nope. :( I opened #14774 to continue to track this. :( Sorry for the problem! Hope I didn't break anything too badly.

[ghost]

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.