(#20607) - do not fail saving lastrunfile when mode is specified

[masterzen]

The current settings system to specify the mode of "settings" file
is quite forgiving, but the method (replace_File) used to save
the last run summary file wasn't.
Hopefully with the help of the symbolic_file_mode system, we're now
able to send replace_file a correct file permission mode.

Signed-off-by: Brice Figureau brice-puppet@daysofwonder.com

[puppetcla]

CLA Signed by masterzen on 2010-08-14 21:00:00 -0700

[Sharpie]

This is a great patch that solves the specific problem with lastrunfile. However, it seems to me that the best fix would be to have the method Puppet::Util.replace_file perform validation and conversion using the routines from puppet/util/symbolic_file_mode.

This way, we provide file mode validation for all replace_file calls instead of just one particular instance.

[masterzen]

Well, that would change the replace_file API (is it public?), it would then require a string instead of an octal number (as it is now).
Note also that all usages of replace_file I could find in the puppet codebase use hardcoded octal values.

But, one thing that could be done is that there are absolutely no mode validation in the settings system (because it is handled in the file resource when the file is created (as was the last run file done beforehand)). We could add the validation in the settings subsystem.

[slippycheeze]

Well, that would change the replace_file API (is it public?), it would then require a string instead of an octal number (as it is now).

Detecting the input format and accepting both Integer and String
arguments would be both reasonable, and efficient enough given the
inherent delays in any replace_file call. A handful of CPU cycles
checking the type tag will not hurt when you have to wait on disk to
ensure content replacement is correct.

[masterzen]

@daniel-pittman I wasn't really concerned by the performance :)
Would you advise changing all call-site to move to a string version of the octal permissions?
Otherwise there's no way to check the mode is correctly written when dealing with only a Fixnum.

[slippycheeze]

Would you advise changing all call-site to move to a string version of the
octal permissions?

Nope.

Otherwise there's no way to check the mode is correctly written when dealing
with only a Fixnum.

Which is the present situation, no? I think we can rely on human
error checking to handle that, since it should only be used
internally, not by random folks out in the world without some
intermediary.

[masterzen]

Also one road-block ahead is that replace_file is part of Puppet::Util which might not work ideally when mixing-in with puppet/util/symbolic_file_mode. So I'll have to refactor this and mode replace_file to its own module/namespace.
I'll publish a revised patch and we'll see how it goes.

[masterzen]

OK, I've rewritten the patch to enhance replace_file so that it can receive any kind of permissions (ie number, octal number as a string, and symbolic mode).
Please review.

[adrienthebo]

summary: merged into master in 9744456; this should be released in 3.3.0. Thanks!

[masterzen]

Apparently this broke the windows tests, which I'm afraid I don't have any way to run.
I'll have a look to see how I can fix that.