In ApacheConf#include_files, check for abs paths

[davidcpell]

If the path is absolute, just use what was passed, otherwise build an
absolute path using @conf_dir.

Fixes #1013

[stevendanna]

Can we use File.expand_path here?

irb(main):027:0> File.expand_path("foo", "/bar/baz")
=> "/bar/baz/foo"
irb(main):028:0> File.expand_path("/foo", "/bar/baz")
=> "/foo"

[stevendanna]

The tests appear to be working now. Left a minor comment, otherwise this looks reasonable to me. 👍

[davidcpell]

@stevendanna oops, yeah I made a typo in the first commit that was resulting in the test failure and I forgot to comment that I had fixed it after doing so.

Also, good thought with File::expand_path. Happy to avoid a conditional if possible. I just pushed another commit with that change because the tests were passing on my machine but they seem to be failing on AppVeyor (on Windows platform?).

[stevendanna]

Also, good thought with File::expand_path. Happy to avoid a conditional if possible. I just pushed another commit with that change because the tests were passing on my machine but they seem to be failing on AppVeyor (on Windows platform?).

AppVeyor does indeed run the test suite on Windows. What I think may be happening (but haven't confirmed yet) is that File.expand_path is adding C:/ or similar to the path, which means it isn't being caught by the mock loader.

[chris-rock]

Not tested it, but I looks like we need to mock the files for Windows as well with c:. I can help on that later this week

[davidcpell]

@chris-rock @stevendanna do you know of a good windows server box I can use locally so I don't have to test by pushing here every time I change something? I didn't see a bento windows box

[chris-rock]

I have really good experiences with the boxes from @mwrock https://atlas.hashicorp.com/mwrock

[davidcpell]

@chris-rock @stevendanna am I understanding correctly that for this to work all of the mock files would need duplicates prepended with c:\? would it be preferable to modify the result of File.expand_path after the fact with something like .gsub('C:/', '')?

[davidcpell]

@chris-rock @stevendanna any thought on my last comment?

[stevendanna]

am I understanding correctly that for this to work all of the mock files would need duplicates prepended with c:?

I feel like that is the best path.

would it be preferable to modify the result of File.expand_path after the fact with something like .gsub('C:/', '')?

I think that path is fraught with peril because we might be passed paths that end up pointing at other drives and then we'll get into trying to parse it out correctly, whereas I'd prefer we just sort out any problems we hit with using expand_path on windows as we find them.

[davidcpell]

@stevendanna @chris-rock my most recent commit (duplicating mock files and prepending with c:\) seems to have bombed. Example failure:

error: unable to create file test/unit/mock/files/c:\apache2.conf (Invalid argument)

Any idea what exactly AppVeyor is complaining about? Is there something else I should add? I went with the windows-style backslash instead of forward slash b/c of linux filenames and forward slashes (though assuming that Windows may be having a problem with the backslash?).

[chris-rock]

@davidcpell I apologize for the late response, we are quite busy with InSpec 1.0 preparation. I took a look on your PR:

From: C:/Users/chris/development/inspec/lib/resources/apache_conf.rb @ line 109 Inspec::Resources::ApacheConf#include_files:

    101: def include_files(params)
    102:   # see if there is more config files to include
    103:   include_files = params['Include'] || []
    104:   include_files_optional = params['IncludeOptional'] || []
    105:
    106:   includes = []
    107:   (include_files + include_files_optional).each do |f|
    108:     # id    = Pathname.new(f).absolute? ? f : File.join(@conf_dir, f)
 => 109:     require "pry"; binding.pry;
    110:     id    = File.expand_path(f, @conf_dir)
    111:     files = find_files(id, depth: 1, type: 'file')
    112:
    113:     includes.push(files) if files
    114:   end
    115:
    116:   # [].flatten! == nil
    117:   includes.flatten! || []
    118: end
[1] pry(#<#<Class:0x00000004ad9c70>>)> f
=> "ports.conf"
[2] pry(#<#<Class:0x00000004ad9c70>>)> @conf_dir
=> "/etc/apache2/"
[3] pry(#<#<Class:0x00000004ad9c70>>)> File.expand_path(f, @conf_dir)
=> "C:/etc/apache2/ports.conf"
[4] pry(#<#<Class:0x00000004ad9c70>>)> Pathname.new(f).absolute?
=> false
[5] pry(#<#<Class:0x00000004ad9c70>>)> File.join(@conf_dir, f)
=> "/etc/apache2/ports.conf"
[6] pry(#<#<Class:0x00000004ad9c70>>)>

We cannot use File.expand_path(f, @conf_dir), since its injecting platform specific behavior (in this case Windows) of the ruby runtime into content that is on a (possibly) remote target. Therefore we should go with File.join(@conf_dir, f), since it is safe for the remote platform. Tests should turn green then again. We do not need commit 67ea41882932bdccfae91e682d143692aca4e337 then anymore. Could you rebase on master as well?

[davidcpell]

@chris-rock thanks for the feedback. I don't think File.join is going to work (by itself) because it doesn't handle absolute paths from the conf file:

    101: def include_files(params)
    102:   # see if there is more config files to include
    103:   include_files = params['Include'] || []
    104:   include_files_optional = params['IncludeOptional'] || []
    105:
    106:   includes = []
    107:   (include_files + include_files_optional).each do |f|
    108:     id    = File.join(@conf_dir, f)
 => 109:     require 'pry'; binding.pry
    110:     files = find_files(id, depth: 1, type: 'file')
    111:
    112:     includes.push(files) if files
    113:   end
    114:
    115:   # [].flatten! == nil
    116:   includes.flatten! || []
    117: end

[1] pry(#<#<Class:0x007ff6e3f8c0c8>>)> f
=> "/etc/httpd/mods-enabled/*.conf"
[2] pry(#<#<Class:0x007ff6e3f8c0c8>>)> id
=> "/etc/httpd/etc/httpd/mods-enabled/*.conf"  # /etc/httpd/ is duplicated
[3] pry(#<#<Class:0x007f97df1d0ac8>>)> File.join(f, @conf_dir)
=> "/etc/httpd/mods-enabled/*.conf/etc/httpd/"  # /etc/httpd tacked onto the end

So far the only solution that has handled absolute paths coming in from the conf file and is also multi-platform was my original submission (that used File.join(@conf_dir, f)), but that included a conditional statement...

Pathname.new(f).absolute? ? f : File.join(@conf_dir, f)

[chris-rock]

@davidcpell lets go for this solution then

[davidcpell]

@chris-rock I rebased with master and squashed my commits to implement the solution using Pathname#absolute? and File::join. It passed with AppVeyor, however there was one failure in one of the Travis builds:

1) Failure:
inspec exec::when using profiles on the supermarket#test_0001_can run supermarket profiles directly from the command line [/home/travis/build/chef/inspec/test/functional/inspec_exec_test.rb:200]:
Expected "[2016-09-20T13:16:12+00:00] WARN: URL target https://github.com/nathenharvey/tmp_compliance_profile/ transformed to https://github.com/nathenharvey/tmp_compliance_profile/archive/master.tar.gz. Consider using the git fetcher\n" to include "Summary: \e[32m2 successful\e[0m, \e[31m0 failures\e[0m, \e[37m0 skipped\e[0m\n".
105 runs, 422 assertions, 1 failures, 0 errors, 0 skips

Not sure what that test is or whether it could've been affected by the change. Do you have any insight?

[chris-rock]

Let me double-check...

[chris-rock]

💯 Thanks @davidcpell this is a great improvement!