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

Change automatic_restart to be computed if not specified #206

Closed
wants to merge 1 commit into from

Conversation

selmanj
Copy link
Contributor

@selmanj selmanj commented Jul 17, 2017

Fixes #107

@rileykarson rileykarson self-requested a review July 24, 2017 21:01
@rileykarson
Copy link
Collaborator

I definitely agree that the deprecated value should be computed & optional, if not removed entirely.

My concern with setting the second value to computed as well means that when we move from

scheduling {
  automatic_restart = true
}

to

scheduling {
}

Terraform won't see any diffs, and the resource won't get recreated. This means that the second scheduling block doesn't guarantee that the server-side resource's property is false; it can be true if there was an already existing resource and we modified the config.

What happens if the deprecated value is computed and the new one isn't? Especially in the case where users were relying on the old default of true in their configs. I expect we'll have enough diffs/recreates that we need to use Computed like this, but being able to stop that corner case before it happens would be nice as well.

@rileykarson
Copy link
Collaborator

Also, linking #224 since this should fix it as well as #107.

ForceNew: true,
Computed: true,
Copy link
Contributor

@rosbo rosbo Jul 24, 2017

Choose a reason for hiding this comment

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

If you set this field to Computed, it means a user cannot specify a value for automatic_restart. GCP default value for this field is true. It means a user could never create an instance with terraform with automatic_restart = false. I am confused about why that would be something desirable.

Am I missing something?

Copy link
Collaborator

Choose a reason for hiding this comment

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

To my understanding, Computed means that it can't be set and can only be read, but Computed and Optional means that it acts like it is Computed until a value is set in config, at which point it is treated as Optional.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for the clarification, I wasn't aware of this "trick"

@selmanj
Copy link
Contributor Author

selmanj commented Jul 25, 2017

@rosbo The docs on Computed say that you can override the value by setting it in your config:

	// If Computed is true, then the result of this value is computed
	// (unless specified by config) on creation.

@selmanj
Copy link
Contributor Author

selmanj commented Jul 25, 2017

A main problem here is that there's no way to infer whether or not automatic_restart was set explicitly or not. Ideally we want to not specify it all when creating a resource, as GCP will automatically choose the correct value whether or not is_preemptible is specified. We can't do that here unfortunately...

@dgolja
Copy link

dgolja commented Jul 25, 2017

What about just making sure that you can not define automatic_restart twice in the config file ? This would for sure fix the #224

@grubernaut grubernaut added the bug label Jul 25, 2017
@selmanj
Copy link
Contributor Author

selmanj commented Jul 25, 2017

Good point @dgolja; this PR is not complete and is off-topic from the original issue. I'll close this for now and submit a new one specifically around removing the deprecated parameter (which has been deprecated since 2015) and fixing scheduling.automatic_restart.

@selmanj selmanj closed this Jul 25, 2017
@selmanj selmanj deleted the gh-107 branch July 25, 2017 15:48
@selmanj selmanj restored the gh-107 branch September 17, 2017 19:49
luis-silva pushed a commit to luis-silva/terraform-provider-google that referenced this pull request May 21, 2019
@ghost
Copy link

ghost commented Mar 31, 2020

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 Mar 31, 2020
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.

Provide better default scheduling behaviour for Google Cloud preemptible instances in Instance Templates
5 participants