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.

Sign up for GitHub

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

Jump to bottom

Support Debian 11 #108

Draft
wants to merge 10 commits into
base: master
Choose a base branch
from
Draft

Support Debian 11 #108

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
8599229
Support Debian 11
FlorentPoinsaut Mar 14, 2022
8543460
Use versioncmp instead of >=
FlorentPoinsaut Mar 14, 2022
dad103b
add debian version and bareos version compatibility check
FlorentPoinsaut Mar 14, 2022
812ae9f
Merge branch 'master' into FlorentPoinsaut-patch-2
FlorentPoinsaut Mar 14, 2022
7d5719c
Add Debian 11 compatibility mention
FlorentPoinsaut Mar 14, 2022
16c5559
fix unit test
FlorentPoinsaut Mar 15, 2022
50bca89
Fix Debian 11 unit test
FlorentPoinsaut Mar 16, 2022
2e42adb
add release param depends on OS version
FlorentPoinsaut Mar 16, 2022
cbb7ca5
add compatibility of Debian 11 with Bareos 20
FlorentPoinsaut Mar 16, 2022
fbaa120
Merge branch 'master' into FlorentPoinsaut-patch-2
FlorentPoinsaut May 11, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion manifests/init.pp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,7 @@
$file_group = $bareos::params::file_group,
$file_mode = $bareos::params::file_mode,
$file_dir_mode = $bareos::params::file_dir_mode,
String $repo_release = '21',
String $repo_release = $bareos::params::repo_release,
Boolean $repo_subscription = false,
Optional[String[1]] $repo_username = undef,
Optional[String[1]] $repo_password = undef,
Expand Down
1 change: 1 addition & 0 deletions manifests/params.pp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
$file_group = 'bareos'
$config_dir = '/etc/bareos'
$config_dir_webui = '/etc/bareos-webui'
$repo_release = '21'

# service/package specific
# bconsole
Expand Down
10 changes: 7 additions & 3 deletions manifests/repository.pp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,13 +21,13 @@
# Whether https should be used in repo URL
#
class bareos::repository (
Enum['19.2', '20', '21'] $release = '21',
Enum['19.2', '20', '21'] $release = $bareos::params::repo_release,
Optional[String[1]] $gpg_key_fingerprint = undef,
Boolean $subscription = false,
Optional[String] $username = undef,
Optional[String] $password = undef,
Boolean $https = true,
) {
) inherits bareos::params {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm wondering if that's required. Do we allow people to use this class directly or should it be marked private? the init.pp calls bareos::repository, that means $bareos::repo_release should work.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we can continue to allow people to use this class directly.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

in that case we maybe should inherit from bareos instead? that would be a common pattern. Otherwise you end up in situations where people set bareos::repo_release in hiera but that won't be picked up by this class.

Copy link
Member Author

@FlorentPoinsaut FlorentPoinsaut Mar 17, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As it is, it is impossible because init.pp invoque bareos::repository class.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Then we can use this default parameter (no params component):

    Enum['18.2', '19.2', '20', '21']  $release             = $bareos::repo_release,

No need for inheriting, we can even remove the parameter and use the $bareos::repo_release variable directly, and make the class private without impacting end-users 😉

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm sorry but I don't understand what is the advantage of making the param class private.
What is the problem to allow user to only add Bareos repository and nothing else?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not speaking about the params class sorry… As you said, bareos::repository cannot be declared by the end-user because the bareos class declares it. So the user has no possibility to pass custom parameters unless using hiera which is not a good idea, it is some implementation detail internal to the module.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ooohh no, my bad, I write param instead of repository . I'm sorry.
This scenario is true only if bareos::repository inherit of bareos but we have no problem if bareos::repository inherit of bareos::param as I wrote in code.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe that @smortex is suggesting to reference the repo_release parameter of the bareos class and no inheritance is needed.

if $https {
$scheme = 'https://'
} else {
Expand Down Expand Up @@ -127,7 +127,11 @@
}
$location = "${url}xUbuntu_${osrelease}"
} else {
if $osmajrelease == '10' {
if versioncmp($osmajrelease, '10') >= 0 {
if (versioncmp($release, '18.2') <= 0)
or ((versioncmp($release, '20') < 0) and (versioncmp($osmajrelease, '11') >= 0)) {
fail("Bareos ${release} is not distributed for Debian ${osmajrelease}")
}
$location = "${url}Debian_${osmajrelease}"
} else {
$location = "${url}Debian_${osmajrelease}.0"
Expand Down
3 changes: 2 additions & 1 deletion metadata.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,8 @@
"operatingsystem": "Debian",
"operatingsystemrelease": [
"9",
"10"
"10",
"11"
]
},
{
Expand Down
42 changes: 29 additions & 13 deletions spec/classes/repository_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,22 +64,38 @@
end
end

case facts[:operatingsystemmajrelease]
when '20'
context 'with subscription: true, username: "test", password: "test"' do
let(:params) do
{
subscription: true,
username: 'test',
password: 'test'
}
case facts[:operatingsystem]
when 'Debian'
case facts[:operatingsystemmajrelease]
when '11'
context 'with release: "19.2"' do
let(:params) do
{
release: '19.2'
}
end

it { is_expected.to compile.and_raise_error(%r{Bareos 19.2 is not distributed for Debian 11}) }
end
end
when 'Ubuntu'
case facts[:operatingsystemmajrelease]
when '20'
context 'with subscription: true, username: "test", password: "test"' do
let(:params) do
{
subscription: true,
username: 'test',
password: 'test'
}
end

it { is_expected.to compile }
it { is_expected.to compile }

it do
expect(subject).to contain_apt__source('bareos').
with_location('https://test:[email protected]/bareos/release/latest/xUbuntu_20.04')
it do
expect(subject).to contain_apt__source('bareos').
with_location('https://test:[email protected]/bareos/release/latest/xUbuntu_20.04')
end
end
end
end
Expand Down