Fix scope of disable_maintenance param #73
Conversation
pe_databases is a classthat may have no external impact to Forge modules. This module is declared in 0 of 576 indexed public
|
|
Gave it the ol' "Works on my box" test 👍 |
manifests/init.pp
Outdated
| @@ -30,7 +30,7 @@ | |||
| if $facts.dig('pe_databases', 'have_systemd') { | |||
| if $manage_database_maintenance and (versioncmp('2019.0.2', $facts['pe_server_version']) <= 0) { | |||
| class {'pe_databases::pg_repack': | |||
| disable_maintenance => lookup('pe_databases::maintenance::disable_maintenance', {'default_value' => false}), | |||
| disable_maintenance => lookup('pe_databases::disable_maintenance', {'default_value' => false}), | |||
There was a problem hiding this comment.
What does the $manage_database_maintenance parameter do from line 7? Can we move this lookup to that parameter?
There was a problem hiding this comment.
yeah the lookup seems like an extra step, can we just do something like maintenance_ensure = 'present' and set disable_maintenance (or change that naming also) in the pe_databases::pg_repack based on the parameter and not the lookup
There was a problem hiding this comment.
I'm not sure it was written this way originally. Looking at it again, I think it had something to do with disabling the vacuum class when enabling the repack class. Since we don't do that anymore, I think it makes sense to just yank out this disable_maintenance part. To me it makes sense to have this determined by pe_databases::manage_database_maintenance alone.
There was a problem hiding this comment.
Alright my previous comment is totally wrong. We need to keep the parameter at least for backwards compatibility, but also because we need a way to include the repack class and manage absent/ensure on the services and timers. So I think moving the lookup to a new parameter makes sense. I'll test that out and push a commit here if it works.
There was a problem hiding this comment.
The warning message triggers when $manage_database_maintenance is set to false
puppetlabs-pe_databases/manifests/init.pp
Line 48 in c9755e1
There was a problem hiding this comment.
This all seems to work as expected, iv tested enabling and disabling backups, and the maintenance stuff.
The one thing that still is a little odd to me is this message:
notify { 'pe_databases_version_warn':
message => 'This module only supports PE 2019.0.2 and later',
loglevel => warning,
this else triggers when $manage_database_maintenance is set to false, so the message isn't quite right, as the one IF evaluates intentional disablement and version exclusion but the else only warn log message is for the versioning. This may be cumbersome but could we put an if inside the else, and only do the log warning if its the version check that gets you there?
|
That's just a small restructuring of the if/else, and it is a logic problem so we should fix it. I pushed a commit that should do it, and I also moved the databases maintenance |
Prior to this commit, the disable_maintenance param was part of a maintenance class that had been removed. This commit fixes that and moves the lookup to the param definition.
Prior to this commit, the ensure for the cron entries in the backup class were hard coded to 'present' and could not be removed once set. This commit adds a new parameter to manage the cron jobs based on if $manage_database_backups is set. Restructure if/else logic Prior to this commit, the checks for managing the databases and having systemd were done in the same check, so the warning was confusing. This commit moves $manage_database_maintenance to its own if clause. It also moves the $manage_database_backups inside the version check if, because it makes more sense to put all of the resources behind the version check.
m0dular
force-pushed
the
aph/disable-maintenance-fix
branch
from
ddb2fd7 to
b6ef64d
Compare
No description provided.