Add OSX support for host resource #1608
Conversation
RyanJarv
commented
Mar 30, 2017
RyanJarv
commented
- Ping on OSX uses -W instead of -w
- Can use nc for testing TCP connections. There is also a UDP mode for this but doesn't seem to work (and would be unreliable anyways) so following the behavior on windows here.
Signed-off-by: Ryan Gerstenkorn <ryan_gerstenkorn@fastmail.fm>
There was a problem hiding this comment.
Thanks for your submission, @RyanJarv! This looks like a reasonable approach to me, and the TCP check is something we need to do for Linux as well.
We need to get your tests green, and it appears it should hopefully be as easy as fixing the parameter named for your #ping method.
lib/resources/host.rb
Outdated
| @@ -93,6 +95,30 @@ def initialize(inspec) | |||
| end | |||
| end | |||
|
|
|||
| class DarwinHostProvider < HostProvider | |||
| def ping(hostname, _port = nil, _proto = nil) | |||
There was a problem hiding this comment.
Prefixing a parameter name with an underscore is generally used to indicate that the parameter is not going to be used. Now that we're using it, we should change _proto to proto, and _port to port. This is a Rubocop check, and appears to be the reason why the Travis tests are failing for your change, as well. 🙂
There was a problem hiding this comment.
@RyanJarv Thank you for this great addition! I added a minor improvement request.
| @@ -48,6 +48,8 @@ def initialize(hostname, params = {}) | |||
| @host_provider = LinuxHostProvider.new(inspec) | |||
| elsif inspec.os.windows? | |||
| @host_provider = WindowsHostProvider.new(inspec) | |||
| elsif inspec.os.darwin? | |||
There was a problem hiding this comment.
We should add a check for the proto and skip the resource if udp is used, since we do not support if for darwin. Same should be true for Linux but this is not related to this PR.
There was a problem hiding this comment.
I just copied this behavior from the windows resource, but yeah I see what you're saying.. Going to put a skip_resource in the beginning of the initialize for UDP since this isn't supported on any platform if that makes sense.
There was a problem hiding this comment.
Also I kinda feel like you shouldn't really be checking for UDP remotely.. just going to lead to confusing results half the time. But that's kinda a different issue.
Signed-off-by: Ryan Gerstenkorn <ryan_gerstenkorn@fastmail.fm>
Signed-off-by: Ryan Gerstenkorn <ryan_gerstenkorn@fastmail.fm>
Signed-off-by: Ryan Gerstenkorn <ryan_gerstenkorn@fastmail.fm>
|
Thanks! Pushed a change for the variable names (guess I was looking at Edit: nvm |
|
Looks like the travis build fail is unrelated? Not getting that running the tests locally. |
|
@RyanJarv Yes, likely unrelated. The functional tests are a bit fragile at the moment. I've rerun the Travis job. |
|
Thanks for your contribution @RyanJarv! |
