google_compute_instance reads scheduling fields from GCP.
#237
Conversation
|
I realized that the default value for |
rosbo
changed the title
automatic_restart and preemptible scheduling fields from GCP.automatic_restart and preemptible scheduling fields from GCP.
rosbo
force-pushed
the
compute_instance_read_scheduling_field
branch
from
1cefa21 to
f44c8d2
Compare
rosbo
changed the title
automatic_restart and preemptible scheduling fields from GCP.scheduling fields from GCP.
|
This PR is now ready for review. I am now handling all the |
google/resource_compute_instance.go
Outdated
| // of 1 with the default values for `automatic_restart` and `preemptible`. | ||
| // This suppress the diff `scheduling.#: "1" => "0"` when the `scheduling` block | ||
| // is not specified by the user. | ||
| if k == "scheduling.#" { |
There was a problem hiding this comment.
So, I think I understand why this is needed, but would appreciate your guidance. Correct me if this is wrong; because the scheduling section is currently not read before this change, any users with a state file will see a diff showing a change to scheduling.
However, shouldn't this block check the value rather than the key? Also, if we always read the scheduling section, this would only need to be suppressed once, right?
There was a problem hiding this comment.
Not exactly,
If the user don't add the scheduling block to the config because they are fine with the default values. Then, the desired state as scheduling.# = 0.
But the current state gets refreshed (calls the read method), the populates the values for the current state scheduling. Hence, scheduling.# will always be 0 for the current state.
This means a user gets a perpetual diff if they don't specify the scheduling block (which is an okay thing to do if you are fine with the default values):
scheduling.#: "1" => "0"
There was a problem hiding this comment.
Is there a possibility of this over-suppressing? Like if an element is added/removed/changed but scheduling.# is still 1? Why not Computed?
There was a problem hiding this comment.
Thanks. Computed true on the TypeList itself effectively does the same thing. This is the elegant solution I was looking for.
| 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, |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
google/resource_compute_instance.go
Outdated
| scheduling.Preemptible = val.(bool) | ||
| } | ||
|
|
||
| // The Preemptible field cannot be updated. The instance must be recreated. |
There was a problem hiding this comment.
Hmm, so I think the following scenario would fail:
- preemptible is true
- I want to change one of the other attributes
because I think this would try to update preemptible to false.
However, we can't actually change either of the other attributes when preemptible is true- it would fail server-side. So two options for fixing this:
- put scheduling.Preemptible back in the request- we know that we won't hit this if it changes since it's marked
ForceNew, so this is just to propagate the current value - throw an error when trying to update other parts of scheduling if preemptible is true
I think #1 is clearer in the code and more future-proof, so I recommend that one- up to you though.
|
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
locked and limited conversation to collaborators
issue #211
Marked
preemptibleas ForceNew because this field can only be set at the creation of an instance.Reads
schedulingfields from GCP. Make sure this does not introduce a diff.There should be at most one
schedulingblock. Enforcing it with MaxItems=1cc/ @selmanj @danawillow