Automatically include modules used in vhost directories #2255

ekohl · 2022-06-28T09:22:12Z

The data passed to the directories parameter of apache::vhost often implies a module should be present. This change inspects the passed data and automatically includes modules if needed. This makes it easier to use via Hiera while disabling all default mods.

Because of this change one acceptance test becomes obsolete: it no longer fails and just works.

To make this easier the data type is simplified. This means users need to be a bit stricter about their input, but they need to worry less about including the right modules.

This replaces #2250.

puppet-community-rangefinder · 2022-06-28T09:22:37Z

apache::mod::proxy_ajp is a class

Breaking changes to this file WILL impact these 1 modules (exact match):

abiquo-abiquo

apache::mod::proxy_balancer is a class

Breaking changes to this file WILL impact these 1 modules (exact match):

urgi-galaxy_roles_profiles

apache::mod::proxy_connect is a class

Breaking changes to this file WILL impact these 1 modules (exact match):

osc-openondemand

apache::mod::proxy_fcgi is a class

Breaking changes to this file WILL impact these 2 modules (exact match):

Breaking changes to this file MAY impact these 1 modules (near match):

icinga-icinga

apache::mod::proxy_html is a class

Breaking changes to this file WILL impact these 1 modules (exact match):

cristifalcas-kibana

apache::mod::proxy_http is a class

Breaking changes to this file WILL impact these 10 modules (exact match):

apache::mod::proxy_wstunnel is a class

Breaking changes to this file WILL impact these 2 modules (exact match):

apache::vhost is a type

Breaking changes to this file WILL impact these 128 modules (exact match):

Breaking changes to this file MAY impact these 35 modules (near match):

This module is declared in 174 of 579 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.

david22swan · 2022-06-28T10:00:43Z

@ekohl Look's like another few spec failures have shown their heads, almost there 🤞

ekohl · 2022-06-28T13:36:57Z

Tests have been running for a long time. I suspect there is a failure in many cases which means it generates a ton of errors. I pushed a commit that massively reduces the spec tests so hopefully I get a faster result.

ekohl · 2022-06-28T13:43:27Z

Ah, it doesn't run the vhost tests on non-Debian. That's surprising since other code suggests it does run on that. Will update again.

ekohl · 2022-06-28T14:19:22Z

This is much easier to iterate on.

ekohl · 2022-06-28T14:29:49Z

I tried fixing it, but somehow it still doesn't recognize the param docs for gssapi:

manifests/vhost.pp - WARNING: No matching defined type parameter for documentation of apache::vhost::gssapi on line 1470 (check: parameter_documentation)
manifests/vhost.pp - WARNING: No matching defined type parameter for documentation of apache::vhost::gssapi on line 1470 (check: parameter_documentation)

ekohl · 2022-06-28T14:53:31Z

Well, that's green. Removing the temporary commit to see if it all fits together.

david22swan · 2022-06-29T08:19:23Z

@ekohl Seems to be a few spec test failures left. All coming from the same section:

  1) apache::vhost os-independent items on scientific-6-x86_64  not everything can be set together... is expected to contain Concat::Fragment[rspec.example.com-directories] with content =~ /^\s+Allow from 127\.0\.0\.1$/
     Failure/Error:
       is_expected.to contain_concat__fragment('rspec.example.com-directories').with(
         content: %r{^\s+Allow from 127\.0\.0\.1$},
       )
       expected that the catalogue would contain Concat::Fragment[rspec.example.com-directories]
     # ./spec/defines/vhost_spec.rb:2140:in `block (6 levels) in <top (required)>'
     # ./vendor/bundle/ruby/2.7.0/bin/rspec:23:in `load'
     # ./vendor/bundle/ruby/2.7.0/bin/rspec:23:in `<top (required)>'
     # /opt/hostedtoolcache/Ruby/2.7.6/x64/bin/bundle:23:in `load'
     # /opt/hostedtoolcache/Ruby/2.7.6/x64/bin/bundle:23:in `<main>'

Only appearing on scientific, oracle and redhat 6 so they didn't show when you cut to Ubuntu

ekohl · 2022-06-29T09:50:14Z

Ah yes, I only ran the quick tests with Debian enabled. Now with the full matrix we test a bit more. Looks like there are some tests there that verify behavior when the vhost is set to absent. Now that the whole directories concat fragment is only rendered when it's present, it's not in the catalog. I'll rewrite the test.

ekohl · 2022-06-29T10:09:47Z

Before moving it I'd like to merge #2256 because it makes the refactor easier.

david22swan · 2022-06-29T13:59:53Z

@ekohl Look's like the spec test failures are still there

ekohl · 2022-06-29T14:08:21Z

I ended up wanting to do more refactors so I've split off yet another part in a more focused PR: #2257. Apologies for the scope blowing up, but I believe it makes it easier to maintain in the future.

david22swan · 2022-06-29T15:44:51Z

No need to apologize, I'm liking all the changes

This ensures the proxy module is always loaded at the right time.

This makes it easier to run with default mods disabled while still using this mod.

This tightens the data type to a simple array of hashes (as the parameter documentation suggests). This allows the logic to be simpler. Because of this, it is now fairly easy to include the correct modules if needed. Some options are implemented but there are more which could be automatically included.

ekohl · 2022-07-04T09:30:31Z

Rebased to resolve the conflicts. Let's see if the tests pass.

ekohl · 2022-07-04T09:42:56Z

I'm not quite sure why GH actions don't run.

The data passed to the directories parameter of apache::vhost often implies a module should be present. This change inspects the passed data and automatically includes modules if needed. This makes it easier to use via Hiera while disabling all default mods. Because of this change one acceptance test becomes obsolete: it no longer fails and just works.

david22swan

LGTM

david22swan · 2022-07-04T10:54:56Z

Ok, everything's gone green so I'm happy to merge.

ekohl requested a review from a team as a code owner June 28, 2022 09:22

ekohl mentioned this pull request Jun 28, 2022

Stricter data types and better module inclusion in vhost.pp #2250

Closed

ia-content added the community label Jun 28, 2022

ekohl force-pushed the include-mods-in-directories branch from 2081493 to 10a3e5f Compare June 28, 2022 10:24

ekohl force-pushed the include-mods-in-directories branch 4 times, most recently from d31eae1 to 23f775a Compare June 28, 2022 14:17

ekohl marked this pull request as draft June 28, 2022 14:19

ekohl force-pushed the include-mods-in-directories branch from 23f775a to a7876e7 Compare June 28, 2022 14:30

ekohl force-pushed the include-mods-in-directories branch from a7876e7 to d21c139 Compare June 28, 2022 14:53

ekohl mentioned this pull request Jun 29, 2022

Group assertions in vhost spec #2256

Merged

ekohl force-pushed the include-mods-in-directories branch from d21c139 to a396669 Compare June 29, 2022 12:15

ekohl mentioned this pull request Jul 4, 2022

Narrow down Datatypes #2245

Merged

ekohl added 3 commits July 4, 2022 11:28

Properly require proxy mods

44eb382

This ensures the proxy module is always loaded at the right time.

Factor out mod_authz_groupfile to its own class

c1a9538

This makes it easier to run with default mods disabled while still using this mod.

ekohl force-pushed the include-mods-in-directories branch from a396669 to 3241a6d Compare July 4, 2022 09:30

ekohl marked this pull request as ready for review July 4, 2022 09:30

ekohl force-pushed the include-mods-in-directories branch from 3241a6d to 040eab9 Compare July 4, 2022 10:08

david22swan approved these changes Jul 4, 2022

View reviewed changes

david22swan merged commit 3839c93 into puppetlabs:main Jul 4, 2022

david22swan added the feature label Jul 4, 2022

ekohl deleted the include-mods-in-directories branch July 4, 2022 10:57

ekohl mentioned this pull request Apr 14, 2025

Detect module dependencies when modules are not default-loaded #2598

Open

3 tasks

Automatically include modules used in vhost directories #2255

Automatically include modules used in vhost directories #2255

Uh oh!

Conversation

ekohl commented Jun 28, 2022

Uh oh!

puppet-community-rangefinder bot commented Jun 28, 2022

apache::mod::proxy_ajp is a class

apache::mod::proxy_balancer is a class

apache::mod::proxy_connect is a class

apache::mod::proxy_fcgi is a class

apache::mod::proxy_html is a class

apache::mod::proxy_http is a class

apache::mod::proxy_wstunnel is a class

apache::vhost is a type

This module is declared in 174 of 579 indexed public Puppetfiles.

Uh oh!

david22swan commented Jun 28, 2022

Uh oh!

ekohl commented Jun 28, 2022

Uh oh!

ekohl commented Jun 28, 2022

Uh oh!

ekohl commented Jun 28, 2022

Uh oh!

ekohl commented Jun 28, 2022

Uh oh!

ekohl commented Jun 28, 2022

Uh oh!

david22swan commented Jun 29, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ekohl commented Jun 29, 2022

Uh oh!

ekohl commented Jun 29, 2022

Uh oh!

david22swan commented Jun 29, 2022

Uh oh!

ekohl commented Jun 29, 2022

Uh oh!

david22swan commented Jun 29, 2022

Uh oh!

ekohl commented Jul 4, 2022

Uh oh!

ekohl commented Jul 4, 2022

Uh oh!

david22swan left a comment

Choose a reason for hiding this comment

Uh oh!

david22swan commented Jul 4, 2022

Uh oh!

Uh oh!

This module is declared in 174 of 579 indexed public `Puppetfiles`.

david22swan commented Jun 29, 2022 •

edited

Loading