From 01ce85c51ee3aa849ba540f7f79a217f39eedb31 Mon Sep 17 00:00:00 2001 From: mackenziestarr Date: Sun, 14 Jul 2019 20:18:25 -0700 Subject: [PATCH 1/8] add ability to update Bigtable instance cluster node count --- google/resource_bigtable_instance.go | 59 ++++++++++++++++--- google/resource_bigtable_instance_iam_test.go | 29 +++++---- google/resource_bigtable_instance_test.go | 27 +++++++-- 3 files changed, 89 insertions(+), 26 deletions(-) diff --git a/google/resource_bigtable_instance.go b/google/resource_bigtable_instance.go index ed1435b54d5..e4bf48459c1 100644 --- a/google/resource_bigtable_instance.go +++ b/google/resource_bigtable_instance.go @@ -15,6 +15,7 @@ func resourceBigtableInstance() *schema.Resource { return &schema.Resource{ Create: resourceBigtableInstanceCreate, Read: resourceBigtableInstanceRead, + Update: resourceBigtableInstanceUpdate, Delete: resourceBigtableInstanceDestroy, Schema: map[string]*schema.Schema{ @@ -25,10 +26,8 @@ func resourceBigtableInstance() *schema.Resource { }, "cluster": { - Type: schema.TypeSet, + Type: schema.TypeList, Required: true, - ForceNew: true, - MaxItems: 4, Elem: &schema.Resource{ Schema: map[string]*schema.Schema{ "cluster_id": { @@ -44,7 +43,6 @@ func resourceBigtableInstance() *schema.Resource { "num_nodes": { Type: schema.TypeInt, Optional: true, - ForceNew: true, ValidateFunc: validation.IntAtLeast(3), }, "storage_type": { @@ -57,11 +55,9 @@ func resourceBigtableInstance() *schema.Resource { }, }, }, - "display_name": { Type: schema.TypeString, Optional: true, - ForceNew: true, Computed: true, }, @@ -137,7 +133,7 @@ func resourceBigtableInstanceCreate(d *schema.ResourceData, meta interface{}) er conf.InstanceType = bigtable.PRODUCTION } - conf.Clusters = expandBigtableClusters(d.Get("cluster").(*schema.Set).List(), conf.InstanceID) + conf.Clusters = expandBigtableClusters(d.Get("cluster").([]interface{}), conf.InstanceID) c, err := config.bigtableClientFactory.NewInstanceAdminClient(project) if err != nil { @@ -181,7 +177,8 @@ func resourceBigtableInstanceRead(d *schema.ResourceData, meta interface{}) erro d.Set("project", project) - clusters := d.Get("cluster").(*schema.Set).List() + clusters := d.Get("cluster").([]interface{}) + clusterState := []map[string]interface{}{} for _, cl := range clusters { cluster := cl.(map[string]interface{}) @@ -197,6 +194,7 @@ func resourceBigtableInstanceRead(d *schema.ResourceData, meta interface{}) erro } err = d.Set("cluster", clusterState) + if err != nil { return fmt.Errorf("Error setting clusters in state: %s", err.Error()) } @@ -207,6 +205,51 @@ func resourceBigtableInstanceRead(d *schema.ResourceData, meta interface{}) erro return nil } +func resourceBigtableInstanceUpdate(d *schema.ResourceData, meta interface{}) error { + config := meta.(*Config) + ctx := context.Background() + + project, err := getProject(d, config) + if err != nil { + return err + } + + c, err := config.bigtableClientFactory.NewInstanceAdminClient(project) + if err != nil { + return fmt.Errorf("Error starting instance admin client. %s", err) + } + defer c.Close() + + if d.Get("instance_type").(string) == "DEVELOPMENT" { + return resourceBigtableInstanceRead(d, meta) + } + + clusters, err := c.Clusters(ctx, d.Get("name").(string)) + if err != nil { + return fmt.Errorf("Error retrieving clusters for instance %s", err.Error()) + } + + clusterMap := make(map[string]*bigtable.ClusterInfo, len(clusters)) + for _, cluster := range clusters { + clusterMap[cluster.Name] = cluster + } + + for _, cluster := range d.Get("cluster").([]interface{}) { + config := cluster.(map[string]interface{}) + cluster_id := config["cluster_id"].(string) + if cluster, ok := clusterMap[cluster_id]; ok { + if cluster.ServeNodes != config["num_nodes"].(int) { + err = c.UpdateCluster(ctx, d.Get("name").(string), cluster.Name, int32(config["num_nodes"].(int))) + if err != nil { + return fmt.Errorf("Error updating cluster %s for instance %s", cluster.Name, d.Get("name").(string)) + } + } + } + } + + return resourceBigtableInstanceRead(d, meta) +} + func resourceBigtableInstanceDestroy(d *schema.ResourceData, meta interface{}) error { config := meta.(*Config) ctx := context.Background() diff --git a/google/resource_bigtable_instance_iam_test.go b/google/resource_bigtable_instance_iam_test.go index a67b6f0eaa0..a9b80fad0ea 100644 --- a/google/resource_bigtable_instance_iam_test.go +++ b/google/resource_bigtable_instance_iam_test.go @@ -12,6 +12,7 @@ func TestAccBigtableInstanceIamBinding(t *testing.T) { t.Parallel() instance := "tf-bigtable-iam-" + acctest.RandString(10) + cluster := "c-" + acctest.RandString(10) account := "tf-bigtable-iam-" + acctest.RandString(10) role := "roles/bigtable.user" @@ -24,7 +25,7 @@ func TestAccBigtableInstanceIamBinding(t *testing.T) { Steps: []resource.TestStep{ { // Test IAM Binding creation - Config: testAccBigtableInstanceIamBinding_basic(instance, account, role), + Config: testAccBigtableInstanceIamBinding_basic(instance, cluster, account, role), Check: resource.ComposeTestCheckFunc( resource.TestCheckResourceAttr( "google_bigtable_instance_iam_binding.binding", "role", role), @@ -38,7 +39,7 @@ func TestAccBigtableInstanceIamBinding(t *testing.T) { }, { // Test IAM Binding update - Config: testAccBigtableInstanceIamBinding_update(instance, account, role), + Config: testAccBigtableInstanceIamBinding_update(instance, cluster, account, role), }, { ResourceName: "google_bigtable_instance_iam_binding.binding", @@ -54,6 +55,7 @@ func TestAccBigtableInstanceIamMember(t *testing.T) { t.Parallel() instance := "tf-bigtable-iam-" + acctest.RandString(10) + cluster := "c-" + acctest.RandString(10) account := "tf-bigtable-iam-" + acctest.RandString(10) role := "roles/bigtable.user" @@ -69,7 +71,7 @@ func TestAccBigtableInstanceIamMember(t *testing.T) { Steps: []resource.TestStep{ { // Test IAM Binding creation - Config: testAccBigtableInstanceIamMember(instance, account, role), + Config: testAccBigtableInstanceIamMember(instance, cluster, account, role), Check: resource.ComposeTestCheckFunc( resource.TestCheckResourceAttr( "google_bigtable_instance_iam_member.member", "role", role), @@ -91,6 +93,7 @@ func TestAccBigtableInstanceIamPolicy(t *testing.T) { t.Parallel() instance := "tf-bigtable-iam-" + acctest.RandString(10) + cluster := "c-" + acctest.RandString(10) account := "tf-bigtable-iam-" + acctest.RandString(10) role := "roles/bigtable.user" @@ -103,7 +106,7 @@ func TestAccBigtableInstanceIamPolicy(t *testing.T) { Steps: []resource.TestStep{ { // Test IAM Binding creation - Config: testAccBigtableInstanceIamPolicy(instance, account, role), + Config: testAccBigtableInstanceIamPolicy(instance, cluster, account, role), }, { ResourceName: "google_bigtable_instance_iam_policy.policy", @@ -115,7 +118,7 @@ func TestAccBigtableInstanceIamPolicy(t *testing.T) { }) } -func testAccBigtableInstanceIamBinding_basic(instance, account, role string) string { +func testAccBigtableInstanceIamBinding_basic(instance, cluster, account, role string) string { return fmt.Sprintf(testBigtableInstanceIam+` resource "google_service_account" "test-account1" { @@ -135,10 +138,10 @@ resource "google_bigtable_instance_iam_binding" "binding" { "serviceAccount:${google_service_account.test-account1.email}", ] } -`, instance, acctest.RandString(10), account, account, role) +`, instance, cluster, account, account, role) } -func testAccBigtableInstanceIamBinding_update(instance, account, role string) string { +func testAccBigtableInstanceIamBinding_update(instance, cluster, account, role string) string { return fmt.Sprintf(testBigtableInstanceIam+` resource "google_service_account" "test-account1" { account_id = "%s-1" @@ -158,10 +161,10 @@ resource "google_bigtable_instance_iam_binding" "binding" { "serviceAccount:${google_service_account.test-account2.email}", ] } -`, instance, acctest.RandString(10), account, account, role) +`, instance, cluster, account, account, role) } -func testAccBigtableInstanceIamMember(instance, account, role string) string { +func testAccBigtableInstanceIamMember(instance, cluster, account, role string) string { return fmt.Sprintf(testBigtableInstanceIam+` resource "google_service_account" "test-account" { account_id = "%s" @@ -173,10 +176,10 @@ resource "google_bigtable_instance_iam_member" "member" { role = "%s" member = "serviceAccount:${google_service_account.test-account.email}" } -`, instance, acctest.RandString(10), account, role) +`, instance, cluster, account, role) } -func testAccBigtableInstanceIamPolicy(instance, account, role string) string { +func testAccBigtableInstanceIamPolicy(instance, cluster, account, role string) string { return fmt.Sprintf(testBigtableInstanceIam+` resource "google_service_account" "test-account" { account_id = "%s" @@ -194,7 +197,7 @@ resource "google_bigtable_instance_iam_policy" "policy" { instance = "${google_bigtable_instance.instance.name}" policy_data = "${data.google_iam_policy.policy.policy_data}" } -`, instance, acctest.RandString(10), account, role) +`, instance, cluster, account, role) } // Smallest instance possible for testing @@ -204,7 +207,7 @@ resource "google_bigtable_instance" "instance" { instance_type = "DEVELOPMENT" cluster { - cluster_id = "c-%s" + cluster_id = "%s" zone = "us-central1-b" storage_type = "HDD" } diff --git a/google/resource_bigtable_instance_test.go b/google/resource_bigtable_instance_test.go index d3bb9d69cc4..ad94474e231 100644 --- a/google/resource_bigtable_instance_test.go +++ b/google/resource_bigtable_instance_test.go @@ -25,14 +25,14 @@ func TestAccBigtableInstance_basic(t *testing.T) { Config: testAccBigtableInstance(instanceName, 3), Check: resource.ComposeTestCheckFunc( testAccBigtableInstanceExists( - "google_bigtable_instance.instance"), + "google_bigtable_instance.instance", 3), ), }, { Config: testAccBigtableInstance(instanceName, 4), Check: resource.ComposeTestCheckFunc( testAccBigtableInstanceExists( - "google_bigtable_instance.instance"), + "google_bigtable_instance.instance", 4), ), }, }, @@ -57,7 +57,7 @@ func TestAccBigtableInstance_cluster(t *testing.T) { Config: testAccBigtableInstance_cluster(instanceName), Check: resource.ComposeTestCheckFunc( testAccBigtableInstanceExists( - "google_bigtable_instance.instance"), + "google_bigtable_instance.instance", 3), ), }, }, @@ -78,7 +78,7 @@ func TestAccBigtableInstance_development(t *testing.T) { Config: testAccBigtableInstance_development(instanceName), Check: resource.ComposeTestCheckFunc( testAccBigtableInstanceExists( - "google_bigtable_instance.instance"), + "google_bigtable_instance.instance", 0), ), }, }, @@ -109,7 +109,7 @@ func testAccCheckBigtableInstanceDestroy(s *terraform.State) error { return nil } -func testAccBigtableInstanceExists(n string) resource.TestCheckFunc { +func testAccBigtableInstanceExists(n string, numNodes int) resource.TestCheckFunc { var ctx = context.Background() return func(s *terraform.State) error { rs, ok := s.RootModule().Resources[n] @@ -117,6 +117,8 @@ func testAccBigtableInstanceExists(n string) resource.TestCheckFunc { return fmt.Errorf("Not found: %s", n) } + fmt.Println(s) + if rs.Primary.ID == "" { return fmt.Errorf("No ID is set") } @@ -133,6 +135,21 @@ func testAccBigtableInstanceExists(n string) resource.TestCheckFunc { return fmt.Errorf("Error retrieving instance %s.", rs.Primary.Attributes["name"]) } + clusters, err := c.Clusters(ctx, rs.Primary.Attributes["name"]) + if err != nil { + return fmt.Errorf("Error retrieving cluster list for instance %s.", rs.Primary.Attributes["name"]) + } + + for _, c := range clusters { + if c.ServeNodes != numNodes { + return fmt.Errorf("Expected cluster %s to have %d nodes but got %d nodes for instance %s.", + c.Name, + numNodes, + c.ServeNodes, + rs.Primary.Attributes["name"]) + } + } + return nil } } From 47b23d9237e6c098e2be86d92837ce766e40aeb1 Mon Sep 17 00:00:00 2001 From: mackenziestarr Date: Mon, 15 Jul 2019 11:48:15 -0400 Subject: [PATCH 2/8] cleanup diff --- google/resource_bigtable_instance.go | 2 -- google/resource_bigtable_instance_test.go | 2 -- 2 files changed, 4 deletions(-) diff --git a/google/resource_bigtable_instance.go b/google/resource_bigtable_instance.go index e4bf48459c1..fb5e5be4abe 100644 --- a/google/resource_bigtable_instance.go +++ b/google/resource_bigtable_instance.go @@ -178,7 +178,6 @@ func resourceBigtableInstanceRead(d *schema.ResourceData, meta interface{}) erro d.Set("project", project) clusters := d.Get("cluster").([]interface{}) - clusterState := []map[string]interface{}{} for _, cl := range clusters { cluster := cl.(map[string]interface{}) @@ -194,7 +193,6 @@ func resourceBigtableInstanceRead(d *schema.ResourceData, meta interface{}) erro } err = d.Set("cluster", clusterState) - if err != nil { return fmt.Errorf("Error setting clusters in state: %s", err.Error()) } diff --git a/google/resource_bigtable_instance_test.go b/google/resource_bigtable_instance_test.go index ad94474e231..75a77c6288c 100644 --- a/google/resource_bigtable_instance_test.go +++ b/google/resource_bigtable_instance_test.go @@ -117,8 +117,6 @@ func testAccBigtableInstanceExists(n string, numNodes int) resource.TestCheckFun return fmt.Errorf("Not found: %s", n) } - fmt.Println(s) - if rs.Primary.ID == "" { return fmt.Errorf("No ID is set") } From 6b6cc5f274ed5dbfc8d3c52acf77898c81b45bff Mon Sep 17 00:00:00 2001 From: mackenziestarr Date: Tue, 30 Jul 2019 10:19:32 -0400 Subject: [PATCH 3/8] test is passing for reordering clusters --- google/resource_bigtable_instance.go | 82 ++++++++++++++++++++++- google/resource_bigtable_instance_test.go | 49 +++++++++++--- 2 files changed, 119 insertions(+), 12 deletions(-) diff --git a/google/resource_bigtable_instance.go b/google/resource_bigtable_instance.go index fb5e5be4abe..4c118bb4a09 100644 --- a/google/resource_bigtable_instance.go +++ b/google/resource_bigtable_instance.go @@ -5,6 +5,7 @@ import ( "fmt" "log" + "github.com/hashicorp/terraform/helper/customdiff" "github.com/hashicorp/terraform/helper/schema" "github.com/hashicorp/terraform/helper/validation" @@ -18,6 +19,10 @@ func resourceBigtableInstance() *schema.Resource { Update: resourceBigtableInstanceUpdate, Delete: resourceBigtableInstanceDestroy, + CustomizeDiff: customdiff.All( + resourceBigtableInstanceClusterReorderTypeList, + ), + Schema: map[string]*schema.Schema{ "name": { Type: schema.TypeString, @@ -27,7 +32,8 @@ func resourceBigtableInstance() *schema.Resource { "cluster": { Type: schema.TypeList, - Required: true, + Optional: true, + Computed: true, Elem: &schema.Resource{ Schema: map[string]*schema.Schema{ "cluster_id": { @@ -306,3 +312,77 @@ func expandBigtableClusters(clusters []interface{}, instanceID string) []bigtabl } return results } + +func resourceBigtableInstanceClusterReorderTypeList(diff *schema.ResourceDiff, meta interface{}) error { + keys := diff.GetChangedKeysPrefix("cluster") + if len(keys) == 0 { + return nil + } + + oldCount, newCount := diff.GetChange("cluster.#") + var count int + if oldCount.(int) < newCount.(int) { + count = newCount.(int) + } else { + count = oldCount.(int) + } + + // simulate Required:true and MinItems:1 + if count < 1 { + return nil + } + + var old_ids []string + var new_ids []string + for i := 0; i < count; i++ { + old, new := diff.GetChange(fmt.Sprintf("cluster.%d.cluster_id", i)) + if old != nil { + old_ids = append(old_ids, old.(string)) + } + if new != nil { + new_ids = append(new_ids, new.(string)) + } + } + + d := difference(old_ids, new_ids) + + // clusters have been reordered + if len(new_ids) == len(old_ids) && len(d) == 0 { + + clusterMap := make(map[string]interface{}, len(new_ids)) + for i := 0; i < count; i++ { + _, id := diff.GetChange(fmt.Sprintf("cluster.%d.cluster_id", i)) + _, c := diff.GetChange(fmt.Sprintf("cluster.%d", i)) + clusterMap[id.(string)] = c + } + + // build a slice of the new clusters ordered by the old cluster order + var old_cluster_order []interface{} + for _, id := range old_ids { + if c, ok := clusterMap[id]; ok { + old_cluster_order = append(old_cluster_order, c) + } + } + + err := diff.SetNew("cluster", old_cluster_order) + if err != nil { + return fmt.Errorf("Error setting cluster diff: %s", err) + } + } + + return nil +} + +func difference(a, b []string) []string { + var c []string + m := make(map[string]bool) + for _, o := range a { + m[o] = true + } + for _, n := range b { + if _, ok := m[n]; !ok { + c = append(c, n) + } + } + return c +} diff --git a/google/resource_bigtable_instance_test.go b/google/resource_bigtable_instance_test.go index 75a77c6288c..b0fd0cfbdb2 100644 --- a/google/resource_bigtable_instance_test.go +++ b/google/resource_bigtable_instance_test.go @@ -54,12 +54,19 @@ func TestAccBigtableInstance_cluster(t *testing.T) { ExpectError: regexp.MustCompile("config is invalid: Too many cluster blocks: No more than 4 \"cluster\" blocks are allowed"), }, { - Config: testAccBigtableInstance_cluster(instanceName), + Config: testAccBigtableInstance_cluster(instanceName, 3), Check: resource.ComposeTestCheckFunc( testAccBigtableInstanceExists( "google_bigtable_instance.instance", 3), ), }, + { + Config: testAccBigtableInstance_cluster_reordered(instanceName, 5), + Check: resource.ComposeTestCheckFunc( + testAccBigtableInstanceExists( + "google_bigtable_instance.instance", 5), + ), + }, }, }) } @@ -166,36 +173,36 @@ resource "google_bigtable_instance" "instance" { `, instanceName, instanceName, numNodes) } -func testAccBigtableInstance_cluster(instanceName string) string { +func testAccBigtableInstance_cluster(instanceName string, numNodes int) string { return fmt.Sprintf(` resource "google_bigtable_instance" "instance" { name = "%s" cluster { cluster_id = "%s-a" zone = "us-central1-a" - num_nodes = 3 + num_nodes = %d storage_type = "HDD" } cluster { cluster_id = "%s-b" zone = "us-central1-b" - num_nodes = 3 + num_nodes = %d storage_type = "HDD" } cluster { cluster_id = "%s-c" zone = "us-central1-c" - num_nodes = 3 + num_nodes = %d storage_type = "HDD" } cluster { cluster_id = "%s-d" zone = "us-central1-f" - num_nodes = 3 + num_nodes = %d storage_type = "HDD" } } -`, instanceName, instanceName, instanceName, instanceName, instanceName) +`, instanceName, numNodes, instanceName, numNodes, instanceName, numNodes, instanceName, numNodes) } func testAccBigtableInstance_clusterMax(instanceName string) string { @@ -217,23 +224,43 @@ resource "google_bigtable_instance" "instance" { cluster { cluster_id = "%s-c" zone = "us-central1-c" - num_nodes = 3 + num_nodes = %d + storage_type = "HDD" + } +} +`, instanceName, instanceName, instanceName, instanceName, instanceName, instanceName) +} + +func testAccBigtableInstance_cluster_reordered(instanceName string, numNodes int) string { + return fmt.Sprintf(` +resource "google_bigtable_instance" "instance" { + name = "%s" + cluster { + num_nodes = %d + cluster_id = "%s-b" + zone = "us-central1-c" storage_type = "HDD" } cluster { cluster_id = "%s-d" zone = "us-central1-f" - num_nodes = 3 + num_nodes = %d + storage_type = "HDD" + } + cluster { + zone = "us-central1-b" + cluster_id = "%s-a" + num_nodes = %d storage_type = "HDD" } cluster { cluster_id = "%s-e" zone = "us-east1-a" - num_nodes = 3 + num_nodes = %d storage_type = "HDD" } } -`, instanceName, instanceName, instanceName, instanceName, instanceName, instanceName) +`, instanceName, numNodes, instanceName, instanceName, numNodes, instanceName, numNodes, instanceName, numNodes, instanceName) } func testAccBigtableInstance_development(instanceName string) string { From 376017de467c478f109afa8a90d061f892465b5e Mon Sep 17 00:00:00 2001 From: mackenziestarr Date: Wed, 7 Aug 2019 17:30:44 -0400 Subject: [PATCH 4/8] refactor and address code review comments --- google/resource_bigtable_instance.go | 78 ++++++++-------------------- 1 file changed, 22 insertions(+), 56 deletions(-) diff --git a/google/resource_bigtable_instance.go b/google/resource_bigtable_instance.go index 4c118bb4a09..7f2ec5a6820 100644 --- a/google/resource_bigtable_instance.go +++ b/google/resource_bigtable_instance.go @@ -314,75 +314,41 @@ func expandBigtableClusters(clusters []interface{}, instanceID string) []bigtabl } func resourceBigtableInstanceClusterReorderTypeList(diff *schema.ResourceDiff, meta interface{}) error { - keys := diff.GetChangedKeysPrefix("cluster") - if len(keys) == 0 { - return nil - } + old_count, new_count := diff.GetChange("cluster.#") - oldCount, newCount := diff.GetChange("cluster.#") - var count int - if oldCount.(int) < newCount.(int) { - count = newCount.(int) - } else { - count = oldCount.(int) + // simulate Required:true and MinItems:1 for "cluster" + if new_count.(int) < 1 { + return fmt.Errorf("Error applying diff. Resource definition should contain one or more cluster blocks, got %d blocks", new_count.(int)) } - // simulate Required:true and MinItems:1 - if count < 1 { + if old_count.(int) != new_count.(int) { return nil } var old_ids []string - var new_ids []string - for i := 0; i < count; i++ { - old, new := diff.GetChange(fmt.Sprintf("cluster.%d.cluster_id", i)) - if old != nil { - old_ids = append(old_ids, old.(string)) - } - if new != nil { - new_ids = append(new_ids, new.(string)) - } - } + clusters := make(map[string]interface{}, new_count.(int)) - d := difference(old_ids, new_ids) - - // clusters have been reordered - if len(new_ids) == len(old_ids) && len(d) == 0 { - - clusterMap := make(map[string]interface{}, len(new_ids)) - for i := 0; i < count; i++ { - _, id := diff.GetChange(fmt.Sprintf("cluster.%d.cluster_id", i)) - _, c := diff.GetChange(fmt.Sprintf("cluster.%d", i)) - clusterMap[id.(string)] = c + for i := 0; i < new_count.(int); i++ { + old_id, new_id := diff.GetChange(fmt.Sprintf("cluster.%d.cluster_id", i)) + if old_id != nil && old_id != "" { + old_ids = append(old_ids, old_id.(string)) } + _, c := diff.GetChange(fmt.Sprintf("cluster.%d", i)) + clusters[new_id.(string)] = c + } - // build a slice of the new clusters ordered by the old cluster order - var old_cluster_order []interface{} - for _, id := range old_ids { - if c, ok := clusterMap[id]; ok { - old_cluster_order = append(old_cluster_order, c) - } + // reorder clusters according to the old cluster order + var old_cluster_order []interface{} + for _, id := range old_ids { + if c, ok := clusters[id]; ok { + old_cluster_order = append(old_cluster_order, c) } + } - err := diff.SetNew("cluster", old_cluster_order) - if err != nil { - return fmt.Errorf("Error setting cluster diff: %s", err) - } + err := diff.SetNew("cluster", old_cluster_order) + if err != nil { + return fmt.Errorf("Error setting cluster diff: %s", err) } return nil } - -func difference(a, b []string) []string { - var c []string - m := make(map[string]bool) - for _, o := range a { - m[o] = true - } - for _, n := range b { - if _, ok := m[n]; !ok { - c = append(c, n) - } - } - return c -} From 87aacdeaaa285e811cfb92ac79edec7316c90c85 Mon Sep 17 00:00:00 2001 From: mackenziestarr Date: Wed, 7 Aug 2019 17:38:27 -0400 Subject: [PATCH 5/8] add check for MaxItems:4 for cluster attribute --- google/resource_bigtable_instance.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/google/resource_bigtable_instance.go b/google/resource_bigtable_instance.go index 7f2ec5a6820..e8610f9b317 100644 --- a/google/resource_bigtable_instance.go +++ b/google/resource_bigtable_instance.go @@ -316,9 +316,9 @@ func expandBigtableClusters(clusters []interface{}, instanceID string) []bigtabl func resourceBigtableInstanceClusterReorderTypeList(diff *schema.ResourceDiff, meta interface{}) error { old_count, new_count := diff.GetChange("cluster.#") - // simulate Required:true and MinItems:1 for "cluster" - if new_count.(int) < 1 { - return fmt.Errorf("Error applying diff. Resource definition should contain one or more cluster blocks, got %d blocks", new_count.(int)) + // simulate Required:true, MinItems:1, MaxItems:4 for "cluster" + if new_count.(int) < 1 || new_count.(int) > 4 { + return fmt.Errorf("Error applying diff. Resource definition should contain at least one cluster block but no more than four, got %d blocks", new_count.(int)) } if old_count.(int) != new_count.(int) { From 4fdeabeed41a74a066c1899efd828764492b6c86 Mon Sep 17 00:00:00 2001 From: mackenziestarr Date: Wed, 7 Aug 2019 18:32:39 -0400 Subject: [PATCH 6/8] fixup diff after rebasing master --- google/resource_bigtable_instance.go | 7 ++-- google/resource_bigtable_instance_test.go | 40 ++++++++++++++++++----- 2 files changed, 37 insertions(+), 10 deletions(-) diff --git a/google/resource_bigtable_instance.go b/google/resource_bigtable_instance.go index e8610f9b317..ed1c7508a3d 100644 --- a/google/resource_bigtable_instance.go +++ b/google/resource_bigtable_instance.go @@ -317,8 +317,11 @@ func resourceBigtableInstanceClusterReorderTypeList(diff *schema.ResourceDiff, m old_count, new_count := diff.GetChange("cluster.#") // simulate Required:true, MinItems:1, MaxItems:4 for "cluster" - if new_count.(int) < 1 || new_count.(int) > 4 { - return fmt.Errorf("Error applying diff. Resource definition should contain at least one cluster block but no more than four, got %d blocks", new_count.(int)) + if new_count.(int) < 1 { + return fmt.Errorf("config is invalid: Too few cluster blocks: Should have at least 1 \"cluster\" block") + } + if new_count.(int) > 4 { + return fmt.Errorf("config is invalid: Too many cluster blocks: No more than 4 \"cluster\" blocks are allowed") } if old_count.(int) != new_count.(int) { diff --git a/google/resource_bigtable_instance_test.go b/google/resource_bigtable_instance_test.go index b0fd0cfbdb2..7da0aebf3e8 100644 --- a/google/resource_bigtable_instance_test.go +++ b/google/resource_bigtable_instance_test.go @@ -21,6 +21,10 @@ func TestAccBigtableInstance_basic(t *testing.T) { Providers: testAccProviders, CheckDestroy: testAccCheckBigtableInstanceDestroy, Steps: []resource.TestStep{ + { + Config: testAccBigtableInstance_invalid(instanceName), + ExpectError: regexp.MustCompile("config is invalid: Too few cluster blocks: Should have at least 1 \"cluster\" block"), + }, { Config: testAccBigtableInstance(instanceName, 3), Check: resource.ComposeTestCheckFunc( @@ -173,6 +177,14 @@ resource "google_bigtable_instance" "instance" { `, instanceName, instanceName, numNodes) } +func testAccBigtableInstance_invalid(instanceName string) string { + return fmt.Sprintf(` +resource "google_bigtable_instance" "instance" { + name = "%s" +} +`, instanceName) +} + func testAccBigtableInstance_cluster(instanceName string, numNodes int) string { return fmt.Sprintf(` resource "google_bigtable_instance" "instance" { @@ -202,7 +214,7 @@ resource "google_bigtable_instance" "instance" { storage_type = "HDD" } } -`, instanceName, numNodes, instanceName, numNodes, instanceName, numNodes, instanceName, numNodes) +`, instanceName, instanceName, numNodes, instanceName, numNodes, instanceName, numNodes, instanceName, numNodes) } func testAccBigtableInstance_clusterMax(instanceName string) string { @@ -224,7 +236,19 @@ resource "google_bigtable_instance" "instance" { cluster { cluster_id = "%s-c" zone = "us-central1-c" - num_nodes = %d + num_nodes = 3 + storage_type = "HDD" + } + cluster { + cluster_id = "%s-d" + zone = "us-central1-f" + num_nodes = 3 + storage_type = "HDD" + } + cluster { + cluster_id = "%s-e" + zone = "us-east1-a" + num_nodes = 3 storage_type = "HDD" } } @@ -236,9 +260,9 @@ func testAccBigtableInstance_cluster_reordered(instanceName string, numNodes int resource "google_bigtable_instance" "instance" { name = "%s" cluster { - num_nodes = %d - cluster_id = "%s-b" + cluster_id = "%s-c" zone = "us-central1-c" + num_nodes = %d storage_type = "HDD" } cluster { @@ -248,19 +272,19 @@ resource "google_bigtable_instance" "instance" { storage_type = "HDD" } cluster { - zone = "us-central1-b" cluster_id = "%s-a" + zone = "us-central1-a" num_nodes = %d storage_type = "HDD" } cluster { - cluster_id = "%s-e" - zone = "us-east1-a" + cluster_id = "%s-b" + zone = "us-central1-b" num_nodes = %d storage_type = "HDD" } } -`, instanceName, numNodes, instanceName, instanceName, numNodes, instanceName, numNodes, instanceName, numNodes, instanceName) +`, instanceName, instanceName, numNodes, instanceName, numNodes, instanceName, numNodes, instanceName, numNodes) } func testAccBigtableInstance_development(instanceName string) string { From 0dd98aedd32d8dea8d6b6ac32b34aba54a07c65b Mon Sep 17 00:00:00 2001 From: mackenziestarr Date: Sat, 17 Aug 2019 05:23:04 -0400 Subject: [PATCH 7/8] add additional tests and logic for validating DEVELOPMENT instances per review comments - add ForceNew back to display_name attribute --- google/resource_bigtable_instance.go | 15 +++++++++++ google/resource_bigtable_instance_test.go | 31 +++++++++++++++++++++++ 2 files changed, 46 insertions(+) diff --git a/google/resource_bigtable_instance.go b/google/resource_bigtable_instance.go index ed1c7508a3d..312381573bc 100644 --- a/google/resource_bigtable_instance.go +++ b/google/resource_bigtable_instance.go @@ -20,6 +20,7 @@ func resourceBigtableInstance() *schema.Resource { Delete: resourceBigtableInstanceDestroy, CustomizeDiff: customdiff.All( + resourceBigtableInstanceValidateDevelopment, resourceBigtableInstanceClusterReorderTypeList, ), @@ -64,6 +65,7 @@ func resourceBigtableInstance() *schema.Resource { "display_name": { Type: schema.TypeString, Optional: true, + ForceNew: true, Computed: true, }, @@ -313,6 +315,19 @@ func expandBigtableClusters(clusters []interface{}, instanceID string) []bigtabl return results } +func resourceBigtableInstanceValidateDevelopment(diff *schema.ResourceDiff, meta interface{}) error { + if diff.Get("instance_type").(string) != "DEVELOPMENT" { + return nil + } + if diff.Get("cluster.#").(int) != 1 { + return fmt.Errorf("config is invalid: instance with instance_type=\"DEVELOPMENT\" should have exactly one \"cluster\" block") + } + if diff.Get("cluster.0.num_nodes").(int) != 0 { + return fmt.Errorf("config is invalid: num_nodes cannot be set for instance_type=\"DEVELOPMENT\"") + } + return nil +} + func resourceBigtableInstanceClusterReorderTypeList(diff *schema.ResourceDiff, meta interface{}) error { old_count, new_count := diff.GetChange("cluster.#") diff --git a/google/resource_bigtable_instance_test.go b/google/resource_bigtable_instance_test.go index 7da0aebf3e8..16426f7f6c7 100644 --- a/google/resource_bigtable_instance_test.go +++ b/google/resource_bigtable_instance_test.go @@ -85,6 +85,14 @@ func TestAccBigtableInstance_development(t *testing.T) { Providers: testAccProviders, CheckDestroy: testAccCheckBigtableInstanceDestroy, Steps: []resource.TestStep{ + { + Config: testAccBigtableInstance_development_invalid_no_cluster(instanceName), + ExpectError: regexp.MustCompile("config is invalid: instance with instance_type=\"DEVELOPMENT\" should have exactly one \"cluster\" block"), + }, + { + Config: testAccBigtableInstance_development_invalid_num_nodes(instanceName), + ExpectError: regexp.MustCompile("config is invalid: num_nodes cannot be set for instance_type=\"DEVELOPMENT\""), + }, { Config: testAccBigtableInstance_development(instanceName), Check: resource.ComposeTestCheckFunc( @@ -299,3 +307,26 @@ resource "google_bigtable_instance" "instance" { } `, instanceName, instanceName) } + +func testAccBigtableInstance_development_invalid_num_nodes(instanceName string) string { + return fmt.Sprintf(` +resource "google_bigtable_instance" "instance" { + name = "%s" + cluster { + cluster_id = "%s" + zone = "us-central1-b" + num_nodes = 3 + } + instance_type = "DEVELOPMENT" +} +`, instanceName, instanceName) +} + +func testAccBigtableInstance_development_invalid_no_cluster(instanceName string) string { + return fmt.Sprintf(` +resource "google_bigtable_instance" "instance" { + name = "%s" + instance_type = "DEVELOPMENT" +} +`, instanceName) +} From d41fd38e0a6fecbad6224d4aec3cde667116e87a Mon Sep 17 00:00:00 2001 From: mackenziestarr Date: Sat, 17 Aug 2019 05:35:31 -0400 Subject: [PATCH 8/8] remove explicit check for DEVELOPMENT instance in update method --- google/resource_bigtable_instance.go | 4 ---- 1 file changed, 4 deletions(-) diff --git a/google/resource_bigtable_instance.go b/google/resource_bigtable_instance.go index 312381573bc..ead906450e3 100644 --- a/google/resource_bigtable_instance.go +++ b/google/resource_bigtable_instance.go @@ -226,10 +226,6 @@ func resourceBigtableInstanceUpdate(d *schema.ResourceData, meta interface{}) er } defer c.Close() - if d.Get("instance_type").(string) == "DEVELOPMENT" { - return resourceBigtableInstanceRead(d, meta) - } - clusters, err := c.Clusters(ctx, d.Get("name").(string)) if err != nil { return fmt.Errorf("Error retrieving clusters for instance %s", err.Error())