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.

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

Restructure MPM disabling #2227

Merged
merged 1 commit into from
Apr 5, 2022

Conversation

ekohl
Copy link
Collaborator

@ekohl ekohl commented Mar 29, 2022

This takes the approach of listing the disabling steps once rather than repeating it for every mpm.

I wasn't quite sure on the prefork disabling. Does prefork need to stay enabled for itk?

@david22swan
Copy link
Member

Not sure about this change, while it cuts down on repetition it may also make the code harder for new users to read through. In addition, look's like it's causing a mass failure in the spec tests.

@ekohl
Copy link
Collaborator Author

ekohl commented Mar 30, 2022

Not sure about this change, while it cuts down on repetition it may also make the code harder for new users to read through

Funny, I had great problems with the duplication and think this is much easier to follow. My logic is:

If MPM is used
  configure it
else
  remove it
end

If there is no configuring it ends up being

if not MPM is used
  remove it
end

And the mass failures can easily be explained by my syntax error. Due to https://tickets.puppetlabs.com/browse/MODULES-11161 I can't run PDK module tests locally so I have to rely on CI. Sadly it makes contributing to Puppetlabs modules really hard for me. Normally I ignore any PDK module but puppetlabs modules are hard to ignore.

Edit: Because of 400c75d it meant it ran the whole test suite instead of verifying it.

This takes the approach of listing the disabling steps once rather than
repeating it for every mpm.

I wasn't quite sure on the prefork disabling. Does prefork need to stay
enabled for itk?
@david22swan david22swan merged commit 8c38469 into puppetlabs:main Apr 5, 2022
@david22swan
Copy link
Member

Looking back at this, and after confusing myself and then realising mistake. This looks like a good change.
Thank's for putting in the work :)

@ekohl ekohl deleted the restructure-mpm-disabling branch April 5, 2022 10:01
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