Add Windows support #79
Conversation
|
|
||
| # find all the files in the fragments directory, sort them numerically and concat to fragments.concat in the working dir | ||
| open('fragments.concat', 'a') do |f| | ||
| Dir.entries("fragments").sort.each{ |entry| |
There was a problem hiding this comment.
This is the default sort, the script should be improved to change the criteria according to SETTINGS[:sortarg].
|
Can I get you to rebase this against master? We've merged in a bunch of syntax/formatting changes and I think this'll significantly shrink the size of this PR. :) |
| $mode = '0644', | ||
| $owner = $concat::params::id, | ||
| $group = $concat::params::root_group, | ||
| $mode = $concat::params::fragment_mode, |
There was a problem hiding this comment.
Since the concat define is the entry point for this module, concat::setup or concat::params won't be loaded unless it's been manually included into the manifest. I'm not a big fan of that and got a PR merged to remove the requirement to do that (#77). This is a limitation of defined types since they don't support inheritance.
| @@ -44,9 +56,11 @@ | |||
| mode => '0750', | |||
| } | |||
|
|
|||
| ## Old versions of this module used a different path. | |||
There was a problem hiding this comment.
If you don't mind lets just remove this entire section, I don't think we care about people with 2-3 year old versions of this module anymore. :)
There was a problem hiding this comment.
How about I submit a separate PR for this since dropping it isn't related to windows support?
There was a problem hiding this comment.
Ok @jhoblitt . I'll wait for your PR to rebase it. Ok?
|
Other than the failures with travis, which I'm sure you'll resolve, I'm definitely happy with the direction this is going. Long term I think we'll move to a type/provider for concat and do away with the script altogether, but this is awesome for our windows users! |
| # can we write to -o? | ||
| if File.file?(SETTINGS[:outfile]) | ||
| if !File.writable?(SETTINGS[:outfile]) | ||
| raise "Cannot write to %s" % [SETTINGS[:outfile]] |
There was a problem hiding this comment.
Interpolation is preferred over format string, eg "Cannot write to #{SETTINGS[:outfile]}"
There was a problem hiding this comment.
Thanks. I'm not a ruby expert, so these kind of things are really welcome. I'll update it for the next rebase.
|
Code rebased, next round will be to improve ruby script according to @adrienthebo recommendations. |
|
Thanks for being so fast with these changes! Looking good, once I see the code changes I'll make sure travis is happy then get this merged. |
| @@ -191,7 +193,7 @@ | |||
| exec { "concat_${name}": | |||
| alias => "concat_${fragdir}", | |||
| command => 'true', | |||
There was a problem hiding this comment.
I've only presently have a winxp test VM up and it's cmd.exe shell does not seem to have true. Have you tested file removal on windows?
There was a problem hiding this comment.
Ummm good catch. Have to take a look to this scenario... the idea is just to return true?
There was a problem hiding this comment.
Just to return any "noop" in this case really, doesn't have to be true, could be something else for windows. Really we need to work out how to refactor so there's not a massive if/else with fake resources, but we can do that as a followup to this work.
- It adds a ruby version of the bash script. - Refactor setup.pp to include new variables. - Generalizes command execution according to variables in setup.pp.
|
New version rebased:
|
| OptionParser.new do |opts| | ||
| opts.banner = "Usage: #{$0} [options]" | ||
| opts.separator "" | ||
| opts.separator "Specific options:" |
There was a problem hiding this comment.
#separator is being called twice.
|
@luisfdez Thank you for all your effort on this PR! It's looking really good now, we're down to just very minor nit-picks now. |
|
@jhoblitt you're welcome 👍 , it's worth to have your module available for windows machines. I've just added a commit to remove the duplicated call. |
|
The rspec-system tests passed with the @luisfdez Thank you again for this PR! Would you be willing to follow up with another PR to add spec coverage for |
|
@jhoblitt & others, thanks for your time! It's great to have this patch merged. Regarding the spec coverage, it's a good idea. Could you give some guidelines about how to structure the code mixing platforms? |
Hi.
I've created a patch to add support for windows hosts, aimed to close #49. The basics of the patch are:
Creates a new params.pp to store platform specific variables.(file mode, owners, paths, names,...)
Refactor to point variable references to params.pp.params.ppsetup.pp.I'd like your input/test in this modifications.
The ruby script is a first version trying to match 1-1 the bash structure. Anyway, it needs to be polished and improved as not all the ordering options available in .sh are implemented.