diff --git a/.changelog/4556.txt b/.changelog/4556.txt new file mode 100644 index 0000000000..57632ca930 --- /dev/null +++ b/.changelog/4556.txt @@ -0,0 +1,3 @@ +```release-note:bug +bigtable: fixed bug where gc_policy would attempt to recreate the resource when switching from deprecated attribute but maintaining the same value underlying value +``` diff --git a/google-beta/resource_bigtable_gc_policy.go b/google-beta/resource_bigtable_gc_policy.go index 02a4f99cc5..5d72268726 100644 --- a/google-beta/resource_bigtable_gc_policy.go +++ b/google-beta/resource_bigtable_gc_policy.go @@ -16,11 +16,50 @@ const ( GCPolicyModeUnion = "UNION" ) +func resourceBigtableGCPolicyCustomizeDiffFunc(diff TerraformResourceDiff) error { + count := diff.Get("max_age.#").(int) + if count < 1 { + return nil + } + + oldDays, newDays := diff.GetChange("max_age.0.days") + oldDuration, newDuration := diff.GetChange("max_age.0.duration") + log.Printf("days: %v %v", oldDays, newDays) + log.Printf("duration: %v %v", oldDuration, newDuration) + + if oldDuration == "" && newDuration != "" { + // flatten the old days and the new duration to duration... if they are + // equal then do nothing. + do, err := time.ParseDuration(newDuration.(string)) + if err != nil { + return err + } + dn := time.Hour * 24 * time.Duration(oldDays.(int)) + if do == dn { + err := diff.Clear("max_age.0.days") + if err != nil { + return err + } + err = diff.Clear("max_age.0.duration") + if err != nil { + return err + } + } + } + + return nil +} + +func resourceBigtableGCPolicyCustomizeDiff(_ context.Context, d *schema.ResourceDiff, meta interface{}) error { + return resourceBigtableGCPolicyCustomizeDiffFunc(d) +} + func resourceBigtableGCPolicy() *schema.Resource { return &schema.Resource{ - Create: resourceBigtableGCPolicyCreate, - Read: resourceBigtableGCPolicyRead, - Delete: resourceBigtableGCPolicyDestroy, + Create: resourceBigtableGCPolicyCreate, + Read: resourceBigtableGCPolicyRead, + Delete: resourceBigtableGCPolicyDestroy, + CustomizeDiff: resourceBigtableGCPolicyCustomizeDiff, Schema: map[string]*schema.Schema{ "instance_name": { @@ -64,6 +103,7 @@ func resourceBigtableGCPolicy() *schema.Resource { "days": { Type: schema.TypeInt, Optional: true, + Computed: true, ForceNew: true, Deprecated: "Deprecated in favor of duration", Description: `Number of days before applying GC policy.`, @@ -72,6 +112,7 @@ func resourceBigtableGCPolicy() *schema.Resource { "duration": { Type: schema.TypeString, Optional: true, + Computed: true, ForceNew: true, Description: `Duration before applying GC policy`, ValidateFunc: validateDuration(), diff --git a/google-beta/resource_bigtable_gc_policy_test.go b/google-beta/resource_bigtable_gc_policy_test.go index c504ec3976..8bad8fe1e1 100644 --- a/google-beta/resource_bigtable_gc_policy_test.go +++ b/google-beta/resource_bigtable_gc_policy_test.go @@ -34,6 +34,38 @@ func TestAccBigtableGCPolicy_basic(t *testing.T) { }) } +func TestAccBigtableGCPolicy_swapOffDeprecated(t *testing.T) { + // bigtable instance does not use the shared HTTP client, this test creates an instance + skipIfVcr(t) + t.Parallel() + + instanceName := fmt.Sprintf("tf-test-%s", randString(t, 10)) + tableName := fmt.Sprintf("tf-test-%s", randString(t, 10)) + familyName := fmt.Sprintf("tf-test-%s", randString(t, 10)) + + vcrTest(t, resource.TestCase{ + PreCheck: func() { testAccPreCheck(t) }, + Providers: testAccProviders, + CheckDestroy: testAccCheckBigtableGCPolicyDestroyProducer(t), + Steps: []resource.TestStep{ + { + Config: testAccBigtableGCPolicy_days(instanceName, tableName, familyName), + Check: resource.ComposeTestCheckFunc( + testAccBigtableGCPolicyExists( + t, "google_bigtable_gc_policy.policy"), + ), + }, + { + Config: testAccBigtableGCPolicy(instanceName, tableName, familyName), + Check: resource.ComposeTestCheckFunc( + testAccBigtableGCPolicyExists( + t, "google_bigtable_gc_policy.policy"), + ), + }, + }, + }) +} + func TestAccBigtableGCPolicy_union(t *testing.T) { // bigtable instance does not use the shared HTTP client, this test creates an instance skipIfVcr(t) @@ -59,6 +91,82 @@ func TestAccBigtableGCPolicy_union(t *testing.T) { }) } +func TestUnitBigtableGCPolicy_customizeDiff(t *testing.T) { + for _, tc := range testUnitBigtableGCPolicyCustomizeDiffTestcases { + tc.check(t) + } +} + +func (testcase *testUnitBigtableGCPolicyCustomizeDiffTestcase) check(t *testing.T) { + d := &ResourceDiffMock{ + Before: map[string]interface{}{}, + After: map[string]interface{}{}, + } + + d.Before["max_age.0.days"] = testcase.oldDays + d.Before["max_age.0.duration"] = testcase.oldDuration + + d.After["max_age.#"] = testcase.arraySize + d.After["max_age.0.days"] = testcase.newDays + d.After["max_age.0.duration"] = testcase.newDuration + + err := resourceBigtableGCPolicyCustomizeDiffFunc(d) + if err != nil { + t.Errorf("error on testcase %s - %w", testcase.testName, err) + } + + var cleared bool = d.Cleared != nil && d.Cleared["max_age.0.duration"] == true && d.Cleared["max_age.0.days"] == true + if cleared != testcase.cleared { + t.Errorf("%s: expected diff clear to be %v, but was %v", testcase.testName, testcase.cleared, cleared) + } +} + +type testUnitBigtableGCPolicyCustomizeDiffTestcase struct { + testName string + arraySize int + oldDays int + newDays int + oldDuration string + newDuration string + cleared bool +} + +var testUnitBigtableGCPolicyCustomizeDiffTestcases = []testUnitBigtableGCPolicyCustomizeDiffTestcase{ + { + testName: "ArraySize0", + arraySize: 0, + cleared: false, + }, + { + testName: "DaysChange", + arraySize: 1, + oldDays: 3, + newDays: 2, + cleared: false, + }, + { + testName: "DurationChanges", + arraySize: 1, + oldDuration: "3h", + newDuration: "4h", + cleared: false, + }, + { + testName: "DaysToDurationEq", + arraySize: 1, + oldDays: 3, + newDuration: "72h", + cleared: true, + }, + { + testName: "DaysToDurationNotEq", + arraySize: 1, + oldDays: 3, + newDuration: "70h", + cleared: false, + }, +} + func testAccCheckBigtableGCPolicyDestroyProducer(t *testing.T) func(s *terraform.State) error { return func(s *terraform.State) error { var ctx = context.Background() @@ -129,6 +237,41 @@ func testAccBigtableGCPolicyExists(t *testing.T, n string) resource.TestCheckFun } } +func testAccBigtableGCPolicy_days(instanceName, tableName, family string) string { + return fmt.Sprintf(` +resource "google_bigtable_instance" "instance" { + name = "%s" + + cluster { + cluster_id = "%s" + zone = "us-central1-b" + } + + instance_type = "DEVELOPMENT" + deletion_protection = false +} + +resource "google_bigtable_table" "table" { + name = "%s" + instance_name = google_bigtable_instance.instance.id + + column_family { + family = "%s" + } +} + +resource "google_bigtable_gc_policy" "policy" { + instance_name = google_bigtable_instance.instance.id + table = google_bigtable_table.table.name + column_family = "%s" + + max_age { + days = 3 + } +} +`, instanceName, instanceName, tableName, family, family) +} + func testAccBigtableGCPolicy(instanceName, tableName, family string) string { return fmt.Sprintf(` resource "google_bigtable_instance" "instance" { diff --git a/google-beta/test_utils.go b/google-beta/test_utils.go index e72a5ee145..f83f1e2089 100644 --- a/google-beta/test_utils.go +++ b/google-beta/test_utils.go @@ -74,7 +74,7 @@ func (d *ResourceDataMock) Timeout(key string) time.Duration { type ResourceDiffMock struct { Before map[string]interface{} After map[string]interface{} - Cleared map[string]struct{} + Cleared map[string]interface{} IsForceNew bool } @@ -98,9 +98,9 @@ func (d *ResourceDiffMock) GetOk(key string) (interface{}, bool) { func (d *ResourceDiffMock) Clear(key string) error { if d.Cleared == nil { - d.Cleared = map[string]struct{}{} + d.Cleared = map[string]interface{}{} } - d.Cleared[key] = struct{}{} + d.Cleared[key] = true return nil }