-
Notifications
You must be signed in to change notification settings - Fork 89
Feature: add set-nsxfirewallrule appliedTo support #614
base: master
Are you sure you want to change the base?
Feature: add set-nsxfirewallrule appliedTo support #614
Conversation
Hi, I think, you have make a change on file format, and github diff view it is not happy... |
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... |
After a quick look, it look the return line change... I using Visual Code |
may be try to squash change (or recreate a new change / PR) |
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? |
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 |
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 |
Checks for successful output from New-NsxAppliedToListNode before attempting to overwrite the AppliedTo field.
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. |
@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. |
Modify set-nsxFirewallRule function to support the AppliedTo field.