-
Notifications
You must be signed in to change notification settings - Fork 495
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
(FACT-3163) Add support for OpenBSD #2642
Conversation
buzzdeee
commented
Nov 18, 2023
Can one of the admins verify this patch? |
this is the follow-up of #2531 I believe what was missing, and what confused the random order was that the OpenBSD related tests were missing in ./spec/facter/util/facts/posix/virtual_detector_spec.rb because in lib/facter/util/facts/posix/virtual_detector.rb some OpenBSD related detections were added. Also seems, the "before do" has to be kind of in-order with how the fact checks are called in virtual_detector.rb Now everything aligned, I can't reproduce the issue anymore. I did let the tests run for some time in random order. |
Thanks @buzzdeee can you squash your commits? |
@joshcooper sure, squashed and rebased, hope all test going to pass again (: |
fix order dependency of tests when run: $ rspec -fd './spec/facter/resolvers/openbsd/virtual_spec.rb[1:2:1]' \ './spec/facter/util/facts/posix/virtual_detector_spec.rb[1:1:12:1]' \ --order random:17181 additionally recognized, the 'virtual' fact didn't return 'physical' when on a physical machine.
Most inspiration taken from FreeBSD, rest from other OS, and some own creations. Not all core facts supported, but makes puppet facts works well, as well as puppet agent --test Developed/Tested on OpenBSD 7.2, AMD64, Puppet 7.20.0, Ruby 3.1.2 also disable RuboCop Metrics/PerceivedComplexity and Metrics/CyclomaticComplexity as suggested by @joshcooper Additionally: Case insensitive OS detection ran into trouble when facter installed via puppetserver gem install, there the OS returned seems to be OpenBSD instead of all lowercase.
there have been merge errors, rebased ... |
OpenBSD went into ABI lock for 7.5 release, if this would be merged, and make it into the next release hopefully getting out before OpenBSD ports tree freeze, would be awesome. |
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.
This look good and there seems to be decent test coverage for the new code so LGTM.
As somebody who did similar work for FreeBSD, I feel like this can be merged as-is. Maybe some corner cases are not correctly handled by this code, but it is a move in the right direction and will allow small and easy to review pull-request to fix them if needed in the future. OpenBSD not being officially supported by Puppet (just like FreeBSD), a "not-perfect-from-the-first-release" seems more acceptable than for officially supported operating systems.
Well done @buzzdeee ! 👏 🎉