Adding update support for labels to google_container_node_pool #6599

NA2047 · 2022-09-23T17:21:43Z

Adding support for Labels to google_container_node_pool.
This should fix hashicorp/terraform-provider-google#10995

If this PR is for Terraform, I acknowledge that I have:

Searched through the issue tracker for an open issue that this either resolves or contributes to, commented on it to claim it, and written "fixes {url}" or "part of {url}" in this PR description. If there were no relevant open issues, I opened one and commented that I would like to work on it (not necessary for very small changes).
Generated Terraform, and ran make test and make lint to ensure it passes unit and linter tests.
Ensured that all new fields I added that can be set by a user appear in at least one example (for generated resources) or third_party test (for handwritten resources or update tests).
Ran relevant acceptance tests (If the acceptance tests do not yet pass or you are unable to run them, please let your reviewer know).
Read the Release Notes Guide before writing my release note below.

Release Note Template for Downstream PRs (will be copied)

container: added support for in-place update of `node_config.0.tags` for `google_container_node_pool` resource

modular-magician · 2022-09-23T17:21:58Z

Oops! It looks like no changelog entry is attached to this PR. Please include a release note block in the PR body, as described in https://github.com/GoogleCloudPlatform/magic-modules/blob/master/.ci/RELEASE_NOTES_GUIDE.md:

```release-note:TYPE
Release note
```

modular-magician · 2022-09-23T17:22:42Z

Hello! I am a robot who works on Magic Modules PRs.

I've detected that you're a community contributor. @shuyama1, a repository maintainer, has been assigned to assist you and help review your changes.

❓ First time contributing? Click here for more details

Your assigned reviewer will help review your code by:

Ensuring it's backwards compatible, covers common error cases, etc.
Summarizing the change into a user-facing changelog note.
Passes tests, either our "VCR" suite, a set of presubmit tests, or with manual test runs.

You can help make sure that review is quick by running local tests and ensuring they're passing in between each push you make to your PR's branch. Also, try to leave a comment with each push you make, as pushes generally don't generate emails.

If your reviewer doesn't get back to you within a week after your most recent change, please feel free to leave a comment on the issue asking them to take a look! In the absence of a dedicated review dashboard most maintainers manage their pending reviews through email, and those will sometimes get lost in their inbox.

modular-magician · 2022-09-23T17:33:35Z

Hi! I'm the modular magician. Your PR generated some diffs in downstreams - here they are.

Diff report:

Terraform Beta: Diff ( 2 files changed, 49 insertions(+))

modular-magician · 2022-09-23T18:37:54Z

Tests analytics

Total tests: 2178
Passed tests 1936
Skipped tests: 238
Failed tests: 4

Action taken

Triggering VCR tests in RECORDING mode for the tests that failed during VCR. Click here to see the failed tests

TestAccFirebaserulesRelease_BasicRelease|TestAccContainerNodePool_withNodeConfig|TestAccComputeInstance_soleTenantNodeAffinities|TestAccCGCSnippet_eventarcWorkflowsExample

modular-magician · 2022-09-23T18:51:01Z

Tests passed during RECORDING mode:
TestAccFirebaserulesRelease_BasicRelease[Debug log]
TestAccContainerNodePool_withNodeConfig[Debug log]

Tests failed during RECORDING mode:
TestAccComputeInstance_soleTenantNodeAffinities[Error message] [Debug log]
TestAccCGCSnippet_eventarcWorkflowsExample[Error message] [Debug log]

Please fix these to complete your PR
View the build log or the debug log for each test

modular-magician · 2022-09-28T20:14:41Z

Oops! It looks like no changelog entry is attached to this PR. Please include a release note block in the PR body, as described in https://github.com/GoogleCloudPlatform/magic-modules/blob/master/.ci/RELEASE_NOTES_GUIDE.md:

```release-note:TYPE
Release note
```

modular-magician · 2022-09-28T20:26:48Z

Hi! I'm the modular magician. Your PR generated some diffs in downstreams - here they are.

Diff report:

Terraform GA: Diff ( 3 files changed, 59 insertions(+), 1 deletion(-))
Terraform Beta: Diff ( 3 files changed, 59 insertions(+), 1 deletion(-))
TF Validator: Diff ( 2 files changed, 3 insertions(+), 3 deletions(-))

modular-magician · 2022-09-28T21:37:21Z

Tests analytics

Total tests: 2192
Passed tests 1936
Skipped tests: 240
Failed tests: 16

Action taken

Triggering VCR tests in RECORDING mode for the tests that failed during VCR. Click here to see the failed tests

TestAccSqlDatabaseInstance_mysqlMajorVersionUpgrade|TestAccFirebaserulesRelease_BasicRelease|TestAccContainerNodePool_withNodeConfig|TestAccComputeInstance_soleTenantNodeAffinities|TestAccComputeRouterInterface_basic|TestAccComputeVpnTunnel_vpnTunnelBetaExample|TestAccComputeForwardingRule_networkTier|TestAccComputeForwardingRule_update|TestAccComputeForwardingRule_forwardingRuleRegionalHttpXlbExample|TestAccClouddeployDeliveryPipeline_DeliveryPipeline|TestAccComputeGlobalForwardingRule_externalTcpProxyLbMigBackendExample|TestAccComputeForwardingRule_forwardingRuleExternallbExample|TestAccComputeForwardingRule_internalTcpUdpLbWithMigBackendExample|TestAccCGCSnippet_eventarcWorkflowsExample|TestAccComputeFirewallPolicy_update|TestAccComputeFirewallPolicyRule_update

modular-magician · 2022-09-28T22:10:11Z

Tests passed during RECORDING mode:
TestAccSqlDatabaseInstance_mysqlMajorVersionUpgrade[Debug log]
TestAccFirebaserulesRelease_BasicRelease[Debug log]
TestAccContainerNodePool_withNodeConfig[Debug log]
TestAccComputeRouterInterface_basic[Debug log]
TestAccComputeVpnTunnel_vpnTunnelBetaExample[Debug log]
TestAccComputeForwardingRule_networkTier[Debug log]
TestAccComputeForwardingRule_update[Debug log]
TestAccComputeForwardingRule_forwardingRuleRegionalHttpXlbExample[Debug log]
TestAccClouddeployDeliveryPipeline_DeliveryPipeline[Debug log]
TestAccComputeGlobalForwardingRule_externalTcpProxyLbMigBackendExample[Debug log]
TestAccComputeForwardingRule_forwardingRuleExternallbExample[Debug log]
TestAccComputeForwardingRule_internalTcpUdpLbWithMigBackendExample[Debug log]
TestAccComputeFirewallPolicy_update[Debug log]
TestAccComputeFirewallPolicyRule_update[Debug log]

Tests failed during RECORDING mode:
TestAccComputeInstance_soleTenantNodeAffinities[Error message] [Debug log]
TestAccCGCSnippet_eventarcWorkflowsExample[Error message] [Debug log]

Please fix these to complete your PR
View the build log or the debug log for each test

shuyama1 · 2022-09-30T17:05:43Z

mmv1/third_party/terraform/utils/node_config.go.erb

@@ -277,7 +277,7 @@ func schemaNodeConfig() *schema.Schema {
 				"tags": {
 					Type:     schema.TypeList,
 					Optional: true,
-					ForceNew: true,
+					ForceNew: false,


ForceNew is set to false as default and we don't need to set it explicitly.

modular-magician · 2022-09-30T18:35:04Z

Hi! I'm the modular magician. Your PR generated some diffs in downstreams - here they are.

Diff report:

Terraform GA: Diff ( 3 files changed, 58 insertions(+), 1 deletion(-))
Terraform Beta: Diff ( 3 files changed, 58 insertions(+), 1 deletion(-))
TF Validator: Diff ( 2 files changed, 3 insertions(+), 3 deletions(-))

modular-magician · 2022-09-30T19:16:04Z

Tests analytics

Total tests: 2193
Passed tests 1951
Skipped tests: 240
Failed tests: 2

Action taken

Triggering VCR tests in RECORDING mode for the tests that failed during VCR. Click here to see the failed tests

TestAccSqlDatabaseInstance_mysqlMajorVersionUpgrade|TestAccComputeInstance_soleTenantNodeAffinities

modular-magician · 2022-09-30T19:48:07Z

Tests passed during RECORDING mode:
TestAccSqlDatabaseInstance_mysqlMajorVersionUpgrade[Debug log]

Tests failed during RECORDING mode:
TestAccComputeInstance_soleTenantNodeAffinities[Error message] [Debug log]

Please fix these to complete your PR
View the build log or the debug log for each test

shuyama1 · 2022-10-03T22:15:58Z

mmv1/third_party/terraform/utils/node_config.go.erb

@@ -277,7 +277,6 @@ func schemaNodeConfig() *schema.Schema {
 				"tags": {
 					Type:     schema.TypeList,
 					Optional: true,
-					ForceNew: true,


This file is also used in google_container_cluster resource. Do we know if the tags under google_container_cluster resource can also be updated. If not, we probably want to add it to forceNewClusterNodeConfigFields in resource_container_cluster.go.erb file at https://github.com/GoogleCloudPlatform/magic-modules/blob/main/mmv1/third_party/terraform/resources/resource_container_cluster.go.erb#L78.

Yes I see, looking at the documentation

nodeConfig (deprecated)

Parameters used in creating the cluster's nodes. For requests, this field should only be used in lieu of a "nodePool" object, since this configuration (along with the "initialNodeCount") will be used to create a "NodePool" object with an auto-generated name. Do not use this and a nodePool at the same time. For responses, this field will be populated with the node configuration of the first node pool. (For configuration of each node pool, see nodePool.config)

It looks like the preferred method is to not define the nodeConfig within the google_container_cluster
https://cloud.google.com/kubernetes-engine/docs/reference/rest/v1/projects.locations.clusters

node_pool - (Optional) List of node pools associated with this cluster. Warning: node pools defined inside a cluster can't be changed (or added/removed) after cluster creation without deleting and recreating the entire cluster. Unless you absolutely need the ability to say "these are the only node pools associated with this cluster", use the [google_container_node_pool] resource instead of this property.

let me know what you think @shuyama1

UPDATE:

I have tested defining the node_pool inside of the google_container_cluster and the tags do update as expected without changing/updating forceNewClusterNodeConfigFields in resource_container_cluster.go.erb

I believe this is due to this code block, I could be wrong

(

magic-modules/mmv1/third_party/terraform/resources/resource_container_cluster.go.erb

Line 2813 in 81a823e

if n, ok := d.GetOk("node_pool.#"); ok {

)

I believe if the tag can be updated by defining the node_pool inside of the google_container_cluster, which means the in-place update functionality can be supported also in google_container_cluster, then we don't need to add this field to forceNewClusterNodeConfigFields (it should only contains the field that can be updated for google_container_node_pool but not google_container_cluster).

Understood makes sense to me

modular-magician · 2022-10-06T18:44:26Z

Hi! I'm the modular magician. Your PR generated some diffs in downstreams - here they are.

Diff report:

Terraform GA: Diff ( 3 files changed, 60 insertions(+), 1 deletion(-))
Terraform Beta: Diff ( 3 files changed, 60 insertions(+), 1 deletion(-))
TF Validator: Diff ( 2 files changed, 3 insertions(+), 3 deletions(-))

modular-magician · 2022-10-06T19:20:36Z

Tests analytics

Total tests: 2206
Passed tests 1962
Skipped tests: 240
Failed tests: 4

Action taken

Triggering VCR tests in RECORDING mode for the tests that failed during VCR. Click here to see the failed tests

TestAccContainerNodePool_withNodeConfig|TestAccComputeInstance_soleTenantNodeAffinities|TestAccCGCSnippet_eventarcWorkflowsExample|TestAccFirebaserulesRelease_BasicRelease

modular-magician · 2022-10-06T19:31:42Z

Tests passed during RECORDING mode:
TestAccContainerNodePool_withNodeConfig[Debug log]
TestAccFirebaserulesRelease_BasicRelease[Debug log]

Tests failed during RECORDING mode:
TestAccComputeInstance_soleTenantNodeAffinities[Error message] [Debug log]
TestAccCGCSnippet_eventarcWorkflowsExample[Error message] [Debug log]

Please fix these to complete your PR
View the build log or the debug log for each test

modular-magician · 2022-10-07T14:55:58Z

Hi! I'm the modular magician. Your PR generated some diffs in downstreams - here they are.

Diff report:

Terraform GA: Diff ( 3 files changed, 59 insertions(+), 1 deletion(-))
Terraform Beta: Diff ( 3 files changed, 59 insertions(+), 1 deletion(-))
TF Validator: Diff ( 2 files changed, 3 insertions(+), 3 deletions(-))

modular-magician · 2022-10-07T15:32:48Z

Tests analytics

Total tests: 2207
Passed tests 1963
Skipped tests: 240
Failed tests: 4

Action taken

Triggering VCR tests in RECORDING mode for the tests that failed during VCR. Click here to see the failed tests

TestAccFirebaserulesRelease_BasicRelease|TestAccComputeInstance_soleTenantNodeAffinities|TestAccComputeForwardingRule_internalTcpUdpLbWithMigBackendExample|TestAccCGCSnippet_eventarcWorkflowsExample

modular-magician · 2022-10-07T15:37:47Z

Tests passed during RECORDING mode:
TestAccFirebaserulesRelease_BasicRelease[Debug log]
TestAccComputeInstance_soleTenantNodeAffinities[Debug log]
TestAccComputeForwardingRule_internalTcpUdpLbWithMigBackendExample[Debug log]

Tests failed during RECORDING mode:
TestAccCGCSnippet_eventarcWorkflowsExample[Error message] [Debug log]

Please fix these to complete your PR
View the build log or the debug log for each test

shuyama1

LGTM! Thanks!

korenlev · 2023-01-27T02:39:31Z

If the node_pool is configured with auto scaling enabled then Update any metadata in node pool will FAIL with the following google error:

"Updates for 'labels' are not supported in node pools with autoscaling enabled (as a workaround, consider temporarily disabling autoscaling or recreating the node pool with the updated values.)"

You can publish this error back to the user,
so he can decide to use this temp workaround by google or not.
You can validate this with upstream (the relationship with auto scaling property).

jgogstad · 2023-02-08T07:54:17Z

The title of this PR is a bit misleading, this PR adds support for tags, not labels

NA2047 marked this pull request as draft September 23, 2022 17:22

modular-magician requested a review from shuyama1 September 23, 2022 17:22

g-awmalik mentioned this pull request Sep 27, 2022

GKE Nodepool adding tags & labels should not recreate hashicorp/terraform-provider-google#12620

Closed

2 tasks

NA2047 force-pushed the b/239739524 branch from 9ba09c5 to 48a74ab Compare September 28, 2022 20:14

NA2047 marked this pull request as ready for review September 28, 2022 20:17

shuyama1 reviewed Sep 30, 2022

View reviewed changes

NA2047 force-pushed the b/239739524 branch from 48a74ab to e4a726a Compare September 30, 2022 18:21

modular-magician requested a review from shuyama1 September 30, 2022 18:22

shuyama1 reviewed Oct 3, 2022

View reviewed changes

NA2047 force-pushed the b/239739524 branch from e4a726a to 2ff095e Compare October 6, 2022 18:32

modular-magician requested a review from shuyama1 October 6, 2022 18:33

NA2047 force-pushed the b/239739524 branch 2 times, most recently from 56f0bec to 5f8ce27 Compare October 7, 2022 14:42

Adding update support for labels to google_container_node_pool

75a014d

NA2047 force-pushed the b/239739524 branch from 5f8ce27 to 75a014d Compare October 7, 2022 14:43

shuyama1 approved these changes Oct 12, 2022

View reviewed changes

shuyama1 merged commit b38ecf2 into GoogleCloudPlatform:main Oct 12, 2022

This was referenced Oct 12, 2022

Adding update support for labels to google_container_node_pool hashicorp/terraform-provider-google-beta#4781

Merged

Adding update support for labels to google_container_node_pool hashicorp/terraform-provider-google#12773

Merged

NA2047 mentioned this pull request Oct 12, 2022

Support labels for instances in node pool hashicorp/terraform-provider-google#1758

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adding update support for labels to google_container_node_pool #6599

Adding update support for labels to google_container_node_pool #6599

NA2047 commented Sep 23, 2022 •

edited by shuyama1

modular-magician commented Sep 23, 2022

modular-magician commented Sep 23, 2022

modular-magician commented Sep 23, 2022

modular-magician commented Sep 23, 2022

modular-magician commented Sep 23, 2022

modular-magician commented Sep 28, 2022

modular-magician commented Sep 28, 2022

modular-magician commented Sep 28, 2022

modular-magician commented Sep 28, 2022

shuyama1 Sep 30, 2022

NA2047 Sep 30, 2022

modular-magician commented Sep 30, 2022

modular-magician commented Sep 30, 2022

modular-magician commented Sep 30, 2022

shuyama1 Oct 3, 2022

NA2047 Oct 4, 2022 •

edited

shuyama1 Oct 11, 2022

NA2047 Oct 12, 2022

modular-magician commented Oct 6, 2022

modular-magician commented Oct 6, 2022

modular-magician commented Oct 6, 2022

modular-magician commented Oct 7, 2022

modular-magician commented Oct 7, 2022

modular-magician commented Oct 7, 2022

shuyama1 left a comment

korenlev commented Jan 27, 2023

jgogstad commented Feb 8, 2023

Adding update support for labels to google_container_node_pool #6599

Adding update support for labels to google_container_node_pool #6599

Conversation

NA2047 commented Sep 23, 2022 • edited by shuyama1

modular-magician commented Sep 23, 2022

modular-magician commented Sep 23, 2022

modular-magician commented Sep 23, 2022

Diff report:

modular-magician commented Sep 23, 2022

Tests analytics

Action taken

modular-magician commented Sep 23, 2022

modular-magician commented Sep 28, 2022

modular-magician commented Sep 28, 2022

Diff report:

modular-magician commented Sep 28, 2022

Tests analytics

Action taken

modular-magician commented Sep 28, 2022

shuyama1 Sep 30, 2022

Choose a reason for hiding this comment

NA2047 Sep 30, 2022

Choose a reason for hiding this comment

modular-magician commented Sep 30, 2022

Diff report:

modular-magician commented Sep 30, 2022

Tests analytics

Action taken

modular-magician commented Sep 30, 2022

shuyama1 Oct 3, 2022

Choose a reason for hiding this comment

NA2047 Oct 4, 2022 • edited

Choose a reason for hiding this comment

shuyama1 Oct 11, 2022

Choose a reason for hiding this comment

NA2047 Oct 12, 2022

Choose a reason for hiding this comment

modular-magician commented Oct 6, 2022

Diff report:

modular-magician commented Oct 6, 2022

Tests analytics

Action taken

modular-magician commented Oct 6, 2022

modular-magician commented Oct 7, 2022

Diff report:

modular-magician commented Oct 7, 2022

Tests analytics

Action taken

modular-magician commented Oct 7, 2022

shuyama1 left a comment

Choose a reason for hiding this comment

korenlev commented Jan 27, 2023

jgogstad commented Feb 8, 2023

NA2047 commented Sep 23, 2022 •

edited by shuyama1

NA2047 Oct 4, 2022 •

edited