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

Add a cloudflare_zone_cache_variants resource #1444

Merged
merged 6 commits into from
Feb 12, 2022

Conversation

maleblond
Copy link
Contributor

@maleblond maleblond commented Feb 11, 2022

We'd like to be able to manage cache variants through terraform so I added a.cloudflare_zone_cache_variants resource.

I added some unit tests + I did some manual testing. A cloudflare_zone_cache_variants resource would look like this:

resource "cloudflare_zone_cache_variants" "variants" {
  zone_id = <zone_id>
  gif     = ["image/avif", "image/webp", "image/gif"]
  webp    = ["image/whatever"]
  ...
}

It's my first time updating a terraform provider, so there may be some very ugly things in there not following the best practices 😅

@github-actions
Copy link
Contributor

Oops! It looks like no changelog entry is attached to this PR. Please include a release note as described in https://github.com/cloudflare/terraform-provider-cloudflare/blob/master/docs/changelog-process.md.

Example:

```release-note:TYPE
Release note
```

If you do not require a release note to be included, please add the workflow/skip-changelog-entry label.

@maleblond maleblond marked this pull request as ready for review February 11, 2022 19:35
go.mod Outdated Show resolved Hide resolved
@maleblond maleblond force-pushed the maleblond/cache-variants branch from 8b17699 to fc211f7 Compare February 11, 2022 20:21
},
}

for _, ext := range supportedCacheVariantsExtensions {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not really sure how i feel about the dynamic nature of creating schema items due to making it difficult for discovery. i'll have a think about this.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 My main goal was not to repeat the whole schema over and over again. An alternative would be to do something like:

"avif": extensionSchema(),
"gif": extensionSchema(),

I think an upside with this alternative is that the schema is a little bit easier to read. I'll do that, LMK what you think

func resourceCloudflareZoneCacheVariants() *schema.Resource {
return &schema.Resource{
Schema: resourceCloudflareZoneCacheVariantsSchema(),
Create: resourceCloudflareZoneCacheVariantsCreate,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

given create and update are essentially the same funcitonality, you could point Create and Update here to the same method.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 I merged both function

@jacobbednarz
Copy link
Member

acceptance tests are green

TF_ACC=1 go test $(go list ./...) -v -run "^TestAccCloudflareZoneCacheVariants_" -count 1 -parallel 1 -timeout 120m -parallel 1
?   	github.com/cloudflare/terraform-provider-cloudflare	[no test files]
=== RUN   TestAccCloudflareZoneCacheVariants_OneExt
--- PASS: TestAccCloudflareZoneCacheVariants_OneExt (9.69s)
=== RUN   TestAccCloudflareZoneCacheVariants_AllExt
--- PASS: TestAccCloudflareZoneCacheVariants_AllExt (9.12s)
PASS
ok  	github.com/cloudflare/terraform-provider-cloudflare/cloudflare	19.301s
?   	github.com/cloudflare/terraform-provider-cloudflare/tools/cmd/changelog-check	[no test files]
?   	github.com/cloudflare/terraform-provider-cloudflare/version	[no test files]

really solid effort here @maleblond, your first contribution no less! thanks for such a solid PR.

@jacobbednarz jacobbednarz merged commit bc8873b into cloudflare:master Feb 12, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants