-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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
Conversation
|
@@ -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" |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 👍
There was a problem hiding this comment.
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
* 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
* 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
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! |
This PR fixes a handful of failing tests related to Disks. @danawillow can you take a peak? Related to hashicorp/terraform#15095 (comment)