Skip to content

feat:add support for provisioning windows node pools #1402

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

Merged

Conversation

g-awmalik
Copy link
Contributor

@g-awmalik g-awmalik commented Sep 15, 2022

Addresses #1383

Tested with

  • Adding one windows_node_pools to an existing cluster
  • Creating a new cluster with 1 node_pools and 1 windows_node_pools
  • Adding one windows_node_pools to an existing cluster with 2 node_pools (one windows based and one linux based)

Additionally, it doesn't cause breaking changes to existing plans.

@g-awmalik g-awmalik requested review from a team, Jberlinsky and bharathkkb as code owners September 15, 2022 06:24
@g-awmalik
Copy link
Contributor Author

@bharathkkb - PTAL?

Copy link
Member

@bharathkkb bharathkkb left a comment

Choose a reason for hiding this comment

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

I like this approach much better! Thanks for driving this and testing! Mostly minor nits.

Comment on lines 38 to 52
ip_range_pods = var.ip_range_pods
ip_range_services = var.ip_range_services
create_service_account = var.compute_engine_service_account == "create"
service_account = var.compute_engine_service_account
istio = var.istio
cloudrun = var.cloudrun
dns_cache = var.dns_cache
gce_pd_csi_driver = var.gce_pd_csi_driver
sandbox_enabled = var.sandbox_enabled
database_encryption = var.database_encryption
enable_binary_authorization = var.enable_binary_authorization
enable_pod_security_policy = var.enable_pod_security_policy
enable_identity_service = true
release_channel = "REGULAR"
logging_enabled_components = ["SYSTEM_COMPONENTS"]
Copy link
Member

Choose a reason for hiding this comment

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

Let's hardcode most of these. Ideally only project ID is needed as input which we will pass in via fixture.

upper = false
}

resource "google_compute_network" "main" {
Copy link
Member

Choose a reason for hiding this comment

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

We can move this to examples with fixtures being a thin wrapper around examples.

@comment-bot-dev
Copy link

@g-awmalik
Thanks for the PR! 🚀
✅ Lint checks have passed.

@g-awmalik
Copy link
Contributor Author

@bharathkkb - can you take one more look? I want to release this soon. Thanks!

@g-awmalik g-awmalik merged commit 92d7c67 into terraform-google-modules:master Sep 27, 2022
@g-awmalik g-awmalik deleted the feat/add-windows-np branch September 27, 2022 19:15
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.

3 participants