Added domain_name_label to read to fix import. (#784) #1287
Conversation
There was a problem hiding this comment.
Hi @shanepoint,
Thank you for opening this PR. This LGTM aside from one thing: the dns_name_label used for testing should be random to prevent conflicts.
Look forward to merging this once thats fixed 🙂
| @@ -46,6 +46,7 @@ func resourceArmPublicIp() *schema.Resource { | |||
|
|
|||
| "zones": singleZonesSchema(), | |||
|
|
|||
| //should this perhaps be allocation_method? | |||
There was a problem hiding this comment.
I'd kind of agree with this, but I don't think it's one for this PR.
It's worth noting that IPv4 and IPv6 will both be supported here at some point in the future (the SDK supports it today, but the underlying Azure infra didn't last time I tried it) - so it may make sense to rename/differentiate them at that point.
| testCheckAzureRMPublicIpExists(resourceName), | ||
| resource.TestCheckResourceAttrSet(resourceName, "ip_address"), | ||
| resource.TestCheckResourceAttr(resourceName, "public_ip_address_allocation", "static"), | ||
| resource.TestCheckResourceAttr(resourceName, "domain_name_label", "dnstestlabel"), |
There was a problem hiding this comment.
Could we please make this random to prevent conflicts?
There was a problem hiding this comment.
hey @shanepoint
Thanks for this PR - I've added couple of minor comments in addition to what @katbyte mentioned - but this is otherwise looking good :)
Thanks!
azurerm/import_arm_public_ip_test.go
Outdated
| resourceName := "azurerm_public_ip.test" | ||
|
|
||
| ri := acctest.RandInt() | ||
| dnl := fmt.Sprintf("tfacc%d", ri) |
There was a problem hiding this comment.
minor can we make this prefix acctest so that it's picked up by our sweepers? (these try and automatically delete any spurious test resources)
| @@ -46,6 +46,7 @@ func resourceArmPublicIp() *schema.Resource { | |||
|
|
|||
| "zones": singleZonesSchema(), | |||
|
|
|||
| //should this perhaps be allocation_method? | |||
There was a problem hiding this comment.
I'd kind of agree with this, but I don't think it's one for this PR.
It's worth noting that IPv4 and IPv6 will both be supported here at some point in the future (the SDK supports it today, but the underlying Azure infra didn't last time I tried it) - so it may make sense to rename/differentiate them at that point.
azurerm/resource_arm_public_ip.go
Outdated
| d.Set("domain_name_label", dnl) | ||
| } else { | ||
| d.Set("domain_name_label", "") | ||
| } |
There was a problem hiding this comment.
minor given this is returning a *string - d.Set will take care of this for us, thus we can consolidate this down to:
d.Set("domain_name_label", settings.DomainNameLabel)
There was a problem hiding this comment.
Hey @shanepoint,
Thank you for the changes! This LGTM now 👍
|
Tests pass: |
|
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 👉 [email protected]. Thanks! |
ghost
locked and limited conversation to collaborators
Fixes #784