filters: ignore \n's at the end of expressions

[xens]

This commit fixes an issue where filters using the heredoc syntax are always
seen as "to change" by Terraform. This is due to the fact that the heredoc
syntax includes a trailing character (\n) before EOF and because Cloudflare
always strips these trailing characters before storing expressions, leading
to Terraform always thinking that something must be changed. This change
also mimic the behavior of Cloudflare that always strips carriage-return,
whitespaces and tabs at the beginning and the end of expressions.

[jacobbednarz]

Thanks for submitting this @xens! It's greatly appreciated. Before we merge, we should add a test to cover the case you're making the change for. This prevents other committers in the future from introducing a regression and serves as a "golden state" for what we expect the behaviour to be if it's ever refactored.

I've also added a couple of notes against some lines for your consideration.

[xens]

Regarding unit-tests that make a lot of sense, I'm not quite 100% how to implement that as I understand we'll need to have a real Cloudflare account to do our tests against. I'll need to dig a bit into it, suggestions welcome :)

[jacobbednarz]

Unit tests don’t however majority of the tests here are integration tests and they do require various Cloudflare credentials and options to run them.

[BenoitKnecht]

@xens I implemented a test for this issue: 748ac34.

I verified that it does fail without your commit, and that it passes once the fix is applied. Feel free to cherry-pick it in this PR.

[BenoitKnecht]

@jacobbednarz Have you had a chance to take a look at the updated commits?

[jacobbednarz]

I had not, no so thanks for the ping as I generally don't watch individual PR commits.

The test that we have added here doesn't actually fail when we remove the fix so it's not really giving us any benefit. For it to help, we need to add a Check entry that confirms that a change isn't performed and the values are stored in the state without the additional spaces. You should be able to have a look at some Update tests to see how this can be done if you'd like a guide.

[BenoitKnecht]

@jacobbednarz Are you sure the test doesn't work? As mentioned above, I did run it with and without the fix applied and verified that it failed in the latter case.

Here's the output I get, for reference; with the fix applied:

$ go test -v -run TestFilterWhitespace
=== RUN   TestFilterWhitespace
--- PASS: TestFilterWhitespace (4.42s)
PASS
ok  	github.com/terraform-providers/terraform-provider-cloudflare/cloudflare	4.455s

and without the fix:

$ go test -v -run TestFilterWhitespace
=== RUN   TestFilterWhitespace
--- FAIL: TestFilterWhitespace (3.78s)
    testing.go:568: Step 0 error: After applying this step, the plan was not empty:
        
        DIFF:
        
        UPDATE: cloudflare_filter.beynllntyc
          description: "multi-line filter" => "multi-line filter"
          expression:  "http.request.method in {\"PUT\" \"DELETE\"} and\nhttp.request.uri.path eq \"/\"" => "\t\nhttp.request.method in {\"PUT\" \"DELETE\"} and\nhttp.request.uri.path eq \"/\"  \n\n"
          id:          "68e0dbcc59a04a7890ed5502797fbe5b" => "68e0dbcc59a04a7890ed5502797fbe5b"
          paused:      "true" => "true"
          ref:         "" => ""
          zone:        "terraform.com" => "terraform.com"
          zone_id:     "c5fd73fc39814cd53b036e98d56dafe0" => "c5fd73fc39814cd53b036e98d56dafe0"
        
        
        
        STATE:
        
        cloudflare_filter.beynllntyc:
          ID = 68e0dbcc59a04a7890ed5502797fbe5b
          provider = provider.cloudflare
          description = multi-line filter
          expression = http.request.method in {"PUT" "DELETE"} and
        http.request.uri.path eq "/"
          paused = true
          ref = 
          zone = terraform.com
          zone_id = c5fd73fc39814cd53b036e98d56dafe0
FAIL
exit status 1
FAIL	github.com/terraform-providers/terraform-provider-cloudflare/cloudflare	3.806s

In both cases, I had the following environment variables exported:

• CLOUDFLARE_EMAIL
• CLOUDFLARE_TOKEN
• CLOUDFLARE_DOMAIN
• CLOUDFLARE_ZONE_ID
• TF_ACC

So it seems that any Terraform test case makes sure that applying a resource twice generates an empty plan the second time: https://github.com/hashicorp/terraform/blob/master/helper/resource/testing_config.go#L117-L147. I'm not sure why you're seeing different results.

[jacobbednarz]

You're right @BenoitKnecht, I had opened this branch in a new session and forgot to reset up the environment for the integration tests resulting in them just constantly passing. Probably something to address there to make it harder to forget bits like that...

We're good to go on this. CI integration is all 💚 .

[jacobbednarz]

Thank you @xens and @BenoitKnecht, you both rock ! 🚀

[BenoitKnecht]

Right back at you @jacobbednarz! Thanks!