Loadbalancer probe #23
Conversation
There was a problem hiding this comment.
Hi @thetonymaster,
as with lb nat rule this mostly LGTM, i've let some similar comments inline to address and again backported them to azurerm here
| ImportState: true, | ||
| ImportStateVerify: true, | ||
| // location is deprecated and was never actually used | ||
| ImportStateVerifyIgnore: []string{"location"}, |
There was a problem hiding this comment.
As we have removed the deprecate property, we can probably remove this line.
| "github.com/hashicorp/terraform/helper/resource" | ||
| ) | ||
|
|
||
| func TestAccAzureStackLoadBalancerProbe_importBasic(t *testing.T) { |
There was a problem hiding this comment.
As with the other resource, feel free to move this import test to the end of the regular test.
| return &schema.Resource{ | ||
| Create: resourceArmLoadBalancerProbeCreate, | ||
| Read: resourceArmLoadBalancerProbeRead, | ||
| Update: resourceArmLoadBalancerProbeCreate, |
There was a problem hiding this comment.
Could we change this to resourceArmLoadBalancerProbeCreateUpdate?
| Type: schema.TypeString, | ||
| Required: true, | ||
| ForceNew: true, | ||
| }, |
There was a problem hiding this comment.
Could we get some validation here? at the very least a ValidateFunc: validation.NoZeroValues,
| Type: schema.TypeString, | ||
| Required: true, | ||
| ForceNew: true, | ||
| }, |
There was a problem hiding this comment.
As well here: ValidateFunc: azure.ValidateResourceID,
|
|
||
| d.Set("name", config.Name) | ||
| d.Set("resource_group_name", id.ResourceGroup) | ||
| d.Set("protocol", config.ProbePropertiesFormat.Protocol) |
There was a problem hiding this comment.
Could we get a nil check on config.ProbePropertiesFormat?
if properties := config.ProbePropertiesFormat; properties != nil {
d.Set("protocol", properties.Protocol)
d.Set("interval_in_seconds", properties.IntervalInSeconds)
d.Set("number_of_probes", properties.NumberOfProbes)
d.Set("port", properties.Port)
d.Set("request_path", properties.RequestPath)
}| d.Set("port", config.ProbePropertiesFormat.Port) | ||
| d.Set("request_path", config.ProbePropertiesFormat.RequestPath) | ||
|
|
||
| var load_balancer_rules []string |
There was a problem hiding this comment.
And add nill checks here too:
var load_balancer_rules []string
if rules := properties.LoadBalancingRules; rules != nil {
for _, ruleConfig := range *rules {
if id := ruleConfig.ID; id != nil {
load_balancer_rules = append(load_balancer_rules, *id)
}
}
}
d.Set("load_balancer_rules", load_balancer_rules)| return nil | ||
| } | ||
|
|
||
| func expandAzureRmLoadBalancerProbe(d *schema.ResourceData, lb *network.LoadBalancer) (*network.Probe, error) { |
There was a problem hiding this comment.
This function never returns an error so we can remove that return value.
It also doesn't use lb *network.LoadBalancer so it could also be removed.
| ProbePropertiesFormat: &properties, | ||
| } | ||
|
|
||
| return &probe, nil |
There was a problem hiding this comment.
This could be written as:
return &network.Probe{
Name: utils.String(d.Get("name").(string)),
ProbePropertiesFormat: &properties,
}| Type: schema.TypeInt, | ||
| Optional: true, | ||
| Default: 15, | ||
| }, |
There was a problem hiding this comment.
Could we get some validation here too?
ValidateFunc: validation.IntAtLeast(5),
| StateFunc: ignoreCaseStateFunc, | ||
| DiffSuppressFunc: ignoreCaseDiffSuppressFunc, | ||
| ValidateFunc: validation.StringInSlice([]string{ | ||
| "Tcp", |
There was a problem hiding this comment.
Could we please use the SDK values here? ie string(network.ProbeProtocolHTTP),
|
Tests all 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.