Skip to content
This repository has been archived by the owner on Oct 28, 2022. It is now read-only.

Feature: add set-nsxfirewallrule appliedTo support #614

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

NamedJason
Copy link

Modify set-nsxFirewallRule function to support the AppliedTo field.

@alagoutte
Copy link
Contributor

Hi,

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

@NamedJason
Copy link
Author

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
Copy link
Contributor

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

I using Visual Code

@alagoutte
Copy link
Contributor

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

@NamedJason
Copy link
Author

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
Copy link
Contributor

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
Copy link
Author

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
Copy link
Contributor

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.
@NamedJason
Copy link
Author

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 dcoghlan added the Test Required Tests are required before this request will be merged. label Mar 7, 2021
@dcoghlan
Copy link
Contributor

dcoghlan commented Mar 7, 2021

@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.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Test Required Tests are required before this request will be merged.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants