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

ekohl · 2022-06-15T11:39:09Z

There is no need to call defined() before calling include since include handles it already.

It also consistently checks if $ensure is set to present before including modules.

puppet-community-rangefinder · 2022-06-15T11:39:17Z

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.

ekohl · 2022-06-15T12:31:46Z

I included some of these changes in #2251 so perhaps that should be merged before this and then I can rebase.

ekohl · 2022-06-22T10:51:25Z

So the scope of this PR expanded a bit. It continues the trend that #2251 started. I've tried to keep it reviewable by having small commits.

david22swan · 2022-06-23T08:49:17Z

@ekohl Look's like the error is coming from aliases

  2267) apache::vhost os-independent items on debian-11-x86_64  set everything! is expected to contain Concat::Fragment[rspec.example.com-apache-header] with content =~ /^MDomain example\.com example\.net auto$/
        Failure/Error:
          is_expected.to contain_concat__fragment('rspec.example.com-apache-header').with(
            content: %r{^MDomain example\.com example\.net auto$},
          )
        Puppet::PreformattedError:
          Evaluation Error: Error while evaluating a Resource Statement, Apache::Vhost[rspec.example.com]: parameter 'aliases' index 0 expects a Hash value, got String (line: 3) on node fv-az180-264.aibneqh1mxpuhl3tnnuy2ifbce.bx.internal.cloudapp.net
        # /opt/hostedtoolcache/Ruby/2.5.9/x64/lib/ruby/2.5.0/benchmark.rb:308:in `realtime'
        # ./spec/defines/vhost_spec.rb:1715:in `block (6 levels) in <top (required)>'
        # ./vendor/bundle/ruby/2.5.0/bin/rspec:23:in `load'
        # ./vendor/bundle/ruby/2.5.0/bin/rspec:23:in `<top (required)>'
        # ./vendor/bundle/ruby/2.5.0/bin/bundle:23:in `load'
        # ./vendor/bundle/ruby/2.5.0/bin/bundle:23:in `<main>'
        # ------------------
        # --- Caused by: ---
        # Puppet::ParseError:
        #   Apache::Vhost[rspec.example.com]: parameter 'aliases' index 0 expects a Hash value, got String
        #   /opt/hostedtoolcache/Ruby/2.5.9/x64/lib/ruby/2.5.0/benchmark.rb:308:in `realtime'

Think this is the only one, just seems to trigger a lot of tests

This also implements a real test on the rendered template. It appears it was passing in something invalid ever since it was introduced in f1d64a0.

There is no need to check if the variables are true because the data type doesn't allow anything that evaluates to false. The logic to include it is also moved to the template which makes it easier to follow the logic. The same is applied to the template.

The data type only allows an array for scriptaliases so there is no need to check for the type. It is also checked at in vhost.pp that the value is set so there's no need to repeat that logic in the template.

This is now only included if docroot evaluates to true. The template does suggest that it should be `($docroot or $virtual_docroot)` but this remains compatible with the previous code.

The apache::mod::mime inclusion is dropped since apache::mod::ssl already includes it (since fc8fee7) and apache/vhost/_ssl.erb does not use AddType.

The data type does not allow a value that evaluates to false so no need ot check for it.

Fixes: 2349ba7

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

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-06-23T09:14:49Z

Yes, I had some trouble finding all instances. I do wonder how it ever worked. It was introduced in f1d64a0, but it doesn't look like it ever produced anything valid by passing a plain string. Let's see if this version is better. I also had to fix a test related to directories.

ekohl · 2022-06-23T12:16:25Z

The rubocop violation looks weird to me. Possibly an issue with Rubocop? I don't quite see it.

Edit: and the acceptance test now on longer fails because it automatically loads the right modules. Should I drop the test? I'm not sure this should be the expected behavior.

david22swan · 2022-06-23T14:16:13Z

@ekohl In regards to the acceptance test....... it should probably be removed now yes if the behaviour it is expected to catch will no longer occur with your improvement in place.
Though would like a more descriptive title and comment for what what seems a substantial change in behaviour.

In regards to the Rubocop error........ Rubocop is just being annoying. Took a look and if you move aliases up so that it is not immediately followed by directories and there is another variable between them, the error goes away.
Don't know why, guess Rubocop doesn't like two arrays formatted in that manner, one after another?

ekohl · 2022-06-23T14:20:06Z

I think I'll pull out the last 2 patches to their own PR. Would it make sense to even make it 3 PRs? The first commit, then a series of patches to localize the imports and then a third for the last 2 commits?

david22swan · 2022-06-23T14:29:03Z

If you're willing to separate them out into three then that does sound good to me yes.
This PR does seem to be doing multiple different things and that did throw me for a bit when I first looked at it, so dividing it up and given each piece of work its own pr may be for the best.

ekohl · 2022-06-23T15:32:26Z

Setting this back to a draft. I've submitted #2253 as a first PR. Once it's merged I'll submit the next.

ekohl · 2022-06-23T17:38:12Z

Part 2: #2254

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

And the last part: #2255

ekohl requested a review from a team as a code owner June 15, 2022 11:39

ia-content added the community label Jun 15, 2022

ekohl marked this pull request as draft June 15, 2022 12:31

chelnak added the feature label Jun 20, 2022

ekohl force-pushed the consistent-include branch from d57be7f to d1d8bc4 Compare June 22, 2022 10:48

ekohl marked this pull request as ready for review June 22, 2022 10:48

ekohl force-pushed the consistent-include branch from d1d8bc4 to dd2b2b9 Compare June 22, 2022 10:50

ekohl force-pushed the consistent-include branch 2 times, most recently from 4dfbdab to 256cb0f Compare June 22, 2022 15:55

ekohl changed the title ~~Make module inclusion consistent in vhost.pp~~ Stricter data types and make module inclusion consistent in vhost.pp Jun 22, 2022

ekohl changed the title ~~Stricter data types and make module inclusion consistent in vhost.pp~~ Stricter data types and better module inclusion in vhost.pp Jun 22, 2022

ekohl force-pushed the consistent-include branch from 256cb0f to da6ba3b Compare June 22, 2022 16:11

ekohl added 14 commits June 23, 2022 11:13

Use a stricter data type on apache::vhost::aliases

11d37f4

This also implements a real test on the rendered template. It appears it was passing in something invalid ever since it was introduced in f1d64a0.

Correct parameter alignment

6446580

Include apache::mod::alias locally

7bf8263

Include apache::mod::proxy locally

a69c33a

Inlcude apache::mod::fastcgi locally

3d50157

Simplify scriptalias template

0df7f8f

The data type only allows an array for scriptaliases so there is no need to check for the type. It is also checked at in vhost.pp that the value is set so there's no need to repeat that logic in the template.

Localize apache::mod::vhost_alias inclusion

e92c195

This is now only included if docroot evaluates to true. The template does suggest that it should be `($docroot or $virtual_docroot)` but this remains compatible with the previous code.

Localize apache::mod::wsgi inclusion

2f376bc

Localize apache::mod::suexec inclusion

8ba9830

Localize apache::mod::passenger inclusion

dfd14f9

Localize apache::mod::auth_kerb inclusion

e05f97f

Localize apache::mod::auth_openidc inclusion

b5d74a9

Localize apache::mod::ssl inclusion

342b160

The apache::mod::mime inclusion is dropped since apache::mod::ssl already includes it (since fc8fee7) and apache/vhost/_ssl.erb does not use AddType.

ekohl added 5 commits June 23, 2022 11:14

Simplify apache::vhost::serveraliases logic

d4320a7

The data type does not allow a value that evaluates to false so no need ot check for it.

Document uses of $mdomain

0c5c847

Fixes: 2349ba7

Only include modules if ensure is present

9951bec

Properly require proxy mods

619e9c9

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

ekohl force-pushed the consistent-include branch from da6ba3b to 18eed8e Compare June 23, 2022 09:14

david22swan added backwards-incompatible feature and removed feature backwards-incompatible labels Jun 23, 2022

ekohl marked this pull request as draft June 23, 2022 15:32

ekohl mentioned this pull request Jun 28, 2022

Automatically include modules used in vhost directories #2255

Merged

ekohl closed this Jun 28, 2022

ekohl deleted the consistent-include branch June 28, 2022 09:24

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

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

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

Uh oh!

ekohl commented Jun 15, 2022

Uh oh!

puppet-community-rangefinder bot commented Jun 15, 2022

Uh oh!

ekohl commented Jun 15, 2022

Uh oh!

ekohl commented Jun 22, 2022

Uh oh!

david22swan commented Jun 23, 2022

Uh oh!

ekohl commented Jun 23, 2022

Uh oh!

ekohl commented Jun 23, 2022 •

edited

Loading

Uh oh!

david22swan commented Jun 23, 2022

Uh oh!

ekohl commented Jun 23, 2022

Uh oh!

david22swan commented Jun 23, 2022

Uh oh!

ekohl commented Jun 23, 2022

Uh oh!

ekohl commented Jun 23, 2022

Uh oh!

ekohl commented Jun 28, 2022

Uh oh!

Uh oh!

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

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

Uh oh!

Conversation

ekohl commented Jun 15, 2022

Uh oh!

puppet-community-rangefinder bot commented Jun 15, 2022

apache::vhost is a type

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

Uh oh!

ekohl commented Jun 15, 2022

Uh oh!

ekohl commented Jun 22, 2022

Uh oh!

david22swan commented Jun 23, 2022

Uh oh!

ekohl commented Jun 23, 2022

Uh oh!

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

Uh oh!

Uh oh!

david22swan commented Jun 23, 2022

Uh oh!

ekohl commented Jun 23, 2022

Uh oh!

david22swan commented Jun 23, 2022

Uh oh!

ekohl commented Jun 23, 2022

Uh oh!

ekohl commented Jun 23, 2022

Uh oh!

ekohl commented Jun 28, 2022

Uh oh!

Uh oh!

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

ekohl commented Jun 23, 2022 •

edited

Loading