-
Notifications
You must be signed in to change notification settings - Fork 2
Add support for FreeBSD #45
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
The patterndb module use 'installed' for ensure_resource() to make sure the package is installed. 'installed' is prefered to 'present' and using different values cause a conflict that can be avoided by this change.
FreeBSD has no root group, the wheel group is basically the equivalent for system files.
If I am correct, both CentOS 7 and 8 have reached EOL and we can ignore these failures. |
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.
I'm just nervous about the changes in init.pp why and how are they related to fvreebsd ?
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.
I added in-line notes about the reasoning of the changes in the init.pp file. That's indeed a lot of moving pieces, I hope this makes the logic easier to grasp.
String[1] $module_prefix, | ||
Stdlib::Absolutepath $init_config_file, | ||
Hash $init_config_hash, | ||
String[0] $module_prefix, |
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.
module_prefix is set to ''
on FreeBSD (we do not have modules as packages (yet?)). It is necessary to allow empty String hence the change from:
- String[1] $module_prefix,
+ String[0] $module_prefix,
String[1] $config_file_header, | ||
String[1] $package_ensure, | ||
Stdlib::Absolutepath $sbin_path, |
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 was hard-coded to a location that is wrong on FreeBSD. As this is os-dependant it was moved to Hiera. Parameter without an explicit default value are expected to be before those with a default one hence:
- Stdlib::Absolutepath $sbin_path = '/usr/sbin',
+ Stdlib::Absolutepath $sbin_path,
String[1] $config_file_header, | ||
String[1] $package_ensure, | ||
Stdlib::Absolutepath $sbin_path, | ||
String[1] $package_ensure = 'installed', |
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.
The value in Hiera was the same for all OS. In such a case it is preferred to put the default value in the manifest so that puppet-strings can integrate the default value in the API documentation. See caf1aca for the reason of changing present
to installed
.
- String[1] $package_ensure,
+ String[1] $package_ensure = 'installed',
String[1] $package_ensure, | ||
Stdlib::Absolutepath $sbin_path, | ||
String[1] $package_ensure = 'installed', | ||
Optional[Stdlib::Absolutepath] $init_config_file = undef, |
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.
There is no init/default config file on FreeBSD, the parameter is therefore made optional, to allow it and we provide a default value overridden by existing os-specific info in Hiera:
- Stdlib::Absolutepath $init_config_file,
+ Optional[Stdlib::Absolutepath] $init_config_file = undef,
Stdlib::Absolutepath $sbin_path, | ||
String[1] $package_ensure = 'installed', | ||
Optional[Stdlib::Absolutepath] $init_config_file = undef, | ||
Hash $init_config_hash = {}, |
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.
We also provide a default value which is overridden by existing os-specific info in Hiera and only allows the catalog to be compiled when this unused data is not provided explicitly:
- Hash $init_config_hash,
+ Hash $init_config_hash = {},
Thanks for all these details, and sorry for my infinite lag |
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.
Thanks so much !
I have been using these patches for quite some time. Time to have them upstreamed?
I think that we can also remove EOL OS from metadata.json in a few more PR and roll a new release? What do you think?