- 
                Notifications
    
You must be signed in to change notification settings  - Fork 1.1k
 
Drop Apache 2.2 support #2329
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
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
  
    | 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.