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

Add cli_settings parameter to php class #491

Merged
merged 3 commits into from
Mar 6, 2019
Merged

Add cli_settings parameter to php class #491

merged 3 commits into from
Mar 6, 2019

Conversation

sunnz
Copy link
Contributor

@sunnz sunnz commented Oct 21, 2018

Pull Request (PR) description

Adds cli_settings parameter to ::php class to allow specific settings for PHP CLI's php.ini file.

For example this allows you to specific 256M memory_limit for PHP and PHP-FPM but increase the limit for PHP CLI, ie, PHP scripts running detached from the a web browser, like:

class profile::php {
  class { '::php':
    ensure       => latest,
    manage_repos => false,
    fpm          => true,
    dev          => true,
    composer     => true,
    pear         => true,
    phpunit      => false,
    settings => {
      'PHP/max_execution_time' => '60',
      'PHP/memory_limit'       => '256M',
    },
    cli_settings => {
      'PHP/memory_limit'       => '1000M',
    },
  }
}

Then the memory_limit would be set to 256M in /etc/php.ini and /etc/php-fpm.ini and 1000M in /etc/php-cli.ini on CentOS 7.

This Pull Request (PR) fixes the following issues

Fixes #489

Allow specific settings for PHP CLI's php.ini file.
@bastelfreak bastelfreak added the enhancement New feature or request label Oct 21, 2018
@bastelfreak
Copy link
Member

Hi @sunnz. Can you add a unit or acceptance test to verify that the php.ini for the CLI really contains the required content?

@bastelfreak bastelfreak added the needs-work not ready to merge just yet label Oct 21, 2018
@sunnz
Copy link
Contributor Author

sunnz commented Oct 21, 2018

Hi @bastelfreak, I would be very happy to but I haven't written any unit or acceptance test for Puppet... would you be able to point me to what tool do you use run the tests and the existing tests for this repository?

@sunnz
Copy link
Contributor Author

sunnz commented Oct 21, 2018

Ok I am going through the test part of documentation now: https://github.com/voxpupuli/puppet-php/blob/master/.github/CONTRIBUTING.md

I am trying to run the unit test of my end now. (bundle exec rake spec)

I am guessing unit tests are written in rspec-puppet?

@bastelfreak
Copy link
Member

yes, the tests are written in rspec-puppet. The current unit tests are at https://github.com/voxpupuli/puppet-php/tree/master/spec/classes

@bastelfreak
Copy link
Member

Hey @sunnz, any update here? Let us know if you need some help. You can reach us in the IRC channel #voxpupuli on freenode or the #voxpupuli channel on https://puppetcommunity.slack.com (signup at https://slack.puppet.com )

@sunnz
Copy link
Contributor Author

sunnz commented Nov 28, 2018

Hey @bastelfreak thanks for the follow up. Yes I am stuck on something but then I went overseas so didn't have a lot of access to update!

I will try to give you an update and if you can help me that would be great!

@sunnz
Copy link
Contributor Author

sunnz commented Nov 29, 2018

So... I was adding some test to php_spec.rb as follows:

diff --git a/spec/classes/php_spec.rb b/spec/classes/php_spec.rb
index e253899..34f2db1 100644
--- a/spec/classes/php_spec.rb
+++ b/spec/classes/php_spec.rb
@@ -214,6 +214,15 @@ describe 'php', type: :class do
       end
 
       if facts[:osfamily] == 'RedHat' || facts[:osfamily] == 'CentOS'
+       describe 'when called with cli_settings parameter' do
+         let(:params) do
+           { settings: { 'PHP/memory_limit' => '300M' } }
+         end
+
+         it { is_expected.to contain_ini_setting('PHP/memory_limit') }
+         it { is_expected.to contain_php__config__setting('PHP/memory_limit') }
+       end
+
         describe 'when called with global option for rhscl_mode' do
           describe 'when called with mode "remi"' do
             scl_php_version = 'php56'

So I was actually trying to write a simply test for an existing parameter "settings" and thought that if I can get that to work before I try to do the same to "cli_settings".

But then I got:

Failures:

  1) php on centos-6-x86_64 when called with cli_settings parameter should contain Ini_setting[PHP/memory_limit]
     Failure/Error: it { is_expected.to contain_ini_setting('PHP/memory_limit') }
       expected that the catalogue would contain Ini_setting[PHP/memory_limit]
     # ./spec/classes/php_spec.rb:222:in `block (5 levels) in <top (required)>'

  2) php on centos-6-x86_64 when called with cli_settings parameter should contain Php::Config::Setting[PHP/memory_limit]
     Failure/Error: it { is_expected.to contain_php__config__setting('PHP/memory_limit') }
       expected that the catalogue would contain Php::Config::Setting[PHP/memory_limit]
     # ./spec/classes/php_spec.rb:223:in `block (5 levels) in <top (required)>'

But it should pass right? I am expecting the init_setting and class to be there for each setting in settings declared from: https://github.com/voxpupuli/puppet-php/blob/master/manifests/config/setting.pp

So I am stuck with this... please let me know if I can provide anything else.

@jackdpeterson
Copy link

I'd like to +1 this PR generally speaking because in the scenario that I was looking at today ... I would like to have display_errors=on in CLI but not in FPM. Thanks for the work on this. Looking forward to seeing where this goes.

@sunnz
Copy link
Contributor Author

sunnz commented Mar 6, 2019

Thanks for the +1 @jackdpeterson, I should keep trying to get unit test working! The fork does work though I have it running it my environment, feel free to try it out.

@sunnz
Copy link
Contributor Author

sunnz commented Mar 6, 2019

Got the test working! See commit 856a12b where settings and cli_settings parameter are added under RHEL/CentOS. Commit 457591f just fixes the styling for Travis CI.

@bastelfreak bastelfreak changed the title Added cli_settings parameter to ::php class. Add cli_settings parameter to php class Mar 6, 2019
@bastelfreak bastelfreak removed the needs-work not ready to merge just yet label Mar 6, 2019
@bastelfreak
Copy link
Member

thanks @sunnz !

@bastelfreak bastelfreak merged commit 64bbc0b into voxpupuli:master Mar 6, 2019
@sunnz
Copy link
Contributor Author

sunnz commented Mar 6, 2019

Thanks for the help and accepting the pull request @bastelfreak! :))))

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.

Cannot declare both php and php::cli classes
3 participants