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

Remove superfluous daemon-reload code and raise minimum required puppet version to 6.1.0 #885

Closed
wants to merge 2 commits into from

Conversation

TwizzyDizzy
Copy link

Pull Request (PR) description

This removes the superfluous daemon-reload code in this module.

The upstream module (https:/camptocamp/puppet-systemd) removed the support for puppet 4 and 5 that made this reload construct necessary.

They removed their code in this PR: voxpupuli/puppet-systemd#171

On the other hand, #884 doesn't go far enough, as it simply raises the possible version of the systemd module without taking into account the EOL of Puppet 4 and 5.

This Pull Request (PR) fixes the following issues

Fixes #875

Cheers
Thomas

@wyardley
Copy link
Contributor

May want to wait for #878 for that - I’d like to get 11.1 out and then we can ship the breaking release. I think #884 was an incremental step in that direction.

@TwizzyDizzy
Copy link
Author

TwizzyDizzy commented Apr 22, 2021

Agreed, thanks for pointing this out! I'll ignore the failing Debian 8 test for the time being, as support for this seems to also be removed with #878.

Cheers
Thomas

@wyardley
Copy link
Contributor

It does mean that we probably should continue without #884 either for now. Let’s keep this open and rebase / merge after 11.1.

@wyardley
Copy link
Contributor

wyardley commented May 6, 2021

@TwizzyDizzy can you resolve the conflicts from #886, which will ship first in a non-breaking release?

@wyardley
Copy link
Contributor

wyardley commented May 6, 2021

After #878 also won't need to raise min puppet version anymore, because it includes that.

@TwizzyDizzy
Copy link
Author

TwizzyDizzy commented May 7, 2021

Hey @wyardley

Looking at the changes in master, I think my PR is obsolete

The daemon-reload code has not been removed but has an additional condition that catches the case of the upstream systemd-module version being 3.0.0 and above.

Hence, I don't think there is any need to raise the minimum required puppet version either.

On the other hand, one might still vote for raising the minimum required version on the basis of the upstream module not giving the guarantee of puppet 4+5 support anymore, so breaking changes to the rabbitmq module might occur at any given point which is something one might want to prevent categorically.

If this construct defined(Class['systemd::systemctl::daemon_reload']) is valid in Puppet 4 and 5 code, this module would still be working on 4+5.

Your thoughts?

Cheers
Thomas

@wyardley
Copy link
Contributor

wyardley commented May 7, 2021

Hence, I don't think there is any need to raise the minimum required puppet version either.

On the other hand, one might still vote for raising the minimum required version on the basis of the upstream module not giving the guarantee of puppet 4+5 support anymore, so breaking changes to the rabbitmq module might occur at any given point which is something one might want to prevent categorically.

I don't have a strong opinion on it, personally. I think if 4 / 5 are already EOL, since we've just cut 11.1, there's no reason not to drop support for it in the next release (which is already a breaking one), as well as simplify the code here further.

@wyardley
Copy link
Contributor

@TwizzyDizzy if you want to proceed with this one, could you rebase + resolve the conflicts?

@wyardley
Copy link
Contributor

Closing based on comments in #898

@wyardley wyardley closed this May 12, 2023
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.

Not compatible with latest puppet-systemd
3 participants