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

Use ensure_packages to simpify packages management #122

Merged
merged 1 commit into from
Jul 3, 2018

Conversation

smortex
Copy link
Member

@smortex smortex commented Jul 3, 2018

Each one of these classes included bacula::virtual which in turn included both classes. As a consequence, any node declaring either a bacula::director or a bacula::storage implicitly declares the other class. This did not allowed to follow strictly the Role & Profiles pattern (see #121 (comment)) where you would like to use resource-style declaration for both classes.

Thanks to stdlib's ensure_packages (available from stdlib 3.2.0, released in 2012) we can avoid the virtual resources.

This was tested successfully on FreeBSD and Debian.

Known side effects:

  • Users have to include / declare both bacula::director and bacula::storage in their configuration (this is a breaking change I guess… The README suggests to use resource-style declaration of both classes, but this setup was not working so users may have used trial-and-error to find a working setup where only one of these classes is explicitly present, and the other one was incidentally included);
  • The Class[Bacula::Storage] is now reported correctly by PuppetDB (for some undetermined reason, I could not query PuppetDB for this class when it was included, and this change fix the problem… likely a problem in PuppetDB itself).

@zachfi
Copy link
Contributor

zachfi commented Jul 3, 2018

Thanks for the PR. This change looks good, though I'm not sure why the Class[Bacula::Storage] didn't show up in PuppetDB. I'm pretty sure, though not currently looking at it, that I have bacula storage boxes that are not also bacula directors. Looking at the bacula::virtual class here, it doesn't look to include either the bacula::storage or the bacula::director.

That said, this seems like a good change, and one that would be less confusing perhaps. Does the EPP in the package name still work for the director?

@smortex
Copy link
Member Author

smortex commented Jul 3, 2018

Wow! I should be on drugs… I was so sure about what I wrote in my comments that I did not re-checked the removed code and indeed, the commit message is just madness 😳

/me is looking into this to understand what the hell is going on…

@zachfi
Copy link
Contributor

zachfi commented Jul 3, 2018

Hehe, don't worry. :) Let me know if you still want this in. I think its a fine change, but I suspect that understanding what is happening would be good regardless.

Thanks to stdlib's ensure_packages (available from stdlib 3.2.0,
released in 2012) we can avoid the virtual resources.
@smortex smortex changed the title Separate bacula::director and bacula::storage Use ensure_packages to simpify packages management Jul 3, 2018
@smortex
Copy link
Member Author

smortex commented Jul 3, 2018

Well… I found nothing out of the ordinary. I rephrased the commit message to remove the nonsense and updated the issue accordingly.

I guess the simplification worth it, so ready for review / merging.

Thanks!

@smortex smortex added this to the 5.4.1 milestone Jul 3, 2018
@smortex smortex requested a review from zachfi July 3, 2018 17:01
Copy link
Contributor

@zachfi zachfi left a comment

Choose a reason for hiding this comment

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

Looks good. Thanks for the efforts.

@zachfi zachfi merged commit 43b635f into voxpupuli:master Jul 3, 2018
@smortex smortex deleted the ensure-package branch July 20, 2018 07:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants