Cleanup useless $connect_setting validation #1487
Conversation
bastelfreak
requested review from
alexjfisher,
deric,
ekohl,
SimonHoenscheid,
smortex and
a team
as code owners
| if $port { | ||
| $port_override = $port | ||
| } elsif $connect_settings != undef and 'PGPORT' in $connect_settings { | ||
| } elsif 'PGPORT' in $connect_settings { | ||
| $port_override = undef | ||
| } else { | ||
| $port_override = $postgresql::server::port | ||
| } |
There was a problem hiding this comment.
the whole block doesn't make sense? Port is always set and cannot be undef. so $port_override is always set to $port? I don't thinky anybody uses $connect_settings. The whole setup seems broken?
| @@ -178,11 +178,11 @@ | |||
| environment => 'PGOPTIONS=--client-min-messages=error', | |||
| } | |||
|
|
|||
| if($role != undef and defined(Postgresql::Server::Role[$role])) { | |||
There was a problem hiding this comment.
$role cannot be undef
| Postgresql::Server::Role[$role] -> Postgresql_psql["default_privileges:${name}"] | ||
| } | ||
|
|
||
| if($db != undef and defined(Postgresql::Server::Database[$db])) { |
There was a problem hiding this comment.
$db cannot be undef.
| if $port != undef { | ||
| if $port { | ||
| $port_override = $port | ||
| } elsif $connect_settings != undef and 'PGPORT' in $connect_settings { | ||
| } elsif 'PGPORT' in $connect_settings { |
There was a problem hiding this comment.
Here it makes sense because $port is undef by default
| if $port != undef { | ||
| if $port { | ||
| $port_override = $port | ||
| } elsif $connect_settings != undef and 'PGPORT' in $connect_settings { | ||
| $port_override = undef |
There was a problem hiding this comment.
whole block doesn't make sense because $port isn't undef?
In the past we checked if $connect_setting isn't undef. That is not
required because the default is {} and the Datatype is hash. Undef isn't
allowed.
bastelfreak
force-pushed
the
connection
branch
from
4d95248
to
ece7751
Compare
|
I think the changes in this PR don't change the current behaviour so they should be safe to merge. But I think they point out the connect_setting implementation is currently broken, at least in some classes. |
| @@ -53,17 +53,17 @@ | |||
| # | |||
| # Port, order of precedence: $port parameter, $connect_settings[PGPORT], $postgresql::server::port | |||
| # | |||
| if $port != undef { | |||
| if $port { | |||
There was a problem hiding this comment.
Bit of confusing for me due to the priority changing in each file, not sure its expected.
Coz here we are giving high priority to $port but in manifests/server/reassign_owned_by.pp we are giving high priority to $connect_settings.
Just thinking if we can make identical across the board?
There was a problem hiding this comment.
In this case, the priority doesn't change. $port is still undef by default. In some classes the logic seems to be broken because $port doesn't default to undef, but that problem exists on the main branch already.
There was a problem hiding this comment.
I think this is fine to merge because it only simplifies the code, it doesn't change the logic. but the logic is broken and should probably be checked in a dedicated issue.
There was a problem hiding this comment.
#1469 was aimed at making it more consistent. I haven't checked back on the most recent implementation, but you're right that it is inconsistent between files.
In the past we checked if $connect_setting isn't undef. That is not required because the default is {} and the Datatype is hash. Undef isn't allowed.
Summary
Provide a detailed description of all the changes present in this pull request.
Additional Context
Add any additional context about the problem here.
Related Issues (if any)
Mention any related issues or pull requests.
Checklist
puppet apply)