search

chef-boneyard / windows_firewall

Chef cookbook to configure Windows Firewall
Apache License 2.0
6 stars 9 forks source link

more arguments added #2

Closed chlsmith closed 9 years ago

chlsmith commented 9 years ago

I added more arguments so I could create more specific Windows Firewall rules. I still need to figure out how to make it work when the protocol is ICMPv4. I can add it as an option and all, but the netsh command has different required options when you have ICMPv4 as the protocol.

This is the first one I've ever messed with, so I'm just learning. Maybe you can make use of some of the options I added.

mattstratton commented 9 years ago

One issue I see here is that you've put in a recipe in the cookbook itself just for testing purposes. For resource-only cookbooks, the pattern is to create fixture cookbooks for use in testing; this is the pattern @someara demonstrates in the https://github.com/chef-cookbooks/httpd cookbook.

If you take a look at my branch over here - https://github.com/mattstratton/windows_firewall/tree/version-2 you'll see a bit of how I did this.

The question is how we want to handle the merge. I'll submit a PR for what I have so far to the branch that @lynx44 created for your PR and we can see how crazy the diff is :)

chlsmith commented 9 years ago

It's my first github thing ever....feel free to beat the crap out of whatever is wrong with it. :)

On Fri, Feb 6, 2015 at 12:18 AM, Matt Stratton notifications@github.com wrote:

One issue I see here is that you've put in a recipe in the cookbook itself just for testing purposes. For resource-only cookbooks, the pattern is to create fixture cookbooks for use in testing; this is the pattern @someara https://github.com/someara demonstrates in the https://github.com/chef-cookbooks/httpd cookbook.

If you take a look at my branch over here - https://github.com/mattstratton/windows_firewall/tree/ip_address you'll see a bit of how I did this.

The question is how we want to handle the merge. I'll submit a PR for what I have so far to the branch that @lynx44 https://github.com/lynx44 created for your PR and we can see how crazy the diff is :)

— Reply to this email directly or view it on GitHub https://github.com/lynx44/windows_firewall/pull/2#issuecomment-73184350.

mattstratton commented 9 years ago

I would say at this point we're just waiting for @lynx44 to pull in my PR and then I can pull that branch down and see what we've got :)

mattstratton commented 9 years ago

I merged down your commits and will do some manual cleanup and testing. I suggest that this PR be closed and I'll submit a new PR that includes @chlsmith changes and my own.

mattstratton commented 9 years ago

I've submitted a new PR #5 with your stuff merged into mine. This PR should probably be closed by @lynx44 after reviewing the new PR.