-
Notifications
You must be signed in to change notification settings - Fork 495
(fact-155) Fix operatingsystemrelease on Solaris 11 #594
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
CLA signed by all contributors. |
it would be nice to get some feedback here :-) |
+1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this change backwards incompatible? Can we release this in Facter 2.1 or does this have to be released in a backwards incompatible release?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It introduces a change in behaviour on Solaris 11 systems. Before the change the old regex did not match, so sadly the following resolution was used as a (suboptimal) fallback value.
Facter.add(:operatingsystemrelease) do
setcode do Facter[:kernelrelease].value end
end
If this change should therefore be targeted against facter 3.x i think we should remove the above rather pointless resolver in the same run (as discuessed in https://projects.puppetlabs.com/issues/11308)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is falling back to kernelrelease on Solaris really meaningful? That is, is the fallback value usable as is or is it just broken?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In my opinion the fallback is not useful, and that's why I have filed the linked redmine ticket two years ago ;-). Nevertheless it is possible that someone who is using the current facter version has asked himself "How can I determine if the current agent runs Solaris 11?" and has answered this question with "looks like operatingsystemrelease gives me what I want, so let's write `if $::operatingsystemrelease == '5.11' { }" This will fail of course if we change the fact now.
The /etc/release file has changed from Solaris 10 to Solaris 11 and because the regular expression does not match anymore the operatingsystemrelease shows the kernelrelease fact on Solaris 11. Change the regular expression to also identify Solaris 11
I really wish that I saw this before we cut Facter 2.0.1-rc2 because I want this behavior and it would make merging this much less controversial, but I really don't want to do an rc3 for this fix alone. Is there any way we could call this a bugfix or resolving a defect, or do we have to defer this to Facter 3? If it's the latter then we may want to consider working on Facter 3 relatively soon, so that we can make more necessary breaking changes. |
No option is ideal, but my leaning is to do an rc3 to include this because a) it is a breaking change, b) I'd hate to wait til Facter 3. |
I agree with @kylog - it's better to fix it now. |
Backported onto stable and merged in 8835bc1; this should be released in Facter 2.0.1. Thanks for the contribution! |
since facter 1.7.0 we have an explicit
operatingsystemrelase
fact for Solaris so we do not just use thekernel
fact anymore. Unfortunately the fact does only work on Solaris 10 and not on Solaris 11. So while on Solaris 10 we have a meaningful value now, we have a copy of thekernel
fact on Solaris 11.Update the fact to print a meaningful value on Solaris 11 (strictly speaking this is a change in behaviour) and also implement an
operatingsystemmajrelease
fact to easily check for Solaris 10 or Solaris 11 in manifests.