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

Issue 43: Fix issue with single zone, and update test for the problem. #50

Merged
merged 13 commits into from
Jan 8, 2019

Conversation

ogreface
Copy link

@ogreface ogreface commented Jan 2, 2019

No description provided.

@@ -51,7 +51,7 @@ locals {
}

cluster_type_output_regional_zones = "${concat(google_container_cluster.primary.*.additional_zones, list(list()))}"
cluster_type_output_zonal_zones = "${concat(google_container_cluster.zonal_primary.*.additional_zones, list(list()))}"
cluster_type_output_zonal_zones = "${concat(slice(var.zones,1,length(var.zones)), list(list()))}"
Copy link
Author

Choose a reason for hiding this comment

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

This is the relevant change

Choose a reason for hiding this comment

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

For my own understanding, is the issue google_container_cluster.zonal_primary.*.additional_zones sometimes resolves to null instead of a list?

Choose a reason for hiding this comment

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

For my own understanding, is the issue google_container_cluster.zonal_primary.*.additional_zones sometimes resolves to null instead of a list?

Ah, I was able to clarify my understanding back in the bug report in #43

Error: Error running plan: 1 error(s) occurred:

* module.gke.local.cluster_type_output_zonal_zones: local.cluster_type_output_zonal_zones: Resource 'google_container_cluster.zonal_primary' does not have attribute 'additional_zones' for variable 'google_container_cluster.zonal_primary.*.additional_zones'

Copy link
Author

Choose a reason for hiding this comment

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

Yep, that's exactly right. When there's no additional zones, the provider returns null, which breaks that logic.

Copy link
Contributor

@morgante morgante left a comment

Choose a reason for hiding this comment

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

Don't we also need to address this?

additional_zones = ["${slice(var.zones,1,length(var.zones))}"]

@@ -0,0 +1,47 @@
# Simple Zonal Cluster
Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of introducing a full new example/fixture, could we just make the simple_zonal example use a single zone?

Copy link
Author

Choose a reason for hiding this comment

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

Sure

@ogreface
Copy link
Author

ogreface commented Jan 3, 2019

@morgante That line is fine. Slice returns an empty array, not a null.

@ogreface
Copy link
Author

ogreface commented Jan 3, 2019

Updated, tests running now.

morgante
morgante previously approved these changes Jan 4, 2019
Copy link
Contributor

@morgante morgante left a comment

Choose a reason for hiding this comment

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

LGTM

@ogreface ogreface changed the title Issue 43: Fix issue with single zone, and add test/example for the problem. Issue 43: Fix issue with single zone, and update test for the problem. Jan 4, 2019
@Jberlinsky Jberlinsky merged commit eeca20a into terraform-google-modules:master Jan 8, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants