From 9d1e01a5de9fda90eec953274c565eb0e98d233a Mon Sep 17 00:00:00 2001 From: Scott Suarez Date: Tue, 2 Mar 2021 19:50:32 -0800 Subject: [PATCH 1/4] Add customize diff for deprecated attribute on gc_policy --- mmv1/compiler.rb | 1 + .../resources/resource_bigtable_gc_policy.go | 37 +++++++++- .../tests/resource_bigtable_gc_policy_test.go | 67 +++++++++++++++++++ 3 files changed, 102 insertions(+), 3 deletions(-) diff --git a/mmv1/compiler.rb b/mmv1/compiler.rb index 01c4f2b7c7ea..b398dca5745f 100755 --- a/mmv1/compiler.rb +++ b/mmv1/compiler.rb @@ -121,6 +121,7 @@ end if override_dir + Google::LOGGER.info "Using override directory '#{override_dir}'" Dir["#{override_dir}/products/**/api.yaml"].each do |file_path| product = File.dirname(Pathname.new(file_path).relative_path_from(override_dir)) all_product_files.push(product) unless all_product_files.include? product diff --git a/mmv1/third_party/terraform/resources/resource_bigtable_gc_policy.go b/mmv1/third_party/terraform/resources/resource_bigtable_gc_policy.go index 02a4f99cc540..1b063d7a7df9 100644 --- a/mmv1/third_party/terraform/resources/resource_bigtable_gc_policy.go +++ b/mmv1/third_party/terraform/resources/resource_bigtable_gc_policy.go @@ -16,11 +16,40 @@ const ( GCPolicyModeUnion = "UNION" ) +func resourceBigtableGCPolicyCustomizeDiff(_ context.Context, diff *schema.ResourceDiff, meta interface{}) 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 { + diff.Clear("max_age.0.days") + diff.Clear("max_age.0.duration") + } + } + + return nil +} + 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 +93,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 +102,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/mmv1/third_party/terraform/tests/resource_bigtable_gc_policy_test.go b/mmv1/third_party/terraform/tests/resource_bigtable_gc_policy_test.go index c504ec39768f..fbab8ac4b4f6 100644 --- a/mmv1/third_party/terraform/tests/resource_bigtable_gc_policy_test.go +++ b/mmv1/third_party/terraform/tests/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) @@ -129,6 +161,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" { From fc00915c91ff8b4d1fd39555fcac999afb976887 Mon Sep 17 00:00:00 2001 From: Scott Suarez Date: Fri, 5 Mar 2021 09:48:32 -0800 Subject: [PATCH 2/4] handle error for linter --- .../terraform/resources/resource_bigtable_gc_policy.go | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/mmv1/third_party/terraform/resources/resource_bigtable_gc_policy.go b/mmv1/third_party/terraform/resources/resource_bigtable_gc_policy.go index 1b063d7a7df9..88721aeab693 100644 --- a/mmv1/third_party/terraform/resources/resource_bigtable_gc_policy.go +++ b/mmv1/third_party/terraform/resources/resource_bigtable_gc_policy.go @@ -36,8 +36,14 @@ func resourceBigtableGCPolicyCustomizeDiff(_ context.Context, diff *schema.Resou } dn := time.Hour * 24 * time.Duration(oldDays.(int)) if do == dn { - diff.Clear("max_age.0.days") + err := diff.Clear("max_age.0.days") + if err != nil { + return err + } diff.Clear("max_age.0.duration") + if err != nil { + return err + } } } From 7069cb58d44cc3dd348d87320802db3f34d638e9 Mon Sep 17 00:00:00 2001 From: Scott Suarez Date: Fri, 5 Mar 2021 10:48:54 -0800 Subject: [PATCH 3/4] add unit test --- .../resources/resource_bigtable_gc_policy.go | 6 +- .../tests/resource_bigtable_gc_policy_test.go | 76 +++++++++++++++++++ .../third_party/terraform/utils/test_utils.go | 6 +- 3 files changed, 84 insertions(+), 4 deletions(-) diff --git a/mmv1/third_party/terraform/resources/resource_bigtable_gc_policy.go b/mmv1/third_party/terraform/resources/resource_bigtable_gc_policy.go index 88721aeab693..07812c98b57e 100644 --- a/mmv1/third_party/terraform/resources/resource_bigtable_gc_policy.go +++ b/mmv1/third_party/terraform/resources/resource_bigtable_gc_policy.go @@ -16,7 +16,7 @@ const ( GCPolicyModeUnion = "UNION" ) -func resourceBigtableGCPolicyCustomizeDiff(_ context.Context, diff *schema.ResourceDiff, meta interface{}) error { +func resourceBigtableGCPolicyCustomizeDiffFunc(diff TerraformResourceDiff) error { count := diff.Get("max_age.#").(int) if count < 1 { return nil @@ -50,6 +50,10 @@ func resourceBigtableGCPolicyCustomizeDiff(_ context.Context, diff *schema.Resou return nil } +func resourceBigtableGCPolicyCustomizeDiff(_ context.Context, d *schema.ResourceDiff, meta interface{}) error { + return resourceBigtableGCPolicyCustomizeDiffFunc(d) +} + func resourceBigtableGCPolicy() *schema.Resource { return &schema.Resource{ Create: resourceBigtableGCPolicyCreate, diff --git a/mmv1/third_party/terraform/tests/resource_bigtable_gc_policy_test.go b/mmv1/third_party/terraform/tests/resource_bigtable_gc_policy_test.go index fbab8ac4b4f6..8bad8fe1e11a 100644 --- a/mmv1/third_party/terraform/tests/resource_bigtable_gc_policy_test.go +++ b/mmv1/third_party/terraform/tests/resource_bigtable_gc_policy_test.go @@ -91,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() diff --git a/mmv1/third_party/terraform/utils/test_utils.go b/mmv1/third_party/terraform/utils/test_utils.go index e72a5ee145fb..f83f1e20894d 100644 --- a/mmv1/third_party/terraform/utils/test_utils.go +++ b/mmv1/third_party/terraform/utils/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 } From d53db146ef4166ed7a53a397da635c59a8e42930 Mon Sep 17 00:00:00 2001 From: Scott Suarez Date: Tue, 9 Mar 2021 10:14:10 -0800 Subject: [PATCH 4/4] rake issue --- .../terraform/resources/resource_bigtable_gc_policy.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/mmv1/third_party/terraform/resources/resource_bigtable_gc_policy.go b/mmv1/third_party/terraform/resources/resource_bigtable_gc_policy.go index 07812c98b57e..5d72268726d3 100644 --- a/mmv1/third_party/terraform/resources/resource_bigtable_gc_policy.go +++ b/mmv1/third_party/terraform/resources/resource_bigtable_gc_policy.go @@ -40,7 +40,7 @@ func resourceBigtableGCPolicyCustomizeDiffFunc(diff TerraformResourceDiff) error if err != nil { return err } - diff.Clear("max_age.0.duration") + err = diff.Clear("max_age.0.duration") if err != nil { return err }