New data source - azurerm_virtual_machine_scale_set_network_interfaces #6752
Conversation
ghost
added
|
@tombuildsstuff @katbyte - Anyone available to review this? |
|
@tombuildsstuff @katbyte - Anyone available to review this? |
| resGroup := d.Get("resource_group_name").(string) | ||
| vmssName := d.Get("virtual_machine_scale_set_name").(string) | ||
|
|
||
| resp, err := client.ListVirtualMachineScaleSetNetworkInterfaces(ctx, resGroup, vmssName) |
There was a problem hiding this comment.
Since this API is a pagable request, could we switch to the ListVirtualMachineScaleSetNetworkInterfacesComplete method and use the iterator to iterate the result?
|
|
||
| results := make([]interface{}, 0) | ||
|
|
||
| for _, iface := range resp.Values() { |
There was a problem hiding this comment.
Actually the result may have multiple pages, therefore you should add an outer loop to iterate all of the pages.
But it would be more clear if you use the ListVirtualMachineScaleSetNetworkInterfacesComplete and iterate over the iterator.
| if iface.Name != nil { | ||
| result["name"] = *iface.Name | ||
| } else { | ||
| result["name"] = "" | ||
| } |
There was a problem hiding this comment.
Usually we would write this like this:
| if iface.Name != nil { | |
| result["name"] = *iface.Name | |
| } else { | |
| result["name"] = "" | |
| } | |
| name := "" // var name string also works | |
| if iface.Name != nil { | |
| name = *iface.Name | |
| } | |
| // other fields | |
| results = append(results, map[string]interface{}{ | |
| "name": name, | |
| // other fields | |
| }) |
| addresses := make([]interface{}, 0) | ||
| for _, config := range configs { | ||
| if config.InterfaceIPConfigurationPropertiesFormat != nil { | ||
| addresses = append(addresses, config.InterfaceIPConfigurationPropertiesFormat.PrivateIPAddress) | ||
| } | ||
| } |
There was a problem hiding this comment.
Could we please extract this into an flatten function? Which may make this more clear
| var appliedDNSServers []string | ||
| var dnsServers []string | ||
| if dnsSettings := iface.DNSSettings; dnsSettings != nil { | ||
| if s := dnsSettings.AppliedDNSServers; s != nil { | ||
| appliedDNSServers = *s | ||
| } | ||
|
|
||
| if s := dnsSettings.DNSServers; s != nil { | ||
| dnsServers = *s | ||
| } |
There was a problem hiding this comment.
Actually we have a built-in helper function utils.FlattenStringSlice(), could we please use this function instead? By doing this, these nil-checks can be removed.
| result["enable_ip_forwarding"] = *iface.EnableIPForwarding | ||
| result["enable_accelerated_networking"] = *iface.EnableAcceleratedNetworking |
There was a problem hiding this comment.
These two would also need nil-checks
| "dns_servers": { | ||
| Type: schema.TypeSet, | ||
| Computed: true, | ||
| Elem: &schema.Schema{Type: schema.TypeString}, | ||
| Set: schema.HashString, | ||
| }, |
There was a problem hiding this comment.
Since this is a Computed attribute, we should just use TypeList instead of TypeSet. And this would also benefit the users when they are trying to use one of the element.
| "applied_dns_servers": { | ||
| Type: schema.TypeSet, | ||
| Computed: true, | ||
| Elem: &schema.Schema{Type: schema.TypeString}, | ||
| Set: schema.HashString, | ||
| }, |
There was a problem hiding this comment.
Similar with the above, should use TypeList
| "application_gateway_backend_address_pools_ids": { | ||
| Type: schema.TypeSet, | ||
| Computed: true, | ||
| Elem: &schema.Schema{Type: schema.TypeString}, | ||
| Set: schema.HashString, | ||
| }, | ||
|
|
||
| "load_balancer_backend_address_pools_ids": { | ||
| Type: schema.TypeSet, | ||
| Computed: true, | ||
| Elem: &schema.Schema{Type: schema.TypeString}, | ||
| Set: schema.HashString, | ||
| }, | ||
|
|
||
| "load_balancer_inbound_nat_rules_ids": { | ||
| Type: schema.TypeSet, | ||
| Computed: true, | ||
| Elem: &schema.Schema{Type: schema.TypeString}, | ||
| Set: schema.HashString, | ||
| }, | ||
|
|
||
| "application_security_group_ids": { | ||
| Type: schema.TypeSet, | ||
| Computed: true, | ||
| Elem: &schema.Schema{Type: schema.TypeString}, | ||
| Set: schema.HashString, | ||
| }, |
There was a problem hiding this comment.
Since this is a Computed attribute, we should just use TypeList instead of TypeSet. And this would also benefit the users when they are trying to use one of the element.
|
@ArcturusZhang - I think I've addressed everything |
There was a problem hiding this comment.
Hi @hbuckle thanks for the revision! LGTM aside from one extra empty line
azurerm/internal/services/network/virtual_machine_scale_set_network_interfaces_data_source.go
Outdated
Show resolved
Hide resolved
|
This looks like a useful/required PR 👍 |
|
It should be ready to go, @ArcturusZhang can you confirm? |
|
This looks like a really valuable change that is very close to actually happening. Will it be merged or is it advisable to look for workarounds for this instead of waiting for this to be accepted? |
|
I've resolved the merge conflict so just needs a reviewer |
|
@ArcturusZhang @tombuildsstuff bumping this one again |
|
Looks like this fell through the cracks. Any update on this? |
|
Hey @hbuckle - is there a reason for this to be specific to NICs and not a general windows/linux vmss datasource? |
|
@katbyte I think it was following the model of |
|
@hbuckle - i'm still not sure what we gain by having this as a separate resource other then part of existing data source giving us
|
|
@katbyte sure it can go as part of the existing data source. TBH I've forgotten what I even needed this for now. |
|
Sounds good @hbuckle - mind if i close this until you've done so and can open a new PR? |
|
It looks like there is a PR already open to do this! #10585 - as such i'm going to close this in favour of that pr |
|
I'm going to lock this pull request because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active contributions. |
Data source for scale set NICs - particularly useful to get the IP addresses of a scale set
Resolves #7333