checks if universal_ssl is set in resource configuration to avoid permission issues with strictly scoped API tokens

[cehrig]

@jacobbednarz we came across backwards compatibility issues with strictly scoped API tokens, causing Reads to fail when the token has no permissions reading from the Universal SSL API.

I'm checking if unviersal_ssl is set in the current configuration and skipping API calls if that's not the case

[cehrig]

That should be it.

[jacobbednarz]

Any reason why we can't have the if above this as if setting.ID == "universal_ssl && setting.Value.(string) != "" to avoid the extra nesting?

[cehrig]

In case universal_ssl is an empty string in initial_settings we would not remove the element from the slice, passing it to client.UpdateZoneSettings where it's unknown. That could happen when the resource is removed and we did not have initial_settings for this

[jacobbednarz]

As this identified a regression, we should add a test case to ensure that the scenario encountered is covered going forward.

[cehrig]

As this identified a regression, we should add a test case to ensure that the scenario encountered is covered going forward.

I've added to TestAccCloudflareZoneSettingsOverride_Empty checking if initial_settings.0.universal_ssl is an empty string, since we won't make the API call when settings.0.universal_ssl is not provided

[jacobbednarz]

This has introduced a new failure in the CI suite.

------- Stdout: -------
=== RUN   TestAccCloudflareZoneSettingsOverride_Full
--- FAIL: TestAccCloudflareZoneSettingsOverride_Full (15.07s)
    testing.go:734: Error destroying resource! WARNING: Dangling resources
        may exist. The full state and error is shown below.
        
        Error: Check failed: Final setting for "universal_ssl": off not equal to initial setting: on
        
        State: <no state>
FAIL

[cehrig]

This has introduced a new failure in the CI suite.

------- Stdout: -------
=== RUN   TestAccCloudflareZoneSettingsOverride_Full
--- FAIL: TestAccCloudflareZoneSettingsOverride_Full (15.07s)
    testing.go:734: Error destroying resource! WARNING: Dangling resources
        may exist. The full state and error is shown below.
        
        Error: Check failed: Final setting for "universal_ssl": off not equal to initial setting: on
        
        State: <no state>
FAIL

Can you share how you executed them? This is me running the tests for zone settings override:

cehrig@che-box ~/dev/cf/terraform-provider-cloudflare/cloudflare $ go test -v -timeout 120s github.com/terraform-providers/terraform-provider-cloudflare/cloudflare -run TestAccCloudflareZoneSettingsOverride
=== RUN   TestAccCloudflareZoneSettingsOverride_Empty
--- PASS: TestAccCloudflareZoneSettingsOverride_Empty (26.63s)
=== RUN   TestAccCloudflareZoneSettingsOverride_Full
--- PASS: TestAccCloudflareZoneSettingsOverride_Full (47.68s)
=== RUN   TestAccCloudflareZoneSettingsOverride_RemoveAttributes
--- PASS: TestAccCloudflareZoneSettingsOverride_RemoveAttributes (42.83s)
PASS
ok      github.com/terraform-providers/terraform-provider-cloudflare/cloudflare 117.151s

[jacobbednarz]

This is coming from our CI suite which piggy backs off make testacc. From your command, it looks like you are missing at least TF_ACC=1 and the credentials to actually perform these tests.

[cehrig]

I have them exported and can see the test making API calls. I can't reproduce right now but will investigate. I'm actually wondering because this part of the existing test wasn't touched.

[cehrig]

Okay, that one happens when the initial setting of the zone the test is running against has universal SSL enabled.

[cehrig]

This should fix the test. We won't pull from the Universal SSL API when we did not have an initial setting. Furthermore we don't know to which initial setting to revert when the resource is deleted. That's actually the scenario that is covered in the Full test. If the universal_ssl setting was set to off is covered by the second step of the test. So I'm just skipping the validation in the final test step testAccCheckInitialZoneSettings.

[jacobbednarz]

Thanks, we're all good on the integration tests ⭐