Skip to content
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.

Sign up for GitHub

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

Jump to bottom

ensure mpm_event is disabled under debian 9 if mpm itk is used #1766

Merged
merged 1 commit into from
Nov 9, 2018
Merged

ensure mpm_event is disabled under debian 9 if mpm itk is used #1766

merged 1 commit into from
Nov 9, 2018

Conversation

zivis
Copy link
Contributor

@zivis zivis commented Feb 28, 2018

On debian 9 using mod_itk as mpm with:
apache 2.4.25-3+deb9u3 and
libapache2-mpm-itk 2.4.7-04-1

You have to disable mpm_event before installing libapache2-mpm-itk …

smells a bit like https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=734865 although it should be fixed…

@eputnam eputnam added the bugfix label Mar 2, 2018
eputnam added a commit to eputnam/puppetlabs-apache that referenced this pull request Mar 2, 2018
@eputnam
(MODULES-6677) make sure event mpm is disabled
654a496 
Currently, on Debian 9 specifically, the event mod may be enabled when trying to use a different mpm. This ensures event is disabled if another mpm is chosen. Also closes puppetlabs#1766
@eputnam eputnam mentioned this pull request Mar 2, 2018
Closed
@punycode punycode force-pushed the mpm_event_disable_debian9_itk branch from bfad6fa to b22bdbb Compare August 3, 2018 22:08
zivis pushed a commit to synyx/puppetlabs-apache that referenced this pull request Nov 8, 2018
@eputnam @zivis
(MODULES-6677) make sure event mpm is disabled
2155e7c 
Currently, on Debian 9 specifically, the event mod may be enabled when trying to use a different mpm. This ensures event is disabled if another mpm is chosen. Also closes puppetlabs#1766
@zivis zivis force-pushed the mpm_event_disable_debian9_itk branch 11 times, most recently from b92c400 to 6e8537a Compare November 8, 2018 15:28
@HelenCampbell HelenCampbell mentioned this pull request Nov 8, 2018
Closed
@HelenCampbell
Copy link
Contributor

Hey @zivis , I've started work to try and get a fix in for this mpm issue merged. As you've seen there are multiple PRs that were open to address this issue.
There are 3 PR's I can see that provide similar functionality for fixing this: #1766 , #1767 and #1815 .
I'd definitely like to consolidate and work with one of these PRs to get this merged. Currently I think that #1767 is in the best shape for this, although I do note that you're still active on this PR so wanted to message before doing anything.
Would you be happy for us to focus on #1767 instead of this PR? I think there's only a small fix that needs to go into #1767 to get it across the line that I can work on to get it merged.
Thanks!

@zivis
Copy link
Contributor Author

zivis commented Nov 8, 2018

Hi @HelenCampbell
I had the same intention wanna get that fixed :)

I have reworked this branch/commit to also fix the ubuntu 18.04 issue mentioned in #1767
so just consider to merge this one…

@zivis zivis force-pushed the mpm_event_disable_debian9_itk branch from 6e8537a to a81d1af Compare November 8, 2018 16:04
@HelenCampbell
Copy link
Contributor

More than happy enough to get this PR merged instead, though there's a few changes that'd be required if you'd be happy enough I put this through review:
Would you consider moving back to the 'exec' format instead of using 'ensure_resource'? Seems like it causes duplicate declaration issues: #1767 (comment)

HelenCampbell
HelenCampbell reviewed Nov 8, 2018
View reviewed changes
manifests/mpm.pp Outdated Show resolved Hide resolved
@zivis zivis force-pushed the mpm_event_disable_debian9_itk branch from a81d1af to 38f9adc Compare November 8, 2018 16:31
@zivis
Copy link
Contributor Author

zivis commented Nov 8, 2018

i have a problem now, the run is not idempotent, i'll try to fix it…

@zivis zivis force-pushed the mpm_event_disable_debian9_itk branch from 38f9adc to 3c2f61a Compare November 8, 2018 16:44
@zivis
Copy link
Contributor Author

zivis commented Nov 8, 2018

it seems the run is now idempotent.

@zivis zivis force-pushed the mpm_event_disable_debian9_itk branch 4 times, most recently from 385d2d1 to af2d48d Compare November 8, 2018 17:02
@zivis zivis force-pushed the mpm_event_disable_debian9_itk branch from af2d48d to 517a776 Compare November 8, 2018 17:05
@zivis
Copy link
Contributor Author

zivis commented Nov 8, 2018

sorry @HelenCampbell for this being so hard in progress… i think travis should be happy now ;) but many thanks to you for taking time to review this :)

the ensure_resource is needed to prevent duplicate resources.
The duplicate resources mentioned in #1767 are the problem if "file" is used.

@HelenCampbell
Copy link
Contributor

No problem @zivis ! Thank you for working with all my feedback, this is looking in great shape :) It's the end of my working day but I'll run this through one of our pipelines tomorrow and if it's good to go I'll get it merged in.

@zivis zivis force-pushed the mpm_event_disable_debian9_itk branch 3 times, most recently from b289d0c to eceba27 Compare November 9, 2018 06:15
@zivis zivis force-pushed the mpm_event_disable_debian9_itk branch from eceba27 to 22cece7 Compare November 9, 2018 06:42
@zivis
Copy link
Contributor Author

zivis commented Nov 9, 2018

I have tested against ubuntu 18.04, Debian 8/9 with mod_prefork and itk.
Tests were successful

@HelenCampbell HelenCampbell merged commit d297285 into puppetlabs:master Nov 9, 2018
@HelenCampbell
Copy link
Contributor

Merged, Thank you for all your work! 🥇

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

@HelenCampbell HelenCampbell HelenCampbell left review comments

@david22swan david22swan david22swan approved these changes

Assignees
No one assigned
Labels
bugfix
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@zivis @HelenCampbell @david22swan @eputnam