Skip to content

google_compute_instance reads scheduling fields from GCP. #237

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.

Sign up for GitHub

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

Merged
merged 3 commits into from
Aug 3, 2017
Merged

google_compute_instance reads scheduling fields from GCP. #237

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
f44c8d2
google_compute_instance reads `scheduling` fields from GCP.
rosbo Jul 24, 2017
fae9812
ForceSend boolean value and use Computed instead of a diffsuppress on…
rosbo Aug 3, 2017
037d553
Put preemptible back in the update method
rosbo Aug 3, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 11 additions & 2 deletions google/resource_compute_instance.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -378,22 +378,28 @@ func resourceComputeInstance() *schema.Resource {

"scheduling": &schema.Schema{
Type: schema.TypeList,
MaxItems: 1,
Optional: true,
Computed: true,
Elem: &schema.Resource{
Schema: map[string]*schema.Schema{
"on_host_maintenance": &schema.Schema{
Type: schema.TypeString,
Optional: true,
Computed: true,
},

"automatic_restart": &schema.Schema{
Type: schema.TypeBool,
Optional: true,
Copy link
Contributor

Choose a reason for hiding this comment

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

It's not clear to me that this should be Computed (in fact, I'm not sure if we should or shouldn't use Computed for optional values... would love some guidance here).

Because this field is of type bool, there's no way to determine if it's specified or not in the config (Get and GetOk will both always give you a boolean value). This means that you'll always be reading a value for automatic_restart and therefore never really letting GCP compute it, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

automatic_restart default value depends on the preemptible field value. DefaultFunc cannot look at the value of another field to determine its default value.

The Optional & Computed means that unless the user specify it in the config, take the GCP value.

Copy link
Contributor

Choose a reason for hiding this comment

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

yeah, optional+computed is generally fine but booleans get weird since we can't tell the difference between false and unset. We need the user to be able to set automatic_restart to false, so we ForceSendFields it, which means that we'll always be providing a value to GCP and the computedness of it doesn't really end up mattering.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

Default: true,
},

"preemptible": &schema.Schema{
Type: schema.TypeBool,
Optional: true,
Default: false,
ForceNew: true,
},
},
},
Expand Down Expand Up @@ -787,6 +793,7 @@ func resourceComputeInstanceCreate(d *schema.ResourceData, meta interface{}) err
if val, ok := d.GetOk(prefix + ".on_host_maintenance"); ok {
scheduling.OnHostMaintenance = val.(string)
}
scheduling.ForceSendFields = []string{"AutomaticRestart", "Preemptible"}

// Read create timeout
var createTimeout int
Expand Down Expand Up @@ -1052,6 +1059,9 @@ func resourceComputeInstanceRead(d *schema.ResourceData, meta interface{}) error
d.Set("attached_disk", attachedDisks)
d.Set("scratch_disk", scratchDisks)

scheduling, _ := flattenScheduling(instance.Scheduling)
d.Set("scheduling", scheduling)

d.Set("self_link", instance.SelfLink)
d.SetId(instance.Name)

Expand Down Expand Up @@ -1160,14 +1170,13 @@ func resourceComputeInstanceUpdate(d *schema.ResourceData, meta interface{}) err
if val, ok := d.GetOk(prefix + ".automatic_restart"); ok {
scheduling.AutomaticRestart = googleapi.Bool(val.(bool))
}

if val, ok := d.GetOk(prefix + ".preemptible"); ok {
scheduling.Preemptible = val.(bool)
}

if val, ok := d.GetOk(prefix + ".on_host_maintenance"); ok {
scheduling.OnHostMaintenance = val.(string)
}
scheduling.ForceSendFields = []string{"AutomaticRestart", "Preemptible"}

op, err := config.clientCompute.Instances.SetScheduling(project,
zone, d.Id(), scheduling).Do()
Expand Down