Allow specifying accelerators in cluster node_config (#1067) #1115
Conversation
|
Tagging @ndmckinley for expedited review :-) |
| accels := v.([]interface{}) | ||
| guestAccelerators := make([]*container.AcceleratorConfig, 0, len(accels)) | ||
| for _, raw := range accels { | ||
| data := raw.(map[string]interface{}) |
There was a problem hiding this comment.
It seems like you could make accels a []map[string]interface{}, which would mean you'd only have to do one of these casts.
There was a problem hiding this comment.
Making accels a []map[string]interface{} leads to an error:
panic: interface conversion: interface {} is []interface {}, not []map[string]interface {}
goroutine 297 [running]:
github.com/terraform-providers/terraform-provider-google/google.expandNodeConfig(0x16797c0, 0xc4205361c0, 0xb)
/home/i056593/Code/go/src/github.com/terraform-providers/terraform-provider-google/google/node_config.go:154 +0x12e0
There was a problem hiding this comment.
(Side note: Line 154 is basically copied from https://github.com/terraform-providers/terraform-provider-google/blob/master/google/resource_compute_instance_template.go#L504)
There was a problem hiding this comment.
Ah, all right, that makes sense.
| continue | ||
| } | ||
| guestAccelerators = append(guestAccelerators, &container.AcceleratorConfig{ | ||
| AcceleratorCount: int64(data["count"].(int)), |
There was a problem hiding this comment.
Should be able to do data["count"].(int64) - why do it this way?
There was a problem hiding this comment.
Using data["count"].(int64) leads to an error:
panic: interface conversion: interface {} is int, not int64
goroutine 296 [running]:
github.com/terraform-providers/terraform-provider-google/google.expandNodeConfig(0x16797c0, 0xc42035a040, 0xb)
/home/i056593/Code/go/src/github.com/terraform-providers/terraform-provider-google/google/node_config.go:162 +0x12b4
There was a problem hiding this comment.
(Side note: line 162 is basically copied from https://github.com/terraform-providers/terraform-provider-google/blob/master/google/resource_compute_instance_template.go#L512)
| Check: resource.ComposeTestCheckFunc( | ||
| testAccCheckContainerNodePoolMatches("google_container_node_pool.np_with_gpu"), | ||
| ), | ||
| }, |
There was a problem hiding this comment.
Looks great - can you stick an ImportState step on there so we know that accelerated node pools can be imported?
There was a problem hiding this comment.
It would be the only ImportState in this file - why introduce it for just this case?
There was a problem hiding this comment.
We've talked to Hashicorp about best practices recently, and so it's a new thing we're doing with new tests.
| guest_accelerator = [{ | ||
| type="nvidia-tesla-k80" | ||
| count=1 | ||
| }] | ||
| } |
There was a problem hiding this comment.
You'll need to add the details of this structure to the documentation below here.
|
Go ahead and add that test and it'll be good to merge. |
|
ImportState test added & passing - looking OK? |
|
Running the tests now, and then I'll merge it. |
|
Some difficulties with the test environment but it should only be another few hours. Monday at the latest. |
|
Aha, great, tests pass now. Merging. |
|
Awesome - thanks! |
|
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
Fixes #1067