(#4424) Add umask parameter to Exec type

[jjulien]

Setting a umask value for execs can be useful when the exec creates files or directories. Relying on the sytem umask can produce unexpected results as it may be different from system to system or just not what the user intends.

[puppetcla]

CLA signed by all contributors.

[adrienthebo]

Why does this need to be cast to a string and back? Is this to handle numeric values that don't have a leading zero?

[jjulien]

The Integer type does not support converting bases with to_i. String does, so the conversion to_s is to provide a way to switch bases. However, I do think this validation needs a slight rework because if the value is Numeric it does not test that it is a valid octal number. The numerical test does not check that the number provided follows the octal regex, and to_i drops any digit that is not valid to the provided base. (ie, '28'.to_i(8) becomes 2. The 8 is invalid and thus silently dropped.)

Through further testing I'm also not sure that this section of code even gets hit since it appears Puppet is converting all parameter values to a string. I seem to be unable to get a Numeric value to show up here to test the code execution. I'm the paranoid type and wonder what might happen one day if Puppet started passing in the value as a number (ie. umask => 0028) If that code were passed as numeric, it would not be caught by the current validation and would end up being a mask of 0020 since to_i drops the invalid 8. So I think the code should look something like

munge do |value|
  if value.is_a? Numeric then
    value = value.to_s
  end
  if value =~ /^0?[0-7]{1,4}$/ then
    return value.to_i(8)
  else
    raise Puppet::Error, "The umask specification is invalid: #{value.inspect}" 
  end

Unless you feel Puppet will always provide this as a string, in which case just the if/else would suffice.

[jjulien]

New code submitted, please review. Thanks!

[adrienthebo]

If the code path for Numeric values are currently never traversed, then let's pull that out. The unit tests you've added should catch that regression.

Also, one small comment - the then clause for conditionals is optional, and doesn't match the rest of the Puppet code base. It helps keep the code base consistent to avoid optional syntax like this, so they should be removed.

[adrienthebo]

expectations that code should raise an error should also check against the error message, to make sure that both the right error type is thrown, and it's thrown from the right place:

expect { }.to raise_error(Puppet::ResourceError, /umask specification is invalid/)

[adrienthebo]

summary: merged into master in 092f9df; this should be released in 3.4.0. Thanks again for the contribution!