resource_arm_dns_zone: switch dependency from riviera to azure-sdk-for-go #188
Conversation
There was a problem hiding this comment.
Hey @sebastus
Thanks for this PR - I've reviewed and left some comments in-line but this is off to a good start :)
I'm assuming there's no differences when doing a terraform plan using this updated resource, when having created the resource using the Riviera version of the SDK?
Thanks!
azurerm/resource_arm_dns_zone.go
Outdated
| @@ -51,87 +51,78 @@ func resourceArmDnsZone() *schema.Resource { | |||
| Set: schema.HashString, | |||
| }, | |||
|
|
|||
| "etag": { | |||
There was a problem hiding this comment.
can we add this to the documentation?
azurerm/resource_arm_dns_zone.go
Outdated
| parameters := dns.Zone{ | ||
| Name: &zoneName, | ||
| Location: &location, | ||
| Tags: metadata, |
There was a problem hiding this comment.
same question as in the other PR - is metadata the same as tags in the previous API? the documentation is unclear
There was a problem hiding this comment.
yes, metadata == tags.
azurerm/resource_arm_dns_zone.go
Outdated
| result := <-resultChan | ||
| error := <-errorChan | ||
| if result.Response.StatusCode != http.StatusOK { | ||
| return fmt.Errorf("Error deleting DNS zone %s: %s", zoneName, error) |
There was a problem hiding this comment.
can we make this second argument %+v?
| if err != nil { | ||
| return fmt.Errorf("Bad: GetDNSZone: %s", err) | ||
| return nil |
There was a problem hiding this comment.
I think this should be returning the error and checking for the 404 above it?
There was a problem hiding this comment.
the pattern we follow is to first check error, then statusCode, throughout the provider. If this is wrong, we should take it up with the team. I added an error return.
There was a problem hiding this comment.
when I added the error return, it failed 100% of the time. which I guess you could see coming since you suggested checking the 404 first. I really think we need to take this up with the team. I checked a bit and this is the pattern we follow in that routine.
There was a problem hiding this comment.
indeed - in some other resources we check the 404 inside the error condition - but as long as we do it before returning the error we should be fine :)
I really think we need to take this up with the team. I checked a bit and this is the pattern we follow in that routine.
I might be mis-reading this - but with regards to the 404 I'd consider the SDK returning an error for a 404 response to be the expected behaviour, given it's not a successful operation, or were you raising something else?
|
|
||
| for _, rs := range s.RootModule().Resources { | ||
| if rs.Type != "azurerm_dns_zone" { | ||
| if rs.Type != "azurerm_dns_zone_record" { |
There was a problem hiding this comment.
this should be azurerm_dns_zone
| if err != nil { | ||
| return fmt.Errorf("Bad: GetDNSZone: %s", err) | ||
| return fmt.Errorf("Bad: Get zone: %v", err) |
There was a problem hiding this comment.
can we make this Get DNS Zone to be more descriptive with the error?
There was a problem hiding this comment.
Hey @sebastus
Thanks for this contribution - due to changes within Azure detailed in #192 in order for us to merge this change in I've push a commit which contains fixes for the outstanding comments.
Your contribution is still included (and still credited to you), with the appropriate modifications. Feel free to ask about any of the changes.
Tests pass:
$ TF_ACC=1 envchain azurerm go test ./azurerm -v -timeout 120m -run TestAccAzureRMDnsZone
=== RUN TestAccAzureRMDnsZone_importBasic
--- PASS: TestAccAzureRMDnsZone_importBasic (72.97s)
=== RUN TestAccAzureRMDnsZone_importBasicWithTags
--- PASS: TestAccAzureRMDnsZone_importBasicWithTags (82.94s)
=== RUN TestAccAzureRMDnsZone_basic
--- PASS: TestAccAzureRMDnsZone_basic (61.76s)
=== RUN TestAccAzureRMDnsZone_withTags
--- PASS: TestAccAzureRMDnsZone_withTags (92.15s)
PASS
ok github.com/terraform-providers/terraform-provider-azurerm/azurerm 309.857s
Thanks!
|
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 feel this issue should be reopened, we encourage creating a new issue linking back to this one for added context. If you feel I made an error 🤖 🙉 , please reach out to my human friends 👉 hashibot-feedback@hashicorp.com. Thanks! |
ghost
locked and limited conversation to collaborators
No description provided.