Skip to content
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.

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

Add preemptible as an option to node config #341

Merged
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
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
10 changes: 9 additions & 1 deletion google/node_config.go
Original file line number Diff line number Diff line change
Expand Up @@ -84,6 +84,13 @@ var schemaNodeConfig = &schema.Schema{
ForceNew: true,
Elem: &schema.Schema{Type: schema.TypeString},
},

"preemptible": {
Type: schema.TypeBool,
Optional: true,
ForceNew: true,
Default: false,
},
},
},
}
Expand Down Expand Up @@ -148,7 +155,8 @@ func expandNodeConfig(v interface{}) *container.NodeConfig {
}
nc.Tags = tags
}
// Preemptible Is Optional+Default, so it always has a value
nc.Preemptible = nodeConfig["preemptible"].(bool)

return nc

}
1 change: 1 addition & 0 deletions google/resource_container_cluster.go
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Copy link
Contributor

Choose a reason for hiding this comment

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

while you're here- it probably makes sense to have this in node_config.go too, 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.

Agreed, I moved it

},
}

Expand Down
1 change: 1 addition & 0 deletions google/resource_container_cluster_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -712,6 +712,7 @@ resource "google_container_cluster" "with_node_config" {
foo = "bar"
}
tags = ["foo", "bar"]
preemptible = true
Copy link
Contributor

Choose a reason for hiding this comment

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

can you also add a check for this in testAccCheckContainerCluster?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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?

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

}
}`, acctest.RandString(10))

Expand Down
1 change: 1 addition & 0 deletions google/resource_container_node_pool_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Contributor

Choose a reason for hiding this comment

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

likewise for testAccCheckContainerNodePoolMatches

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added

}
}`, acctest.RandString(10), acctest.RandString(10))

Expand Down
4 changes: 4 additions & 0 deletions website/docs/r/container_cluster.html.markdown
Original file line number Diff line number Diff line change
Expand Up @@ -153,6 +153,10 @@ which the cluster's instances are launched
* `tags` - (Optional) The list of instance tags applied to all nodes. Tags are used to identify
valid sources or targets for network firewalls.

* `preemptible` - (Optional) A boolean that represents whether or not the underlying node VMs
are preemptible. See the [official documentation](https://cloud.google.com/container-engine/docs/preemptible-vm)
for more information. Defaults to false.

**Addons Config** supports the following addons:

* `http_load_balancing` - (Optional) The status of the HTTP Load Balancing
Expand Down
4 changes: 4 additions & 0 deletions website/docs/r/container_node_pool.html.markdown
Original file line number Diff line number Diff line change
Expand Up @@ -105,6 +105,10 @@ resource "google_container_cluster" "primary" {

* `image_type` - (Optional) The image type to use for this node.

* `preemptible` - (Optional) A boolean that represents whether or not the underlying node VMs
are preemptible. See the [official documentation](https://cloud.google.com/container-engine/docs/preemptible-vm)
for more information. Defaults to false.

The `autoscaling` block supports:

* `minNodeCount` - (Required) Minimum number of nodes in the NodePool. Must be >=1 and
Expand Down