The default disk_cache.conf.erb caches everything.

[Pawa2NR]

Need more control over the default behaviour. Causes issue when only a particular path is to be cached and not the whole website.

[CLAassistant]

All committers have signed the CLA.

[puppet-community-rangefinder]

apache::mod::disk_cache is a class

that may have no external impact to Forge modules.

This module is declared in 175 of 576 indexed public Puppetfiles.

---

These results were generated with Rangefinder, a tool that helps predict the downstream impact of breaking changes to elements used in Puppet modules. You can run this on the command line to get a full report.

Exact matches are those that we can positively identify via namespace and the declaring modules' metadata. Non-namespaced items, such as Puppet 3.x functions, will always be reported as near matches only.

[daianamezdrea]

Hi @Pawa2NR, the change looks good and it's not breaking the old behaviour, one thing, can you add some tests here in order to be sure that everything is fine? Also, can you move the end from the if else statement in the if from the templates? Maybe this can help you: https://httpd.apache.org/docs/2.4/mod/mod_cache.html

[Pawa2NR]

Hi @daianamezdrea, Can you please elaborate on the last part ? I did not understand what you intend here. Thank you

[Pawa2NR]

HI @daianamezdrea , Can you help me here with writing tests for this ? I tested the code on my local systems. And it works as expected. By setting "apache::mod::disk_cache::default_cache_enable: false" I could suppress the default caching behaviour in the disk_cache.conf.

Here are some snippets from my puppet run attached.
test.txt

[david22swan]

@Pawa2NR Hi, looking at your code I agree with the above that some test's would be required. The primary form of test is unit, these tests validate individual parts of the code and ensure that the logic behind it is correct, The relevant test file for the change your making can be found through the following link: https://github.com/puppetlabs/puppetlabs-apache/blob/babc21bb524d2bae88066fe8abeeb7e77ace960a/spec/classes/mod/disk_cache_spec.rb
You simply need to go in here and copy on of the existing tests in order to have it cover your change. In this case as three separate OS types are checked you would like need to do one for each. I've done a quick example of what the test's may look like though some changes may be needed.

    context 'with $default_cache_enable = false' do
      let :pre_condition do
        'class{ "apache":
          apache_version => "2.4",
          default_mods   => ["cache"],
          mod_dir        => "/tmp/junk",
          $default_cache_enable = false,
         }'
      end

      it { is_expected.to compile }
      it { is_expected.to contain_class('apache::mod::disk_cache') }
      it { is_expected.to contain_class('apache::mod::cache').that_comes_before('Class[Apache::Mod::Disk_cache]') }
      it { is_expected.to contain_apache__mod('cache_disk') }
      it {
        is_expected.to contain_file('disk_cache.conf')
          .with(content: %r{CacheRoot \"\/var\/cache\/apache2\/mod_cache_disk\"\nCacheDirLevels 2\nCacheDirLength 1\nCacheIgnoreHeaders Set-Cookie})
      }
    end

For the other style of tests, referred to as acceptance, these involve making actual changes to a machine in order to ensure that they occur as expected, and the relevant file can be found through here: https://github.com/puppetlabs/puppetlabs-apache/blob/babc21bb524d2bae88066fe8abeeb7e77ace960a/spec/acceptance/apache_parameters_spec.rb
I hope this helps

[Pawa2NR]

Hi @david22swan,

Could you please have a look at the spec test ? I am bit stuck with the passing the parameters to the test. Looks like my pre-condition is not getting passed. Could you please help me here ?

The test result is at https://travis-ci.com/github/puppetlabs/puppetlabs-apache/jobs/502848282. And the tests i wrote are at https://github.com/puppetlabs/puppetlabs-apache/blob/b0dc8820991d3c08b1410c0bb37ef4011e4df039/spec/classes/mod/disk_cache_spec.rb

I have never used Ruby until now, would be great if you could assist here.

[david22swan]

Taking a look, for the missing error returns, you don't seem to have set default_cache_enable to be a specific type of variable so it accepts any value given.
Also, since default_cache_enable is set to true by default you don't need test's where you set it specifically.
As to why the other test's are failing it looks like it may be due to you overwriting the params set at the top, i.e.:
cache_ignore_headers: 'Set-Cookie',
You should add this value to your own param sets so that it is included.

Hope this helps

[Pawa2NR]

Hi @david22swan ,

I corrected the unit tests and also defined default_cache_enable as a Boolean.

[david22swan]

@Pawa2NR Sorry for the wait on a reply.
Anyway taking a look at the unit test's I'm happy with the state there in right now.
My only comments are, and I'm sorry for not mentioning them in my last reply, we would need some information added to the reference at the top of the disk_cache.pp to explain the parameter that you have added.
Again I am sorry that I did not mention this earlier, but once it is done I I'll be more than happy to merge your change.

[Pawa2NR]

Hi @david22swan ,

Thank you for the review. As requested, I have updated the description for the parameter in disk_cache.pp.

[david22swan]

@Pawa2NR Thanks for the quick response :)
Everything looks good now so I'm gonna go ahead and merge.
Once again sorry for the delay in reviewing and thank you forthe work that you have done