search

vmware-archive / powernsx

PowerShell module that abstracts the VMware NSX-v API to a set of easily used PowerShell functions
173 stars 90 forks source link

Feature: add set-nsxfirewallrule appliedTo support #614

Open NamedJason opened 4 years ago

NamedJason commented 4 years ago

Modify set-nsxFirewallRule function to support the AppliedTo field.

alagoutte commented 4 years ago

Hi,

I think, you have make a change on file format, and github diff view it is not happy...

NamedJason commented 4 years ago

I don't really know what I'm doing with git or github, sorry. I see in the "Files Changed" section that it looks like every line has changed; I'm not sure what happened there. I only added a few parameters to the Set-NSXFirewallRule cmdlet (appliedTo, ApplyToDfw, and ApplyToAllEdges) and then a few lines to actually use those parameters (as well as a couple of examples). I used Notepad++ to make my changes; are you aware of any common mistakes while using NPP that might lead to this behavior? I didn't change the file encoding or anything like that, so I'm at a bit of a loss as to what happened...

alagoutte commented 4 years ago

After a quick look, it look the return line change...

I using Visual Code

alagoutte commented 4 years ago

may be try to squash change (or recreate a new change / PR)

NamedJason commented 4 years ago

Ok, I think that I've got it worked out. Maybe the problem was that I originally modified the module file that I got via install-module. I downloaded the module file directly from github, made my changes to it (in this latest version) and it looks like the diff is working now. Has github updated my pull request with this new version automatically or do I need to do something else?

alagoutte commented 4 years ago

Thanks

the diff look better (it may be a good idea to squash commit -> git rebase -i origin and select squash) and repush (git push -f)

Also need to add some test for this new parameter

it will be fix also #532

NamedJason commented 4 years ago

I'm sorry, but I'm not familiar with those commands; my github experience is pretty much limited to the web interface. What tests would you recommend for this parameter? I tried to model this after the AppliedTo implementation in New-NSXFirewallRule, where it looked like most of the error checking was handled by New-NsxAppliedToListNode.

alagoutte commented 4 years ago

I'm sorry, but I'm not familiar with those commands; my github experience is pretty much limited to the web interface. What tests would you recommend for this parameter? I tried to model this after the AppliedTo implementation in New-NSXFirewallRule, where it looked like most of the error checking was handled by New-NsxAppliedToListNode.

Make some change with AppliedTo parameter and check it is "good" with a get

NamedJason commented 4 years ago

I added a check to ensure that New-NsxAppliedToListNode returns a successful result before attempting to overwrite the AppliedTo setting. Is there an example of what you'd like for the get based validation from elsewhere in the Set-NSXFirewallRule cmdlet? I'm a sysadmin and am really outside my normal stomping ground when it comes to software development.

dcoghlan commented 3 years ago

@NamedJason If you can write some tests for this new feature and they pass, I will get this merged into master and it will be included in the next upcoming release.