-
Notifications
You must be signed in to change notification settings - Fork 9.6k
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
provider/google: Support Import of 'google_compute_instance_template' #8147
Conversation
@@ -134,7 +143,6 @@ func resourceComputeInstanceTemplate() *schema.Resource { | |||
"automatic_restart": &schema.Schema{ | |||
Type: schema.TypeBool, | |||
Optional: true, | |||
Default: true, |
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.
Have you verified that this works without automatic_restart
set? Also removing this default could be dangerous to existing configs.
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.
@lwander you are correct - if we are going to change a default, then we may possibly need a state_migration
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'm not sure a state_migration will be able to fix the issue. Previously a config without auto_restart or scheduler set will create an instance group manager with a default scheduler that has auto_restart set to true (which seems like odd behavior to me). Taking away this default will make configs such as this plan to destroy the scheduler. To keep the scheduler it seems like I would need to migrate configs rather than states somehow. Thoughts on this? @stack72
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.
Actually I just went into the gce api and if scheduler is not specified it will still make a scheduler with the default attributes, so keeping the default should be fine. I'm going to make the other properties of the scheduler have defaults set to avoid confusion in the future.
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.
Nice work! One last little change, since your adding a default to preemptible
, could you make it clear that the default is false
in the docs? https://github.com/hashicorp/terraform/blob/master/website/source/docs/providers/google/r/compute_instance_template.html.markdown
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.
Changed the docs
a08929e
to
377fc1a
Compare
377fc1a
to
fe5d7d1
Compare
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 have found a problem that seems similar to this, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further. |
@lwander