Test all parameters

[lorengordon]

Previously, xIPAddress only checked whether the configured IP Address matched the IPAddress parameter. This pull request checks all parameters against the existing configuration.

Fixes #9

[msftclas]

Hi @lorengordon, I'm your friendly neighborhood Microsoft Pull Request Bot (You can call me MSBOT). Thanks for your contribution!

In order for us to evaluate and accept your PR, we ask that you sign a contribution license agreement. It's all electronic and will take just minutes. I promise there's no faxing. https://cla.microsoft.com.

TTYL, MSBOT;

[msftclas]

@lorengordon, Thanks for signing the contribution license agreement so quickly! Actual humans will now validate the agreement and then evaluate the PR.

Thanks, MSBOT;

[tysonjhayes]

This should probably be changed to $IPAddress and the line above it $ip = $IPAddress should just be removed.

[tysonjhayes]

I'm not going to lie the more I dive into this the more frustrated I am with this resource. Your commits are great in that they are targeted to what you wanted to add in the given context, but it's making me believe a more fundemental rewrite is needed of this resource.

One of the things I would look to address is handling more then one IP address being returned. Even if it's just to throw and bail out.

[lorengordon]

Sigh. I was really trying to scope my edits just enough to deal with my use case (plus compliance with style guide). Several of your comments are on bits that were in place previously. I can take a stab at a larger rewrite, but I won't be able to get to it for another week or two.

[tysonjhayes]

Actually to fix the multiple IP addresses that could come back why don't we just add the IPAddress Parameter to this call? This would filter down to the one address that we're looking to manage.

[lorengordon]

I don't want to break the use case where this resource is trying to change the IP address. Scoping only to the IP address we receive as a parameter will cause this call to return nothing. Which might be fine. I'll look at the subsequent checks and think about it some more.

[tysonjhayes]

In theory if the IP address doesn't exist are we actually in compliance? In theory we could design this resource to accept one IP and only one IP. If the IP doesn't exist, add it. If we're trying to change the IP we could call this resource twice (once with the Ensure = 'Absent') to remove the IP address we are looking to "change."

[lorengordon]

I may be misunderstanding the comment, but this raises and interesting issue with state design. I don't always know the prior state, only what I want the state to be. I'm not necessarily intentionally changing the IP, I'm simply specifying the interface configuration I want, and I expect the resource to do all the work to make it so. If I have to know the prior state, then to me there's little benefit to using the stateful functions--I might as well just use the regular PowerShell commandlets.

[tysonjhayes]

I think it's fantastic that you are trying to scope your edits. That's really what we actually want. I'm frustrated at the design of this resource and think it might make more sense to take a step back and rethink it. You've done good work so far, don't let my comments disuade you of that!

[lorengordon]

I can easily take care of the nits and add tests to make sure the mocks are called. Probably knock that out tomorrow afternoon.

I don't mind taking a stab at a larger rewrite of the resource, but I'd prefer to do that in another pull request, if that's alright with you.

[tysonjhayes]

That's great to me, if we're doing a larger rewrite we'll want more eyes on it anyway. 👍

[lorengordon]

Alright, I fixed the nits, added tests to ensure the mocks are called, and I studied the data model for Get-NetIPAddress and Get-NetRoute to try to better support cases where multiple IP addresses or default gateways are present. The behavior currently only supports managing a single IP and gateway -- meaning that any other IP or gateway associated with that interface will be removed. However, I added a switch $NoReset that is internal only for now (pending the larger rewrite), which we can expose later to support managing multiple IP addresses or gateways on one interface. If $NoReset=$true, then other IP addresses or gateways should be left alone.

[tysonjhayes]

@lorengordon This looks pretty good to me, they've given me merge powers, any objections to merging this? I'm going to open some discussions on where we want to take this resource with some of my ideas early next week so let's hold off on any major rewrites.

[lorengordon]

@tysonjhayes, no objections. 😁

[lorengordon]

Oh geez, this is embarrassing, I left a few write-verbose statements in there from when I was checking the behavior around the NoReset flag. I'll clean that up really quick...

[lorengordon]

Ok, all better.

[tysonjhayes]

haha. Totally missed those when I looked. Good catch. 🚢