-
Notifications
You must be signed in to change notification settings - Fork 9.6k
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
[GKE] Allow additional zones to be configured #11018
[GKE] Allow additional zones to be configured #11018
Conversation
@@ -282,6 +288,24 @@ func resourceContainerClusterCreate(d *schema.ResourceData, meta interface{}) er | |||
cluster.InitialClusterVersion = v.(string) | |||
} | |||
|
|||
if v, ok := d.GetOk("additional_zones"); ok { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you add support to the Update method as well?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a way to update this without recreating the whole cluster? I did not find any. I guess I should set ForeNew: True
then. Currently, the only argument that can be updated is node_version
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, you're right. The API docs claim you can update locations but the client library hasn't been updated with that change yet (hopefully that'll get fixed soon, I've been doing a fair amount of poking but it's apparently complicated). ForceNew: True
sounds good then.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added ForceNew: True
.
Config: testAccContainerCluster_withAdditionalZones, | ||
Check: resource.ComposeTestCheckFunc( | ||
testAccCheckContainerClusterExists( | ||
"google_container_cluster.with_additional_zones"), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd love to see a check that confirms that the cluster not only exists but has the additional zones.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, I'll have a look at this. I basically just looked at what the other tests do and followed that principle.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! Yeah, I'm trying to make sure that new tests that get added are more comprehensive than what we currently have, and that they're testing the right things.
@@ -282,6 +288,24 @@ func resourceContainerClusterCreate(d *schema.ResourceData, meta interface{}) er | |||
cluster.InitialClusterVersion = v.(string) | |||
} | |||
|
|||
if v, ok := d.GetOk("additional_zones"); ok { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, you're right. The API docs claim you can update locations but the client library hasn't been updated with that change yet (hopefully that'll get fixed soon, I've been doing a fair amount of poking but it's apparently complicated). ForceNew: True
sounds good then.
if additionalZonesSize, err = strconv.Atoi(rs.Primary.Attributes["additional_zones.#"]); err != nil { | ||
return err | ||
} | ||
if additionalZonesSize < 2 { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
!= 2?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed.
Config: testAccContainerCluster_withAdditionalZones, | ||
Check: resource.ComposeTestCheckFunc( | ||
testAccCheckContainerClusterExists( | ||
"google_container_cluster.with_additional_zones"), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! Yeah, I'm trying to make sure that new tests that get added are more comprehensive than what we currently have, and that they're testing the right things.
Looks good, thanks @unguiculus! |
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 have found a problem that seems similar to this, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further. |
Fixes #6397.