The Puppet Labs Issue Tracker has Moved: https://tickets.puppetlabs.com

Refactor #9708

Remove redundancy from operatingsystem confines

Added by Adrien Thebo about 3 years ago. Updated almost 3 years ago.

Status:ClosedStart date:09/27/2011
Priority:NormalDue date:
Assignee:Ken Barber% Done:

0%

Category:library
Target version:1.6.5
Keywords: Affected Facter version:
Branch:https://github.com/kbarber/facter/tree/ticket/1.6.x/9708-os_confines

We've Moved!

Ticket tracking is now hosted in JIRA: https://tickets.puppetlabs.com

This issue is currently not available for export. If you are experiencing the issue described below, please file a new ticket in JIRA. Once a new ticket has been created, please add a link to it that points back to this Redmine ticket.


Description

There are instances inside facter that that look like this

confine :operatingsystem => %w{Solaris Linux Fedora RedHat CentOS Scientific SuSE SLES Debian Ubuntu Gentoo FreeBSD OpenBSD NetBSD OEL OVS GNU/kFreeBSD}

Which should be written more cleanly as

confine :kernel => %w{SunOS Linux FreeBSD OpenBSD NetBSD GNU/kFreeBSD}


Related issues

Related to Facter - Feature #6792: A fact to identify OS family Closed 03/20/2011
Blocks Facter - Feature #9599: Add Nexenta to operatingsystem fact Closed 09/19/2011

History

#1 Updated by Michael Kincaid about 3 years ago

Looks like hardwareisa, lsbmajdistrelease, macaddress, and uniqueid.

#2 Updated by Anonymous about 3 years ago

Soooooo…. that potentially changes the preference order for facts on those systems. Are you certain that this is a safe change?

#3 Updated by Michael Kincaid about 3 years ago

  • Status changed from Accepted to In Topic Branch Pending Review

https://github.com/mkincaid/facter/tree/ticket/9708

hardwareisa, lsbmajdistrelease, and uniqueid have only one resolution, so precedence is not an issue there. macaddress should behave the same as it did before, but please review. I did a quick smoke test on Solaris and Linux and nothing seems to have broken.

#4 Updated by Adrien Thebo about 3 years ago

To be honest, I’m not sure if this is safe. However, what’s been demonstrated is that the facts with these confines will work before there’s an operatingsystem fact available for it, and when that’s added and these confines are not updated, facts restricted by these confines start failing.

Looking at Michael’s changes, the hardwareisa fact should be fine, because if uname isn’t on a linux then we clearly have other problems to worry about. Same goes for lsbmajdistrelease; if that’s broken on a specific linux, that’s more of the distro’s problem than ours. With respect to the macaddress fact, we do see linux distributions moving away from net-tools and towards iproute2, but since we don’t support the latter in any case, it won’t break anything that isn’t broken already. Again with hostid, if that’s not included in a linux distro then it wouldn’t resolve in any case.

Daniel, what do you think?

#5 Updated by Adrien Thebo almost 3 years ago

  • Assignee changed from Adrien Thebo to Michael Kincaid

Michael, could you file a pull request against puppetlabs facter for this?

#6 Updated by Ken Barber almost 3 years ago

Should this be a 1.6.x change? I’m almost preferring it to be the next release since it seems risky enough – and its not fixing any specific bugs in 1.6.x, its just removing some pain we feel as devs.

#7 Updated by Ken Barber almost 3 years ago

I’ve tried this on:

  • Solaris 10
  • OS X 10.6 (hardwareisa is now fixed with this patch)
  • Dragonfly BSD (found a bug – notes in pull request)
  • OpenBSD 4.9
  • Debian Wheezy
  • Centos 5.6
  • NetBSD 5.0

I don’t have:

  • FreeBSD
  • GNU/kFreeBSD
  • AIX

I wouldn’t mind a quick smoke test (compare facter outputs between this branch and master) on those OS’s if someone has them running somewhere. I would love an automagical way of doing this with CI somehow with all these OS variants. It certainly would help with protecting against regression – take the DragonFly case for example … we just released that support and we may have inadvertently broken it if we didn’t test on the real OS.

#8 Updated by Ken Barber almost 3 years ago

  • Status changed from In Topic Branch Pending Review to Code Insufficient

I’m marking this as Code Insufficient because of the DragonFly BSD failure.

More tests should be done on the other distros before this can be accepted.

#9 Updated by Adrien Thebo almost 3 years ago

  • Target version changed from 144 to 2.0.0

Yeah, this is a really good candidate for 1.7.0.

#10 Updated by Michael Kincaid almost 3 years ago

As an alternative solution, would it work to just remove the confine entirely from the hardwareisa fact? If uname -p returns an answer, it should be considered authoritative, and if not, then there’s no resolution…

#11 Updated by Nigel Kersten almost 3 years ago

Michael Kincaid wrote:

As an alternative solution, would it work to just remove the confine entirely from the hardwareisa fact? If uname -p returns an answer, it should be considered authoritative, and if not, then there’s no resolution…

+1 for this approach unless someone can show ‘uname -p’ returning different info on any of our supported OSes.

#12 Updated by Michael Kincaid almost 3 years ago

  • Status changed from Code Insufficient to In Topic Branch Pending Review

Removed the kernel confine from the hardwareisa fact — it doesn’t seem necessary, and this should generalize to work on any system that returns an answer for “uname -p”.

#13 Updated by Ken Barber almost 3 years ago

  • Status changed from In Topic Branch Pending Review to Merged - Pending Release

#14 Updated by Ken Barber almost 3 years ago

  • Status changed from Merged - Pending Release to Re-opened
  • Target version changed from 2.0.0 to 144

I’m reopening this because I want us to reconsider the target version. I’m proposing we cherry-pick this patch back to the 1.6.x branch:

https://github.com/puppetlabs/facter/pull/121

The reasons are:

  • Continued maintenance in 1.6.x makes it harder for people trying to add new OS. Since most effort like this does occur in 1.6.x, this patch is better placed here.
  • It actually does fix issues already – like hardwareisa and macaddress since the confines are removed, and we fall back to the ‘exec’ style confine
  • Perhaps the risk we first saw in Michael’s first patch wasn’t reconsidered when his second amended patch came out. The current patch is actually less risky the more I think about it.

We hit an issue with #9599 that exasperated the need for this in 1.6.x … and we will continue to hit it without this patch more or less. The alternative is to go back and modify 6 or so facts every time we want to add OS coverage which increases the change of making mistakes.

#15 Updated by Adrien Thebo almost 3 years ago

+1

#16 Updated by Ken Barber almost 3 years ago

  • Assignee changed from Michael Kincaid to Ken Barber

#17 Updated by Ken Barber almost 3 years ago

  • Status changed from Re-opened to In Topic Branch Pending Review
  • Branch set to https://github.com/kbarber/facter/tree/ticket/1.6.x/9708-os_confines

#18 Updated by Ken Barber almost 3 years ago

  • Status changed from In Topic Branch Pending Review to Merged - Pending Release
  • Target version changed from 144 to 1.6.5

#20 Updated by Matthaus Owens almost 3 years ago

  • Status changed from Merged - Pending Release to Closed

Released in 1.6.5rc1

Also available in: Atom PDF