(22617) In Augeas onlyif, compare number strings numerically

[donnieblaw]

Without this patch applied, onlyif comparitors <, >, <=, and >= always
do string comparisions, even when the strings are numbers. This
makes it impossible to enforce relative policy, such as:

onlyif => 'get PASS_MAX_DAYS > 90',

in login.defs, for example. If PASS_MAX_DAYS had the value 120, the
onlyif would evaluate false (not need-to-run). See ticket for more details.

Without this patch, onlyif uses string comparison, such that 120 < 90.

This patch converts strings to integers and uses numeric comparison
when both operands of the relative comparitor are valid numbers.

This patch also fixes another bug that isn't reported in a ticket, namely
that the parser erroneously converts ">=" to ">" and "<=" to "<".

[puppetcla]

CLA signed by all contributors.

[adrienthebo]

Why do we always need to convert this value to a string? What class is s?

[donnieblaw]

It's just defensive programming. For this change, the to_s is probably not relevant. However, if someone makes use of is_numeric?() in the future and passes a nil, it will not throw an exception, but rather return false.

[adrienthebo]

While there is a place for defensive programming, it also adds overhead and obfuscates the code. I would rather have this be specific in what it accepts and ensure that we only give it the correct data rather than always guess.

If there is a demonstrable chance that nil will be passed, it would be better to do something like this:

case s
when NilClass
  false
when String
  # regex
when Fixnum
  true
end

[donnieblaw]

OK - that makes it even stronger - I'll make this change.

[adrienthebo]

Since the array in this line is a standalone value and we already have to wrap this line, could this be extracted to either a class constant or a variable in this method?

[donnieblaw]

How would that improve the code?

I prefer it the way it is. You can see what the conditional is doing without having to go look for the value of a constant.

[adrienthebo]

We generally try to avoid overly long lines, but splitting conditionals across multiple lines isn't much better. We don't have to extract this to a constant (although doing so would remove the number of objects being garbage collected) but moving it to a variable would make the line shorter, like so:

comparators = ['<', '<=', '>=', '>']
result = @aug.get(path) || ''

case comparator
if comparators.include? comparator and # ...

But admittedly this is a small detail, so to avoid bikeshedding if you feel strongly about this, then it's fine. :)

[adrienthebo]

Style nitpick: in case statements, when should be indented at the same level as case: https://github.com/styleguide/ruby

[donnieblaw]

OK - will fix.

[adrienthebo]

This pull request is ready to be merged, but right now the build is failing
(http://goo.gl/HY6uzt). As soon as the build starts passing again we should be
able to merge this.

[adrienthebo]

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