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 support for CustomSSLOptions GeoRestrictions to be nil #714

Merged

Conversation

trjstewart
Copy link
Contributor

This is a minor change to account for underlying API changes in cloudflare-go.

Based on cloudflare/cloudflare-go#480 the GeoRestrictions property for CustomSSLOptions can now possibly be nil. This simply checks if that's the case before flattening the Label property.

Without this change; when GeoRestrictions is nil it results in a segfault as flattenCustomSSLOptions is trying to access a property of an undefined property.

@ghost ghost added the size/XS label Jun 17, 2020
@jacobbednarz
Copy link
Member

Change looks good; do you mind adding a test case exercising this new code path?

@ghost ghost added size/S and removed size/XS labels Jun 18, 2020
@trjstewart
Copy link
Contributor Author

Hey @jacobbednarz. My colleague has added a couple of tests to check both code paths here.

@jacobbednarz
Copy link
Member

@trjstewart @bas-papegaaij Thanks for adding those however they are closer to what I'd expect to see in cloudflare-go in the form of unit tests. Within the Terraform provider, we pretty heavily rely on integration tests such as this one to confirm the Terraform state is correct.

The concept of your tests are spot on however instead we should be looking to assert against the configuration and state generated from the resources. Here is a non-working example but demonstrates what I'd expect.

func TestAccCloudflareCustomSSLWithEmptyGeoRestrictions(t *testing.T) {
  t.Parallel()
  var customSSL cloudflare.ZoneCustomSSL
  zoneID := os.Getenv("CLOUDFLARE_ZONE_ID")
  rnd := generateRandomResourceName()
  resourceName := "cloudflare_custom_ssl." + rnd
  resource.Test(t, resource.TestCase{
    PreCheck:     func() { testAccPreCheck(t) },
    Providers:    testAccProviders,
    CheckDestroy: testAccCheckCloudflareCustomSSLDestroy,
    Steps: []resource.TestStep{
      {
        // Will need to change this to match the setup of the geo restrictions you provide
        Config: testAccCheckCloudflareCustomSSLCertBasic(zoneID, rnd),
        Check: resource.ComposeTestCheckFunc(
          testAccCheckCloudflareCustomSSLExists(resourceName, &customSSL),
          // .. snip
          resource.TestCheckNoResourceAttr(resourceName, "custom_ssl_options.geo_restrictions"),
        ),
      },
    },
  })
}

Is this something you feel you could take a pass at? If not, I'm happy to add them to show you what I mean.

@trjstewart
Copy link
Contributor Author

@jacobbednarz if you have the capacity to add them that would be greatly appreciated. If not, let us know and one of us can take a crack at it. Thanks!

@jacobbednarz
Copy link
Member

I've got a couple of other tasks in front of this one so depending on how important this is to you, I'd recommend you take a crack at it otherwise I can look at it when I get to it.

@jacobbednarz jacobbednarz merged commit d89168f into cloudflare:master Jun 25, 2020
@jacobbednarz
Copy link
Member

Thanks for the work here @trjstewart and @bas-papegaaij. Feel free to check out 7956ce0 if you're curious on the test case that ended up confirming this behaviour.

boekkooi-lengoo pushed a commit to boekkooi-lengoo/terraform-provider-cloudflare that referenced this pull request Feb 28, 2022
Add omitempty to access policy approval_required

Co-authored-by: Eduardo Gomes <egomes@cloudflare.com>
Co-authored-by: Jacob Bednarz <jacob.bednarz@hey.com>
@trjstewart trjstewart deleted the custom_ssl_georestrictions_nil branch March 24, 2022 01:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants