Skip to content

Conversation

@ekohl
Copy link
Collaborator

@ekohl ekohl commented Oct 10, 2022

This is likely incomplete and untested at the moment. However, it shows how big the cleanup is.

Comment on lines 16 to 21
if $facts['os']['name'] != 'Amazon' && $use_systemd {
::apache::mod { 'systemd': }
}
if ($facts['os']['name'] == 'Amazon' and $facts['os']['release']['full'] == '2') {
::apache::mod { 'systemd': }
}
Copy link
Collaborator Author

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.

@ekohl ekohl force-pushed the drop-apache-2.2 branch 5 times, most recently from 4bbb2bd to 9b3daa8 Compare October 19, 2022 18:33
@ekohl ekohl marked this pull request as ready for review October 25, 2022 00:11
@ekohl ekohl requested a review from a team as a code owner October 25, 2022 00:11
@ekohl
Copy link
Collaborator Author

ekohl commented Oct 25, 2022

This is now green.

@david22swan
Copy link
Member

@ekohl This look's good to me so far, haven't finished the review yet though.
Will need to wait a bit on the merge though, give some time to get any other PRs that are near completion in a merged so we can cut a pre-major release, followed by some work to get any other backwards incompatible changes that we want in.

@david22swan
Copy link
Member

david22swan commented Nov 2, 2022

@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.
It is currently pushed up as 2.2removal on my fork. Was planning to work on rebasing it when I had the free time but if you want to use it as a help with this PR here it may be off assistance.

@ekohl
Copy link
Collaborator Author

ekohl commented Nov 2, 2022

I'm pretty occupied for the next week or 2 so feel free to use my code.

@david22swan
Copy link
Member

@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?

@ekohl
Copy link
Collaborator Author

ekohl commented Dec 14, 2022

Since #2329 (comment) I think this is ready. I've just rebased this.

@david22swan
Copy link
Member

Ok, will give it one last review and make a clean release before merging it in.

Apologies for my misunderstanding of the situation

@ekohl
Copy link
Collaborator Author

ekohl commented Dec 14, 2022

No worries, next time I'll be more explicit. I do use the draft feature quite explicitly to mark my work as in progress.

@david22swan
Copy link
Member

@ekohl Look's like a small error in , /spec/classes/mod/worker_spec.rb:55
Possible that an instance of max_clients was added back in by the rebase.

@ekohl
Copy link
Collaborator Author

ekohl commented Dec 14, 2022

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.

Optional[String] $scl_httpd_version = undef,
Optional[String] $scl_php_version = undef,
) {
case $facts['os']['family'] {
Copy link
Member

@david22swan david22swan Dec 14, 2022

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?

Copy link
Collaborator Author

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.

Copy link
Member

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.

Copy link
Member

@david22swan david22swan left a 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

Copy link
Member

@david22swan david22swan left a 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

@david22swan
Copy link
Member

Ok, that's the release cut.
Will get this merged in tomorrow alongside #2350 and whatever PRs arise from #2291

@chelnak
Copy link
Contributor

chelnak commented Dec 14, 2022

This is awesome ❤️

@david22swan david22swan merged commit eee2018 into puppetlabs:main Dec 15, 2022
@david22swan
Copy link
Member

Well, that's that
Thank you for all the work :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants