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

(MODULES-5990) Addition of 'IncludeOptional conf-enabled/*.conf' to apache2.conf' on Debian Family OS #1851

Merged
merged 1 commit into from
Dec 4, 2018

Conversation

david22swan
Copy link
Member

…apache2.conf' on Debian Family OS

@@ -109,6 +109,9 @@ LogFormat "<%= format -%>" <%= nickname %>
<%- end -%>
<% end -%>

<%- if scope.function_versioncmp([@apache_version, '2.4']) >= 0 -%>
IncludeOptional "<%= @conf_dir %>conf-enabled/*.conf"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Missing at least a slash. This will result in /etc/apache2conf-enabled/*.conf.

It's also rather ugly to do this on all distros, even those that don't have this mechanism. I wonder if we need a $conf_enable_dir that's undef on all other distros so you can test for it in the template.

Copy link
Member Author

Choose a reason for hiding this comment

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

@ekohl Have updated the code to be along the lines of what you suggested

@david22swan david22swan force-pushed the MODULES-5990 branch 2 times, most recently from 8a00e20 to 726417f Compare November 27, 2018 10:47
@david22swan david22swan changed the title (MODULES-5990) Addition of 'IncludeOptional conf-enabled/*.conf' to '… (WIP) (MODULES-5990) Addition of 'IncludeOptional conf-enabled/*.conf' to '… Nov 27, 2018
@david22swan david22swan force-pushed the MODULES-5990 branch 2 times, most recently from 764931b to a31a2dc Compare November 30, 2018 15:37
…apache2.conf' on Debian Family OS of 9/18.04 onwards
@david22swan david22swan changed the title (WIP) (MODULES-5990) Addition of 'IncludeOptional conf-enabled/*.conf' to '… (MODULES-5990) Addition of 'IncludeOptional conf-enabled/*.conf' to '… Nov 30, 2018
@david22swan
Copy link
Member Author

screen shot 2018-12-04 at 9 10 31 am

@HelenCampbell HelenCampbell merged commit 518a6dc into puppetlabs:master Dec 4, 2018
@david22swan david22swan deleted the MODULES-5990 branch December 4, 2018 15:50
@pmcmaw pmcmaw changed the title (MODULES-5990) Addition of 'IncludeOptional conf-enabled/*.conf' to '… (MODULES-5990) Addition of 'IncludeOptional conf-enabled/*.conf' to apache2.conf' on Debian Family OS Dec 5, 2018
@pmcmaw pmcmaw added the feature label Dec 14, 2018
aelkiss added a commit to mlibrary/nebula that referenced this pull request Dec 17, 2018
puppetlabs/puppetlabs-apache#1851 in upstream
puppet apache module expects this directory to be present. We still want
to make sure there are no unmanaged files there.
@baurmatt
Copy link
Contributor

baurmatt commented Jan 8, 2019

I think this is missing something. If the folder gets included by default, it should be managed by default as well. Especially since the default for config dirs is to purge them.
If it doesn't get managed/purged, the default files shipped with e.g. Ubuntu 16.04 get included and might break some stuff.

@david22swan What do you think about it? I'm happy to fix it myself in a PR, just want your opinion if this seems reasonable.

@david22swan
Copy link
Member Author

@baurmatt I recently created a pr to set it so it is no longer included by default (#1869) after it caused issues with another user. But if you have some ideas on how to improve it so that it is managed when included then I encourage you to go for it.

@baurmatt
Copy link
Contributor

baurmatt commented Jan 8, 2019

@david22swan Thats, of course, an option as well ;) I thought about doing this:

class apache (
...
  Optional[Stdlib::Absolutepath] $conf_enabled = $apache::params::conf_enabled,
...
) {
...
if $conf_enabled {
  file { $conf_enabled:
    ensure  => directory,
    recurse => true,
    purge    => $purge_confd,
    force     => $purge_confd,
    notify    => Class['Apache::Service'],
    require   => Package['httpd'],
  }
...
}

While writing this I realized the /etc/apache/conf-enabled is the correct directory for Debian based systems instead of conf.d, right? Should we just change the $confd_dir for Debian based systems to "${httpd_dir}/conf-enabled"?

@david22swan
Copy link
Member Author

@baurmatt Sorry for the wait, I looked into what you said and the code addition looks good. However the second part, about changing the directory from conf.d, I am not so sure. Honestly my overall apache knowledge isn't the greatest and after looking into it I can't see a strong reason for it.
Could you explain your reasoning a bit more?

@baurmatt
Copy link
Contributor

baurmatt commented Jan 9, 2019

@david22swan No worries! :)

As far as I remember, the conf.d directory isn't available by default on Debian based systems.It's a RedHat-ish thing. On Debian based systems the folder is called conf-enabled. Therefore I think it makes sense to replace the conf.d directory with conf-enabled.

@david22swan
Copy link
Member Author

@baurmatt I'm still unsure, if you want to go ahead and start work on that I won't stop you, but it's not something that would get merged in quickly and would require some serious discussion throughout my team and not just me.
Sorry for not being more help.

cegeka-jenkins pushed a commit to cegeka/puppet-apache that referenced this pull request Jul 15, 2020
(MODULES-5990) Addition of 'IncludeOptional conf-enabled/*.conf' to '…
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants