-
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
Add import support for google_compute_image. #194
Conversation
google/resource_compute_image.go
Outdated
@@ -195,3 +213,9 @@ func resourceComputeImageDelete(d *schema.ResourceData, meta interface{}) error | |||
d.SetId("") | |||
return nil | |||
} | |||
|
|||
func resourceComputeImageImportState(d *schema.ResourceData, meta interface{}) ([]*schema.ResourceData, error) { | |||
d.Set("create_timeout", computeImageCreateTimeoutDefault) |
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.
Can you explain why this is necessary instead of setting create_timeout
on read every time?
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.
Added a comment explaining why.
Another option would be to set the create_timeout
in the read method if it is not set but I prefer not cluttering the read method logic with logic specific to import.
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, so actually I think we should stop this from being ForceNew. Just from thinking about it logically, it's the amount of time we're willing to wait for the instance to create. IMO, changing it doesn't actually mean I want to create a new one, it just means that should it recreate for some other reason, we would want to wait that long. Does that seem right? If so we can remove ForceNew and just never do a d.Set
on it.
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.
Agree for the ForceNew part, I will do it.
However, if we don't set create_timeout
to the default value on import, then, after importing, the user will have a "plan diff" with:
~ google_compute_image.web-image
create_timeout: "" => "4"
The update is fast (only update the value locally). I am wondering if this is the expected behavior that after an import, we may have a diff that we can't fix without either running apply or fiddling with the state file.
Thoughts?
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.
Oh, you're right. In this case the way you've done it is probably correct, then.
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.
I removed the ForceNew from the create_timeout field.
VM image can be imported using the `name`, e.g. | ||
|
||
``` | ||
$ terraform import google_compute_image.web-image image1 |
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.
Can you change image1
to my-custom-image
since that's the name in the example?
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.
Done
Looks good, thanks!
|
* Add import support for google_compute_image. * Added comment explaning why we set the create_timeout in the import state method * Don't ForceNew for create_timeout field * Update image name in import documentation
* Add import support for google_compute_image. * Added comment explaning why we set the create_timeout in the import state method * Don't ForceNew for create_timeout field * Update image name in import documentation
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 👉 hashibot-feedback@hashicorp.com. Thanks! |
issue #182
/cc @danawillow