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

Allow override of fpm package name #31

Closed
wants to merge 3 commits into from

Conversation

jeffsheltren
Copy link
Contributor

This matches my other recent pull requests to allow overriding the php-fpm package name with parameters. This adds options for specifying package name to both php::fpm::daemon and php::fpm:conf

@thias
Copy link
Owner

thias commented May 12, 2014

It makes sense, but wouldn't it make even more sense to name the parameters identically? So use $fpm_package_name for both?

Also, at this point, I think it would be worth thinking of a better way to set a "package prefix", instead of having to override multiple package names in multiple places.

What would you think of something like this? (untested, just a quick brainstorm!)

class php::params (
  $package_prefix = 'php',
) {
# [...]
    'RedHat': {
      $php_package_name = $package_prefix,
      $php_apc_package_name = "${package_prefix}-pecl-apc"
      $common_package_name = "${package_prefix}-common"
# [...]
}

Thoughts?

@jeffsheltren
Copy link
Contributor Author

I did the naming to match what was done in php::module::ini, but I agree, it's better to match php::fpm::conf with php::fpm::daemon (module::ini is intended for more than one package, this is not). I've pushed an updated commit to change this.

I agree that a more global approach would be best. I'll test out what you suggested and see if that will work for me. I'll follow up on that in another issue since it's going to touch a lot of files. Do you mind merging this and #30 in the meantime while I work on a better way?

@thias
Copy link
Owner

thias commented May 12, 2014

OK, I've actually re-implemented this change in c574ced since it wasn't worth adding 4+ commits for this, since I would have had to fix many cosmetic details. Notes for the next PRs :

  • Don't double quote or use curly brackets for variables (even when they're strings, as long as there is no interpolation)
  • Keep a sane ordering in parameters (I had global params at the top, file specific ones below)
  • Keep consistent (I already had a similar selector using undef and default with a *_final variable)

Thanks for your contribution, and I'll be looking forward to your tries at getting a global "prefix" working.

@thias thias closed this May 12, 2014
@jeffsheltren
Copy link
Contributor Author

Sounds great, thanks.

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.

None yet

2 participants