From 0daef118e1e18d3d5d740233e34b7295834061db Mon Sep 17 00:00:00 2001 From: Jacob Bednarz Date: Fri, 21 Dec 2018 10:35:41 +1100 Subject: [PATCH 1/2] Page rule `zone` attribute change to trigger new resource #180 raised an issue whereby using a `cloudflare_page_rule` resource with a single resource identifier but multiple variables to change the `zone` attribute would attempt to update the page rule in place instead of re-creating it (see the attached issue for full reproduction steps). This isn't good as the `target` attribute needs to contain references to the `zone` in order to be successfully updated. To fix this, the schema details for `zone` has been updated to force a new resource on change. On the surface this change is fine however I can see a couple of scenarios where a `DiffSuppressFunc` that checks the host against the stored state file host might be needed in order to not create and delete unnecessarily and instead only create a new resource if the hosts are different. Fixes #180 --- cloudflare/resource_cloudflare_page_rule.go | 1 + .../resource_cloudflare_page_rule_test.go | 28 +++++++++++++++++++ 2 files changed, 29 insertions(+) diff --git a/cloudflare/resource_cloudflare_page_rule.go b/cloudflare/resource_cloudflare_page_rule.go index 8f5ff9f3a0..167d0d73fd 100644 --- a/cloudflare/resource_cloudflare_page_rule.go +++ b/cloudflare/resource_cloudflare_page_rule.go @@ -26,6 +26,7 @@ func resourceCloudflarePageRule() *schema.Resource { "zone": { Type: schema.TypeString, Required: true, + ForceNew: true, }, "zone_id": { diff --git a/cloudflare/resource_cloudflare_page_rule_test.go b/cloudflare/resource_cloudflare_page_rule_test.go index 321bad09e5..d1efa74eb2 100644 --- a/cloudflare/resource_cloudflare_page_rule_test.go +++ b/cloudflare/resource_cloudflare_page_rule_test.go @@ -193,6 +193,34 @@ func TestAccCloudflarePageRule_CreateAfterManualDestroy(t *testing.T) { }) } +func TestAccCloudflarePageRule_UpdatingZoneForcesNewResource(t *testing.T) { + var before, after cloudflare.PageRule + oldZone := os.Getenv("CLOUDFLARE_DOMAIN") + newZone := os.Getenv("CLOUDFLARE_ALT_DOMAIN") + + resource.Test(t, resource.TestCase{ + PreCheck: func() { testAccPreCheck(t) }, + Providers: testAccProviders, + Steps: []resource.TestStep{ + { + Config: testAccCheckCloudflarePageRuleConfigBasic(oldZone, fmt.Sprintf("test-updating-zone-value.%s", oldZone)), + Check: resource.ComposeTestCheckFunc( + testAccCheckCloudflarePageRuleExists("cloudflare_page_rule.test", &before), + resource.TestCheckResourceAttr("cloudflare_page_rule.test", "zone", oldZone), + ), + }, + { + Config: testAccCheckCloudflarePageRuleConfigBasic(newZone, fmt.Sprintf("test-updating-zone-value.%s", newZone)), + Check: resource.ComposeTestCheckFunc( + testAccCheckCloudflarePageRuleExists("cloudflare_page_rule.test", &after), + testAccCheckCloudflarePageRuleRecreated(&before, &after), + resource.TestCheckResourceAttr("cloudflare_page_rule.test", "zone", newZone), + ), + }, + }, + }) +} + func TestTranformForwardingURL(t *testing.T) { key, val, err := transformFromCloudflarePageRuleAction(&cloudflare.PageRuleAction{ ID: "forwarding_url", From 668ddc8809889f66812f51644559f0137482f8ee Mon Sep 17 00:00:00 2001 From: Jacob Bednarz Date: Fri, 21 Dec 2018 10:58:53 +1100 Subject: [PATCH 2/2] Add `testAccPreCheckAltDomain` Creates a new method `testAccPreCheckAltDomain` to fail hard in the event the required environment variables aren't defined that are needed in order to successfully run these tests. --- cloudflare/provider_test.go | 6 ++++++ cloudflare/resource_cloudflare_page_rule_test.go | 5 ++++- 2 files changed, 10 insertions(+), 1 deletion(-) diff --git a/cloudflare/provider_test.go b/cloudflare/provider_test.go index 597ff7876c..4418f57d86 100644 --- a/cloudflare/provider_test.go +++ b/cloudflare/provider_test.go @@ -44,6 +44,12 @@ func testAccPreCheck(t *testing.T) { } } +func testAccPreCheckAltDomain(t *testing.T) { + if v := os.Getenv("CLOUDFLARE_ALT_DOMAIN"); v == "" { + t.Fatal("CLOUDFLARE_ALT_DOMAIN must be set for this acceptance test") + } +} + func testAccPreCheckOrg(t *testing.T) { if v := os.Getenv("CLOUDFLARE_ORG_ID"); v == "" { t.Fatal("CLOUDFLARE_ORG_ID must be set for this acceptance test") diff --git a/cloudflare/resource_cloudflare_page_rule_test.go b/cloudflare/resource_cloudflare_page_rule_test.go index d1efa74eb2..6d9846a870 100644 --- a/cloudflare/resource_cloudflare_page_rule_test.go +++ b/cloudflare/resource_cloudflare_page_rule_test.go @@ -199,7 +199,10 @@ func TestAccCloudflarePageRule_UpdatingZoneForcesNewResource(t *testing.T) { newZone := os.Getenv("CLOUDFLARE_ALT_DOMAIN") resource.Test(t, resource.TestCase{ - PreCheck: func() { testAccPreCheck(t) }, + PreCheck: func() { + testAccPreCheck(t) + testAccPreCheckAltDomain(t) + }, Providers: testAccProviders, Steps: []resource.TestStep{ {