-
Notifications
You must be signed in to change notification settings - Fork 1.1k
[WIP] Apache 2.2 Removal #1804
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
[WIP] Apache 2.2 Removal #1804
Conversation
39a15f7
to
aace49a
Compare
c7a2d5d
to
3cd8bb7
Compare
2f560eb
to
bd5d626
Compare
spec/classes/mod/status_spec.rb
Outdated
osfamily: 'Debian', | ||
operatingsystemrelease: '8', | ||
concat_basedir: '/dne', | ||
lsbdistcodename: 'squeeze', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lsbdistcodename needs updated to jessie
spec/classes/mod/status_spec.rb
Outdated
osfamily: 'Debian', | ||
operatingsystemrelease: '8', | ||
concat_basedir: '/dne', | ||
lsbdistcodename: 'squeeze', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lsbdistcodename needs updated to jessie
let :facts do | ||
{ | ||
osfamily: 'Debian', | ||
operatingsystemrelease: '6', | ||
operatingsystemrelease: '8', |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
439991a
to
26cb112
Compare
README.md
Outdated
@@ -303,7 +303,7 @@ | |||
|
|||
## Module description | |||
|
|||
[Apache HTTP Server][] (also called Apache HTTPD, or simply Apache) is a widely used web server. This [Puppet module][] simplifies the task of creating configurations to manage Apache servers in your infrastructure. It can configure and manage a range of virtual host setups and provides a streamlined way to install and configure [Apache modules][]. | |||
[Apache HTTP Server][] (also called Apache HTTPD, or simply Apache) is a widely used web server. This [Puppet module][] simplifies the task of creating configurations to manage Apache servers in your infrastructure. It can configure and manage a range of virtual host setups and provides a streamlined way to install and configure [Apache modules][]. Please note that this module has went through an update and is now supporting only Apache 2.4 and above. For Apache < 2.4 please use an older version of the module. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should the last two sentences marked as bolt text? That helped a bit for Vox Pupuli releases. People didn't see the announcement of the breaking change within one paragraph.
@@ -1,5 +1,4 @@ | |||
Facter.add(:apache_version) do | |||
confine kernel: ['FreeBSD', 'Linux'] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is it a good idea to remove this? Was this tested on other platforms? I got many modules that I've to monkeypatch because they have no confine statement, but were made for linux only and ruin puppet runs on my FreeBSD box. I assume the same could happen here on AIX / Solaris / whereever somebody wants to run this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated and added Darwin (line below) so the tests run on our machines locally :)
} | ||
} | ||
'FreeBSD': { | ||
'RedHat','Debian','FreeBSD','Gentoo','Suse': { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure if we gain much from this case statement. What about removing it and hardcode $default
to 2.4
for now? This would allow the module to maybe be compatible to other platforms.
And then, a single class that only exposes one parameter is maybe not ideal. It works fine, but feel wrong. This this be moved into params.pp / data-in-modules?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So the main reason for this was to have the failure condition. Since we'd be dropping support from several OSes I think it's important to have that check for clarity as I expect people to be thrown by the upgrade. I should actually update the error message and make sure people know to check their OS support against the older version of the module? Otherwise I do agree with what you're saying, maybe there's a nicer way of doing it.
manifests/vhost.pp
Outdated
order => 'allow,deny', | ||
allow => 'from all', | ||
} | ||
# $_directory_version = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what was the intention here?
@@ -6,10 +6,10 @@ | |||
{ | |||
id: 'root', | |||
kernel: 'Linux', | |||
lsbdistcodename: 'squeeze', | |||
lsbdistcodename: 'jessie', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oh the fact changes will cause a lot of conflicts during the merge of this + the ubuntu PR :(
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It will, but it'll be worth it! I had to rebase after the concat_dir param was removed so I've been through it once before at least :P
Hi @HelenCampbell, thanks for the work here! I made some inline comments. |
abe3ee9
to
6f53d78
Compare
Hey @bastelfreak, I've done some updates and have some comments for replies. Thanks for taking the time to look at this! |
6f53d78
to
349b1ee
Compare
349b1ee
to
18642f6
Compare
Due to outstanding questions and grey areas around the support matrix we have made the decision to not merge this work going forward, and to halt this effort. Thank you to everyone who has dedicated time and reviews for this. As a result I'm going to close this PR. |
No description provided.