Skip to content
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

Add labels to Bigtable instance #3793

Conversation

ryanyuan
Copy link
Contributor

@ryanyuan ryanyuan commented Jul 29, 2020

Fix hashicorp/terraform-provider-google#6813

Release Note Template for Downstream PRs (will be copied)

bigtable: add support for labels in `google_bigtable_instance`

@google-cla google-cla bot added the cla: yes label Jul 29, 2020
@modular-magician
Copy link
Collaborator

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

I have detected that you are a community contributor, so your PR will be assigned to someone with a commit-bit on this repo for initial review.

Thanks for your contribution! A human will be with you soon.

@danawillow, please review this PR or find an appropriate assignee.

@modular-magician
Copy link
Collaborator

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

Diff report:

Terraform GA: Diff ( 3 files changed, 34 insertions(+))
Terraform Beta: Diff ( 3 files changed, 34 insertions(+))

@danawillow
Copy link
Contributor

The generated code for this doesn't build. I assume there's a go module that would need to be updated in TPG/TPGB first to get a version of the client libraries that supports labels.

Error message:

google-beta/resource_bigtable_instance.go:147:7: conf.Labels undefined (type *"cloud.google.com/go/bigtable".InstanceWithClustersConfig has no field or method Labels)
google-beta/resource_bigtable_instance.go:225:26: instance.Labels undefined (type *"cloud.google.com/go/bigtable".InstanceInfo has no field or method Labels)
google-beta/resource_bigtable_instance.go:258:7: conf.Labels undefined (type *"cloud.google.com/go/bigtable".InstanceWithClustersConfig has no field or method Labels)

@ryanyuan
Copy link
Contributor Author

Thanks @danawillow
I raised the PR to bump the Bigtable version from 1.1.0 to 1.5.0, which supports labels.

@ryanyuan ryanyuan changed the title add-labels-to-bigtable-instance Add labels to Bigtable instance Jul 30, 2020
@modular-magician
Copy link
Collaborator

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

Diff report:

Terraform GA: Diff ( 3 files changed, 34 insertions(+))
Terraform Beta: Diff ( 3 files changed, 34 insertions(+))

@modular-magician
Copy link
Collaborator

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
Copy link
Collaborator

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

Diff report:

Terraform GA: Diff ( 3 files changed, 34 insertions(+))
Terraform Beta: Diff ( 3 files changed, 34 insertions(+))

@danawillow
Copy link
Contributor

Tests are failing:

------- Stdout: -------
=== RUN   TestAccBigtableInstance_basic
=== PAUSE TestAccBigtableInstance_basic
=== CONT  TestAccBigtableInstance_basic
TestAccBigtableInstance_basic: testing.go:674: Step 1 error: config is invalid: Unsupported block type: Blocks of type "labels" are not expected here. Did you mean to define argument "labels"? If so, use the equals sign to assign it a value.
--- FAIL: TestAccBigtableInstance_basic (1.71s)
FAIL

@ryanyuan ryanyuan force-pushed the add-labels-to-bigtable-instance branch from 1c5de35 to 05f8f20 Compare July 31, 2020 01:17
@modular-magician
Copy link
Collaborator

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

Diff report:

Terraform GA: Diff ( 3 files changed, 34 insertions(+))
Terraform Beta: Diff ( 3 files changed, 34 insertions(+))

@ryanyuan
Copy link
Contributor Author

@danawillow Should be all good now. Thanks.

@@ -242,6 +254,10 @@ func resourceBigtableInstanceUpdate(d *schema.ResourceData, meta interface{}) er
}
conf.DisplayName = displayName.(string)

if _, ok := d.GetOk("labels"); ok {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this if statement needs to be removed to allow removing all labels. See this test failure:

------- Stdout: -------
=== RUN   TestAccBigtableInstance_cluster
=== PAUSE TestAccBigtableInstance_cluster
=== CONT  TestAccBigtableInstance_cluster
TestAccBigtableInstance_cluster: testing.go:674: Step 7 error: After applying this step, the plan was not empty:
DIFF:
UPDATE: google_bigtable_instance.instance
cluster.#:              "4" => "4"
cluster.0.cluster_id:   "tf-test-14k76bhjh4-a" => "tf-test-14k76bhjh4-a"
cluster.0.num_nodes:    "5" => "5"
cluster.0.storage_type: "HDD" => "HDD"
cluster.0.zone:         "us-central1-a" => "us-central1-a"
cluster.1.cluster_id:   "tf-test-14k76bhjh4-c" => "tf-test-14k76bhjh4-c"
cluster.1.num_nodes:    "5" => "5"
cluster.1.storage_type: "HDD" => "HDD"
cluster.1.zone:         "us-central1-c" => "us-central1-c"
cluster.2.cluster_id:   "tf-test-14k76bhjh4-b" => "tf-test-14k76bhjh4-b"
cluster.2.num_nodes:    "5" => "5"
cluster.2.storage_type: "HDD" => "HDD"
cluster.2.zone:         "us-central1-b" => "us-central1-b"
cluster.3.cluster_id:   "tf-test-14k76bhjh4-d" => "tf-test-14k76bhjh4-d"
cluster.3.num_nodes:    "5" => "5"
cluster.3.storage_type: "HDD" => "HDD"
cluster.3.zone:         "us-central1-f" => "us-central1-f"
deletion_protection:    "false" => "false"
display_name:           "tf-test-14k76bhjh4" => "tf-test-14k76bhjh4"
id:                     "projects/ci-test-project-188019/instances/tf-test-14k76bhjh4" => "projects/ci-test-project-188019/instances/tf-test-14k76bhjh4"
instance_type:          "PRODUCTION" => "PRODUCTION"
labels.env:             "default" => ""
name:                   "tf-test-14k76bhjh4" => "tf-test-14k76bhjh4"
project:                "ci-test-project-188019" => "ci-test-project-188019"
STATE:
google_bigtable_instance.instance:
ID = projects/ci-test-project-188019/instances/tf-test-14k76bhjh4
provider = provider.google
cluster.# = 4
cluster.0.cluster_id = tf-test-14k76bhjh4-a
cluster.0.num_nodes = 5
cluster.0.storage_type = HDD
cluster.0.zone = us-central1-a
cluster.1.cluster_id = tf-test-14k76bhjh4-c
cluster.1.num_nodes = 5
cluster.1.storage_type = HDD
cluster.1.zone = us-central1-c
cluster.2.cluster_id = tf-test-14k76bhjh4-b
cluster.2.num_nodes = 5
cluster.2.storage_type = HDD
cluster.2.zone = us-central1-b
cluster.3.cluster_id = tf-test-14k76bhjh4-d
cluster.3.num_nodes = 5
cluster.3.storage_type = HDD
cluster.3.zone = us-central1-f
deletion_protection = false
display_name = tf-test-14k76bhjh4
instance_type = PRODUCTION
labels.env = default
name = tf-test-14k76bhjh4
project = ci-test-project-188019
--- FAIL: TestAccBigtableInstance_cluster (51.96s)
FAIL

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@danawillow I've changed the if-GetOk clause to if-HasChange. I reckon that should have fixed the update call.

if d.HasChange("labels") {
	conf.Labels = expandLabels(d)
}

Add labels to Bigtable instance
@ryanyuan ryanyuan force-pushed the add-labels-to-bigtable-instance branch from 05f8f20 to 9bbecfd Compare August 1, 2020 00:13
@modular-magician
Copy link
Collaborator

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

Diff report:

Terraform GA: Diff ( 3 files changed, 34 insertions(+))
Terraform Beta: Diff ( 3 files changed, 34 insertions(+))

Copy link
Contributor

@danawillow danawillow left a comment

Choose a reason for hiding this comment

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

Looks good, thanks @ryanyuan!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support labels for Bigtable Instance
3 participants