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

Support Debian 11 #108

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

Support Debian 11 #108

wants to merge 10 commits into from

Conversation

FlorentPoinsaut
Copy link
Member

Pull Request (PR) description

Support Debian 11

This Pull Request (PR) fixes the following issues

manifests/repository.pp Outdated Show resolved Hide resolved
Co-authored-by: Romain Tartière <[email protected]>
@smortex smortex added the enhancement New feature or request label Mar 14, 2022
@smortex
Copy link
Member

smortex commented Mar 14, 2022

Nice! So now we need Debian 11 added to metadata.json (think about rebasing of top of master to take the changes merged by #107) and the WIP can be gone?.

@FlorentPoinsaut FlorentPoinsaut changed the title WIP: Support Debian 11 Support Debian 11 Mar 14, 2022
@FlorentPoinsaut FlorentPoinsaut force-pushed the FlorentPoinsaut-patch-2 branch 3 times, most recently from 2f0a626 to e1a9447 Compare March 15, 2022 08:11
@FlorentPoinsaut
Copy link
Member Author

I need help finding the right way to test my work in a smart way.
Any ideas?
Thanks in advance!

@smortex
Copy link
Member

smortex commented Mar 15, 2022

For running the spec tests on your machine, you can:

  1. install the bundle of the project (bundle install --path /somewhere/you/want/to/cache/gems/as/a/user/I/use/~/.vendor)
  2. bundle exec rake spec

@FlorentPoinsaut
Copy link
Member Author

FlorentPoinsaut commented Mar 16, 2022

Thank you @smortex for your advice!
I finaly find a way. I created a release param and set by default to version 21 if it is Debian 11.
I think this PR is ready.

manifests/params.pp Outdated Show resolved Hide resolved
@FlorentPoinsaut
Copy link
Member Author

LOL...
Bareos editor add Bareos 20 packages for Debian 11 2 days ago (2022-03-14 19:50).
I can remove my param switch case...

Optional[String[1]] $gpg_key_fingerprint = undef,
Boolean $subscription = false,
Optional[String] $username = undef,
Optional[String] $password = undef,
) {
) 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

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.

Copy link
Member

@jhoblitt jhoblitt left a comment

Choose a reason for hiding this comment

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

I strongly believe that only the base class should inherit bareos::params.

@smortex smortex self-requested a review November 5, 2022 21:24
@FlorentPoinsaut FlorentPoinsaut marked this pull request as draft May 11, 2023 15:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants