Skip to content

Cleanup useless $connect_setting validation #1487

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

Merged
merged 1 commit into from
Aug 31, 2023
Merged

Cleanup useless $connect_setting validation #1487

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions manifests/server/database.pp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,14 +27,14 @@

# If possible use the version of the remote database, otherwise
# fallback to our local DB version
if $connect_settings != undef and 'DBVERSION' in $connect_settings {
if 'DBVERSION' in $connect_settings {
$version = $connect_settings['DBVERSION']
} else {
$version = $postgresql::server::_version
}

# If the connection settings do not contain a port, then use the local server port
if $connect_settings != undef and 'PGPORT' in $connect_settings {
if 'PGPORT' in $connect_settings {
$port = undef
} else {
$port = $postgresql::server::port
Expand Down
12 changes: 6 additions & 6 deletions manifests/server/default_privileges.pp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@
) {
# If possible use the version of the remote database, otherwise
# fallback to our local DB version
if $connect_settings != undef and 'DBVERSION' in $connect_settings {
if 'DBVERSION' in $connect_settings {
$version = $connect_settings['DBVERSION']
} else {
$version = $postgresql::server::_version
Expand All @@ -62,15 +62,15 @@
#
# Port, order of precedence: $port parameter, $connect_settings[PGPORT], $postgresql::server::port
#
if $port != undef {
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
}
Comment on lines +65 to 71
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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?


if $target_role != undef {
if $target_role {
$_target_role = " FOR ROLE ${target_role}"
$_check_target_role = "/${target_role}"
} else {
Expand Down Expand Up @@ -178,11 +178,11 @@
environment => 'PGOPTIONS=--client-min-messages=error',
}

if($role != undef and defined(Postgresql::Server::Role[$role])) {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

$role cannot be undef

if defined(Postgresql::Server::Role[$role]) {
Postgresql::Server::Role[$role] -> Postgresql_psql["default_privileges:${name}"]
}

if($db != undef and defined(Postgresql::Server::Database[$db])) {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

$db cannot be undef.

if defined(Postgresql::Server::Database[$db]) {
Postgresql::Server::Database[$db] -> Postgresql_psql["default_privileges:${name}"]
}
}
4 changes: 2 additions & 2 deletions manifests/server/extension.pp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -77,9 +77,9 @@
#
# Port, order of precedence: $port parameter, $connect_settings[PGPORT], $postgresql::server::port
#
if $port != undef {
if $port {
$port_override = $port
} elsif $connect_settings != undef and 'PGPORT' in $connect_settings {
} elsif 'PGPORT' in $connect_settings {
Comment on lines -80 to +82
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Here it makes sense because $port is undef by default

$port_override = undef
} else {
$port_override = $postgresql::server::port
Expand Down
6 changes: 3 additions & 3 deletions manifests/server/grant.pp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -77,7 +77,7 @@
#
# Port, order of precedence: $port parameter, $connect_settings[PGPORT], $postgresql::server::port
#
if $port != undef {
if $port {
$port_override = $port
} elsif $connect_settings != undef and 'PGPORT' in $connect_settings {
$port_override = undef
Comment on lines -80 to 83
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

whole block doesn't make sense because $port isn't undef?

Expand Down Expand Up @@ -483,11 +483,11 @@
onlyif => $_onlyif,
}

if($role != undef and defined(Postgresql::Server::Role[$role])) {
if defined(Postgresql::Server::Role[$role]) {
Postgresql::Server::Role[$role] -> Postgresql_psql["grant:${name}"]
}

if($db != undef and defined(Postgresql::Server::Database[$db])) {
if defined(Postgresql::Server::Database[$db]) {
Postgresql::Server::Database[$db] -> Postgresql_psql["grant:${name}"]
}
}
2 changes: 1 addition & 1 deletion manifests/server/grant_role.pp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,7 @@
connect_settings => $connect_settings,
}

if ! $connect_settings or empty($connect_settings) {
if empty($connect_settings) {
Class['postgresql::server'] -> Postgresql_psql["grant_role:${name}"]
}
if defined(Postgresql::Server::Role[$role]) {
Expand Down
13 changes: 4 additions & 9 deletions manifests/server/reassign_owned_by.pp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,15 +21,10 @@
$group = $postgresql::server::group
$psql_path = $postgresql::server::psql_path

#
# Port, order of precedence: $port parameter, $connect_settings[PGPORT], $postgresql::server::port
#
if $port {
$port_override = $port
} elsif $connect_settings != undef and 'PGPORT' in $connect_settings {
if 'PGPORT' in $connect_settings {
$port_override = undef
} else {
$port_override = $postgresql::server::port
$port_override = $port
}

$onlyif = "SELECT tablename FROM pg_catalog.pg_tables WHERE
Expand All @@ -54,14 +49,14 @@
onlyif => $onlyif,
}

if($old_role != undef and defined(Postgresql::Server::Role[$old_role])) {
if defined(Postgresql::Server::Role[$old_role]) {
Postgresql::Server::Role[$old_role] -> Postgresql_psql["reassign_owned_by:${db}:${sql_command}"]
}
if($new_role != undef and defined(Postgresql::Server::Role[$new_role])) {
Postgresql::Server::Role[$new_role] -> Postgresql_psql["reassign_owned_by:${db}:${sql_command}"]
}

if($db != undef and defined(Postgresql::Server::Database[$db])) {
if defined(Postgresql::Server::Database[$db]) {
Postgresql::Server::Database[$db] -> Postgresql_psql["reassign_owned_by:${db}:${sql_command}"]
}
}
6 changes: 3 additions & 3 deletions manifests/server/role.pp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,17 +53,17 @@
#
# Port, order of precedence: $port parameter, $connect_settings[PGPORT], $postgresql::server::port
#
if $port != undef {
if $port {
Copy link
Contributor

@Ramesh7 Ramesh7 Aug 31, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

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?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator

Choose a reason for hiding this comment

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

#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.

$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
}

# If possible use the version of the remote database, otherwise
# fallback to our local DB version
if $connect_settings != undef and 'DBVERSION' in $connect_settings {
if 'DBVERSION' in $connect_settings {
$version = $connect_settings['DBVERSION']
} else {
$version = $postgresql::server::_version
Expand Down
2 changes: 1 addition & 1 deletion manifests/server/schema.pp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@
Postgresql::Server::Db <| dbname == $db |> -> Postgresql::Server::Schema[$name]

# If the connection settings do not contain a port, then use the local server port
if $connect_settings != undef and 'PGPORT' in $connect_settings {
if 'PGPORT' in $connect_settings {
$port = undef
} else {
$port = $postgresql::server::port
Expand Down
2 changes: 1 addition & 1 deletion manifests/server/tablespace.pp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@
$module_workdir = $postgresql::server::module_workdir

# If the connection settings do not contain a port, then use the local server port
if $connect_settings != undef and 'PGPORT' in $connect_settings {
if 'PGPORT' in $connect_settings {
$port = undef
} else {
$port = $postgresql::server::port
Expand Down