Release Disk resource - autogenerated. #1521

modular-magician · 2018-05-22T22:10:18Z

nat-henderson · 2018-05-22T22:11:20Z

Running tests in the cloud now. :)

nat-henderson · 2018-05-22T22:21:56Z

Tests pass:

==> Checking that code complies with gofmt requirements...                     
TF_ACC=1 go test ./google -v -run=TestAccComputeDisk -timeout 120m             
=== RUN   TestAccComputeDisk_imageDiffSuppressPublicVendorsFamilyNames         
=== RUN   TestAccComputeDisk_basic     
=== RUN   TestAccComputeDisk_timeout   
=== RUN   TestAccComputeDisk_update    
=== RUN   TestAccComputeDisk_fromSnapshot                                      
=== RUN   TestAccComputeDisk_encryption                                        
=== RUN   TestAccComputeDisk_deleteDetach                                      
=== RUN   TestAccComputeDisk_deleteDetachIGM                                   
=== RUN   TestAccComputeDisk_computeDiskUserRegex                              
--- PASS: TestAccComputeDisk_computeDiskUserRegex (0.00s)                      
--- PASS: TestAccComputeDisk_imageDiffSuppressPublicVendorsFamilyNames (1.42s) 
--- PASS: TestAccComputeDisk_encryption (24.02s)                               
--- PASS: TestAccComputeDisk_timeout (0.04s)                                   
--- PASS: TestAccComputeDisk_update (45.89s)                                   
--- PASS: TestAccComputeDisk_basic (23.35s)                                    
--- PASS: TestAccComputeDisk_fromSnapshot (108.15s)                            
--- PASS: TestAccComputeDisk_deleteDetach (104.83s)                            
--- PASS: TestAccComputeDisk_deleteDetachIGM (485.19s)                         
PASS                                   
ok      github.com/terraform-providers/terraform-provider-google/google 485.206s

danawillow

Wow, lots of stuff going on here. This is a big deal though, thanks!

danawillow · 2018-05-22T22:57:02Z

google/resource_compute_disk.go

 				Type:     schema.TypeString,
+				Optional: true,


name should be required

danawillow · 2018-05-22T23:00:25Z

google/resource_compute_disk.go

 				Type:             schema.TypeString,
 				Optional:         true,
 				ForceNew:         true,
 				DiffSuppressFunc: diskImageDiffSuppress,
 			},
-
-			"project": &schema.Schema{
+			"type": {


this had a default before, probably best to keep it there

Done, thanks for the catch.

danawillow · 2018-05-22T23:07:17Z

google/resource_compute_disk.go

 		},
-		CustomizeDiff: customdiff.All(
-			customdiff.ForceNewIfChange("size", isDiskShrinkage)),
 	}


this isDiskShrinkage customizediff doesn't seem to have made it in

Done - I forgot, I added that after I started working on Disk generation. 🤦‍♂️

danawillow · 2018-05-22T23:13:44Z

google/resource_compute_disk.go

-	return new.(int) < old.(int)
-}
+	d.SetId(id)
+	if zone, err := getZone(d, config); err != nil || zone == "" {


I wouldn't mind a comment in here that explains what's going on

Man, me either. I had to figure it out all over again. Done.

danawillow · 2018-05-22T23:33:03Z

google/resource_compute_disk.go

+		// The raw key won't be returned, so we need to use the original.
+		transformed["raw_key"] = d.Get("source_image_encryption_key.0.raw_key")
+		transformed["sha256"] = original["sha256"]
+		if v, ok := d.GetOk("disk_encryption_key_raw"); ok {


There's diskEncryptionKey stuff hanging out here in the sourceImageEncryptionKey block (and below)- intentional?

Nope! Screwed up transitioning from per-attribute decoders (where they were hidden in an if block) to whole-object decoders. And it didn't trip the unit test, hm.

danawillow · 2018-05-22T23:38:14Z

website/docs/r/compute_disk.html.markdown

-
- - -
+Add a persistent disk to your instance when you need reliable and
+affordable storage with consistent performance characteristics.


Don't forget to add an example :)

danawillow · 2018-05-22T23:39:23Z

website/docs/r/compute_disk.html.markdown

+  If you specify this field along with sourceImage or sourceSnapshot,
+  the value of sizeGb must not be less than the size of the sourceImage
+  or the size of the snapshot.
+* `image` -


We have a bunch of ways we allow specifying the image, this may need an override so we can put them in

danawillow · 2018-05-22T23:42:53Z

google/resource_compute_disk.go

+	if err != nil {
+		return nil, err
+	}
+


I think you need to add something here too for snapshots

You're right! Dang! How did you know that?

danawillow · 2018-05-22T23:43:33Z

website/docs/r/compute_disk.html.markdown

+* `snapshot` -
+  (Optional)
+  The source snapshot used to create this disk. You can provide this as
+  a partial or full URL to the resource. For example, the following are


we also allow just the snapshot name

danawillow · 2018-05-22T23:45:59Z

website/docs/r/compute_disk.html.markdown

 * `self_link` - The URI of the created resource.

-* `users` - The Users of the created resource.
-
-* `label_fingerprint` - The fingerprint of the assigned labels.


label_fingerprint should be added to the docs too

nat-henderson

WHEW. Thank you for the detailed review - I completely forgot to look at the docs, and the tests didn't catch any of those issues. Fixed everything you mentioned, it's all working its way through generation now.

nat-henderson · 2018-05-23T01:56:44Z

google/resource_compute_disk.go

 				Type:     schema.TypeString,
+				Optional: true,


nat-henderson · 2018-05-23T01:58:31Z

google/resource_compute_disk.go

 				Type:             schema.TypeString,
 				Optional:         true,
 				ForceNew:         true,
 				DiffSuppressFunc: diskImageDiffSuppress,
 			},
-
-			"project": &schema.Schema{
+			"type": {


Done, thanks for the catch.

nat-henderson · 2018-05-23T02:03:38Z

google/resource_compute_disk.go

 		},
-		CustomizeDiff: customdiff.All(
-			customdiff.ForceNewIfChange("size", isDiskShrinkage)),
 	}


Done - I forgot, I added that after I started working on Disk generation. 🤦‍♂️

nat-henderson · 2018-05-23T02:06:45Z

google/resource_compute_disk.go

-	return new.(int) < old.(int)
-}
+	d.SetId(id)
+	if zone, err := getZone(d, config); err != nil || zone == "" {


Man, me either. I had to figure it out all over again. Done.

nat-henderson · 2018-05-23T02:13:36Z

google/resource_compute_disk.go

+	if err != nil {
+		return nil, err
+	}
+


You're right! Dang! How did you know that?

nat-henderson · 2018-05-23T02:29:38Z

website/docs/r/compute_disk.html.markdown

-
- - -
+Add a persistent disk to your instance when you need reliable and
+affordable storage with consistent performance characteristics.


Sure! Added to docs. That'll be useful for the interconnect, too.

nat-henderson · 2018-05-23T02:29:54Z

website/docs/r/compute_disk.html.markdown

-
- - -
+Add a persistent disk to your instance when you need reliable and
+affordable storage with consistent performance characteristics.


nat-henderson · 2018-05-23T02:32:24Z

website/docs/r/compute_disk.html.markdown

+  If you specify this field along with sourceImage or sourceSnapshot,
+  the value of sizeGb must not be less than the size of the sourceImage
+  or the size of the snapshot.
+* `image` -


nat-henderson · 2018-05-23T02:34:18Z

website/docs/r/compute_disk.html.markdown

+* `snapshot` -
+  (Optional)
+  The source snapshot used to create this disk. You can provide this as
+  a partial or full URL to the resource. For example, the following are


nat-henderson · 2018-05-23T02:35:48Z

website/docs/r/compute_disk.html.markdown

 * `self_link` - The URI of the created resource.

-* `users` - The Users of the created resource.
-
-* `label_fingerprint` - The fingerprint of the assigned labels.


danawillow · 2018-05-23T23:09:49Z

google/resource_compute_disk.go

+		original := v.(map[string]interface{})
+		transformed := make(map[string]interface{})
+		// The raw key won't be returned, so we need to use the original.
+		transformed["raw_key"] = d.Get("disk_encryption_key.0.raw_key")


I think these have to be rawKey to match what the flatteners are expecting.

On another note, it would be great to have tests for some of these trickier spots.

Shoot, forgot that the flatteners get called here. Writing more, better tests.

danawillow · 2018-05-23T23:11:14Z

website/docs/r/compute_disk.html.markdown

+  (Optional)
+  Specifies a 256-bit customer-supplied encryption key, encoded in
+  RFC 4648 base64 to either encrypt or decrypt this resource.
+* `sha256` -


Hmmm, should this be in the Attributes Reference section?

I thought this was the right place for it. Do you mean because it's computed? Hm. That introduces a challenge for generating the docs, but of course it's doable.

danawillow · 2018-05-23T23:12:19Z

website/docs/r/compute_disk.html.markdown

- `create` - (Default `5 minutes`) Used for creating disks.
- `update` - (Default `5 minutes`) Used for resizing a disk and setting labels on disks.
- `delete` - (Default `5 minutes`) Used for destroying disks (not including time to detach the disk from instances).
+- `create` - Default is 4 minutes.


Huh, the actual resource is showing 5 minutes. I think I fixed this in the redis pr so you might just need to merge.

nat-henderson · 2018-05-31T01:08:45Z

@danawillow There's a test for the encryption stuff (which has changed in order to make the new test pass. :) )

danawillow · 2018-05-31T18:31:32Z

google/resource_compute_disk_test.go

@@ -332,6 +332,26 @@ func TestAccComputeDisk_encryption(t *testing.T) {
 						"google_compute_disk.foobar", &disk),
 				),
 			},
+			// Update from top-level attribute to nested.
+			resource.TestStep{
+				Config: testAccComputeDisk_encryptionMigrate(diskName),


The test passes, but I don't think it's actually doing what you were intending:

DESTROY/CREATE: google_compute_disk.foobar creation_timestamp: "2018-05-31T11:28:11.935-07:00" => "<computed>" disk_encryption_key.0.sha256: "esTuF7d4eatX4cnc4JsiEiaI+Rff78JgPhA/v1zxX9E=" => "<computed>" disk_encryption_key_raw: "SGVsbG8gZnJvbSBHb29nbGUgQ2xvdWQgUGxhdGZvcm0=" => "" (forces new resource) disk_encryption_key_sha256: "esTuF7d4eatX4cnc4JsiEiaI+Rff78JgPhA/v1zxX9E=" => "<computed>" image: "https://www.googleapis.com/compute/v1/projects/debian-cloud/global/images/debian-8-jessie-v20160803" => "debian-8-jessie-v20160803" label_fingerprint: "42WmSpB8rSM=" => "<computed>" last_attach_timestamp: "" => "<computed>" last_detach_timestamp: "" => "<computed>" name: "tf-test-wyqure0ffb" => "tf-test-wyqure0ffb" project: "graphite-test-danahoffman-tf" => "<computed>" self_link: "https://www.googleapis.com/compute/v1/projects/graphite-test-danahoffman-tf/zones/us-central1-a/disks/tf-test-wyqure0ffb" => "<computed>" size: "50" => "50" source_image_id: "675642609008260682" => "<computed>" source_snapshot_id: "" => "<computed>" type: "pd-ssd" => "pd-ssd" users.#: "0" => "<computed>" zone: "us-central1-a" => "us-central1-a"

nat-henderson · 2018-06-01T00:37:32Z

Okay, that should do it. It doesn't seem to recreate the resource anymore!

ghost · 2018-11-18T13:58:26Z

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!

modular-magician mentioned this pull request May 22, 2018

Release Terraform Disk resource. GoogleCloudPlatform/magic-modules#209

Merged

nat-henderson requested a review from danawillow May 22, 2018 22:11

danawillow suggested changes May 22, 2018

View reviewed changes

nat-henderson reviewed May 23, 2018

View reviewed changes

modular-magician force-pushed the codegen-pr-209 branch from 3e7c021 to 2cbbb37 Compare May 23, 2018 02:49

danawillow reviewed May 23, 2018

View reviewed changes

modular-magician force-pushed the codegen-pr-209 branch 2 times, most recently from f939e9d to 3bbc975 Compare May 31, 2018 01:04

danawillow reviewed May 31, 2018

View reviewed changes

modular-magician force-pushed the codegen-pr-209 branch from 4073b32 to 28c556f Compare June 1, 2018 00:33

danawillow approved these changes Jun 1, 2018

View reviewed changes

Release Disk resource - autogenerated.

bdb824e

modular-magician force-pushed the codegen-pr-209 branch from e7013d5 to bdb824e Compare June 1, 2018 01:04

Add encryption test.

8aaa0fb

nat-henderson merged commit fd208dc into hashicorp:master Jun 1, 2018

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

Autogenerate Disk resource (hashicorp#1521).

9f838d1

ghost locked and limited conversation to collaborators Nov 18, 2018

modular-magician deleted the codegen-pr-209 branch November 16, 2024 20:46

Release Disk resource - autogenerated. #1521

Release Disk resource - autogenerated. #1521

Conversation

modular-magician commented May 22, 2018

nat-henderson commented May 22, 2018

nat-henderson commented May 22, 2018

danawillow 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

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

nat-henderson 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

Choose a reason for hiding this comment

nat-henderson commented May 31, 2018

Choose a reason for hiding this comment

nat-henderson commented Jun 1, 2018

ghost commented Nov 18, 2018