Skip to content

fix: use null comparators in cases where zero or false are meaningful #2513

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.

Sign up for GitHub

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

Open
ItsMattL wants to merge 6 commits into
base: main
Choose a base branch
from
Open

fix: use null comparators in cases where zero or false are meaningful #2513

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
fcfd224
modify advanced_machine_features and queued_provisioning dynamics to …
ItsMattL Jan 5, 2026
36266df
Merge branch 'main' into main
apeabody Jan 6, 2026
48d3d10
default threads_per_core to 0
ItsMattL Feb 2, 2026
db16403
Merge branch 'main' into main
apeabody Feb 2, 2026
fcbc819
Merge branch 'main' into main
apeabody Feb 27, 2026
75f5038
Merge branch 'main' into main
apeabody Mar 3, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions autogen/main/cluster.tf.tmpl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1103,7 +1103,7 @@ resource "google_container_node_pool" "windows_pools" {
}

dynamic "queued_provisioning" {
for_each = lookup(each.value, "queued_provisioning", false) ? [true] : []
for_each = lookup(each.value, "queued_provisioning", null) != null ? [true] : []
content {
enabled = lookup(each.value, "queued_provisioning", null)
}
Expand Down Expand Up @@ -1265,7 +1265,7 @@ resource "google_container_node_pool" "windows_pools" {
}

dynamic "advanced_machine_features" {
for_each = lookup(each.value, "threads_per_core", 0) > 0 || lookup(each.value, "enable_nested_virtualization", false) || lookup(each.value, "performance_monitoring_unit", null) != null ? [1] : []
for_each = lookup(each.value, "threads_per_core", null) != null || lookup(each.value, "enable_nested_virtualization", null) != null || lookup(each.value, "performance_monitoring_unit", null) != null ? [1] : []
content {
threads_per_core = lookup(each.value, "threads_per_core", 0)
Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist bot Feb 27, 2026

Choose a reason for hiding this comment

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

high

While the for_each condition for the advanced_machine_features dynamic block has been correctly updated to distinguish between an unset value (null) and 0 for threads_per_core, the value assignment within the block still defaults to 0. This can lead to unintended behavior where threads_per_core is set to 0 even when it's not specified in the input, which is the issue this PR aims to fix. To be consistent with the changes and the PR's intent, you should use null as the default value here as well.

        threads_per_core             = lookup(each.value, "threads_per_core", null)

Copy link
Copy Markdown
Collaborator

@apeabody apeabody Mar 5, 2026

Choose a reason for hiding this comment

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

This is likely still relevant as the content will be used if any of the conditions are not null, so a null default for threads_per_core would be preferable.

Copy link
Copy Markdown
Author

@ItsMattL ItsMattL Mar 12, 2026

Choose a reason for hiding this comment

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

This interaction is so confusing even Gemini can't follow it, but I think it's right the way it currently is. The first iteration of this PR set all fields to null. I believe we then discovered that threads_per_core is in fact required to be set if advanced_machine_features is configured, so 48d3d10 followed up and changed it back to zero. In other words:

  • If no fields are configured (all null), do not set advanced_machine_features.
  • If any fields are set, set advanced_machine_features plus
    • If threads_per_core is configured set it to configured
    • If threads_per_core is unconfigured, set to zero.

The comment in #2513 (review) contains the output stating that

"threads_per_core" is required

enable_nested_virtualization = lookup(each.value, "enable_nested_virtualization", null)
Expand Down
8 changes: 4 additions & 4 deletions cluster.tf
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -812,7 +812,7 @@ resource "google_container_node_pool" "pools" {
}

dynamic "queued_provisioning" {
for_each = lookup(each.value, "queued_provisioning", false) ? [true] : []
for_each = lookup(each.value, "queued_provisioning", null) != null ? [true] : []
content {
enabled = lookup(each.value, "queued_provisioning", null)
}
Expand Down Expand Up @@ -966,7 +966,7 @@ resource "google_container_node_pool" "pools" {
}

dynamic "advanced_machine_features" {
for_each = lookup(each.value, "threads_per_core", 0) > 0 || lookup(each.value, "enable_nested_virtualization", false) || lookup(each.value, "performance_monitoring_unit", null) != null ? [1] : []
for_each = lookup(each.value, "threads_per_core", null) != null || lookup(each.value, "enable_nested_virtualization", null) != null || lookup(each.value, "performance_monitoring_unit", null) != null ? [1] : []
content {
threads_per_core = lookup(each.value, "threads_per_core", 0)
Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist bot Feb 27, 2026

Choose a reason for hiding this comment

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

high

This line still defaults threads_per_core to 0. To align with the goal of this PR, which is to differentiate between 0 and an unset value, the default should be null. This prevents Terraform from sending a 0 value when the attribute is not specified.

        threads_per_core             = lookup(each.value, "threads_per_core", null)

enable_nested_virtualization = lookup(each.value, "enable_nested_virtualization", null)
Expand Down Expand Up @@ -1199,7 +1199,7 @@ resource "google_container_node_pool" "windows_pools" {
}

dynamic "queued_provisioning" {
for_each = lookup(each.value, "queued_provisioning", false) ? [true] : []
for_each = lookup(each.value, "queued_provisioning", null) != null ? [true] : []
content {
enabled = lookup(each.value, "queued_provisioning", null)
}
Expand Down Expand Up @@ -1353,7 +1353,7 @@ resource "google_container_node_pool" "windows_pools" {
}

dynamic "advanced_machine_features" {
for_each = lookup(each.value, "threads_per_core", 0) > 0 || lookup(each.value, "enable_nested_virtualization", false) || lookup(each.value, "performance_monitoring_unit", null) != null ? [1] : []
for_each = lookup(each.value, "threads_per_core", null) != null || lookup(each.value, "enable_nested_virtualization", null) != null || lookup(each.value, "performance_monitoring_unit", null) != null ? [1] : []
content {
threads_per_core = lookup(each.value, "threads_per_core", 0)
Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist bot Feb 27, 2026

Choose a reason for hiding this comment

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

high

Similar to the other advanced_machine_features block, this line for windows_pools should default threads_per_core to null instead of 0 to correctly handle cases where the value is not specified.

        threads_per_core             = lookup(each.value, "threads_per_core", null)

enable_nested_virtualization = lookup(each.value, "enable_nested_virtualization", null)
Expand Down
8 changes: 4 additions & 4 deletions modules/beta-private-cluster-update-variant/cluster.tf
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -956,7 +956,7 @@ resource "google_container_node_pool" "pools" {
}

dynamic "queued_provisioning" {
for_each = lookup(each.value, "queued_provisioning", false) ? [true] : []
for_each = lookup(each.value, "queued_provisioning", null) != null ? [true] : []
content {
enabled = lookup(each.value, "queued_provisioning", null)
}
Expand Down Expand Up @@ -1116,7 +1116,7 @@ resource "google_container_node_pool" "pools" {
}

dynamic "advanced_machine_features" {
for_each = lookup(each.value, "threads_per_core", 0) > 0 || lookup(each.value, "enable_nested_virtualization", false) || lookup(each.value, "performance_monitoring_unit", null) != null ? [1] : []
for_each = lookup(each.value, "threads_per_core", null) != null || lookup(each.value, "enable_nested_virtualization", null) != null || lookup(each.value, "performance_monitoring_unit", null) != null ? [1] : []
content {
threads_per_core = lookup(each.value, "threads_per_core", 0)
Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist bot Feb 27, 2026

Choose a reason for hiding this comment

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

high

The for_each logic was updated to handle null vs 0 for threads_per_core, but the assignment within the block still defaults to 0. To complete the fix and align with the PR's intent, this should be changed to default to null.

        threads_per_core             = lookup(each.value, "threads_per_core", null)

enable_nested_virtualization = lookup(each.value, "enable_nested_virtualization", null)
Expand Down Expand Up @@ -1357,7 +1357,7 @@ resource "google_container_node_pool" "windows_pools" {
}

dynamic "queued_provisioning" {
for_each = lookup(each.value, "queued_provisioning", false) ? [true] : []
for_each = lookup(each.value, "queued_provisioning", null) != null ? [true] : []
content {
enabled = lookup(each.value, "queued_provisioning", null)
}
Expand Down Expand Up @@ -1517,7 +1517,7 @@ resource "google_container_node_pool" "windows_pools" {
}

dynamic "advanced_machine_features" {
for_each = lookup(each.value, "threads_per_core", 0) > 0 || lookup(each.value, "enable_nested_virtualization", false) || lookup(each.value, "performance_monitoring_unit", null) != null ? [1] : []
for_each = lookup(each.value, "threads_per_core", null) != null || lookup(each.value, "enable_nested_virtualization", null) != null || lookup(each.value, "performance_monitoring_unit", null) != null ? [1] : []
content {
threads_per_core = lookup(each.value, "threads_per_core", 0)
Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist bot Feb 27, 2026

Choose a reason for hiding this comment

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

high

The for_each logic was updated to handle null vs 0 for threads_per_core, but the assignment within the block still defaults to 0. To complete the fix and align with the PR's intent, this should be changed to default to null.

        threads_per_core             = lookup(each.value, "threads_per_core", null)

enable_nested_virtualization = lookup(each.value, "enable_nested_virtualization", null)
Expand Down
8 changes: 4 additions & 4 deletions modules/beta-private-cluster/cluster.tf
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -870,7 +870,7 @@ resource "google_container_node_pool" "pools" {
}

dynamic "queued_provisioning" {
for_each = lookup(each.value, "queued_provisioning", false) ? [true] : []
for_each = lookup(each.value, "queued_provisioning", null) != null ? [true] : []
content {
enabled = lookup(each.value, "queued_provisioning", null)
}
Expand Down Expand Up @@ -1030,7 +1030,7 @@ resource "google_container_node_pool" "pools" {
}

dynamic "advanced_machine_features" {
for_each = lookup(each.value, "threads_per_core", 0) > 0 || lookup(each.value, "enable_nested_virtualization", false) || lookup(each.value, "performance_monitoring_unit", null) != null ? [1] : []
for_each = lookup(each.value, "threads_per_core", null) != null || lookup(each.value, "enable_nested_virtualization", null) != null || lookup(each.value, "performance_monitoring_unit", null) != null ? [1] : []
content {
threads_per_core = lookup(each.value, "threads_per_core", 0)
Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist bot Feb 27, 2026

Choose a reason for hiding this comment

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

high

The for_each logic was updated to handle null vs 0 for threads_per_core, but the assignment within the block still defaults to 0. To complete the fix and align with the PR's intent, this should be changed to default to null.

        threads_per_core             = lookup(each.value, "threads_per_core", null)

enable_nested_virtualization = lookup(each.value, "enable_nested_virtualization", null)
Expand Down Expand Up @@ -1270,7 +1270,7 @@ resource "google_container_node_pool" "windows_pools" {
}

dynamic "queued_provisioning" {
for_each = lookup(each.value, "queued_provisioning", false) ? [true] : []
for_each = lookup(each.value, "queued_provisioning", null) != null ? [true] : []
content {
enabled = lookup(each.value, "queued_provisioning", null)
}
Expand Down Expand Up @@ -1430,7 +1430,7 @@ resource "google_container_node_pool" "windows_pools" {
}

dynamic "advanced_machine_features" {
for_each = lookup(each.value, "threads_per_core", 0) > 0 || lookup(each.value, "enable_nested_virtualization", false) || lookup(each.value, "performance_monitoring_unit", null) != null ? [1] : []
for_each = lookup(each.value, "threads_per_core", null) != null || lookup(each.value, "enable_nested_virtualization", null) != null || lookup(each.value, "performance_monitoring_unit", null) != null ? [1] : []
content {
threads_per_core = lookup(each.value, "threads_per_core", 0)
Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist bot Feb 27, 2026

Choose a reason for hiding this comment

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

high

The for_each logic was updated to handle null vs 0 for threads_per_core, but the assignment within the block still defaults to 0. To complete the fix and align with the PR's intent, this should be changed to default to null.

        threads_per_core             = lookup(each.value, "threads_per_core", null)

enable_nested_virtualization = lookup(each.value, "enable_nested_virtualization", null)
Expand Down
8 changes: 4 additions & 4 deletions modules/beta-public-cluster-update-variant/cluster.tf
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -934,7 +934,7 @@ resource "google_container_node_pool" "pools" {
}

dynamic "queued_provisioning" {
for_each = lookup(each.value, "queued_provisioning", false) ? [true] : []
for_each = lookup(each.value, "queued_provisioning", null) != null ? [true] : []
content {
enabled = lookup(each.value, "queued_provisioning", null)
}
Expand Down Expand Up @@ -1094,7 +1094,7 @@ resource "google_container_node_pool" "pools" {
}

dynamic "advanced_machine_features" {
for_each = lookup(each.value, "threads_per_core", 0) > 0 || lookup(each.value, "enable_nested_virtualization", false) || lookup(each.value, "performance_monitoring_unit", null) != null ? [1] : []
for_each = lookup(each.value, "threads_per_core", null) != null || lookup(each.value, "enable_nested_virtualization", null) != null || lookup(each.value, "performance_monitoring_unit", null) != null ? [1] : []
content {
threads_per_core = lookup(each.value, "threads_per_core", 0)
Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist bot Feb 27, 2026

Choose a reason for hiding this comment

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

high

The for_each logic was updated to handle null vs 0 for threads_per_core, but the assignment within the block still defaults to 0. To complete the fix and align with the PR's intent, this should be changed to default to null.

        threads_per_core             = lookup(each.value, "threads_per_core", null)

enable_nested_virtualization = lookup(each.value, "enable_nested_virtualization", null)
Expand Down Expand Up @@ -1335,7 +1335,7 @@ resource "google_container_node_pool" "windows_pools" {
}

dynamic "queued_provisioning" {
for_each = lookup(each.value, "queued_provisioning", false) ? [true] : []
for_each = lookup(each.value, "queued_provisioning", null) != null ? [true] : []
content {
enabled = lookup(each.value, "queued_provisioning", null)
}
Expand Down Expand Up @@ -1495,7 +1495,7 @@ resource "google_container_node_pool" "windows_pools" {
}

dynamic "advanced_machine_features" {
for_each = lookup(each.value, "threads_per_core", 0) > 0 || lookup(each.value, "enable_nested_virtualization", false) || lookup(each.value, "performance_monitoring_unit", null) != null ? [1] : []
for_each = lookup(each.value, "threads_per_core", null) != null || lookup(each.value, "enable_nested_virtualization", null) != null || lookup(each.value, "performance_monitoring_unit", null) != null ? [1] : []
content {
threads_per_core = lookup(each.value, "threads_per_core", 0)
Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist bot Feb 27, 2026

Choose a reason for hiding this comment

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

high

The for_each logic was updated to handle null vs 0 for threads_per_core, but the assignment within the block still defaults to 0. To complete the fix and align with the PR's intent, this should be changed to default to null.

        threads_per_core             = lookup(each.value, "threads_per_core", null)

enable_nested_virtualization = lookup(each.value, "enable_nested_virtualization", null)
Expand Down
8 changes: 4 additions & 4 deletions modules/beta-public-cluster/cluster.tf
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -848,7 +848,7 @@ resource "google_container_node_pool" "pools" {
}

dynamic "queued_provisioning" {
for_each = lookup(each.value, "queued_provisioning", false) ? [true] : []
for_each = lookup(each.value, "queued_provisioning", null) != null ? [true] : []
content {
enabled = lookup(each.value, "queued_provisioning", null)
}
Expand Down Expand Up @@ -1008,7 +1008,7 @@ resource "google_container_node_pool" "pools" {
}

dynamic "advanced_machine_features" {
for_each = lookup(each.value, "threads_per_core", 0) > 0 || lookup(each.value, "enable_nested_virtualization", false) || lookup(each.value, "performance_monitoring_unit", null) != null ? [1] : []
for_each = lookup(each.value, "threads_per_core", null) != null || lookup(each.value, "enable_nested_virtualization", null) != null || lookup(each.value, "performance_monitoring_unit", null) != null ? [1] : []
content {
threads_per_core = lookup(each.value, "threads_per_core", 0)
Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist bot Feb 27, 2026

Choose a reason for hiding this comment

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

high

The for_each logic was updated to handle null vs 0 for threads_per_core, but the assignment within the block still defaults to 0. To complete the fix and align with the PR's intent, this should be changed to default to null.

        threads_per_core             = lookup(each.value, "threads_per_core", null)

enable_nested_virtualization = lookup(each.value, "enable_nested_virtualization", null)
Expand Down Expand Up @@ -1248,7 +1248,7 @@ resource "google_container_node_pool" "windows_pools" {
}

dynamic "queued_provisioning" {
for_each = lookup(each.value, "queued_provisioning", false) ? [true] : []
for_each = lookup(each.value, "queued_provisioning", null) != null ? [true] : []
content {
enabled = lookup(each.value, "queued_provisioning", null)
}
Expand Down Expand Up @@ -1408,7 +1408,7 @@ resource "google_container_node_pool" "windows_pools" {
}

dynamic "advanced_machine_features" {
for_each = lookup(each.value, "threads_per_core", 0) > 0 || lookup(each.value, "enable_nested_virtualization", false) || lookup(each.value, "performance_monitoring_unit", null) != null ? [1] : []
for_each = lookup(each.value, "threads_per_core", null) != null || lookup(each.value, "enable_nested_virtualization", null) != null || lookup(each.value, "performance_monitoring_unit", null) != null ? [1] : []
content {
threads_per_core = lookup(each.value, "threads_per_core", 0)
Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist bot Feb 27, 2026

Choose a reason for hiding this comment

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

high

The for_each logic was updated to handle null vs 0 for threads_per_core, but the assignment within the block still defaults to 0. To complete the fix and align with the PR's intent, this should be changed to default to null.

        threads_per_core             = lookup(each.value, "threads_per_core", null)

enable_nested_virtualization = lookup(each.value, "enable_nested_virtualization", null)
Expand Down
8 changes: 4 additions & 4 deletions modules/private-cluster-update-variant/cluster.tf
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -919,7 +919,7 @@ resource "google_container_node_pool" "pools" {
}

dynamic "queued_provisioning" {
for_each = lookup(each.value, "queued_provisioning", false) ? [true] : []
for_each = lookup(each.value, "queued_provisioning", null) != null ? [true] : []
content {
enabled = lookup(each.value, "queued_provisioning", null)
}
Expand Down Expand Up @@ -1073,7 +1073,7 @@ resource "google_container_node_pool" "pools" {
}

dynamic "advanced_machine_features" {
for_each = lookup(each.value, "threads_per_core", 0) > 0 || lookup(each.value, "enable_nested_virtualization", false) || lookup(each.value, "performance_monitoring_unit", null) != null ? [1] : []
for_each = lookup(each.value, "threads_per_core", null) != null || lookup(each.value, "enable_nested_virtualization", null) != null || lookup(each.value, "performance_monitoring_unit", null) != null ? [1] : []
content {
threads_per_core = lookup(each.value, "threads_per_core", 0)
Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist bot Feb 27, 2026

Choose a reason for hiding this comment

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

high

The for_each logic was updated to handle null vs 0 for threads_per_core, but the assignment within the block still defaults to 0. To complete the fix and align with the PR's intent, this should be changed to default to null.

        threads_per_core             = lookup(each.value, "threads_per_core", null)

enable_nested_virtualization = lookup(each.value, "enable_nested_virtualization", null)
Expand Down Expand Up @@ -1307,7 +1307,7 @@ resource "google_container_node_pool" "windows_pools" {
}

dynamic "queued_provisioning" {
for_each = lookup(each.value, "queued_provisioning", false) ? [true] : []
for_each = lookup(each.value, "queued_provisioning", null) != null ? [true] : []
content {
enabled = lookup(each.value, "queued_provisioning", null)
}
Expand Down Expand Up @@ -1461,7 +1461,7 @@ resource "google_container_node_pool" "windows_pools" {
}

dynamic "advanced_machine_features" {
for_each = lookup(each.value, "threads_per_core", 0) > 0 || lookup(each.value, "enable_nested_virtualization", false) || lookup(each.value, "performance_monitoring_unit", null) != null ? [1] : []
for_each = lookup(each.value, "threads_per_core", null) != null || lookup(each.value, "enable_nested_virtualization", null) != null || lookup(each.value, "performance_monitoring_unit", null) != null ? [1] : []
content {
threads_per_core = lookup(each.value, "threads_per_core", 0)
Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist bot Feb 27, 2026

Choose a reason for hiding this comment

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

high

The for_each logic was updated to handle null vs 0 for threads_per_core, but the assignment within the block still defaults to 0. To complete the fix and align with the PR's intent, this should be changed to default to null.

        threads_per_core             = lookup(each.value, "threads_per_core", null)

enable_nested_virtualization = lookup(each.value, "enable_nested_virtualization", null)
Expand Down
8 changes: 4 additions & 4 deletions modules/private-cluster/cluster.tf
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -834,7 +834,7 @@ resource "google_container_node_pool" "pools" {
}

dynamic "queued_provisioning" {
for_each = lookup(each.value, "queued_provisioning", false) ? [true] : []
for_each = lookup(each.value, "queued_provisioning", null) != null ? [true] : []
content {
enabled = lookup(each.value, "queued_provisioning", null)
}
Expand Down Expand Up @@ -988,7 +988,7 @@ resource "google_container_node_pool" "pools" {
}

dynamic "advanced_machine_features" {
for_each = lookup(each.value, "threads_per_core", 0) > 0 || lookup(each.value, "enable_nested_virtualization", false) || lookup(each.value, "performance_monitoring_unit", null) != null ? [1] : []
for_each = lookup(each.value, "threads_per_core", null) != null || lookup(each.value, "enable_nested_virtualization", null) != null || lookup(each.value, "performance_monitoring_unit", null) != null ? [1] : []
content {
threads_per_core = lookup(each.value, "threads_per_core", 0)
Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist bot Feb 27, 2026

Choose a reason for hiding this comment

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

high

The for_each logic was updated to handle null vs 0 for threads_per_core, but the assignment within the block still defaults to 0. To complete the fix and align with the PR's intent, this should be changed to default to null.

        threads_per_core             = lookup(each.value, "threads_per_core", null)

enable_nested_virtualization = lookup(each.value, "enable_nested_virtualization", null)
Expand Down Expand Up @@ -1221,7 +1221,7 @@ resource "google_container_node_pool" "windows_pools" {
}

dynamic "queued_provisioning" {
for_each = lookup(each.value, "queued_provisioning", false) ? [true] : []
for_each = lookup(each.value, "queued_provisioning", null) != null ? [true] : []
content {
enabled = lookup(each.value, "queued_provisioning", null)
}
Expand Down Expand Up @@ -1375,7 +1375,7 @@ resource "google_container_node_pool" "windows_pools" {
}

dynamic "advanced_machine_features" {
for_each = lookup(each.value, "threads_per_core", 0) > 0 || lookup(each.value, "enable_nested_virtualization", false) || lookup(each.value, "performance_monitoring_unit", null) != null ? [1] : []
for_each = lookup(each.value, "threads_per_core", null) != null || lookup(each.value, "enable_nested_virtualization", null) != null || lookup(each.value, "performance_monitoring_unit", null) != null ? [1] : []
content {
threads_per_core = lookup(each.value, "threads_per_core", 0)
Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist bot Feb 27, 2026

Choose a reason for hiding this comment

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

high

The for_each logic was updated to handle null vs 0 for threads_per_core, but the assignment within the block still defaults to 0. To complete the fix and align with the PR's intent, this should be changed to default to null.

        threads_per_core             = lookup(each.value, "threads_per_core", null)

enable_nested_virtualization = lookup(each.value, "enable_nested_virtualization", null)
Expand Down