Add Bareos 22 (rebase) #169
Conversation
manifests/init.pp
Outdated
| $file_mode = $bareos::params::file_mode, | ||
| $file_dir_mode = $bareos::params::file_dir_mode, | ||
| String $repo_release = '21', | ||
| $user_groups = $bareos::params::user_groups, |
There was a problem hiding this comment.
Can you add a data type for this new parameter? Probably Array[String[1]].
There was a problem hiding this comment.
Also, as it is a new parameter, do not put the default value in params.pp but put it here. That way, the actual value is documented when you generate the documentation.
There was a problem hiding this comment.
If you don't mind, I'd like to add that as part of attempting to get #154 incorporated, as well. If you do mind, of course, I'm not opposed to adding it, here, it just feels vaguely like making a style shift like that all at once is "cleaner?"
There was a problem hiding this comment.
Also, as it is a new parameter, do not put the default value in
params.ppbut put it here. That way, the actual value is documented when you generate the documentation.
I was mostly aiming to port the original author's work mostly-unmolested, but I also find the use of params.pp here icky. Is it better to try and stick with a clean rebase, or should I just go ahead and fix things While I'm In Here™?
There was a problem hiding this comment.
After some discussion out-of-band, I've added updates to remove $user_groups from params.pp and add typing for the param in init.pp.
| $file_dir_mode = $bareos::params::file_dir_mode, | ||
| String $repo_release = '21', | ||
| $user_groups = $bareos::params::user_groups, | ||
| String $repo_release = '22', |
There was a problem hiding this comment.
Changing the default version make it a breaking change. This is just a FYI, if the latest version is 22 we can change the default in the next (major) version. I will add a backward-incompatible label accordingly.
There was a problem hiding this comment.
I see #161 is about adding support for bareos 23. Do you plan to rebase this version too next?
There was a problem hiding this comment.
The latest version is actually 24 - there's another PR that adds support for version 23 that I'd like to ALSO get merged, and if someone wants to cut a new (breaking) release, well, master already contains unreleased breaking changes at the moment.
There was a problem hiding this comment.
👍 we can do a single major release that default to 24 then, I think it is sensible.
smortex
added
the
backwards-incompatible
Freshen code style to bring things more up to date and avoid legacy patterns like params.pp and untyped parameters. Includes updates to REFERENCE.md.
4 participants
Pull Request (PR) description
This is a rebase of #129 onto
master(which should make all the tests pass, now, or at least it does when I run them locally).This Pull Request (PR) fixes the following issues
Fixes #142