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 ability to wait for cert packs to be active #1567

Merged
merged 7 commits into from
Apr 27, 2022
Merged

Add ability to wait for cert packs to be active #1567

merged 7 commits into from
Apr 27, 2022

Conversation

ct-dh
Copy link

@ct-dh ct-dh commented Apr 20, 2022

This is probably not ready for merge - I made this little patch to test the feature request in #1559 and won't be able to follow up in all likelihood by the time there is a response due to a change in employer (so I will no longer personally be working with Cloudflare - but the use case for the employer is still valid/relevant), but I didn't want to just delete the local code.

Please feel free to use/modify/drop this as needed.

It adds a new option to the cloudflare_certificate_pack resource to block/wait on creation until all certificates in the pack are marked as active in the API - with the aim of this making use of create_before_destroy less likely to cause service disruption on updates.

@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.

@jacobbednarz
Copy link
Member

before we consider merging this, we'll also need some tests and to clean up the imports for these files.

@jacobbednarz jacobbednarz added the workflow/pending-op-response Indicates an issue or PR requires a response from the original poster. label Apr 22, 2022
@ct-dh
Copy link
Author

ct-dh commented Apr 26, 2022

before we consider merging this, we'll also need some tests and to clean up the imports for these files.

I've cleaned up the imports, what level of testing do you think we need by the way? I can add a resource.TestCheckResourceAttr check for the new attribute to check the default, but I was struggling to think of a good way to test this properly in the acceptance tests as we cannot use a fake to ensure we actually see the non-active -> active state transition.

I finish my current job on Friday this week so I won't be likely to look at this issue after that point as I don't think I will be using Cloudflare anymore, so any feedback before then would be handy.

@jacobbednarz
Copy link
Member

I can add a resource.TestCheckResourceAttr check for the new attribute to check the default

yep, just a check that it ends up in the state is fine. we can also add another test with the wait_for_active_status flag set to on so that we exercise that code path and there aren't any errors too.

@ct-dh
Copy link
Author

ct-dh commented Apr 27, 2022

I can add a resource.TestCheckResourceAttr check for the new attribute to check the default

yep, just a check that it ends up in the state is fine. we can also add another test with the wait_for_active_status flag set to on so that we exercise that code path and there aren't any errors too.

Understood, I will get this added today. Thanks.

@ct-dh
Copy link
Author

ct-dh commented Apr 27, 2022

Added the tests and rebased from master if you can take another look please @jacobbednarz.

I don't have a test Cloudflare account that would be safe to run the test suite in (from a brief look at the way they work, the sweepers will destroy unrelated resources) so I don't know if the tests actually pass or not.

@jacobbednarz
Copy link
Member

acceptance tests look good

TF_ACC=1 go test $(go list ./...) -v -run "^TestAccCertificatePack_" -count 1 -parallel 1 -timeout 120m -parallel 1
?   	github.com/cloudflare/terraform-provider-cloudflare	[no test files]
=== RUN   TestAccCertificatePack_AdvancedDigicert
--- PASS: TestAccCertificatePack_AdvancedDigicert (9.51s)
=== RUN   TestAccCertificatePack_AdvancedLetsEncrypt
--- PASS: TestAccCertificatePack_AdvancedLetsEncrypt (9.86s)
=== RUN   TestAccCertificatePack_DedicatedCustom
    resource_cloudflare_certificate_pack_test.go:133: Pending investigation into ACM entitlements
--- SKIP: TestAccCertificatePack_DedicatedCustom (0.00s)
=== RUN   TestAccCertificatePack_WaitForActive
--- PASS: TestAccCertificatePack_WaitForActive (14.06s)
PASS
ok  	github.com/cloudflare/terraform-provider-cloudflare/cloudflare	34.009s
?   	github.com/cloudflare/terraform-provider-cloudflare/tools/cmd/changelog-check	[no test files]
?   	github.com/cloudflare/terraform-provider-cloudflare/tools/cmd/maintainer-only-file-check	[no test files]
?   	github.com/cloudflare/terraform-provider-cloudflare/tools/cmd/tf-log-check	[no test files]
?   	github.com/cloudflare/terraform-provider-cloudflare/version	[no test files]

@jacobbednarz jacobbednarz merged commit cbe7709 into cloudflare:master Apr 27, 2022
@jacobbednarz jacobbednarz removed the workflow/pending-op-response Indicates an issue or PR requires a response from the original poster. label Apr 27, 2022
@github-actions github-actions bot added this to the v3.14.0 milestone Apr 27, 2022
@ct-dh ct-dh deleted the feature/add-cert-pack-active-wait branch April 28, 2022 09:59
@github-actions
Copy link
Contributor

github-actions bot commented May 3, 2022

This functionality has been released in v3.14.0 of the Terraform Cloudflare Provider.

Please see the Terraform documentation on provider versioning or reach out if you need any assistance upgrading.

For further feature requests or bug reports with this functionality, please create a new GitHub issue following the template. Thank you!

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.

cloudflare_certificate_pack - optionally block until status is active
2 participants