-
-
Notifications
You must be signed in to change notification settings - Fork 40
Add Bareos 22 (rebase) #169
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
Changes from 5 commits
ecdeafd
007d651
c259c3d
55fc923
b8d798c
b68daee
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -38,7 +38,8 @@ | |
| $file_group = $bareos::params::file_group, | ||
| $file_mode = $bareos::params::file_mode, | ||
| $file_dir_mode = $bareos::params::file_dir_mode, | ||
| String $repo_release = '21', | ||
| $user_groups = $bareos::params::user_groups, | ||
| String $repo_release = '22', | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I see #161 is about adding support for bareos 23. Do you plan to rebase this version too next?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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,
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 👍 we can do a single major release that default to 24 then, I think it is sensible. |
||
| Boolean $repo_subscription = false, | ||
| Optional[String[1]] $repo_username = undef, | ||
| Optional[String[1]] $repo_password = undef, | ||
|
|
@@ -92,7 +93,7 @@ | |
| comment => 'Bareos system user', | ||
| home => '/var/lib/bareos', | ||
| shell => '/bin/false', | ||
| groups => ['disk', 'tape', $file_group], | ||
| groups => ['disk', 'tape', $file_group] + $user_groups, | ||
| system => true, | ||
| tag => ['bareos', 'bareos_core'], | ||
| } | ||
|
|
||
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.
Can you add a data type for this new parameter? Probably
Array[String[1]].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.
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.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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After some discussion out-of-band, I've added updates to remove
$user_groupsfrom params.pp and add typing for the param in init.pp.