Add preemptible as an option to node config #341
Conversation
google/resource_container_cluster.go
Outdated
| @@ -669,6 +669,7 @@ func flattenClusterNodeConfig(c *container.NodeConfig) []map[string]interface{} | |||
| "image_type": c.ImageType, | |||
| "labels": c.Labels, | |||
| "tags": c.Tags, | |||
| "preemptible": c.Preemptible, | |||
There was a problem hiding this comment.
while you're here- it probably makes sense to have this in node_config.go too, right?
| @@ -712,6 +712,7 @@ resource "google_container_cluster" "with_node_config" { | |||
| foo = "bar" | |||
| } | |||
| tags = ["foo", "bar"] | |||
| preemptible = true | |||
There was a problem hiding this comment.
can you also add a check for this in testAccCheckContainerCluster?
There was a problem hiding this comment.
Added!
Interestingly the tests were failing without having to add any changes to check for preemptible (likely because the default is false, and the requested value is true, which creates a perpetual diff unless it's hooked up properly).
BTW, I assumed this method was just checking to see if the resource existed; I didn't realize it did all the checking inside this method. Some other resources look up the remote resource and store the api object in a variable, and then assert on that. Which is preferred for new resources?
There was a problem hiding this comment.
I'm not really sure- I did this one sort of as an experiment, and it definitely doesn't catch everything since it just checks that state matches remote, but not that those are actually the values we want (I actually had a bug slip through once because of this). Right now I've been using TestCheckResourceAttrSet to check that the correct values are in state for what I care about but I don't know if that actually checks that I did it with a .Set.
| @@ -356,6 +356,7 @@ resource "google_container_node_pool" "np_with_node_config" { | |||
| "https://www.googleapis.com/auth/logging.write", | |||
| "https://www.googleapis.com/auth/monitoring" | |||
| ] | |||
| preemptible = true | |||
There was a problem hiding this comment.
likewise for testAccCheckContainerNodePoolMatches
|
@selmanj @danawillow Any idea when this will make it into a release? |
|
👍 , merge at will. @CGavrila this will be in our next release. I don't have a hard date for it yet but probably within the next week or so. |
|
Thanks, @danawillow! Quite excited about this one. |
* Add preemptible as an option to node config * Check for preemptible in test matching functions * Move flattenClusterNodeConfig to node_config * Handle bools properly when comparing in cluster and node pool tests * Use a supported image_type in cluster tests
* Add preemptible as an option to node config * Check for preemptible in test matching functions * Move flattenClusterNodeConfig to node_config * Handle bools properly when comparing in cluster and node pool tests * Use a supported image_type in cluster tests
<!-- This change is generated by MagicModules. --> /cc @rileykarson
|
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
Closes #334