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.

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

merged 4 commits into from
Jun 14, 2017

Conversation

catsby
Copy link
Contributor

@catsby catsby commented Jun 9, 2017

This PR fixes a handful of failing tests related to Disks. @danawillow can you take a peak? Related to hashicorp/terraform#15095 (comment)

@catsby catsby added the bug label Jun 9, 2017
@catsby
Copy link
Contributor Author

catsby commented Jun 9, 2017

TF_ACC=1 go test ./google -v -run=TestAccComputeDisk_ -timeout 120m
=== RUN   TestAccComputeDisk_importBasic
--- PASS: TestAccComputeDisk_importBasic (24.23s)
=== RUN   TestAccComputeDisk_basic
--- PASS: TestAccComputeDisk_basic (23.71s)
=== RUN   TestAccComputeDisk_updateSize
--- PASS: TestAccComputeDisk_updateSize (35.45s)
=== RUN   TestAccComputeDisk_fromSnapshotURI
--- PASS: TestAccComputeDisk_fromSnapshotURI (128.16s)
=== RUN   TestAccComputeDisk_encryption
--- PASS: TestAccComputeDisk_encryption (23.91s)
=== RUN   TestAccComputeDisk_deleteDetach
--- PASS: TestAccComputeDisk_deleteDetach (99.33s)
PASS
ok      github.com/terraform-providers/terraform-provider-google/google 334.824s

@@ -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

@catsby catsby merged commit a12c074 into master Jun 14, 2017
gh-mlfowler pushed a commit to gh-mlfowler/terraform-provider-google that referenced this pull request Jun 20, 2017
* compute_disk: update image in test

* disk_image: add default type, make size computed

* compute_dis: wait on disk size operations to complete before moving on

* update docks on the image
pdecat pushed a commit to pdecat/terraform-provider-google that referenced this pull request Jun 29, 2017
* compute_disk: update image in test

* disk_image: add default type, make size computed

* compute_dis: wait on disk size operations to complete before moving on

* update docks on the image
@chrisst chrisst deleted the b-gcp-test-fixes branch October 29, 2018 19:37
@ghost
Copy link

ghost commented Nov 16, 2018

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!

@ghost ghost locked and limited conversation to collaborators Nov 16, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants