-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Drop Apache 2.2 support #2329
Drop Apache 2.2 support #2329
Conversation
manifests/default_mods.pp
Outdated
| if $facts['os']['name'] != 'Amazon' && $use_systemd { | ||
| ::apache::mod { 'systemd': } | ||
| } | ||
| if ($facts['os']['name'] == 'Amazon' and $facts['os']['release']['full'] == '2') { | ||
| ::apache::mod { 'systemd': } | ||
| } |
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.
Here I wonder what the best approach is. Why doesn't Amazon Linux support the use_systemd flag? And I think Amazon Linux 2022 is also out, which also has systemd. Should we simplify? Related: in other places I dropped Amazon Linux 1 support since it's based on EL6.
4bbb2bd
to
9b3daa8
Compare
9b3daa8
to
1ac7729
Compare
|
This is now green. |
|
@ekohl This look's good to me so far, haven't finished the review yet though. |
|
@ekohl As a note, there was previously work done by a former member of our team to remove 2.2 support from the module. While it was unable to be merged at the time due to push back from customers and the community I have made sure to keep the branch saved. |
|
I'm pretty occupied for the next week or 2 so feel free to use my code. |
|
@ekohl From how you had responded had thought that this needed more work to get it over the line, but looking now it seems to be complete to me. In your opinion is there anything that needs one on it other than rebasing? |
1ac7729
to
fa01560
Compare
|
Since #2329 (comment) I think this is ready. I've just rebased this. |
|
Ok, will give it one last review and make a clean release before merging it in. Apologies for my misunderstanding of the situation |
|
No worries, next time I'll be more explicit. I do use the draft feature quite explicitly to mark my work as in progress. |
|
@ekohl Look's like a small error in , |
|
No, it's because I only partially addressed #2331. The Puppet code gave a merge conflict so I addressed that and fixed up the template as well, but forgot the tests. |
fa01560
to
cedd45b
Compare
| @@ -6,37 +6,4 @@ | |||
| Optional[String] $scl_httpd_version = undef, | |||
| Optional[String] $scl_php_version = undef, | |||
| ) { | |||
| case $facts['os']['family'] { | |||
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.
Seeing as how everything has been removed should this not just be deleted?
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 wonder. It does still have class parameters that can be used. Those could also be added to params.pp, but I don't know what's best here.
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's a hard question. I guess for now it may be best to leave them in place, at least until hiera is properly implemented within the module. Just so we have a clear place to set them.
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.
One comment, otherwise everything LGTM
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.
LGTM
Will merge once I can get a clean pre-major release out
|
This is awesome ❤️ |
|
Well, that's that |
This is likely incomplete and untested at the moment. However, it shows how big the cleanup is.