Skip to content

[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

Closed
wants to merge 9 commits into from

Conversation

HelenCampbell
Copy link
Contributor

No description provided.

@HelenCampbell HelenCampbell force-pushed the 2.2removal branch 7 times, most recently from 39a15f7 to aace49a Compare July 9, 2018 12:50
@HelenCampbell HelenCampbell changed the title Modification of version code and apache_spec fixes [WIP] Apache 2.2 Removal Jul 11, 2018
@HelenCampbell HelenCampbell force-pushed the 2.2removal branch 21 times, most recently from 2f560eb to bd5d626 Compare July 20, 2018 13:58
osfamily: 'Debian',
operatingsystemrelease: '8',
concat_basedir: '/dne',
lsbdistcodename: 'squeeze',
Copy link
Contributor

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

osfamily: 'Debian',
operatingsystemrelease: '8',
concat_basedir: '/dne',
lsbdistcodename: 'squeeze',
Copy link
Contributor

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.

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.
Copy link
Collaborator

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']
Copy link
Collaborator

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?

Copy link
Contributor Author

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': {
Copy link
Collaborator

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?

Copy link
Contributor Author

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.

order => 'allow,deny',
allow => 'from all',
}
# $_directory_version = {
Copy link
Collaborator

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',
Copy link
Collaborator

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 :(

Copy link
Contributor Author

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

@bastelfreak
Copy link
Collaborator

Hi @HelenCampbell, thanks for the work here! I made some inline comments.

@HelenCampbell HelenCampbell force-pushed the 2.2removal branch 4 times, most recently from abe3ee9 to 6f53d78 Compare August 8, 2018 09:56
@HelenCampbell
Copy link
Contributor Author

Hey @bastelfreak, I've done some updates and have some comments for replies. Thanks for taking the time to look at this!

@HelenCampbell
Copy link
Contributor Author

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.

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

Successfully merging this pull request may close these issues.

4 participants