Skip to content
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

Ensure platform-specific code is correctly namespaced #2743

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

smortex
Copy link
Contributor

@smortex smortex commented Jul 30, 2024

Facter facts and providers are sometimes tied to a specific platform (e.g. Linux, FreeBSD, etc). When it is the case, they are placed in a directory named after this platform.

Some files are incorrectly stored in the base directory making it less obvious that they are platform-specific.

This PR fix this issue and is part 1/2 of a fix for #2742

@puppetlabs-jenkins
Copy link
Collaborator

Can one of the admins verify this patch?

@smortex smortex marked this pull request as ready for review July 30, 2024 07:44
@smortex smortex requested a review from a team as a code owner July 30, 2024 07:44
@joshcooper
Copy link
Contributor

joshcooper commented Aug 27, 2024

Thanks @smortex! Moving the containers resolver makes sense based on my reading of https://wiki.freebsd.org/Containers, but what about the others like dmi? What about them makes them linux specific? Is it the /sys/class filesystem?

When adding resolvers in the future, what's the best way to determine if something is linux-specific or not? Wondering if maybe there's a good reference/porting guide we could refer to?

Also could you resolve conflicts?

Move all files that defines Facter::Resolvers::Linux::* into the
lib/facter/resolvers/linux directory.  Also only load them on Linux
systems as they are Linux-specific.
All other filesystems resolvers are named "filesystems" (plural), so
match the same name because all these resolvers have the same purpose.
FreeBSD has containers (jails) but already handled through the 'virutal'
resolver, so for now we just consider Linux being able to handle
containers as the code aleady depend on Linux-specific resolvers.
@smortex
Copy link
Contributor Author

smortex commented Sep 11, 2024

Hey @joshcooper, I am catching up after long holiday away from home

Thanks @smortex! Moving the containers resolver makes sense based on my reading of wiki.freebsd.org/Containers, but what about the others like dmi? What about them makes them linux specific? Is it the /sys/class filesystem?

Indeed, /sys is something that is not available on FreeBSD and probably some other namespacing could be done if dmi facts are collected there. My goal in this PR is to eventually fix a warning that is produced with the latest code. As far as I can see, the dmi facts do not produce warnings when attempting to access /sys/....

When adding resolvers in the future, what's the best way to determine if something is linux-specific or not? Wondering if maybe there's a good reference/porting guide we could refer to?

Ughhhhh 😦 No real idea. Some acceptance tests that would run on various operating systems and ensure that no warning / error is produced may help… but that's non-trivial. Maybe creating a team with people interested in "exotic" operating systems and pinging that team when new provider are added may be simpler 🤷 ?

Also could you resolve conflicts?

Done!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants