Skip to content
This repository has been archived by the owner on Jun 19, 2020. It is now read-only.

Add support for processors facts on *BSD #489

Merged
merged 3 commits into from
Jun 11, 2020

Conversation

smortex
Copy link
Contributor

@smortex smortex commented May 1, 2020

Add a basic sysctl(3) FFI wrapper for getting processors information. This allows Facter 4 to return the same processors facts as returned by Facter 3 on FreeBSD.

While working on this I spotted a typo in an unrelated file, so fixed it in another commit.

@smortex smortex requested review from a team May 1, 2020 22:59
@puppetlabs-jenkins
Copy link
Collaborator

Can one of the admins verify this patch?

@smortex
Copy link
Contributor Author

smortex commented May 1, 2020

I might be moving too fast… this depends of bits in #485 🙄 Edit: PR was merged.


module Facter
module Freebsd
module FfiHelper
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add some tests for Freebsd :: FfiHelper

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did it, but it raise a few concerns for me because these tests just mock everything and we are testing the mock framework more than anything else 😢

@smortex
Copy link
Contributor Author

smortex commented May 9, 2020

I realized that most of the code could work on all *BSD systems with just a few tweaks, so I refactored the PR a bit to move the code from the FreeBSD to the BSD directories (Commit "(maint) Make the code more BSD-generic").

Yet, some differences exist for some aspects of the processors fact, and I could not manage yet to do things correctly. The BSD and FreeBSD resolvers are almost the same, only the way of fetching the CPU speed is different, and I would like to avoid duplicating all the code in this class. I though about inheritance, but it is not really working because both classes are instanced and the common facts are fetched twice :-/

@smortex smortex force-pushed the freebsd-processors branch 5 times, most recently from 111acf4 to 7a1039d Compare May 11, 2020 00:14
@smortex smortex changed the title Add support for processors facts on FreeBSD Add support for processors facts on *BSD May 11, 2020
@Filipovici-Andrei
Copy link
Contributor

We had some problems with the GitHub CI, but now it's fixed. So a merge with master will fix it for your PR as well.

@smortex smortex force-pushed the freebsd-processors branch 2 times, most recently from 8398521 to fb88cc5 Compare May 12, 2020 17:12
@smortex smortex force-pushed the freebsd-processors branch 3 times, most recently from b1e7f1b to c81ad64 Compare May 21, 2020 21:53
@smortex
Copy link
Contributor Author

smortex commented May 21, 2020

Inheritance in resolvers not being really helpful, I completely separated the Bsd resolver and the Freebsd one. That way, facts are not collected twice by the Bsd and the Freebsd resolvers on FreeBSD as it was previously the case.

I reabased the work on top of master, and since the commit where going back and forth, I squashed them to make the change more readable. Please review, thanks!

@smortex smortex requested a review from BogdanIrimie May 21, 2020 21:53
@BogdanIrimie
Copy link
Contributor

@smortex please rebase on master to fix RuboCop. Regarding Checks / coverage there is a known issue with PRs from Forks outside organisation. If all other test pass except Checks / coverage the PR looks good and we will merge it.

For BSD platforms, rely on a sysctl(3) FFI wrapper for getting
processors information.  On FreeBSD, rely on sysctlbyname(3) FFI
wrapper.

This allows Facter 4 to return the same processors facts as returned by
Facter 3 on FreeBSD.
@smortex
Copy link
Contributor Author

smortex commented Jun 10, 2020

@IrimieBogdan done!

@BogdanIrimie BogdanIrimie merged commit 2f9ffd2 into puppetlabs:master Jun 11, 2020
@smortex smortex deleted the freebsd-processors branch June 11, 2020 16:39
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants