Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Adds universal_ssl to cloudflare_zone_settings_override #658

Merged
merged 11 commits into from
Apr 20, 2020

Conversation

cehrig
Copy link
Contributor

@cehrig cehrig commented Apr 18, 2020

This is a new resource cloudflare_universal_ssl talking to zones/:zone_identifier/ssl/universal/settings via client.UniversalSSLSettingDetails and client.EditUniversalSSLSetting.

It would have fit into the zone settings override resource but since USSL works on a different API I decided to create a new resource.

Usage

resource "cloudflare_universal_ssl" "example" {
    zone_id = "d41d8cd98f00b204e9800998ecf8427e"
    settings {
        status = "on"
    }
}

The resource supports importing.

Test Case:

cehrig@che-box ~/go/src/github.com/terraform-providers/terraform-provider-cloudflare $ go test -v -timeout 30s github.com/terraform-providers/terraform-provider-cloudflare/cloudflare -run TestAccCloudflareUniversalSSL
=== RUN   TestAccCloudflareUniversalSSL
=== PAUSE TestAccCloudflareUniversalSSL
=== CONT  TestAccCloudflareUniversalSSL
2020/04/18 16:05:18 DestroyEdgeTransformer: pruning unused resource node cloudflare_universal_ssl.veihdokfpq (prepare state)
--- PASS: TestAccCloudflareUniversalSSL (5.57s)
PASS
ok      github.com/terraform-providers/terraform-provider-cloudflare/cloudflare 5.579s

@ghost ghost added size/L kind/documentation Categorizes issue or PR as related to documentation. labels Apr 18, 2020
@jacobbednarz
Copy link
Member

Thanks for taking the time to put this one together!

I’m on the fence as to whether this should be it’s own resource or whether it should be an option on cloudflare_zone_override as you cannot have multiples of this (as you would a traditional resource). By adding it to cloudflare_zone_override you would also be following the same pattern for other zone level settings.

@cehrig
Copy link
Contributor Author

cehrig commented Apr 18, 2020

That'd be fine I guess. I'm just concerned as this would be the first setting in zone overrides not using /zones/:zone_identifier/settings. Shall we call it settings.0.universal_ssl ?

@jacobbednarz
Copy link
Member

Sounds pretty reasonable. I don’t mind too much about it being the only one as it is portrayed as a zone setting so the association for the end user is still in tact.

@ghost ghost added size/M and removed size/L labels Apr 18, 2020
@cehrig
Copy link
Contributor Author

cehrig commented Apr 18, 2020

@jacobbednarz I would appreciate if you could double check the approach. Since most of the implementation builds on top of cloudflare.ZoneSetting I'm wrapping the USSL status in a struct of this type. This way I can keep the update pretty simple.

@jacobbednarz
Copy link
Member

This seems like a good approach Are you able to add some test coverage?

@cehrig
Copy link
Contributor Author

cehrig commented Apr 19, 2020

Sure. I've added to the existing test

=== RUN   TestAccCloudflareZoneSettingsOverride_Full
2020/04/20 00:33:24 DestroyEdgeTransformer: pruning unused resource node cloudflare_zone_settings_override.test (prepare state)
--- PASS: TestAccCloudflareZoneSettingsOverride_Full (35.13s)
PASS
ok      github.com/terraform-providers/terraform-provider-cloudflare/cloudflare 35.140s```

@jacobbednarz jacobbednarz merged commit eca720c into cloudflare:master Apr 20, 2020
@jacobbednarz jacobbednarz changed the title Adds cloudflare_universal_ssl resource controlling USSL status Adds universal_ssl to cloudflare_zone_settings_override Apr 20, 2020
@jacobbednarz
Copy link
Member

Thanks for this @cehrig 👏

@cehrig cehrig deleted the cehrig/universal_ssl branch April 20, 2020 22:32
boekkooi-lengoo pushed a commit to boekkooi-lengoo/terraform-provider-cloudflare that referenced this pull request Feb 28, 2022
…dding

Support for LoadBalancer load shedding
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/documentation Categorizes issue or PR as related to documentation. size/M
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants