Loadbalancer backend address pool #21
Conversation
There was a problem hiding this comment.
Hi @thetonymaster,
Left a few comments inline but this mostly LGTM 👍, my primary concern is the depreciated location property. As it will eventually be removed from azurerm we should probably not add it here in the first place.
| ForceNew: true, | ||
| }, | ||
|
|
||
| "location": deprecatedLocationSchema(), |
There was a problem hiding this comment.
As this is a new resource it might make more sense to remove location instead of deprecating it?
There was a problem hiding this comment.
makes sense, will remove it from now on.
| "github.com/hashicorp/terraform/helper/resource" | ||
| ) | ||
|
|
||
| func TestAccAzureStackLoadBalancerBackEndAddressPool_importBasic(t *testing.T) { |
There was a problem hiding this comment.
Not a blocker, but there is a push to add the import tests as a final step on the regular ones, feel free to do the same as you move resources over as it'll shorten test times & load on the env.
azurestack/location.go
Outdated
| @@ -35,3 +35,14 @@ func azureStackNormalizeLocation(location interface{}) string { | |||
| func azureStackSuppressLocationDiff(k, old, new string, d *schema.ResourceData) bool { | |||
| return azureStackNormalizeLocation(old) == azureStackNormalizeLocation(new) | |||
| } | |||
|
|
|||
| func deprecatedLocationSchema() *schema.Schema { | |||
There was a problem hiding this comment.
We shouldn't need this in azure stack yet.
| } | ||
|
|
||
| var poolId string | ||
| for _, BackendAddressPool := range *(*read.LoadBalancerPropertiesFormat).BackendAddressPools { |
There was a problem hiding this comment.
Could we get a nil check on read.LoadBalancerPropertiesFormat?
| page_title: "Azure Resource Manager: azurestack_lb_backend_address_pool" | ||
| sidebar_current: "docs-azurestack-resource-loadbalancer-backend-address-pool" | ||
| description: |- | ||
| Create a LoadBalancer Backend Address Pool. |
There was a problem hiding this comment.
Could we change to to Manages?
|
|
||
| # azurestack_lb_backend_address_pool | ||
|
|
||
| Create a LoadBalancer Backend Address Pool. |
There was a problem hiding this comment.
Could we change to to Manages?
| }, | ||
| { | ||
| ResourceName: addressPoolName, | ||
| ImportState: true, |
There was a problem hiding this comment.
@katbyte is this what you meant?
|
Let me know when this is ready for another review, currently CI is failing with: |
|
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
No description provided.