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

Fix default value for $service_status on ArchLinux #1410

Merged
merged 1 commit into from
Jun 30, 2023

Conversation

smortex
Copy link
Collaborator

@smortex smortex commented Mar 27, 2023

The value can be overriden from postgresql::globals, but if no
override is provided by the user, the module should use a valid default.
ArchLinux use systemd, so use the relevant systemctl command.

Fixes #1408

@smortex smortex requested a review from a team as a code owner March 27, 2023 17:13
@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 580 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.

Copy link
Collaborator

@ekohl ekohl left a comment

Choose a reason for hiding this comment

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

Isn't it better to change the type definition to allow Undef? Then the service provider can figure it out by itself.

Actually something we should do for most OSes anyway instead of writing out the systemd command.

@smortex
Copy link
Collaborator Author

smortex commented Mar 27, 2023

That make sense: this module has a load of legacy and I trend to contribute to it by copying the patterns I see in the other OS code and are missing rather than killing everything with fire and make everything simple. I'll work on it on another PR.

@smortex
Copy link
Collaborator Author

smortex commented Mar 27, 2023

Ah, these commands are used by the "instance" reloading code (when a single service manage multiple clusters). This rely on an exec resource that run these commands, so they can't be undef and must be explicitly set ATM.

I guess the initial idea was to allow to restart a single cluster (i.e. pg_ctlcluster 11 main restart on debian) but may have not been fully implemented (yet)?

@ekohl
Copy link
Collaborator

ekohl commented Mar 27, 2023

@SimonHoenscheid is working on that, but this isn't going to work when it's defined in the class. Perhaps it needs to be modified to something interpolated, though a safe implementation would use an array. I wonder what's best to do here.

@smortex
Copy link
Collaborator Author

smortex commented Mar 31, 2023

While the PR is not perfect, it can unbreak the module for ArchLinux and that would unbreak CI for some of our Voxpupuli modules. Maybe this fix can go as it is for now (and be released as a patch version), and updated again when we have some cluster management code which would be at least a minor version bump?

genebean
genebean previously approved these changes Mar 31, 2023
Copy link

@genebean genebean left a comment

Choose a reason for hiding this comment

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

This seems inline with other code within the file. Refactoring the process seems like something for a different PR to me.

ekohl
ekohl previously approved these changes Mar 31, 2023
Copy link
Collaborator

@ekohl ekohl left a comment

Choose a reason for hiding this comment

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

Fair enough.

@smortex smortex dismissed stale reviews from ekohl and genebean via bd412ea March 31, 2023 18:24
@smortex
Copy link
Collaborator Author

smortex commented Mar 31, 2023

Rebased on top of main so that the branch does not appear out-of-date for GitHub (Is that a repo requirement or maybe I got fooled by GitHub, this nuked all reviews 😒)?

@Phil-Friderici
Copy link

@puppetlabs/modules any chance to review this very tiny PR that is only one line and would help me/us greatly ?

The value can be overriden from `postgresql::globals`, but if no
override is provided by the user, the module should use a valid default.
ArchLinux use systemd, so use the relevant `systemctl` command.

Fixes #1408
@bastelfreak
Copy link
Collaborator

I rebased it against main to fix the SLES errors.

Copy link
Collaborator

@bastelfreak bastelfreak left a comment

Choose a reason for hiding this comment

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

as mentioned in the comments, that's not perfect but will fix Arch Linux.

@bastelfreak bastelfreak merged commit 9534a43 into main Jun 30, 2023
37 checks passed
@Phil-Friderici
Copy link

@bastelfreak THANK YOU, Thank You, thank you

@rajat-puppet rajat-puppet deleted the arch-service-status branch June 6, 2024 14:45
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.

postgresql::params does not have a valid default value for $service_status on ArchLinux
7 participants