Add prefix parameters

[TheFlyingCorpse]

Prefix options for etc, var and share, alternatively a generic one for all called dirprefix that is used unless the more specific is used.

[TheFlyingCorpse]

Updated the prefix paths based on my previous PR #189 to be more generic and separate from Windows support for a cleaner merge.

[arioch]

Looking good so far but I have three remarks before merging.

1:

$target_dir = "${::icinga2::params::etcprefix}/conf.d",

It's cleaner to have it point to ::icinga2::etcprefix and have the init class to take care of the resolving. This way it's consistent with the kenbarberism pattern we use throughout the module. Shouldn't be too hard to find/replace.

2: $etcprefix is a little confusing, especially when supporting non-unix OS'es in the future. Most other modules use $config_dir instead. This also shouldn't be too hard to find/replace.

3: There's no spec test to cover this new variable.

Keep those PR's coming! 👍

[zachfi]

+1 to what @arioch says.

[TheFlyingCorpse]

Right, thanks for your feedback, I'll get right to adapting as the feedback here has told me to.

[TheFlyingCorpse]

I'll merge my commits if the update looks good enough.

[TheFlyingCorpse]

I am at least missing a spec test for the variables. I'll look at that tomorrow.

[arioch]

Maybe also replicace $::icinga2::dirprefix with $::icinga2::dir_prefix to match the overall variable naming scheme.

Other than that if you add the spec test it looks good to me. 👍

[zachfi]

And @arioch, once this is merged, I assume that we will be on the lookout for any variables that reference the path without the prefix. True story?

[arioch]

I'm guessing you mean in future PR's... if so you're absolutely right. A quick grep through the code learned that @TheFlyingCorpse was very thorough. 👍

[zachfi]

Yes yes, I meant future PRs.

[TheFlyingCorpse]

Like so. I could not resolve $dir_prefix in params. I moved the path resolving to its own class and all paths resolve correctly now with a test file. If this is not a good/accepted solution please advise how you would like it instead. If I didnt use "$dir_prefix", everything would fit in params.pp and all the used vars would be as suggested with ::icinga2::__dir / dir_prefix, instead of icinga2::paths::__dir / dir_prefix

[zachfi]

The changes here look good to me. Why support both config_dir and dir_prefix? It seems like they solve the same problem, and the paths class will only use one at a time anyway.

[TheFlyingCorpse]

@xaque208 - The reason was to allow for setting up for a case where you compile icinga2 to use /opt/icinga2 with the dir_prefix, making sure then that config can be in /opt/icinga2/etc rather than /opt/icinga2/etc/icinga2. It would also set "good" paths for the other variables to some small effect for simplicity. It is trivial to remove the dir_prefix, and that would remove the need of the path class.

[TheFlyingCorpse]

I redid it without the dir_prefix, it only contains the directories directly in params now and is the simplest I can get it. I havent been able to find a way to test for variables in spec, so I can not at the moment test for the output of sbin_dir (which will be used for Windows) and share_dir.

[zachfi]

This work looks good to me. I think I'd still prefer that the use of i2* variables in the params class get moved to a separate commit, since those variables are not implemented here, though perhaps that is a nitpick. I'm +1 for this change. Thank you for the efforts @TheFlyingCorpse. We'll see what the grown-ups think.

[TheFlyingCorpse]

The i2 variables will be used immediately after this commit is accepted in the Windows PR I have not submitted yet, waiting for this and the puppet_ssldir one. I'll remove them if one more says it shouldnt stay in this commit, since that can come in the Windows commit, though I saw some use in the i2* feature for one of the BSD's which is waiting on this commit to use what I assume is the config_dir parameter at least.

[TheFlyingCorpse]

Like so, I think I solved it by changing out if with unless, so you can override on a per osfamily basis for the parameters you want to. If anything else, let me know!

[zachfi]

I'm still +1 on the changes.

[Reamer]

:+1: (tested in my enviroment)

[TheFlyingCorpse]

@arioch - Anything more I should change? I'm on vacation this week to hopefully get this PR accepted and move on with the Windows support.

[TheFlyingCorpse]

Rebased on current develop!