allow updating node image types #1843
Conversation
| @@ -1120,6 +1120,36 @@ func resourceContainerClusterUpdate(d *schema.ResourceData, meta interface{}) er | |||
| d.SetPartial("logging_service") | |||
| } | |||
|
|
|||
| if d.HasChange("node_config") { | |||
| if d.HasChange("node_config.0.image_type") { | |||
There was a problem hiding this comment.
I'm assuming that the node_config.0 is referencing the first instance of node_config in the HCL. In which case I'm also assuming that there is something in place to stop 2 different node_config blocks from existing in the same container cluster definition?
There was a problem hiding this comment.
Exactly.
MaxItems: 1 at https://github.com/terraform-providers/terraform-provider-google/blob/master/google/node_config.go#L27 is that check. The nice thing about it existing in the schema is that it means that when you run terraform plan, it'll tell you that your config is invalid right away, vs doing so after you've started applying.
| return containerBetaOperationWait(config, op, | ||
| nodePoolInfo.project, | ||
| nodePoolInfo.location, "updating GKE node pool", | ||
| timeoutInMinutes, 2) |
There was a problem hiding this comment.
Is there a standard/process for choosing what timeouts should be? I didn't see any reference to timeouts in the gcp api docs.
There was a problem hiding this comment.
Not really, mostly trial and error. Many of our resources have a Timeouts block, which lets the user specify their own timeouts if they think our default ones are wrong. See:
- https://github.com/terraform-providers/terraform-provider-google/blob/master/google/resource_container_node_pool.go#L23
- https://www.terraform.io/docs/providers/google/r/compute_address.html#timeouts (turns out we don't include that section in the gke docs, oops that's a bug)
| if d.HasChange(prefix + "node_config.0.image_type") { | ||
| req := &containerBeta.UpdateClusterRequest{ | ||
| Update: &containerBeta.ClusterUpdate{ | ||
| DesiredNodePoolId: name, |
There was a problem hiding this comment.
Why pass an update to the name in addition to the image type?
There was a problem hiding this comment.
It's what this particular API wants: https://cloud.google.com/kubernetes-engine/docs/reference/rest/v1beta1/ClusterUpdate (also, the GKE API is super unusual with regards to how properties can be updated. Most other resources just use a regular PUT or PATCH or the occasional custom update verb)
| } | ||
|
|
||
| // Call update serially. | ||
| if err := lockedCall(lockKey, updateF); err != nil { |
There was a problem hiding this comment.
Does this mean if a config gets changed in many different top level blocks we will end up making a different update call for each block? eg: 1 update if node_config is updated and another if auto_scaling is update, etc...
There was a problem hiding this comment.
Yup. Again, that's a GKE-specific thing: https://cloud.google.com/kubernetes-engine/docs/reference/rest/v1beta1/ClusterUpdate
Exactly one update can be applied to a cluster with each request, so at most one field can be provided.
Fixes hashicorp#1702. @chrisst I'm putting you as a reviewer, but no rush. Feel free to ask as many questions as you have! Also feel free to offer suggestions 😃 (or just say it's perfect as-is, that works too)
|
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! |
ghost
locked and limited conversation to collaborators
Fixes #1702.
@chrisst I'm putting you as a reviewer, but no rush. Feel free to ask as many questions as you have! Also feel free to offer suggestions 😃 (or just say it's perfect as-is, that works too)