Add `boot_disk` property to `google_compute_instance` #122

danawillow · 2017-06-14T01:39:03Z

This is change 1 in a series of changes to provide a better experience for attaching/detaching disks to/from instances. The end goal is to have a separate resource for attached disks, in order to have some attached disks that are managed by terraform side-by-side with disks that are managed some other way (such as by k8s), and to allow users to attach/detach disks (see #33)

The order of the changes will look roughly like:

(this) Add boot_disk property
Add scratch_disk property and deprecate disk
Remove disk property, make boot_disk Required
Add google_compute_attached_disk resource and deprecate the attached_disk property

danawillow · 2017-06-14T01:58:59Z

Also note that tests are currently failing, but will be fixed in #1.

rileykarson · 2017-06-14T20:35:00Z

google/resource_compute_instance.go

@@ -394,12 +469,23 @@ func resourceComputeInstanceCreate(d *schema.ResourceData, meta interface{}) err
 	}

 	// Build up the list of disks
+
+	disks := []*compute.AttachedDisk{}
+	var bootDisk *compute.AttachedDisk


We keep the bootdisk object in scope for the entire function but only use it to perform nil checks outside of the if block below. What if we moved it inside the if block, and pulled up the ok result of d.GetOk("boot_disk");, giving it a name like hasBootDisk?

That way, we know that code later in the function can only interact with the bootDisk as part of disks, and we can make lines like

disk.Boot = i == 0 && bootDisk == nil

look like:

disk.Boot = i == 0 && !hasBootDisk

rileykarson · 2017-06-14T20:50:12Z

google/resource_compute_instance.go

@@ -1142,3 +1235,79 @@ func resourceInstanceTags(d *schema.ResourceData) *compute.Tags {

 	return tags
 }
+
+func expandBootDisk(d *schema.ResourceData, config *Config, zone *compute.Zone, project string) (*compute.AttachedDisk, error) {


It looks like we only use schema elements that are properties of boot_disk.0; we can reduce the information this function needs by passing in a limited set of data, like BackendService's expandBackends()

The problem with that approach is that it auto-populates all the map keys, and sets them to default values. So if you want to check the existence of something, you have to check against "", 0, false, and nil, rather than being able to use a consistent d.GetOk("whatever"); ok. It seems like you should be able to just do v, ok := data["whatever"]; ok, but that will return true for elements not set in the config, which is a problem when you're then trying to use that value in another call, like in line 1274. This way feels less bug-prone.

That makes sense, thanks!

rileykarson · 2017-06-14T20:52:15Z

google/resource_compute_instance.go

+	return disk, nil
+}
+
+func flattenBootDisk(d *schema.ResourceData, disk *compute.AttachedDisk) []map[string]interface{} {


Same as expandBootDisk - do we need access to all of schema.ResourceData here? Or just a subset?

rileykarson · 2017-06-14T20:56:32Z

google/resource_compute_instance_test.go

@@ -1187,6 +1248,49 @@ func testAccComputeInstance_attachedDisk(disk, instance string) string {
 	}`, disk, instance)
 }

+func testAccComputeInstance_bootDisk(instance string) string {
+	return fmt.Sprintf(`
+	resource "google_compute_instance" "foobar" {


Nit: Can you de-indent both of these configs so that the top level is against the left side of the file? That way, they are easier to copy out and modify/test interactively, or for users who browse the source to pull out pre-tested .tf configurations.

I'll do it in a separate PR so it's the same for all of them since there are a lot of tests in this file.

Separate PR is submitted, updated here (and other comment as well)

rileykarson · 2017-06-14T20:57:32Z

google/resource_compute_instance_test.go

+		network_interface {
+			network = "default"
+		}
+	}`, instance)


Nit: can the ` be moved to the next line? This would also make using these tests in .tf configs easier.

rileykarson · 2017-06-16T23:36:22Z

LGTM 👍

paddycarver

Only a couple minor comments. This looks great.

paddycarver · 2017-06-22T20:34:03Z

google/resource_compute_instance.go

+						"device_name": &schema.Schema{
+							Type:     schema.TypeString,
+							Optional: true,
+							Computed: true,


Does this need to be ForceNew?

Yup. Fixed.

The Forcenew:true would cause the current resource such as a db instance to be recreated. is this desired? maybe I am asking a terraform basic question instead of a google provider one here. thanks.

It means that if the field in question changes in value then it would, yes.

paddycarver · 2017-06-22T20:34:43Z

google/resource_compute_instance.go

+
+	disks := []*compute.AttachedDisk{}
+	var hasBootDisk bool
+	if _, hasBootDisk = d.GetOk("boot_disk"); hasBootDisk {


Nit: if _, hasBootDisk := means you don't need var hasBootDisk bool.

I need it so I can refer to it outside of the if statement

paddycarver · 2017-06-22T20:38:39Z

google/resource_compute_instance.go

+		"device_name": disk.DeviceName,
+		"source":      sourceUrl[len(sourceUrl)-1],
+		// disk_encryption_key_raw is not returned from the API, so don't store it in state.
+		// If necessary in the future, this can be copied from what the user originally specified.


I think in other places we do the fingerprint of the key. Is that returned from the API?

paddycarver · 2017-06-28T21:41:42Z

Rebase and LGTM. 👍

* Add boot_disk property to google_compute_instance * docs for boot_disk * limit scope of bootDisk, use bool instead * test formatting * make device_name forcenew, add sha256 encryption key

johnmehler · 2018-11-14T19:18:25Z

This seems like the best place to ask without opening a new ticket-- why does resizing the boot disk force creation of a new resource? Example code below:

boot_disk {
    initialize_params {
      image = "${var.machine_image_id}"
      type  = "${var.boot_disk_type}"
      size  = "${var.boot_disk_size_gb}"
    }
  }
allow_stopping_for_update = true

If the size param is changed and terraform apply is run again, the build plan will include something like

boot_disk.0.initialize_params.0.size: "10" => "200" (forces new resource)

In the console, you can resize (or at least increase the size of) a the boot disk without destroying and recreating it. Is it possible to do so in Terraform?

danawillow · 2018-11-14T19:27:11Z

Hey @johnmehler, that's because the size there is in the initialize_params block, which is specific to creating the disk for the first time. If you want to be able to make changes later, you should use a google_compute_disk resource and use the source property of the boot_disk to reference that disk.

ghost · 2018-11-16T13:50: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!

danawillow requested review from paddycarver and rileykarson June 14, 2017 01:39

danawillow added the enhancement label Jun 14, 2017

danawillow mentioned this pull request Jun 14, 2017

Add scratch_disk property to google_compute_instance and deprecate disk #123

Merged

rileykarson reviewed Jun 14, 2017

View reviewed changes

paddycarver mentioned this pull request Jun 19, 2017

google_compute_instance disks not controlled by terraform forces a new resource hashicorp/terraform#12715

Closed

danawillow force-pushed the boot-disk branch from 63d9f0b to 4975a13 Compare June 19, 2017 20:36

paddycarver reviewed Jun 22, 2017

View reviewed changes

danawillow mentioned this pull request Jun 23, 2017

Unnecessary check in resourceComputeInstanceRead #141

Closed

danawillow added 5 commits June 28, 2017 15:31

Add boot_disk property to google_compute_instance

83d258a

docs for boot_disk

313953c

limit scope of bootDisk, use bool instead

41bb4d6

test formatting

cfeef33

make device_name forcenew, add sha256 encryption key

5e14c2d

danawillow force-pushed the boot-disk branch from 9b558e7 to 5e14c2d Compare June 28, 2017 22:35

danawillow merged commit 549e131 into hashicorp:master Jun 28, 2017

danawillow deleted the boot-disk branch June 28, 2017 22:36

danawillow mentioned this pull request Sep 25, 2017

'google_compute_instance' adding 'google_compute_disk' forces new resource #33

Closed

ghost locked and limited conversation to collaborators Nov 16, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add `boot_disk` property to `google_compute_instance` #122

Add `boot_disk` property to `google_compute_instance` #122

danawillow commented Jun 14, 2017

danawillow commented Jun 14, 2017

rileykarson Jun 14, 2017

danawillow Jun 16, 2017

rileykarson Jun 14, 2017

danawillow Jun 16, 2017

rileykarson Jun 16, 2017

rileykarson Jun 14, 2017

rileykarson Jun 14, 2017

danawillow Jun 16, 2017

danawillow Jun 19, 2017

rileykarson Jun 14, 2017

danawillow Jun 16, 2017

rileykarson commented Jun 16, 2017

paddycarver left a comment

paddycarver Jun 22, 2017

danawillow Jun 24, 2017

pydevops Aug 4, 2017

danawillow Aug 4, 2017

paddycarver Jun 22, 2017

danawillow Jun 24, 2017

paddycarver Jun 22, 2017

danawillow Jun 24, 2017

paddycarver commented Jun 28, 2017

johnmehler commented Nov 14, 2018

danawillow commented Nov 14, 2018

ghost commented Nov 16, 2018

Add boot_disk property to google_compute_instance #122

Add boot_disk property to google_compute_instance #122

Conversation

danawillow commented Jun 14, 2017

danawillow commented Jun 14, 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

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

rileykarson commented Jun 16, 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

paddycarver commented Jun 28, 2017

johnmehler commented Nov 14, 2018

danawillow commented Nov 14, 2018

ghost commented Nov 16, 2018

Add `boot_disk` property to `google_compute_instance` #122

Add `boot_disk` property to `google_compute_instance` #122