puppetlabs / puppetlabs-apache

Puppet module for the Apache httpd server, maintained by Puppet, Inc.
https://forge.puppet.com/modules/puppetlabs/apache
Apache License 2.0
366 stars 1.08k forks source link

Added cache_disk #2521

Closed dploeger closed 5 months ago

dploeger commented 8 months ago

Summary

This deprecates apache::mod::disk_cache and instead adds support for apache::mod::cache_disk. Also adds parameters to both apache::mod::cache_disk and apache::mod::cache.

Additional Context

Since Apache 2.4 mod_disk_cache is named mod_cache_disk. This officially supports mod_cache_disk and additionally adds configuration options for it. To be backwards compatible, the old module is still available and simply uses the new module.

Checklist

CLAassistant commented 8 months ago

CLA assistant check
All committers have signed the CLA.

dploeger commented 8 months ago

@smortex Thanks. Done.

dploeger commented 8 months ago

@smortex Should I take a look at the other Rubocop findings?

smortex commented 8 months ago

@smortex Should I take a look at the other Rubocop findings?

Yes please :smile: They are admittedly mostly gratuitous, but allow some consistency across the module which makes maintenance easier. Rubocop has to pass for the unit and integration tests to run.

As it is now, it looks quite good, CI will maybe help us spot some remaining issues. I am just wondering if the unit tests for the legacy class should be updated to check that it instantiate the new class with the right parameters and not more.

dploeger commented 8 months ago

Fixed Rubocop

ThomasMinor commented 7 months ago

It seems the failures during acceptance testing do not relate to the changes in this PR. Any idea when these might get fixed? Can I offer some support here?

smortex commented 7 months ago

Puppet 8 seems to add a check for duplicates that fail. I opened #2525 to address the issue.

smortex commented 7 months ago

@dploeger can you please rebase your changes on top of the main branch? This should fix CI.

From your working directory:

git fetch origin       # Download the latest code we have here
git rebase origin/main # Move your commits on top of the main branch
git push -f            # Send the changes (-f is required because we re-wrote history)
ThomasMinor commented 7 months ago

@smortex rebasing is done.

ThomasMinor commented 7 months ago

Hi @smortex, I just merged the latest changes from main to this repo. Since the 2 failing tests do not seem to be related to the changes in this PR, how do we proceed from here?

smortex commented 7 months ago

@ThomasMinor From my point of view this look okay. The repository require multiple approval before merging so the next move is probably to have some feedback from @puppetlabs maybe?

I re-approved the CI run (any change you do to your commit invalidate the test), I expect it to all pass except the 2 known-bad tests which I think we can ignore.

ThomasMinor commented 7 months ago

Hi @smortex, it seems another test joined the club and failed with connection failures. Can I assist solving the problem?

ThomasMinor commented 6 months ago

Seems I looked wrong, 2 fail, one test was skipped.

smortex commented 6 months ago

@ThomasMinor the Ubuntu ARM tests are broken and mend seems to only run when the PR is from a branch in the repo itself.

spec/classes/mod/disk_cache_spec.rb is maybe testing too much things given the corresponding code is only a wrapper around apache::mod::cache_disk (it act as an integration test rather than a unit test).

Also there is a merge commit in the branch which is not nice. Rebasing on top of master would fix this.

These two annoyances are not stoppers for me.

ThomasMinor commented 5 months ago

How about a final review, @ekohl & @bastelfreak? Then we might be able to close this one.