Making the management of the daemon package optional

[joshsouza]

This tripped up my team. Perhaps a bit of an overkill fix, but at least it retains existing functionality. I'd love to hear of a different approach if you have something you'd suggest.

[rtyler]

I'm curious what about installing the daemon package tripped your team up? Was it not available, defined elsewhere, other?

I don't see anything terribly wrong with this approach, but I'd like to understand the problem a bit more before merging.

[jenkinsadmin]

Thank you for a pull request! Please check this document for how the Jenkins project handles pull requests

[joshsouza]

There's nothing horribly wrong with this approach, except that other
modules might specifically be responsible with ensuring the daemon (or any
other package) are on the machine, as is our case, and as this module isn't
directly tasked with managing that package, but rather is installing it as
a dependency, it makes more sense to make it optional to have this module
manage it.

Specifically we have another class that handles installing several packages
that our machines should have, and collides with this module on that
package. If we tried to work around it in the other module, we would end up
with odd dependency presence checking etc... for things it shouldn't beer
responsible for.

Regards,
Josh
On Sep 17, 2015 11:07 AM, "R. Tyler Croy" notifications@github.com wrote:

I'm curious what about installing the daemon package tripped your team
up? Was it not available, defined elsewhere, other?

I don't see anything terribly wrong with this approach, but I'd like to
understand the problem a bit more before merging.

—
Reply to this email directly or view it on GitHub
#377 (comment)
.

[rtyler]

Perhaps a better route would be simply make a jenkins::config option which is "manage_packages" and use that as a global flag to say either the module is going to install all the necessary packages it needs, or not?

[joshsouza]

Would you like me to take that approach with this? I can adjust my PR to follow that logic. (Also, would it be all-or-nothing with the java install? or keep that unique?)
If I were to adjust it, I'd put in an 'include jenkins::config' to ensure the variables are available, add the parameter there, and then change my if statement based on that. Make sense?

[rtyler]

@joshsouza I was more brainstorming, I'm not convinced that's the right way to go myself but I wanted to bring it up as a potential alternative to adding a single parameter for this one package

[rtyler]

@jhoblitt I wouldn't mind some opinions on this from you as well, if you've got the time.

I'm thinking that needing to specify distinct packages that the module should manage is going to get crufty and confusing, especially for new users, whereas providing a "the module manages its dependencies" or "the module does not manage its dependencies" mode will be much easier to understand and document

[jhoblitt]

@rtyler It's not a perfect solution due to parse order dependence, but I have generally preferred to use ensure_packages() when there is no clear "ownership" of a package.

In a general sense, I don't have an objection to params that disable functionality the user may want to control external to the module (eg, package installation, configuration, service management). I have been drifting towards not providing it by default as every param/branch incurs a small amount of additional test maintenance.

Specific to this PR, I feel the naming is a bit confusing as I would naively expect manage_packages to turn off all software installation including the slave jar. I don't really have a better naming suggestion. ensure_packages() may be a better solution to @joshsouza's issue. If not, I'd recommend that the new parameter be validated and have unit test coverage before being merged.

[rtyler]

@jhoblitt I think this is a good suggestion, I'll respin this PR as soon as I can. I'm also attending puppetconf and all the preparation is really killing the first part of the week for me :/

[jhoblitt]

@joshsouza #391 switches over to using ensure_packages(). Please let us know if that is sufficient for you or if we still need to consider a management param.

[joshsouza]

That works great! Thank you!

[jhoblitt]

Your welcome!