fix of a comparison of an integer to a non-integer in centos7

thias / puppet-postfix

Puppet Postfix module

Other

17 stars 117 forks source link

fix of a comparison of an integer to a non-integer in centos7 #63

Closed jvigneux closed 8 years ago

jvigneux commented 9 years ago

fix of a comparison of an integer to a non-integer in centos7 (operatingsystemrelease vs operatingsystemmajrelease)

thias commented 9 years ago

Could you tell me which versions of puppet and facter you are using, and which parser if puppet < 4 (current or future)?

With puppet 3.7.5 and facter 2.4.1 I see both facts as strings, and haven't encountered any issues using either with versioncmp(). I tend the prefer the "non-maj" fact since it's older (facter 1.6 didn't have the "maj" fact) and also shorter.

jvigneux commented 9 years ago

I currently use puppet 3.7.4 and facter 2.1.0.

It is the same bug encountered here with an openstack installation module on centos7:

"Comparison of String with 7 failed" https://bugzilla.redhat.com/show_bug.cgi?id=1117035

thias commented 9 years ago

I don't see how it's the same bug. I understand why comparing 7.0 as a number worked and why comparing 7.0.1406 as a number doesn't, which is why comparison operators aren't suitable, but the versioncmp() function is here to solve exactly that : Compare two strings as if they were software versions, not numbers. So I'm a bit confused :

Replacing operatingsystemrelease with operatingsystemmajrelease in an "if $x >= 7" works, but isn't ready for Puppet 4 where both facts are now typed as strings.
The correct fix is to use versioncmp() against either fact, which is already being done.

djjudas21 commented 9 years ago

Well, regardless of the root cause, this is still broken on CentOS 7 with puppet 3.7.5 and facter 2.4.3, which are the latest stable versions available for CentOS 7.

My point is that your module is broken on CentOS 7 so either the root cause needs to be addressed, or a workaround has to be added. The best workaround I've seen is the one in this PR.

djjudas21 commented 9 years ago

I should add that the master in your repo does contain the fix already but the latest release on Puppet Forge (0.3.3) does not have the fix and is about a year old now. To fix this issue for Forge users, all you have to do is roll release 0.3.4 from the code you have today. I opened #40 about this in October. Thanks.

thias commented 8 years ago

Forge release 0.3.4 includes the fix for this at last, sorry for the delay.