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

Fix forcing a php::setting to be absent by setting its value to undef #647

Merged
merged 1 commit into from
Mar 13, 2022

Conversation

jadestorm
Copy link
Contributor

@jadestorm jadestorm commented Nov 11, 2021

Pull Request (PR) description

The code for php::config::setting permits passing Undef to force the ini_setting to be absent instead of present but the recent type change does not allow Undef. This simply adds Undef as a valid type.

manifests/config/setting.pp Outdated Show resolved Hide resolved
@root-expert root-expert added the bug Something isn't working label Nov 11, 2021
@root-expert
Copy link
Member

Thanks for the PR! Can you add some unit tests to ensure it won't break again in the future?

@jadestorm
Copy link
Contributor Author

Sure! Give me a bit.

@jadestorm
Copy link
Contributor Author

(if anyone is watching this, bundle was being a pain so I elected to just make use of travis-ci to run the tests, and I intend to squish these commits together once I'm done)

@bastelfreak
Copy link
Member

I'm wondering if it makes more sense to add a ensure parameter. Undef is tricky because you can't really pass that to classes.

@jadestorm
Copy link
Contributor Author

You can't? I could have sworn you could. Obviously we're passing it via a hash of settings so indirectly in this scenario.

Also do you happen to have any ideas of how I properly present the Puppet Undef type in rspec? I'm running out of ideas.

@jadestorm
Copy link
Contributor Author

jadestorm commented Nov 12, 2021

Oh I guess that's not so obviously. =D Meaning we're currently wrapping ::php in this:

  class { '::php':
    manage_repos => false,
    settings     => {
      'PHP/user_ini.filename'                   => $chass_profile::php::user_ini_filename,
      'PHP/user_ini.cache_ttl'                  => $chass_profile::php::user_ini_cache_ttl,
      'PHP/engine'                              => $chass_profile::php::engine,
      'PHP/short_open_tag'                      => $chass_profile::php::short_open_tag,
      'PHP/asp_tags'                            => $chass_profile::php::asp_tags,
      'PHP/precision'                           => $chass_profile::php::precision,
      'PHP/output_buffering'                    => $chass_profile::php::output_buffering,
      'PHP/zlib.output_compression'             => $chass_profile::php::zlib_output_compression,
...

and some of those are undef by default.

@kenyon
Copy link
Member

kenyon commented Nov 12, 2021

Also do you happen to have any ideas of how I properly present the Puppet Undef type in rspec? I'm running out of ideas.

On https://rspec-puppet.com/documentation/classes/:

When passing undef as a parameter value, it should be passed as the symbol :undef.

@jadestorm
Copy link
Contributor Author

Also do you happen to have any ideas of how I properly present the Puppet Undef type in rspec? I'm running out of ideas.

On https://rspec-puppet.com/documentation/classes/:

When passing undef as a parameter value, it should be passed as the symbol :undef.

Ok that's what I ended up on, so I must be doing something wrong on the assertion side of things. I think I know what it is though -- I'll take another whack at it on Monday. =) Thanks!

@jadestorm
Copy link
Contributor Author

jadestorm commented Nov 15, 2021

Edit: Nevermind, need more coffee.. lol. .with_ensure is what I'm looking for.

To save me some time -- do y'all have any idea how to represent this properly?

it { is_expected.not_to contain_php__config__setting('/etc/php.ini: PHP/safe_mode_include_dir') }

Or for that matter, where is contain_php__config_setting even defined?

@jadestorm
Copy link
Contributor Author

Don't accept just yet please -- I am adding the ensure handling as per what @bastelfreak mentioned, but leaving the undef check in there.

@jadestorm
Copy link
Contributor Author

Ok, I decided to balk on the ensure parameter. Since this asserts private, I figured there's little point in offering that. Rebased so it doesn't have 5000 commits and barring other concerns it should be good to go at this point. Thanks y'all!

@jadestorm
Copy link
Contributor Author

(ps, the $ensure -> $_ensure variable name changed I left in there in case someone wants to add a full ensure parameter and ditched the private assertion in the future)

@kenyon
Copy link
Member

kenyon commented Nov 16, 2021

@jadestorm looks like the email address used in your commit is not associated with your GitHub account, if you want to fix that.

@jadestorm
Copy link
Contributor Author

Ah yes I'll fix that tomorrow thanks!

As far as limiting it to Redhat and CentOS -- I don't know =) It was like that before I messed with it and so I just went with it. I assumed maybe it had to do with the /etc/php.ini path reference maybe not being the same on all platforms? I really don't know though.

spec/classes/php_spec.rb Show resolved Hide resolved
@root-expert root-expert merged commit 773336e into voxpupuli:master Mar 13, 2022
@root-expert root-expert changed the title Allow a setting to be undef to force absent. Fix forcing a php::setting to be absent by setting its value to undef Mar 21, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants