Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Compute and Disk fixes #1

Merged
merged 4 commits into from
Jun 14, 2017
Merged

Compute and Disk fixes #1

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
5ff10bd
compute_disk: update image in test
catsby Jun 9, 2017
d0ed258
disk_image: add default type, make size computed
catsby Jun 9, 2017
90ed1ab
compute_dis: wait on disk size operations to complete before moving on
catsby Jun 9, 2017
f87c53c
update docks on the image
catsby Jun 14, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 7 additions & 1 deletion google/resource_compute_disk.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,6 +69,7 @@ func resourceComputeDisk() *schema.Resource {
"size": &schema.Schema{
Type: schema.TypeInt,
Optional: true,
Computed: true,
},

"self_link": &schema.Schema{
Expand All @@ -85,6 +86,7 @@ func resourceComputeDisk() *schema.Resource {
"type": &schema.Schema{
Type: schema.TypeString,
Optional: true,
Default: "pd-standard",
ForceNew: true,
},
"users": &schema.Schema{
Expand Down Expand Up @@ -197,11 +199,15 @@ func resourceComputeDiskUpdate(d *schema.ResourceData, meta interface{}) error {
rb := &compute.DisksResizeRequest{
SizeGb: int64(d.Get("size").(int)),
}
_, err := config.clientCompute.Disks.Resize(
op, err := config.clientCompute.Disks.Resize(
project, d.Get("zone").(string), d.Id(), rb).Do()
if err != nil {
return fmt.Errorf("Error resizing disk: %s", err)
}
err = computeOperationWaitZone(config, op, project, d.Get("zone").(string), "Resizing Disk")
if err != nil {
return err
}
}

return resourceComputeDiskRead(d, meta)
Expand Down
2 changes: 1 addition & 1 deletion google/resource_compute_disk_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -292,7 +292,7 @@ func testAccComputeDisk_deleteDetach(instanceName, diskName string) string {
return fmt.Sprintf(`
resource "google_compute_disk" "foo" {
name = "%s"
image = "debian-8"
image = "debian-8-jessie-v20170523"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, this should work as-is according to https://www.terraform.io/docs/providers/google/r/compute_disk.html#image. This seems to me like the test caught a bug in our code and we should probably look for that rather than changing the test.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems that before hashicorp/terraform#14573 the google_compute_disk resource didn't READ much back from the API. I suppose the API allows debian-8 to initialize as a convince for the user, but the API seems to give it a more concrete value in the backend. 14573 introduced reading these values back, thus we see the diff shown in the test failures.

I believe the new behavior is the correct behavior, and we should discourage users from simply using the debian-8 shorthand going forward.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we should discourage users from simply using the debian-8 shorthand going forward.

Given that these tests fail, I think we've inadvertently already started discouraging users from using it by breaking them 😞

Mind filing an issue to make our lack of support for that shorthand more explicit? I think there are a few places in the code where that needs to be changed.

Once that's done, feel free to merge this when you're ready 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mind filing an issue to make our lack of support for that shorthand more explicit? I think there are a few places in the code where that needs to be changed

I'm not as familiar with the GCP provider as I'd like, what other places need changing?

It's true, we should not support that shorthand anymore, I can update the docs

size = 50
type = "pd-ssd"
zone = "us-central1-a"
Expand Down
8 changes: 3 additions & 5 deletions website/docs/r/compute_disk.html.markdown
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ resource "google_compute_disk" "default" {
name = "test-disk"
type = "pd-ssd"
zone = "us-central1-a"
image = "debian-cloud/debian-8"
image = "debian-8-jessie-v20170523"
}
```

Expand All @@ -41,10 +41,8 @@ The following arguments are supported:
to encrypt this disk.

* `image` - (Optional) The image from which to initialize this disk. This can be
one of: the image's `self_link`, `projects/{project}/global/images/{image}`,
`projects/{project}/global/images/family/{family}`, `global/images/{image}`,
`global/images/family/{family}`, `family/{family}`, `{project}/{family}`,
`{project}/{image}`, `{family}`, or `{image}`.
one of: the image's `self_link`, of a full name and version, e.g.
`debian-8-jessie-v20170523`

* `project` - (Optional) The project in which the resource belongs. If it
is not provided, the provider project is used.
Expand Down