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

provider/dnsimple: Handle 404 on DNSimple records #13131

Merged
merged 1 commit into from
Apr 12, 2017

Conversation

stack72
Copy link
Contributor

@stack72 stack72 commented Mar 28, 2017

When a record was manually deleted from the console, we got an error
saying 404 Record Not Found

//cc @weppos

This PR now handles the usecase:

% make testacc TEST=./builtin/providers/dnsimple
==> Checking that code complies with gofmt requirements...
go generate $(go list ./... | grep -v /terraform/vendor/)
2017/03/28 21:48:19 Generated command/internal_plugin_list.go
TF_ACC=1 go test ./builtin/providers/dnsimple -v  -timeout 120m
=== RUN   TestProvider
--- PASS: TestProvider (0.00s)
=== RUN   TestProvider_impl
--- PASS: TestProvider_impl (0.00s)
=== RUN   TestAccDNSimpleRecord_Basic
--- PASS: TestAccDNSimpleRecord_Basic (1.81s)
=== RUN   TestAccDNSimpleRecord_CreateMxWithPriority
--- PASS: TestAccDNSimpleRecord_CreateMxWithPriority (1.32s)
=== RUN   TestAccDNSimpleRecord_Updated
--- PASS: TestAccDNSimpleRecord_Updated (4.46s)
=== RUN   TestAccDNSimpleRecord_disappears
--- PASS: TestAccDNSimpleRecord_disappears (1.20s)
=== RUN   TestAccDNSimpleRecord_UpdatedMx
--- PASS: TestAccDNSimpleRecord_UpdatedMx (2.91s)
PASS
ok  	github.com/hashicorp/terraform/builtin/providers/dnsimple	11.723s

When a record was manually deleted from the console, we got an error
saying 404 Record Not Found

//cc @weppos

This PR now handles the usecase:

```
% make testacc TEST=./builtin/providers/dnsimple
==> Checking that code complies with gofmt requirements...
go generate $(go list ./... | grep -v /terraform/vendor/)
2017/03/28 21:48:19 Generated command/internal_plugin_list.go
TF_ACC=1 go test ./builtin/providers/dnsimple -v  -timeout 120m
=== RUN   TestProvider
--- PASS: TestProvider (0.00s)
=== RUN   TestProvider_impl
--- PASS: TestProvider_impl (0.00s)
=== RUN   TestAccDNSimpleRecord_Basic
--- PASS: TestAccDNSimpleRecord_Basic (1.81s)
=== RUN   TestAccDNSimpleRecord_CreateMxWithPriority
--- PASS: TestAccDNSimpleRecord_CreateMxWithPriority (1.32s)
=== RUN   TestAccDNSimpleRecord_Updated
--- PASS: TestAccDNSimpleRecord_Updated (4.46s)
=== RUN   TestAccDNSimpleRecord_disappears
--- PASS: TestAccDNSimpleRecord_disappears (1.20s)
=== RUN   TestAccDNSimpleRecord_UpdatedMx
--- PASS: TestAccDNSimpleRecord_UpdatedMx (2.91s)
PASS
ok  	github.com/hashicorp/terraform/builtin/providers/dnsimple	11.723s
```
@@ -104,6 +105,11 @@ func resourceDNSimpleRecordRead(d *schema.ResourceData, meta interface{}) error

resp, err := provider.client.Zones.GetRecord(provider.config.Account, d.Get("domain").(string), recordID)
if err != nil {
if err != nil && strings.Contains(err.Error(), "404") {
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a reason for repeating if err != nil?

Also, this line reminds me we probably have to improve the error reporting in case of 404. Checking against the 404 string is quite error-prone. I can't recall on the top of my head if there is a better way to handle it, I'll check it by tomorrow.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@weppos any luck with the SDK?

@radeksimko radeksimko added the waiting-response An issue/pull request is waiting for a response from the community label Mar 31, 2017
@stack72 stack72 removed the waiting-response An issue/pull request is waiting for a response from the community label Mar 31, 2017
@stack72
Copy link
Contributor Author

stack72 commented Apr 10, 2017

@weppos any thoughts on this?

@weppos
Copy link
Contributor

weppos commented Apr 11, 2017

Sorry, a couple of other priorities stepped in. I apologize for the delay, I didn't have the time to check yet.

I am fine to merge to avoid locking the PR. I will provide an update if I can find a better way, assuming the SDK supports it (or after we add support for 404 specific error types).

@stack72
Copy link
Contributor Author

stack72 commented Apr 12, 2017

Thanks @weppos

we will merge this in it's current state and then improve later :)

@stack72 stack72 merged commit 00e8986 into master Apr 12, 2017
@stack72 stack72 deleted the b-dnsimple-handle-404 branch April 12, 2017 18:49
@ghost
Copy link

ghost commented Apr 14, 2020

I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.

If you have found a problem that seems similar to this, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@ghost ghost locked and limited conversation to collaborators Apr 14, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants