Added ability to configure resource labels on a GKE cluster #1663

garethjevans · 2018-06-15T10:39:58Z

No description provided.

garethjevans · 2018-06-15T10:40:29Z

11:22 $ make testacc TEST=./google TESTARGS='-run=TestAccContainerCluster_withResourceLabels'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./google -v -run=TestAccContainerCluster_withResourceLabels -timeout 120m
=== RUN   TestAccContainerCluster_withResourceLabels
=== PAUSE TestAccContainerCluster_withResourceLabels
=== CONT  TestAccContainerCluster_withResourceLabels
--- PASS: TestAccContainerCluster_withResourceLabels (250.03s)
PASS
ok  	github.com/terraform-providers/terraform-provider-google/google	250.066s

paddycarver

Hi @garethjevans! Thanks for the PR! I have a couple minor suggestions on it. Let me know if you have any questions or concerns.

paddycarver · 2018-06-22T19:54:12Z

google/resource_container_cluster_test.go

@@ -2229,3 +2250,18 @@ resource "google_container_cluster" "shared_vpc_cluster" {
 	]
 }`, projectName, org, billingId, projectName, org, billingId, acctest.RandString(10), acctest.RandString(10), name)
 }
+
+func testAccContainerCluster_withResourceLabels() string {
+	testId := acctest.RandString(10)


Ideally, usually we pass this kind of thing in as an argument, so we have control over whether configs are identical or different when we're referencing them.

paddycarver · 2018-06-22T19:54:48Z

google/resource_container_cluster_test.go

+	return fmt.Sprintf(`
+resource "google_container_cluster" "with_resource_labels" {
+	name = "tf-cluster-resourcelabel-test-%s"
+	zone = "us-central1-a"


nitpick: There's some weird indentation going on here.

paddycarver · 2018-06-22T19:55:20Z

website/docs/r/container_cluster.html.markdown

@@ -172,6 +172,8 @@ output "cluster_ca_certificate" {

 * `remove_default_node_pool` - (Optional) If true, deletes the default node pool upon cluster creation.

+* `resource_labels` - (Optional) The GCE resource labels (key/value pairs) to be applied to the cluster.


Could we specify this is a map, just so users know how to supply the information?

I couldn't find an example of this in the docs (where labels are set elsewhere), but have mentioned its a map

Gah, sorry to not have a good example for you. Thank you for addressing it anyways!

garethjevans · 2018-06-28T08:01:32Z

@paddycarver the requested changes have been made

paddycarver

Tests pass, and I'm happy with the code changes. Looks good to me. Let me just confirm I shouldn't have someone more familiar with GKE double check this, and I'll get it merged in, likely within the next hour or two.

rolandkool · 2018-07-05T15:48:33Z

Hi,

I was testing this change and found two issues:

If you run terraform with the provider containing this code against a cluster that does not have a resource_labels parameter configured, it crashes. The diff shows it want to remove the labels that already exist but fails with this error:

[DEBUG] plugin.terraform-provider-google:
[DEBUG] plugin.terraform-provider-google: goroutine 92 [running]:
[DEBUG] plugin.terraform-provider-google: github.com/terraform-providers/terraform-provider-google/google.resourceContainerClusterUpdate(0xc420443960, 0x16b0800, 0xc4200a86e0, 0x0, 0xc4208dbb88)
[DEBUG] plugin.terraform-provider-google: /home/rkool/go/src/github.com/terraform-providers/terraform-provider-google/google/resource_container_cluster.go:1148 +0x4362
[DEBUG] plugin.terraform-provider-google: github.com/terraform-providers/terraform-provider-google/vendor/github.com/hashicorp/terraform/helper/schema.(*Resource).Apply(0xc420355960, 0xc42040e870, 0xc42028b920, 0x16b0800, 0xc4200a86e0, 0x1, 0xc4208c1a10, 0xc420615740)
[DEBUG] plugin.terraform-provider-google: /home/rkool/go/src/github.com/terraform-providers/terraform-provider-google/vendor/github.com/hashicorp/terraform/helper/schema/resource.go:233 +0x259
[DEBUG] plugin.terraform-provider-google: github.com/terraform-providers/terraform-provider-google/vendor/github.com/hashicorp/terraform/helper/schema.(*Provider).Apply(0xc42038d340, 0xc42040e820, 0xc42040e870, 0xc42028b920, 0x7fde2da14d90, 0x0, 0x18)
[DEBUG] plugin.terraform-provider-google: /home/rkool/go/src/github.com/terraform-providers/terraform-provider-google/vendor/github.com/hashicorp/terraform/helper/schema/provider.go:283 +0xa4
[DEBUG] plugin.terraform-provider-google: github.com/terraform-providers/terraform-provider-google/vendor/github.com/hashicorp/terraform/plugin.(*ResourceProviderServer).Apply(0xc4204ba9c0, 0xc42028b4c0, 0xc420745ed0, 0x0, 0x0)
[DEBUG] plugin.terraform-provider-google: /home/rkool/go/src/github.com/terraform-providers/terraform-provider-google/vendor/github.com/hashicorp/terraform/plugin/resource_provider.go:527 +0x57
[DEBUG] plugin.terraform-provider-google: reflect.Value.call(0xc420404900, 0xc42040b590, 0x13, 0x19e7888, 0x4, 0xc4203fcf20, 0x3, 0x3, 0x20002, 0xc42044e71c, ...)
[DEBUG] plugin.terraform-provider-google: /home/rkool/go/src/reflect/value.go:434 +0x905
[DEBUG] plugin.terraform-provider-google: reflect.Value.Call(0xc420404900, 0xc42040b590, 0x13, 0xc42044e720, 0x3, 0x3, 0x80c0bf, 0x1, 0x0)
[DEBUG] plugin.terraform-provider-google: /home/rkool/go/src/reflect/value.go:302 +0xa4
[DEBUG] plugin.terraform-provider-google: net/rpc.(*service).call(0xc4205db400, 0xc42051a7d0, 0xc420273128, 0xc4203f8300, 0xc420495d20, 0x1556f80, 0xc42028b4c0, 0x16, 0x1556fc0, 0xc420745ed0, ...)
[DEBUG] plugin.terraform-provider-google: /home/rkool/go/src/net/rpc/server.go:381 +0x142
[DEBUG] plugin.terraform-provider-google: created by net/rpc.(*Server).ServeCodec
[DEBUG] plugin.terraform-provider-google: /home/rkool/go/src/net/rpc/server.go:475 +0x36b
[TRACE] root: eval: *terraform.EvalWriteState
[TRACE] root: eval: *terraform.EvalApplyProvisioners
[TRACE] root: eval: *terraform.EvalIf
[TRACE] root: eval: *terraform.EvalWriteState
[TRACE] root: eval: *terraform.EvalWriteDiff
[TRACE] root: eval: *terraform.EvalApplyPost
[DEBUG] plugin: plugin process exited: path=/usr/local/bin/terraform-provider-google

Second issue, when adding a resource_labels configuration to my terraform file for the gke cluster, it tries to create a new cluster, which fails as it already exists. Doesn't matter if the resources_labels contains any labels or not. UPDATE: this seems to be caused by the crashes which empties the state file.

Added ability to configure resource labels on a GKE cluster

ghost · 2018-11-17T14:12:55Z

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 feel this issue should be reopened, we encourage creating a new issue linking back to this one for added context. If you feel I made an error 🤖 🙉 , please reach out to my human friends 👉 [email protected]. Thanks!

garethjevans added 2 commits June 15, 2018 11:10

Added ability to configure resource labels on a GKE cluster

e14aa8b

Type conversion issue

fee1c7f

paddycarver suggested changes Jun 22, 2018

View reviewed changes

Minor code changes as requested

bf7c70b

danawillow mentioned this pull request Jun 28, 2018

Support GKE cluster labels for google_container_cluster #1238

Closed

paddycarver approved these changes Jun 28, 2018

View reviewed changes

paddycarver merged commit cf72a65 into hashicorp:master Jul 2, 2018

wmuizelaar mentioned this pull request Jul 13, 2018

Terraform crash on google_container_cluster #1767

Closed

paddycarver mentioned this pull request Jul 16, 2018

Panic on apply for google_container_cluster that has tags #1777

Closed

chrisst pushed a commit to chrisst/terraform-provider-google that referenced this pull request Nov 9, 2018

Merge pull request hashicorp#1663 from garethjevans/resourcelabels

75009b0

Added ability to configure resource labels on a GKE cluster

ghost locked and limited conversation to collaborators Nov 17, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added ability to configure resource labels on a GKE cluster #1663

Added ability to configure resource labels on a GKE cluster #1663

garethjevans commented Jun 15, 2018

garethjevans commented Jun 15, 2018

paddycarver left a comment

paddycarver Jun 22, 2018

garethjevans Jun 25, 2018

paddycarver Jun 22, 2018

garethjevans Jun 25, 2018

paddycarver Jun 22, 2018

garethjevans Jun 25, 2018

paddycarver Jun 28, 2018

garethjevans commented Jun 28, 2018

paddycarver left a comment

rolandkool commented Jul 5, 2018 •

edited

Loading

ghost commented Nov 17, 2018

		@@ -172,6 +172,8 @@ output "cluster_ca_certificate" {

		* `remove_default_node_pool` - (Optional) If true, deletes the default node pool upon cluster creation.

		* `resource_labels` - (Optional) The GCE resource labels (key/value pairs) to be applied to the cluster.

Added ability to configure resource labels on a GKE cluster #1663

Added ability to configure resource labels on a GKE cluster #1663

Conversation

garethjevans commented Jun 15, 2018

garethjevans commented Jun 15, 2018

paddycarver left a comment

Choose a reason for hiding this comment

paddycarver Jun 22, 2018

Choose a reason for hiding this comment

garethjevans Jun 25, 2018

Choose a reason for hiding this comment

paddycarver Jun 22, 2018

Choose a reason for hiding this comment

garethjevans Jun 25, 2018

Choose a reason for hiding this comment

paddycarver Jun 22, 2018

Choose a reason for hiding this comment

garethjevans Jun 25, 2018

Choose a reason for hiding this comment

paddycarver Jun 28, 2018

Choose a reason for hiding this comment

garethjevans commented Jun 28, 2018

paddycarver left a comment

Choose a reason for hiding this comment

rolandkool commented Jul 5, 2018 • edited Loading

ghost commented Nov 17, 2018

rolandkool commented Jul 5, 2018 •

edited

Loading