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

Fix error handling in cloudflare_turnstile #3284

Merged
merged 1 commit into from
May 6, 2024

Conversation

punkeel
Copy link
Member

@punkeel punkeel commented May 3, 2024

When we receive an error from the API, we would record an error (not return) and write an invalid State. For some reason, Terraform accepts it... yet it seems to corrupt the tf state.

Add some returns to handle error more cleanly.

Related: #3093 (but unsure if that's the only fix required)

@punkeel punkeel requested a review from jacobbednarz as a code owner May 3, 2024 13:20
When we receive an error from the API, we would record an error (not return)
and write an invalid State. For some reason, Terraform accepts it... yet
it seems to corrupt the tf state.

Add some returns to handle error more cleanly.
Copy link
Contributor

github-actions bot commented May 3, 2024

changelog detected ✅

@punkeel
Copy link
Member Author

punkeel commented May 3, 2024

cc @vasilzhigilei
kudos to @KianNH for the help

@vasilzhigilei
Copy link
Contributor

Did you mean to cc me?

@punkeel
Copy link
Member Author

punkeel commented May 3, 2024

My bad, I meant to cc @vasilevalex.

@jacobbednarz
Copy link
Member

When we receive an error from the API, we would record an error (not return) and write an invalid State. For some reason, Terraform accepts it... yet it seems to corrupt the tf state.

terraform doesn't prevent the state from being written as it cannot make decisions about what values should and should not be modified with the error response so it's up to the provider to determine the correct course of action.

i'm on the fence here if we want to return early here as it basically negates anything that happens after this in the resource operation (including the good values). however, given the id is used as an anchor, this seems appropriate for the failure mode.

@jacobbednarz
Copy link
Member

acceptance tests all passing

TF_ACC=1 go test ./internal/framework/service/turnstile -v -run "^TestAccCloudflareTurnstileWidget_" -count 1 -timeout 120m -parallel 1
=== RUN   TestAccCloudflareTurnstileWidget_Basic
=== PAUSE TestAccCloudflareTurnstileWidget_Basic
=== RUN   TestAccCloudflareTurnstileWidget_Minimum
=== PAUSE TestAccCloudflareTurnstileWidget_Minimum
=== RUN   TestAccCloudflareTurnstileWidget_NoDomains
=== PAUSE TestAccCloudflareTurnstileWidget_NoDomains
=== CONT  TestAccCloudflareTurnstileWidget_Basic
--- PASS: TestAccCloudflareTurnstileWidget_Basic (12.13s)
=== CONT  TestAccCloudflareTurnstileWidget_NoDomains
--- PASS: TestAccCloudflareTurnstileWidget_NoDomains (9.71s)
=== CONT  TestAccCloudflareTurnstileWidget_Minimum
--- PASS: TestAccCloudflareTurnstileWidget_Minimum (9.93s)
PASS
ok  	github.com/cloudflare/terraform-provider-cloudflare/internal/framework/service/turnstile	33.164s

@jacobbednarz jacobbednarz merged commit 24226cd into cloudflare:master May 6, 2024
9 checks passed
@github-actions github-actions bot added this to the v4.32.0 milestone May 6, 2024
github-actions bot pushed a commit that referenced this pull request May 6, 2024
@punkeel punkeel deleted the maxime/turnstile-err branch May 6, 2024 01:22
@punkeel
Copy link
Member Author

punkeel commented May 6, 2024

@jacobbednarz thanks & thanks for the link!

The weird part to me is: we declare a Schema, yet Terraform accepts to write invalid states with State.Set. Why isn't the schema enforced.
This wouldn't prevent the bad error-handling, but it would make it obvious at the time it is (wrongly) set, not used again. 😕

See #3285 for other places where we may want to return on errors (with a bonus semgrep)

Copy link
Contributor

github-actions bot commented May 8, 2024

This functionality has been released in v4.32.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!

@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 8, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants