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

Migrate to hiera and fix spec test setup #202

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

Conversation

cruelsmith
Copy link
Contributor

Pull Request (PR) description

Migrate from params setup to hiera layer setup and fix broken spec test setup.

The spec test setup used some strange variable override that never testet the different variables for each OS.
That is now fixed with the new helper function freeradius_settings_hash(os_facts) that defines the wanted value difference based on the os_facts.

This Pull Request (PR) fixes the following issues

Fixes #162
Fixes #200

@nward
Copy link
Collaborator

nward commented Aug 18, 2023

Hi @cruelsmith - this is a lot of work you've put in, thanks! Note that there has been some work on #162 already but I had not yet created a PR.

I am a bit confused about the purpose of the spec test changes, and how it relates to moving to Hiera from the params pattern. Should this be a separate PR? Note #183 - which I've just rebased on to the current main and pushed and intend to merge shortly, which may significantly change your approach here, or perhaps the changes would not be required?

@cruelsmith
Copy link
Contributor Author

cruelsmith commented Aug 18, 2023

The spec fix could be separate PR but the hiera migration with 💯 % kill the current spec test setup.
Since this

redhat_params_class = 'class freeradius::params {
$fr_basepath = "/etc/raddb"
$fr_configdir = "mods-config"
$fr_db_dir = "\${localstatedir}/lib/radiusd"
$fr_group = "radiusd"
$fr_libdir = "/usr/lib64/freeradius"
$fr_logpath = "/var/log/radius"
$fr_moduleconfigpath = "/etc/raddb/mods-config"
$fr_moduledir = "mods-enabled"
$fr_modulepath = "/etc/raddb/mods-enabled"
$fr_package = "freeradius"
$fr_pidfile = "/var/run/radiusd/radiusd.pid"
$fr_raddbdir = "\${sysconfdir}/raddb"
$fr_service = "radiusd"
$fr_service_has_status = true
$fr_user = "radiusd"
$fr_version = "3"
$fr_wbpriv_user = "wbpriv"
$fr_wpa_supplicant = "wpa_supplicant"
$radacctdir = "\${logdir}/radacct"
}
include freeradius::params'

is a wild setup that does not work after the migration.

In my 👀 #191 was completely unneeded and i prefer package { $package_name: } over package { 'fix_name': name => $package_name }

Also #183 is not fixed at all. You guys still just test against your variable overrides you do via

redhat_params_class = 'class freeradius::params {
$fr_basepath = "/etc/raddb"
$fr_configdir = "mods-config"
$fr_db_dir = "\${localstatedir}/lib/radiusd"
$fr_group = "radiusd"
$fr_libdir = "/usr/lib64/freeradius"
$fr_logpath = "/var/log/radius"
$fr_moduleconfigpath = "/etc/raddb/mods-config"
$fr_moduledir = "mods-enabled"
$fr_modulepath = "/etc/raddb/mods-enabled"
$fr_package = "freeradius"
$fr_pidfile = "/var/run/radiusd/radiusd.pid"
$fr_raddbdir = "\${sysconfdir}/raddb"
$fr_service = "radiusd"
$fr_service_has_status = true
$fr_user = "radiusd"
$fr_version = "3"
$fr_wbpriv_user = "wbpriv"
$fr_wpa_supplicant = "wpa_supplicant"
$radacctdir = "\${logdir}/radacct"
}
include freeradius::params'
and not against the os based ones from the params itself... that is what the spec test rewrite here fixes in the end.

@nward
Copy link
Collaborator

nward commented Aug 18, 2023

Re #191 - this is really off topic here so any further comments on #191 being a good idea or not, let's put that in the #191 discussion. My comment however is that the change in #191 allows us to much more easily include multiple OSes in specs, as different OSes have different package names, and store freeradius config in different places. This way, the vast majority of the spec can reference consistent names, and per-OS filenames, package names, etc. can be tested if required, without requiring the specs to be defined per-OS.

Yes - the redhat_params_class is unusual. It was an interim step, before #191 was done. The intent was to be pragmatic and get tests working "enough" by forcing every OS to have redhat-like values - and it did, and it allowed us to have useful tests.

Now that #191 is done, we can update the specs to take advantage of this. I will put together a PR.

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