Skip to content

Respect $service_status on Red Hat-based distros #1351

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
Merged
merged 1 commit into from
Jul 4, 2022
Merged

Respect $service_status on Red Hat-based distros #1351

merged 1 commit into from
Jul 4, 2022

Conversation

ekohl
Copy link
Collaborator

@ekohl ekohl commented Jul 4, 2022

Fixes: 7b282bd

@ekohl ekohl requested a review from a team as a code owner July 4, 2022 09:52
@puppet-community-rangefinder
Copy link

postgresql::params is a class

Breaking changes to this file WILL impact these 2 modules (exact match):
Breaking changes to this file MAY impact these 3 modules (near match):

This module is declared in 70 of 579 indexed public Puppetfiles.

These results were generated with Rangefinder, a tool that helps predict the downstream impact of breaking changes to elements used in Puppet modules. You can run this on the command line to get a full report.

Exact matches are those that we can positively identify via namespace and the declaring modules' metadata. Non-namespaced items, such as Puppet 3.x functions, will always be reported as near matches only.

@ekohl ekohl mentioned this pull request Jul 4, 2022
Merged
bastelfreak
bastelfreak reviewed Jul 4, 2022
View reviewed changes
manifests/params.pp
@@ -84,10 +84,10 @@

if pick($service_provider, $facts['service_provider']) == 'systemd' {
$service_reload = "systemctl reload ${service_name}"
$service_status = "systemctl status ${service_name}"
$service_status = pick($service_status, "systemctl status ${service_name}")
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I checked how/when $service_status is set and there is a lot that could be improved/simplified here :(

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See #1349 for more discussion on that. I also wonder if this isn't really redundant since AFAIK Puppet itself can determine the status using these commands.

david22swan
david22swan approved these changes Jul 4, 2022
View reviewed changes
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

@david22swan
Copy link
Member

Looks like a good improvement to me

@david22swan david22swan merged commit f417f00 into puppetlabs:main Jul 4, 2022
@pmcmaw pmcmaw added the bugfix label Jul 21, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bastelfreak bastelfreak bastelfreak approved these changes

@david22swan david22swan david22swan approved these changes

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

Successfully merging this pull request may close these issues.
5 participants
@ekohl @david22swan @bastelfreak @pmcmaw @ia-content