provider/google : add a new resource : google_compute_snapshot #12482

tpoindessous · 2017-03-06T23:30:42Z

Hi

here is a WIP for addressing issue #11690

This PR adds a new resource : google_compute_snapshot

For today, I only tested creating and deleting a simple snapshot.

Also, I don't know how the documentation is made.

I will add tests acceptance in next days.

Thanks.

tpoindessous · 2017-03-07T09:51:09Z

Hi,

my test acceptance doesn't work for the moment, terraform doesn't handle very the delete command, so created disks are not deleted yet.

Thanks

…customer's self encrypted disk

tpoindessous · 2017-03-13T11:36:58Z

Hi

I corrected the test acceptance. Everything works now.

Thanks.

tpoindessous · 2017-03-14T09:34:04Z

Hi @paddyforan

I think this PR is ready to be reviewed, I added the documentation. Please tell me if I need to change something.

Thanks !

paddycarver · 2017-03-14T09:37:27Z

Hey @tpoindessous! Thanks for the PR! I literally have a browser tab open to review it, but unfortunately, it's 2:30 in the morning here, so I need to get some sleep. I'm not sure I'll get to it tomorrow, as I already queued myself a pretty big workload (trying to recover from Next!) but this is on my radar and will get a review ASAP. I just need to set aside some time to do a deep dive and understand it better, along with the problem space. Thanks so much for the patience and PR, however; both are incredibly appreciated.

paddycarver

Hey @tpoindessous! Thanks for the PR, and sorry for the delayed review. This looks super great and follows a lot of our best practices. 👏

The only comments/concerns I have are basically polish:

I prefer sourcedisk as source_disk, as it's two words. Open to discussion on that.
We're trying to make sure all new resources are implemented with an Exists function, as a best practice.
We're trying to make sure everything gets set in state how the API returns it (unless there's a good reason not to).
I'd love to see the tests expanded to verify that the fields were actually set on the resource.

Really appreciate the effort you've put into this; I think it's basically ready. If you have any problems with these changes, would like to discuss any of them, or would like someone else to take over this PR and push it past the finish line, we're here to help. :)

paddycarver · 2017-03-21T21:26:40Z

builtin/providers/google/resource_compute_snapshot.go

+				Computed: true,
+			},
+
+			"sourcedisk_encryption_key_raw": &schema.Schema{


sourcedisk => source_disk?

paddycarver · 2017-03-21T21:26:56Z

builtin/providers/google/resource_compute_snapshot.go

+				Sensitive: true,
+			},
+
+			"sourcedisk_encryption_key_sha256": &schema.Schema{


sourcedisk => source_disk?

paddycarver · 2017-03-21T21:27:00Z

builtin/providers/google/resource_compute_snapshot.go

+				Computed: true,
+			},
+
+			"sourcedisk_id": &schema.Schema{


sourcedisk => source_disk?

paddycarver · 2017-03-21T21:27:06Z

builtin/providers/google/resource_compute_snapshot.go

+				Computed: true,
+			},
+
+			"sourcedisk": &schema.Schema{


sourcedisk => source_disk?

paddycarver · 2017-03-21T21:29:35Z

builtin/providers/google/resource_compute_snapshot.go

+	d.Set("self_link", snapshot.SelfLink)
+	if snapshot.SnapshotEncryptionKey != nil && snapshot.SnapshotEncryptionKey.Sha256 != "" {
+		d.Set("snapshot_encryption_key_sha256", snapshot.SnapshotEncryptionKey.Sha256)
+	}


It may sounds silly, but do you mind setting the other fields here to what the API returns? Especially sourcedisk_encryption_key_sha256, but the others too. I know they're in the config, but setting what the API returns will help Terraform detect if something on the API side gets out of sync with something on the Terraform side. For this resource it isn't as big a deal, but it's better to follow the best practice, unless we can't.

Hi, just to be sure : I need to duplicate lines 149-151 for each other fields returned by the API ?
Thanks

Forget my question. I just understood that my code was missing setting computed fields.

Thanks

I'm not sure the if is necessary for every one, but yeah, ideally, we'd have a Set for every field in the resource that the API returns.

This is because the Set is how data from the API gets into the state, and Terraform diffs the state and the config to find if there's a difference. So the only way Terraform knows if the API and the config don't match is if we Set the data appropriately.

paddycarver · 2017-03-21T21:29:57Z

builtin/providers/google/resource_compute_snapshot.go

+	return &schema.Resource{
+		Create: resourceComputeSnapshotCreate,
+		Read:   resourceComputeSnapshotRead,
+		Delete: resourceComputeSnapshotDelete,


I'd love to see an Exists function, as well!

paddycarver · 2017-03-21T21:31:19Z

builtin/providers/google/resource_compute_snapshot_test.go

+
+		_, err := config.clientCompute.Snapshots.Get(
+			config.Project, rs.Primary.ID).Do()
+		if err == nil {


What happens if there's a server error/network error/anything but a 404?

Would be it OK to add this test :

if gerr, ok := err.(*googleapi.Error); ok && gerr.Code == 404 {

That would be perfect!

(Assuming, of course, we check for non-404 errors and return that error, as well.)

paddycarver · 2017-03-21T21:35:19Z

builtin/providers/google/resource_compute_snapshot_test.go

+	}
+}
+
+func testAccCheckSnapshotEncryptionKey(n string, snapshot *compute.Snapshot) resource.TestCheckFunc {


I'd love to see checks for the other fields, as well. They could probably be combined into testAccCheckComputeSnapshotExists.

paddycarver · 2017-03-21T21:35:36Z

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

+
+* `zone` - (Required) The zone where the source disk is located.
+
+* `disk` - (Required)


A description would be great here.

…snapshot

tpoindessous

Hi

I think I did all that you asked for.

I don't know how to test "Exists" function and I don't know what its purpose. Can you, please, send me to the right doc ? I would like to understand its usage.

Thanks !

paddycarver

Looks super great, thanks @tpoindessous. Couple minor things, mostly doc updates. The only other thing I'm curious about is why we have both disk and source_disk. Couldn't we just make source_disk required and not computed and get rid of disk? Wouldn't that be the same effect, or am I missing something?

As for testing Exists, Exists just adds a layer of code safety to the resource by letting Terraform detect whether it exists or not, so Terraform doesn't need to make API calls to figure out that it's recreating something that already exists, or deleting something that doesn't exist, etc. I'm not sure about testing it--I've never seen dedicated tests. Generally, if the configs work with the Exists function in place, that's exercising that code path, as far as I know.

paddycarver · 2017-03-24T00:05:42Z

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

+
+- - -
+
+* `sourcedisk_encryption_key_raw` - (Optional) A 256-bit [customer-supplied encryption key]


Think this needs to be updated to source_disk_encryption_key_raw.

paddycarver · 2017-03-24T00:06:05Z

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

+    [customer-supplied encryption key](https://cloud.google.com/compute/docs/disks/customer-supplied-encryption)
+    that protects this resource.
+
+* `sourcedisk_encryption_key_sha256` - The [RFC 4648 base64]


Think this needs to be updated to source_disk_encryption_key_sha256.

paddycarver · 2017-03-24T00:06:18Z

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

+    [customer-supplied encryption key](https://cloud.google.com/compute/docs/disks/customer-supplied-encryption)
+    that protects the source disk.
+
+* `sourcedisk_id` - The ID value of the source disk used to create this snapshot.


Think this needs to be updated to source_disk_id.

Also, how is this different from source_disk?

paddycarver · 2017-03-24T00:06:33Z

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

+    This value may be used to determine whether the snapshot was taken from the
+    current or a previous instance of a given disk name.
+
+* `sourcedisk` - The source disk used to create this snapshot.


Think this needs to be updated to source_disk.

paddycarver · 2017-03-24T00:07:19Z

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

+
+* `zone` - (Required) The zone where the source disk is located.
+
+* `disk` - (Required) The disk which will be used as the source of the snapshot


How is disk different from source_disk? Won't they always match?

…ce_disk. Deleted sourcedisk_id

tpoindessous · 2017-03-27T09:44:13Z

Hi @paddyforan

I first tried to replicate Google API to Terraform, but since Terraform keeps the state, some attributes are not needed, as you requested.

I dropped sourcedisk_id and disk attributes.

Thanks.

tpoindessous

HI @paddyforan

Thank for your comments.

I changed code to resolve them.

Thanks.

tpoindessous · 2017-04-10T11:39:45Z

Hi @paddyforan

sorry to bother you ... I take the risk to ping you about this PR :)

Thanks for your time :)

paddycarver · 2017-04-13T17:34:04Z

Hey @tpoindessous! No risk in pinging--it is my job to review it, after all! You were generous enough to write code to add the feature, the least I can do is look at it. Apologies for the delay on this, I could've sworn I replied to it already. I'll take a look right now and hopefully we can get this in the 0.9.4 release; you've certainly been patient enough about it.

paddycarver

Just a couple of test changes (and a documentation fix, if you're so inclined) and this looks good to me. Feel free to ping me as soon as that's done and you'll jump to the top of my to-do list so we can merge this.

If you've (understandably) moved on to other things because feedback took so long, that's fine, just let me know and I'll push this across the finish line myself. :)

paddycarver · 2017-04-13T17:57:56Z

builtin/providers/google/resource_compute_snapshot.go

+		d.Set("source_disk_encryption_key_sha256", snapshot.SourceDiskEncryptionKey.Sha256)
+	}
+
+	d.Set("source_disk", snapshot.SourceDisk)


I'd love to see

d.Set("name", snapshot.Name)

Here as well, but I'm not willing to hold this any longer over it.

paddycarver · 2017-04-13T17:59:02Z

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

@@ -0,0 +1,64 @@
+---


We're going to need to add this resource to the menu as well, but I can always fix that up after.

paddycarver · 2017-04-13T18:02:15Z

builtin/providers/google/resource_compute_snapshot_test.go

+
+resource "google_compute_snapshot" "foobar" {
+	name = "%s"
+	disk = "${google_compute_disk.foobar.name}"


Tests don't run for me, looks like you want source_disk here.

paddycarver · 2017-04-13T18:02:37Z

builtin/providers/google/resource_compute_snapshot_test.go

+}
+resource "google_compute_snapshot" "foobar" {
+	name = "%s"
+	disk = "${google_compute_disk.foobar.name}"


Tests don't run for me, looks like you want source_disk here.

tpoindessous · 2017-04-14T14:08:02Z

Thanks @paddyforan

Sorry for the failing tests ...

I will do all your points.

I'm stuck with a design error : gcp.sourceDisk is an URL when TF source_disk is a name. So it's not matching. I will look at it in the next days, maybe I will revert change to use "disk" in TF and exposing source_disk only in output (it can be useful). But I need to find the right word in documentation.

I will come back to you only when tests will be OK and all your points are corrected.

Have a nice week-end and a nice Easter.

…ource_disk_link

tpoindessous

Hi !

I just finished all the tasks (I think).

The two tests are OK.

Thanks.

tpoindessous · 2017-04-20T11:01:06Z

Hi

I corrected all the remarks you made. I added the new resource doc in the nav panel.

the two tests are OK :
$ make testacc TEST=./builtin/providers/google TESTARGS='-run=TestAccComputeSnapshot_basic'
==> Checking that code complies with gofmt requirements...
go generate $(go list ./... | grep -v /terraform/vendor/)
2017/04/20 12:51:32 Generated command/internal_plugin_list.go
TF_ACC=1 go test ./builtin/providers/google -v -run=TestAccComputeSnapshot_basic -timeout 120m
=== RUN TestAccComputeSnapshot_basic
--- PASS: TestAccComputeSnapshot_basic (72.11s)
PASS
ok github.com/hashicorp/terraform/builtin/providers/google 72.130s

$ make testacc TEST=./builtin/providers/google TESTARGS='-run=TestAccComputeSnapshot_encryption'
==> Checking that code complies with gofmt requirements...
go generate $(go list ./... | grep -v /terraform/vendor/)
2017/04/20 12:53:26 Generated command/internal_plugin_list.go
TF_ACC=1 go test ./builtin/providers/google -v -run=TestAccComputeSnapshot_encryption -timeout 120m
=== RUN TestAccComputeSnapshot_encryption
--- PASS: TestAccComputeSnapshot_encryption (72.97s)
PASS
ok github.com/hashicorp/terraform/builtin/providers/google 72.986s

Thanks !

paddycarver · 2017-04-25T16:08:59Z

Hey @tpoindessous!

Sorry for the failing tests ...

No worries! It happens to the best of us. :)

I'm stuck with a design error : gcp.sourceDisk is an URL when TF source_disk is a name. So it's not matching. I will look at it in the next days, maybe I will revert change to use "disk" in TF and exposing source_disk only in output (it can be useful). But I need to find the right word in documentation.

Last question on this:

Is there a reason not to accept the name or URL for the source_disk parameter, and just expand to the URL? That would fix the problem, right?

Thanks so much for your effort and patience on this. If this is getting too drawn-out and your interest wanes, just let me know and I'm happy to do the work to push it over the finish line. :) You've been amazing, but I don't want to assume your generosity and patience are infinite.

(Also, I promise a quick review and reply.)

nodesocket · 2017-05-01T15:36:38Z

👍 on this PR. I wonder if it makes sense once this is merged and deployed to take backup snapshots of disks using Terraform? Then the question becomes, how to schedule snapshots with Terraform.

tpoindessous · 2017-05-02T11:47:02Z

Hi @paddycarver

we can't use get method of Disk object with a URL. Maybe we could use this method
https://cloud.google.com/compute/docs/reference/latest/disks/aggregatedList to find the right Disk but I think this may be a lot of work ... I'm not sure I can do it.

Can you please review my last commit ? I added some tests to be sure that the source_disk matches the same disk as source_disk_link.

I have prepared a new PR to add dataSourceGoogleComputeSnapshot, I think I will push it in the next 2 weeks.

Thanks.

Thomas

paddycarver · 2017-05-03T22:27:49Z

we can't use get method of Disk object with a URL.

I guess I'm confused. The URL contains the Disk name, couldn't we just pull it out of that? I'd like to, if possible, accept either here and just Do The Right Thing. But I also feel like I'm missing context. I'm going to read through all the code again right now and see if I can spot the problem.

paddycarver

After reading through the code, the only wart I could find is source_disk vs. source_disk_link. And while that has a solution, I'm not sure the solution is worth another round of back and forth. I think this offers ample benefit and is in great condition for a merge, and that wart can be fixed without any serious backwards compatibility concerns, so I think it's worthwhile to merge this now and wait for that to actually be a problem before trying to solve it.

Thanks so much for all the dedication, work, and patience on this.

ghost · 2020-04-13T01:56:05Z

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.

WIP: added a new resource type : google_compute_snapshot

758259b

paddycarver added enhancement new-resource provider/google-cloud labels Mar 7, 2017

[WIP]: added a test acceptance for google_compute_snapshot

a19849c

tpoindessous added 6 commits March 8, 2017 12:21

Cleanup

b62f978

Provided sourcedisk_encryption_key_raw when creating a snapshot of a …

64b6743

…customer's self encrypted disk

Snapshot operations are global by project

d8e6b2b

Use a new image type

89a3073

Use a new image type

1dbedf2

Test encrypted snapshot of a encrypted disk

2d1e174

tpoindessous changed the title ~~[WIP] provider/google : add a new resource : google_compute_snapshot~~ provider/google : add a new resource : google_compute_snapshot Mar 13, 2017

This was referenced Mar 13, 2017

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

Closed

Support disk snapshots operations for Google provider #11690

Closed

Documentation for the new resource : google_compute_snapshot

882a876

paddycarver suggested changes Mar 21, 2017

View reviewed changes

tpoindessous added 6 commits March 22, 2017 12:09

Review by @paddyforan: Rename sourcedisk to source_disk

84dc163

Review by @paddyforan: Add a resourceComputeSnapshotExists function

00c3553

Review by @paddyforan: Missing description in documentation

0c4e343

Merge remote-tracking branch 'upstream/master' into gcp_compute_disk_…

731fcea

…snapshot

Review by @paddyforan: Set attributes returned by API

3d08cd0

Review by @paddyforan: better test possible network error

6e76b90

tpoindessous commented Mar 23, 2017

View reviewed changes

paddycarver suggested changes Mar 24, 2017

View reviewed changes

Review by @paddyforan: corrected documentation. Replaced disk by sour…

5fcc659

…ce_disk. Deleted sourcedisk_id

tpoindessous commented Mar 29, 2017

View reviewed changes

paddycarver suggested changes Apr 13, 2017

View reviewed changes

tpoindessous added 5 commits April 20, 2017 12:12

Added a new attribute : source_disk_link

573da4d

Corrected test for snapshot. Simplified tests. Added a new test for s…

1b22200

…ource_disk_link

Added the new resource to the nav panel

68af7d4

merge upstream/master

1fa91df

Merged upstream for nav

7ec1c66

tpoindessous commented Apr 20, 2017

View reviewed changes

paddycarver approved these changes May 3, 2017

View reviewed changes

paddycarver merged commit dc63d8c into hashicorp:master May 3, 2017

tpoindessous deleted the gcp_compute_disk_snapshot branch May 5, 2017 09:47

ghost locked and limited conversation to collaborators Apr 13, 2020


		* `zone` - (Required) The zone where the source disk is located.

		* `disk` - (Required)


		- - -

		* `sourcedisk_encryption_key_raw` - (Optional) A 256-bit [customer-supplied encryption key]


		* `zone` - (Required) The zone where the source disk is located.

		* `disk` - (Required) The disk which will be used as the source of the snapshot

provider/google : add a new resource : google_compute_snapshot #12482

provider/google : add a new resource : google_compute_snapshot #12482

Conversation

tpoindessous commented Mar 6, 2017

tpoindessous commented Mar 7, 2017

tpoindessous commented Mar 13, 2017

tpoindessous commented Mar 14, 2017

paddycarver commented Mar 14, 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

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

tpoindessous left a comment • edited Loading

Choose a reason for hiding this comment

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

tpoindessous commented Mar 27, 2017

tpoindessous left a comment • edited Loading

Choose a reason for hiding this comment

tpoindessous commented Apr 10, 2017

paddycarver commented Apr 13, 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

tpoindessous commented Apr 14, 2017

tpoindessous left a comment

Choose a reason for hiding this comment

tpoindessous commented Apr 20, 2017

paddycarver commented Apr 25, 2017

nodesocket commented May 1, 2017 • edited Loading

tpoindessous commented May 2, 2017

paddycarver commented May 3, 2017

paddycarver left a comment

Choose a reason for hiding this comment

ghost commented Apr 13, 2020

tpoindessous left a comment •

edited

Loading

tpoindessous left a comment •

edited

Loading

nodesocket commented May 1, 2017 •

edited

Loading