setup.ps1: added ansible_all_ipv4_addresses ansible_all_ipv6_addresses #560
Conversation
|
@jborean93 @nitzmahone sorry for direct pinging. |
There was a problem hiding this comment.
Sorry for the delay in reviewing this, I've been working on a few other things which has taken up my time recently. I'm not opposed to adding the extra ipv4 and ipv6 only addresses but I do have some concerns about moving back to CIM. While it may be faster we've had issues with CIM when it comes to non-admin users as well as corruption in the cache causing either a timeout or a slowdown in the task causing issues for users. We would need to continue using the dotnet API or something else other than CIM that gives us the data that is needed.
It would also be great to figure out why GetAllNetworkInterfaces() is returning more interfaces than the CIM method you have in this PR. What are those extra entries, can they be omitted, can we easily omit them?
Finally we would need to have a changelog fragment added to the PR to document these new return values. More details on this can be found at https://docs.ansible.com/ansible/latest/community/development_process.html#creating-changelog-fragments.
| $_.Address.ToString() -notin @('::1', '127.0.0.1') | ||
| } | ForEach-Object -Process { | ||
| $_.Address.ToString() | ||
| $NetIPAddress = Get-CimInstance -Namespace ROOT/StandardCimv2 -ClassName MSFT_NetIPAddress |
There was a problem hiding this comment.
We are purposefully avoiding WMI/CIM in this module where we can as it won't work when a user is running as a non-admin. The data returned by the dotnet methods should contain enough information in the $_.Address object that tells you whether it's an IPv4 vs IPv6 address. As for why it's returning extra entries compared to the CIM method that might be something you need to investigate on your host to figure out what those extra entries are from and whether we should filter them out or not.
| $_.Address.ToString() | ||
| $NetIPAddress = Get-CimInstance -Namespace ROOT/StandardCimv2 -ClassName MSFT_NetIPAddress | ||
|
|
||
| $ips = @() |
There was a problem hiding this comment.
This is a minor nit but it might be nicer to use a generic list here instead of += with an array. At the number of ips being added it's mostly inconsequential but I find it always nice to avoid += on arrays regardless.
|
This pull request is stale because it has been open for 4 weeks with no activity. Remove stale label or comment or this will be closed in 2 weeks. |
SUMMARY
This changes collects now all_ipv4_addresses and all_ipv6_addresses as facts like on linux.
The Filtering in the Code is a bit ugly but it returns the ansible_ip_addresses fact the same as here https://github.com/ansible/ansible/blob/74f6e6a134fa4fe57a80c6c0748772ac13182f1b
The Script before this PR did return more IPs than the Test here https://github.com/ansible/ansible/blob/74f6e6a134fa4fe57a80c6c0748772ac13182f1b
Test Output:
Before this PR:
This PR:
It does also report ipv4 and ipv6 addresses seperate like on Linux
ISSUE TYPE
COMPONENT NAME
setup
ADDITIONAL INFORMATION
First Pr opened on the wrong Repo ansible/ansible#81991
The Timing got even better with the CIM call instead of the Net Method. (Timing in ms)
net-ps5
net-ps7
cim-ps5
cim-ps7