Fix browser_cache_ttl page rule action bugs

[stevehodgkiss]

Addresses 2 issues with browser_cache_ttl due to 0 being the default value and also a valid & desirable value to define it with.

• Deleting: After a browser_cache_ttl page rule action has been applied, it can't be deleted because its value is the default of 0 when it's removed, so it ends up in the update PUT request as being set to 0.
• Setting to 0 initially: Defining browser_cache_ttl = 0 initially doesn't work because it's not seen as a change (current workaround is to set to something else first, then 0).

Using an integer for this value in the schema makes that quite challenging. Another option might be to make -1 a special and default value used to mean "not set". The approach in this PR is to use a string instead.

I'd like to take a closer look at some of the test cases before this merged (hence WIP in the title).

[jacobbednarz]

Let's swap this one for a specific configuration setup (like we do for testAccCheckCloudflarePageRuleConfigMinify). It's probably a bit more verbose but it will be explicit about what is getting setup.

[jacobbednarz]

Thanks for getting this up @stevehodgkiss! 🍰 I like the additional tests we've added here however a couple things we should update to align with the other tests.

1. The integration tests you've added here should be named TestAccCloudflarePageRule_*. The TestAcc is important as that is what the CI runner looks for when it's searching for tests to run. See https://www.terraform.io/docs/extend/testing/acceptance-tests/index.html#test-files for more details. Right now, these new tests aren't being automatically run.
2. We should structure the tests similar to the ones we already have where each uses an explicit resource definition and the test setup just relies on that. An example:

• Rule configuration: https://github.com/terraform-providers/terraform-provider-cloudflare/blob/b1dbaa88a803d4b069bb77c2b6c702ba2c507253/cloudflare/resource_cloudflare_page_rule_test.go#L557-L567
• Usage: https://github.com/terraform-providers/terraform-provider-cloudflare/blob/b1dbaa88a803d4b069bb77c2b6c702ba2c507253/cloudflare/resource_cloudflare_page_rule_test.go#L123-L146

[jacobbednarz]

Suggested change

[patryk]

Yes, I think changing the field to string is a good compromise, and in the end should not deal any major damage, provided Terraform uses strings implicitly everywhere in .tf files anyway.

[jacobbednarz]

I think this is all the scenarios.

scenario	state expected outcome	remote expected outcome
page rule with browser_cache_ttl = 0 can be imported	browser_cache_ttl = 0	page rule exists with browser_cache_ttl value of 0
page rule with browser_cache_ttl = 10 can be imported	browser_cache_ttl = 10	page rule exists with browser_cache_ttl value of 10
page rule with browser_cache_ttl absent can be imported	browser_cache_ttl should not be present	page rule without browser_cache_ttl should exist
create a new rule browser_cache_ttl = 0	browser_cache_ttl = 0	page rule exists with browser_cache_ttl value of 0
create a new rule browser_cache_ttl = 10	browser_cache_ttl = 10	page rule exists with browser_cache_ttl value of 10
create a new rule browser_cache_ttl absent	browser_cache_ttl should not be present	page rule doesn't get created with browser_cache_ttl attribute
delete an existing page rule where browser_cache_ttl was set to 0	browser_cache_ttl should not be present	page rule isn't present with browser_cache_ttl value
delete an existing page rule where browser_cache_ttl was set to 10	browser_cache_ttl should not be present	page rule isn't present with browser_cache_ttl value
delete an existing page rule where browser_cache_ttl is absent	browser_cache_ttl should not be present	page rule isn't present with browser_cache_ttl value
update an existing page rule from not having browser_cache_ttl present to browser_cache_ttl value of 0	browser_cache_ttl = 0	page rule is updated with browser_cache_ttl value of 0
update an existing page rule from browser_cache_ttl = 0 to be 10	browser_cache_ttl = 10	page rule is updated with browser_cache_ttl value of 10
update an existing page rule from browser_cache_ttl = 10 to be 0	browser_cache_ttl = 0	page rule is updated with browser_cache_ttl value of 0

[jacobbednarz]

Looking great @stevehodgkiss! The only three test cases I think we're missing here is the ones for importing resources (one with a value of 0, one with a value of > 0 and one without the attribute entirely). Here is an example of another resource testing the import.

https://github.com/terraform-providers/terraform-provider-cloudflare/blob/5d7a3d1b81a79f6489691f3d74330a841fe2e55e/cloudflare/resource_cloudflare_zone_lockdown_test.go#L36-L57

Looking at the code, this should Just Work™ once the tests are added.

I've run the integration tests on CI and in another couple of accounts and so far, 💚 all the way.

[jacobbednarz]

this is awesome work! thanks @stevehodgkiss 🍭

[patryk]

Big thanks @stevehodgkiss for helping out with this issue! Appreciated.