Make zone configuration optional when creating a regional cluster #19
Conversation
cluster_regional.tf
Outdated
| @@ -24,7 +24,7 @@ resource "google_container_cluster" "primary" { | |||
| project = "${var.project_id}" | |||
|
|
|||
| region = "${var.region}" | |||
| additional_zones = "${var.zones}" | |||
| additional_zones = ["${coalescelist(compact(var.zones), data.google_compute_zones.available.names)}"] | |||
There was a problem hiding this comment.
I like the idea of this approach much better than having to do a separate resource.
When additional_zones is not set, the provider will pick 3 available zones. This will pick all available zones.
I think we could accomplish randomly picking 3 with the Random Provider
resource "random_shuffle" "available_zones" {
input = "${data.google_compute_zones.available.names}"
result_count = 3
}
additional_zones = ["${coalescelist(sort(compact(var.zones)), sort(random_shuffle.available_zones))}"]
| @@ -0,0 +1,17 @@ | |||
| # Simple Regional Cluster | |||
There was a problem hiding this comment.
Since we're explicitly calling out that it's a regional example, lets delete examples/simple since that one one was inherently a regional cluster. I like it being simple_regional better... makes it more clear.
|
@ryanckoch Changes made; pinging for re-review. |
| @@ -27,18 +27,34 @@ function export_vars() { | |||
| export TEST_ID="modules_gke_integration_gcloud_${RANDOM}" | |||
| export KUBECONFIG="${TEMPDIR}/${CLUSTER_TYPE}/${TEST_ID}.kubeconfig" | |||
| if [[ $CLUSTER_TYPE = "regional" ]]; then | |||
| if [ -f "./regional_config.sh" ]; then | |||
There was a problem hiding this comment.
The complex variable export logic defined in this function adds a barrier to troubleshooting tests. If I needed to test out some modifications to the module, run a kitchen converge, and then kitchen verify, I would have to either mangle this script to generate the proper exports, or manually export the variables myself.
In addition, the sourcing of additional scripts means that if I set some variables in config.sh, the zonal_config.sh and regional_config.sh will silently stomp on those variables, which is no fun.
This only affects the tests and not the correctness of the module, so I think that we can merge this. @ryanckoch @Jberlinsky what do you think of filing a ticket/doing a followup pull request that converts the tests to something like what we do for the cloud-datastore module?
There was a problem hiding this comment.
In general, I like what we're doing in cloud-datastore. I agree that this doesn't affect module correctness, and believe that the correct path is to merge this, and follow on in the future with a test conversion. There are two ways that come to mind as to how we could accomplish this -- I tend to prefer the former:
- Converge on a singular methodology as we go -- the next person to pick up an adequately significant task affecting tests in this repository would get the "converge on this methodology" ticket; or
- Add explicit tickets to the backlog to execute a convergence of testing systems for each repository, and prioritize them along with functional requests.
@morgante, would love to hear your thoughts.
There was a problem hiding this comment.
It looks like my reply got lost here.
My preference is that we merge towards the style of testing which @adrienthebo developed (and away from the bash scripts). We can add tickets to the backlog and whoever picks up each module next will also work on the test.
There was a problem hiding this comment.
@morgante Once this is merged in, I'll create tickets. Thanks for weighing in!
|
@ryanckoch Pinging for re-review. |
|
Since @adrienthebo approved and all of @ryanckoch's comments were addressed, I'm merging. |
…optional-for-regional-cluster Make zone configuration optional when creating a regional cluster
When creating a regional cluster, and no zones are provided in
var.zones, create the cluster in all available zones in the specified region.Addresses #6