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

Clean up includes and templates in vhost.pp #2254

Merged
merged 16 commits into from
Jun 28, 2022

Conversation

ekohl
Copy link
Collaborator

@ekohl ekohl commented Jun 23, 2022

This localizes the includes in vhost.pp which removes some duplicated logic. Sometimes the logic was also slightly different. This keeps it consistent.

It also only includes modules if ensure is set to present. This allows users to toggle the ensure easily and only having the minimal amount of modules present.

@ekohl ekohl requested a review from a team as a code owner June 23, 2022 17:37
@puppet-community-rangefinder
Copy link

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
Copy link
Member

@ekohl Looks like some spec test failures have crept in

ekohl added 16 commits June 24, 2022 15:08
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.
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.
The data type does not allow a value that evaluates to false so no need
ot check for it.
@ekohl ekohl force-pushed the localize-includes branch from 88c2f54 to 3262f23 Compare June 24, 2022 13:08
@ekohl
Copy link
Collaborator Author

ekohl commented Jun 24, 2022

Acceptance is still running, but this looks like it'll be green.

@ekohl ekohl added the feature label Jun 24, 2022
Copy link
Member

@david22swan david22swan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@david22swan david22swan merged commit 472aa1f into puppetlabs:main Jun 28, 2022
@david22swan
Copy link
Member

And that's another one in, sorry for the wait on merge, was off yesterday

@ekohl ekohl deleted the localize-includes branch June 28, 2022 09:06
@ekohl
Copy link
Collaborator Author

ekohl commented Jun 28, 2022

No worries, thanks for the merge.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants