-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
feat!: add service_externalips option, and add default for autoscaling location_policy #1440
Conversation
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
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 for the PR @jpetrucciani
Looks like this was added in 4.35 so we need to bump the minimum version in
version = ">= 4.29.0, < 5.0" |
@@ -202,6 +202,10 @@ resource "google_container_cluster" "primary" { | |||
} | |||
} | |||
|
|||
service_external_ips_config { |
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 we make this a dynamic block if enabled?
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.
Sure! let me include in the changes I'll make locally
@@ -96,6 +96,12 @@ variable "http_load_balancing" { | |||
default = true | |||
} | |||
|
|||
variable "service_externalips" { |
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.
variable "service_externalips" { | |
variable "service_external_ips" { |
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.
sure, sounds good - I will make some changes locally and update!
max_node_count = lookup(autoscaling.value, "max_count", 100) | ||
min_node_count = lookup(autoscaling.value, "min_count", 1) | ||
max_node_count = lookup(autoscaling.value, "max_count", 100) | ||
location_policy = "BALANCED" |
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.
This seems like an unrelated change?
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.
Unrelated to external ips yes, but this was something I've seen happen in clusters as of 1.24 - they get this default value from the provider, but this module will remove it on the next plan + apply. I can separate into second PR (and make it a var) if you'd like!
made a new branch to represent just the main feature #1441 |
@jpetrucciani |
This PR adds the ability to set
service_external_ips_config
on the underlying cluster upon creation, which otherwise can (in my experience) take up to 20-30 minutes to enable independently after the cluster settles after initial creation. I've also added a default value for autoscalinglocation_policy
for now, as this is something that appears to have a default on clusters created with 1.24+, but appears to be removed after a second plan+apply with this module.I added the changes to the templates in
./autogen/
and ranmake build
in order to generate the changes across the repoI've been testing cluster creation with these new settings already via my fork (and it appears to be working well! shaves a good amount of time off of our new cluster creation process), but would love to see it included in the official module if possible! Please let me know if there are any changes you'd like, or things I should take into account!