thias / puppet-php

Puppet module to manage PHP
Other
49 stars 67 forks source link

Parameterize classes #20

Closed jeffsheltren closed 10 years ago

jeffsheltren commented 10 years ago

I did some work to better parameterize the classes so that the class parameters can be more easily overridden using Hiera. Are you open to something like this? I'd prefer not to maintain my own fork.

I don't have a Debian box handy, so I left a couple of the parameter defaults as 'TODO' -- that'll need to be corrected.

I made a couple other changes while I was at it, so if you would prefer me to split those out into separate PRs, I'm happy to do so. Specifically:

thias commented 10 years ago

I see a lot of minor things wrong, but my main question would be : Why move all of those class specific parameters to the params class? It doesn't make much sense to me, as they won't be any easier to override. Those are : $cli_ensure, $mod_php_ensure, $manage_httpd_php_conf and $mod_ini_notify_httpd.

I do see where you're going with $cli_package_name for instance, but not with the above.

jeffsheltren commented 10 years ago

Thanks for looking. My only reason to include those in params.pp is to give people an easy way to edit them without editing the class .pp files directly. Totally a convenience thing, and doesn't need to be that way as long as the variables are made available as class parameters.

thias commented 10 years ago

I've included the $cli_package_name change in 7f63cec8115042482a9f216b85138e64f684ba27. For the rest, I still think the defaults just need to be properly set in params.pp for each distribution.

If you do feel like there are still useful and relevant changes worth including, then please reopen and update the pull request content (or just open another one).

Thanks for the feedback!