On 27/04/11 05:56, Mike wrote:
> I'm guessing that this has slipped through a filter somewhere, but I'm
> hoping it could be reviewed and included soon.

Apologies Mike, I've been waiting for a chance.  Now I'm at Puppet Camp,
I have free time on my hands :-)

Comments inline:

>> +            saved_files.each do |tmp_file|
>> +              saved_file = tmp_file.sub(/^\/files/, '')
>> +              info(diff(saved_file, saved_file + ".augnew"))
>> +              if Puppet[:noop]
>> +                File.delete(saved_file + ".augnew")
>> +              end
>> +            end

This diff works slightly differently to the file type, could they be
made consistent?  See lib/puppet/type/file/content.rb.

Specifically, the file type checks the show_diff setting and uses
"print" instead of "info".

>>      ensure
>> -      close_augeas
>> +      if Puppet[:noop]
>> +        close_augeas
>> +      end
>>      end

The close_augeas method should also be called when no files are changed
(return_value is false) or when in noop.

Also when checking noop, it should check the resource noop? method which
checks the resource's noop attribute and the global setting.  I'd
suggest this:

  if not return_value or resource.noop?
    close_augeas
  end

Otherwise works and looks great - thanks for patching it!

Cheers,

-- 
Dominic Cleal
Red Hat Consulting
m: +44 (0)7818 512168

-- 
You received this message because you are subscribed to the Google Groups 
"Puppet Developers" group.
To post to this group, send email to [email protected].
To unsubscribe from this group, send email to 
[email protected].
For more options, visit this group at 
http://groups.google.com/group/puppet-dev?hl=en.

Reply via email to