Prefer $connect_settings over explicit parameters

This is the outcome of a large refactoring in #1450 and discussions on
IRC and slack. We noticed that the behaviour of `$connect_settings` is
currently inconsistent. Sometimes it's preferred over explicity
parameters, sometimes not.

Reminder: The idea of `$connect_settings` is to provide a hash with
environment variable to tell puppet to manage a remote database instead
of a local instance.

One example is: https://github.com/puppetlabs/puppetlabs-postgresql/blob/93386b48861ff41d5f47033afee592e0506526c5/manifests/server/grant.pp#L80-L86

```
if $port {
  $port_override = $port
} elsif 'PGPORT' in $connect_settings {
  $port_override = undef
} else {
  $port_override = $postgresql::server::port
}
```

Here the local $port parameter is preferred over `$connect_settings`.
The problem is that we now cannot set a default in the resource for
`$port`. The default is hardcoded to `$postgresql::server::port`. This
becomes. Other classes handle it in a different way:

https://github.com/puppetlabs/puppetlabs-postgresql/blob/93386b48861ff41d5f47033afee592e0506526c5/manifests/server/database.pp#L41-L46

```
if 'PGPORT' in $connect_settings {
  $port_override = undef
} else {
  $port_override = $port
}
```

Here `$connct_settings` is checked first. It defaults to an empty hash.
If `PGPORT` isn't in it, `$port` is used. The logic is shorter and
enables us to expose $port as a parameter with a default value. With
this approach users can decide if they pass `$port` or
`$connect_settings`.

At the moment the remote database support is broken because the logic to
parse `$connect_settings` isn't consistent. The goal of this PR is to
unify the logic and prefer `$connect_settings` if it's provided by the
user.

Author: bastelfreak

README.md

@@ -175,11 +175,15 @@ class { 'postgresql::server':
 
 ### Manage remote users, roles, and permissions
 
-Remote SQL objects are managed using the same Puppet resources as local SQL objects, along with a [`connect_settings`](#connect_settings) hash. This provides control over how Puppet connects to the remote Postgres instances and which version is used for generating SQL commands.
+Remote SQL objects are managed using the same Puppet resources as local SQL objects, along with a `$connect_settings` hash. This provides control over how Puppet connects to the remote Postgres instances and which version is used for generating SQL commands.
 
-The `connect_settings` hash can contain environment variables to control Postgres client connections, such as 'PGHOST', 'PGPORT', 'PGPASSWORD', and 'PGSSLKEY'. See the [PostgreSQL Environment Variables](http://www.postgresql.org/docs/9.4/static/libpq-envars.html)  documentation for a complete list of variables.
+The `connect_settings` hash can contain environment variables to control Postgres client connections, such as 'PGHOST', 'PGPORT', 'PGPASSWORD', 'PGUSER' and 'PGSSLKEY'. See the [PostgreSQL Environment Variables](https://www.postgresql.org/docs/current/libpq-envars.html)  documentation for a complete list of variables.
 
-Additionally, you can specify the target database version with the special value of 'DBVERSION'. If the `connect_settings` hash is omitted or empty, then Puppet connects to the local PostgreSQL instance.
+Additionally, you can specify the target database version with the special value of 'DBVERSION'. If the `$connect_settings` hash is omitted or empty, then Puppet connects to the local PostgreSQL instance.
+
+**The $connect_settings hash has priority over the explicit variables like $port and $user**
+
+When a user provides only the `$port` parameter to a resource and no `$connect_settings`, `$port` will be used. When `$connect_settings` contains `PGPORT` and `$port` is set, `$connect_settings['PGPORT']` will be used.
 
 You can provide a `connect_settings` hash for each of the Puppet resources, or you can set a default `connect_settings` hash in `postgresql::globals`. Configuring `connect_settings` per resource allows SQL objects to be created on multiple databases by multiple users.
 

REFERENCE.md

@@ -420,7 +420,7 @@ Default value: `undef`
 
 ##### <a name="-postgresql--globals--datadir"></a>`datadir`
 
-Data type: `Optional[String[1]]`
+Data type: `Optional[Stdlib::Absolutepath]`
 
 Overrides the default PostgreSQL data directory for the target platform.
 Changing the datadir after installation causes the server to come to a full stop before making the change.
@@ -434,31 +434,31 @@ Default value: `undef`
 
 ##### <a name="-postgresql--globals--confdir"></a>`confdir`
 
-Data type: `Optional[String[1]]`
+Data type: `Optional[Stdlib::Absolutepath]`
 
 Overrides the default PostgreSQL configuration directory for the target platform.
 
 Default value: `undef`
 
 ##### <a name="-postgresql--globals--bindir"></a>`bindir`
 
-Data type: `Optional[String[1]]`
+Data type: `Optional[Stdlib::Absolutepath]`
 
 Overrides the default PostgreSQL binaries directory for the target platform.
 
 Default value: `undef`
 
 ##### <a name="-postgresql--globals--xlogdir"></a>`xlogdir`
 
-Data type: `Optional[String[1]]`
+Data type: `Optional[Stdlib::Absolutepath]`
 
 Overrides the default PostgreSQL xlog directory.
 
 Default value: `undef`
 
 ##### <a name="-postgresql--globals--logdir"></a>`logdir`
 
-Data type: `Optional[String[1]]`
+Data type: `Optional[Stdlib::Absolutepath]`
 
 Overrides the default PostgreSQL log directory.
 
@@ -660,7 +660,7 @@ Default value: `false`
 
 ##### <a name="-postgresql--globals--module_workdir"></a>`module_workdir`
 
-Data type: `Optional[String[1]]`
+Data type: `Optional[Stdlib::Absolutepath]`
 
 Specifies working directory under which the psql command should be executed.
 May need to specify if '/tmp' is on volume mounted with noexec option.
@@ -1124,23 +1124,23 @@ Default value: `$postgresql::params::recovery_conf_path`
 
 ##### <a name="-postgresql--server--datadir"></a>`datadir`
 
-Data type: `String[1]`
+Data type: `Stdlib::Absolutepath`
 
 PostgreSQL data directory
 
 Default value: `$postgresql::params::datadir`
 
 ##### <a name="-postgresql--server--xlogdir"></a>`xlogdir`
 
-Data type: `Optional[String[1]]`
+Data type: `Optional[Stdlib::Absolutepath]`
 
 PostgreSQL xlog directory
 
 Default value: `$postgresql::params::xlogdir`
 
 ##### <a name="-postgresql--server--logdir"></a>`logdir`
 
-Data type: `Optional[String[1]]`
+Data type: `Optional[Stdlib::Absolutepath]`
 
 PostgreSQL log directory
 
@@ -1268,7 +1268,7 @@ Default value: `$postgresql::params::manage_selinux`
 
 ##### <a name="-postgresql--server--module_workdir"></a>`module_workdir`
 
-Data type: `String[1]`
+Data type: `Stdlib::Absolutepath`
 
 Working directory for the PostgreSQL module
 
@@ -1754,19 +1754,19 @@ Default value: `$postgresql::server::group`
 
 ##### <a name="-postgresql--server--database_grant--connect_settings"></a>`connect_settings`
 
-Data type: `Optional[Hash]`
+Data type: `Hash`
 
 Specifies a hash of environment variables used when connecting to a remote server.
 
-Default value: `undef`
+Default value: `$postgresql::server::default_connect_settings`
 
 ##### <a name="-postgresql--server--database_grant--port"></a>`port`
 
-Data type: `Optional[Stdlib::Port]`
+Data type: `Stdlib::Port`
 
 Port to use when connecting.
 
-Default value: `undef`
+Default value: `$postgresql::server::port`
 
 ### <a name="postgresql--server--db"></a>`postgresql::server::db`
 
@@ -2116,11 +2116,11 @@ Default value: `undef`
 
 ##### <a name="-postgresql--server--extension--port"></a>`port`
 
-Data type: `Optional[Stdlib::Port]`
+Data type: `Stdlib::Port`
 
 Port to use when connecting.
 
-Default value: `undef`
+Default value: `postgresql::default('port')`
 
 ##### <a name="-postgresql--server--extension--connect_settings"></a>`connect_settings`
 
@@ -2267,11 +2267,11 @@ Default value: `$postgresql::server::user`
 
 ##### <a name="-postgresql--server--grant--port"></a>`port`
 
-Data type: `Optional[Stdlib::Port]`
+Data type: `Stdlib::Port`
 
 Port to use when connecting.
 
-Default value: `undef`
+Default value: `$postgresql::server::port`
 
 ##### <a name="-postgresql--server--grant--onlyif_exists"></a>`onlyif_exists`
 
@@ -2585,15 +2585,15 @@ Default value: `$postgresql::server::manage_postgresql_conf_perms`
 
 ##### <a name="-postgresql--server--instance--config--datadir"></a>`datadir`
 
-Data type: `String[1]`
+Data type: `Stdlib::Absolutepath`
 
 PostgreSQL data directory
 
 Default value: `$postgresql::server::datadir`
 
 ##### <a name="-postgresql--server--instance--config--logdir"></a>`logdir`
 
-Data type: `Optional[String[1]]`
+Data type: `Optional[Stdlib::Absolutepath]`
 
 PostgreSQL log directory
 
@@ -2700,7 +2700,7 @@ Default value: `$postgresql::server::data_checksums`
 
 ##### <a name="-postgresql--server--instance--initdb--datadir"></a>`datadir`
 
-Data type: `String[1]`
+Data type: `Stdlib::Absolutepath`
 
 PostgreSQL data directory
 
@@ -2753,7 +2753,7 @@ Default value: `$postgresql::server::locale`
 
 ##### <a name="-postgresql--server--instance--initdb--logdir"></a>`logdir`
 
-Data type: `Optional[String[1]]`
+Data type: `Optional[Stdlib::Absolutepath]`
 
 PostgreSQL log directory
 
@@ -2785,7 +2785,7 @@ Default value: `$postgresql::server::manage_xlogdir`
 
 ##### <a name="-postgresql--server--instance--initdb--module_workdir"></a>`module_workdir`
 
-Data type: `String[1]`
+Data type: `Stdlib::Absolutepath`
 
 Working directory for the PostgreSQL module
 
@@ -2818,7 +2818,7 @@ Default value: `$postgresql::server::username`
 
 ##### <a name="-postgresql--server--instance--initdb--xlogdir"></a>`xlogdir`
 
-Data type: `Optional[String[1]]`
+Data type: `Optional[Stdlib::Absolutepath]`
 
 PostgreSQL xlog/WAL directory
 
@@ -2884,7 +2884,7 @@ Default value: `$postgresql::server::port`
 
 ##### <a name="-postgresql--server--instance--late_initdb--module_workdir"></a>`module_workdir`
 
-Data type: `String[1]`
+Data type: `Stdlib::Absolutepath`
 
 Working directory for the PostgreSQL module
 
@@ -2951,7 +2951,7 @@ Default value: `$postgresql::server::default_database`
 
 ##### <a name="-postgresql--server--instance--passwd--module_workdir"></a>`module_workdir`
 
-Data type: `String[1]`
+Data type: `Stdlib::Absolutepath`
 
 Working directory for the PostgreSQL module
 
@@ -3554,11 +3554,11 @@ Default value: `$postgresql::server::default_database`
 
 ##### <a name="-postgresql--server--role--port"></a>`port`
 
-Data type: `Optional[Stdlib::Port]`
+Data type: `Stdlib::Port`
 
 Port to use when connecting.
 
-Default value: `undef`
+Default value: `postgresql::default('port')`
 
 ##### <a name="-postgresql--server--role--login"></a>`login`
 
@@ -3697,6 +3697,7 @@ The following parameters are available in the `postgresql::server::schema` defin
 * [`owner`](#-postgresql--server--schema--owner)
 * [`schema`](#-postgresql--server--schema--schema)
 * [`connect_settings`](#-postgresql--server--schema--connect_settings)
+* [`port`](#-postgresql--server--schema--port)
 
 ##### <a name="-postgresql--server--schema--db"></a>`db`
 
@@ -3730,6 +3731,14 @@ Specifies a hash of environment variables used when connecting to a remote serve
 
 Default value: `$postgresql::server::default_connect_settings`
 
+##### <a name="-postgresql--server--schema--port"></a>`port`
+
+Data type: `Stdlib::Port`
+
+the post the postgresql instance is listening on.
+
+Default value: `$postgresql::server::port`
+
 ### <a name="postgresql--server--table_grant"></a>`postgresql::server::table_grant`
 
 This resource wraps the grant resource to manage table grants specifically.
@@ -3840,6 +3849,7 @@ The following parameters are available in the `postgresql::server::tablespace` d
 * [`owner`](#-postgresql--server--tablespace--owner)
 * [`spcname`](#-postgresql--server--tablespace--spcname)
 * [`connect_settings`](#-postgresql--server--tablespace--connect_settings)
+* [`port`](#-postgresql--server--tablespace--port)
 
 ##### <a name="-postgresql--server--tablespace--location"></a>`location`
 
@@ -3879,6 +3889,14 @@ Specifies a hash of environment variables used when connecting to a remote serve
 
 Default value: `$postgresql::server::default_connect_settings`
 
+##### <a name="-postgresql--server--tablespace--port"></a>`port`
+
+Data type: `Stdlib::Port`
+
+the port of the postgresql instance that sould be used.
+
+Default value: `$postgresql::server::port`
+
 ## Resource types
 
 ### <a name="postgresql_conf"></a>`postgresql_conf`

manifests/server/database.pp

@@ -30,20 +30,8 @@
   String[1]            $default_db       = $postgresql::server::default_database,
   Stdlib::Port         $port             = $postgresql::server::port
 ) {
-  # If possible use the version of the remote database, otherwise
-  # fallback to our local DB version
-  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 'PGPORT' in $connect_settings {
-    $port_override = undef
-  } else {
-    $port_override = $port
-  }
+  $version = pick($connect_settings['DBVERSION'], $postgresql::server::_version)
+  $port_override = pick($connect_settings['PGPORT'], $port)
 
   # Set the defaults for the postgresql_psql resource
   Postgresql_psql {

manifests/server/database_grant.pp

@@ -16,9 +16,9 @@
   Optional[Enum['present', 'absent']] $ensure           = undef,
   Optional[String[1]]                 $psql_db          = undef,
   String[1]                           $psql_user        = $postgresql::server::user,
-  Optional[Hash]                      $connect_settings = undef,
+  Hash $connect_settings = $postgresql::server::default_connect_settings,
   String[1] $psql_group = $postgresql::server::group,
-  Optional[Stdlib::Port] $port = undef,
+  Stdlib::Port $port = $postgresql::server::port,
 ) {
   postgresql::server::grant { "database:${name}":
     ensure           => $ensure,

manifests/server/default_privileges.pp

@@ -35,13 +35,8 @@
   Stdlib::Absolutepath      $psql_path         = $postgresql::server::psql_path,
   Optional[String]          $target_role       = undef,
 ) {
-  # If possible use the version of the remote database, otherwise
-  # fallback to our local DB version
-  if 'DBVERSION' in $connect_settings {
-    $version = $connect_settings['DBVERSION']
-  } else {
-    $version = $postgresql::server::_version
-  }
+  $version = pick($connect_settings['DBVERSION'],postgresql::default('version'))
+  $port_override = pick($connect_settings['PGPORT'], $port)
 
   if (versioncmp($version, '9.6') == -1) {
     fail 'Default_privileges is only useable with PostgreSQL >= 9.6'
@@ -59,17 +54,6 @@
     }
   }
 
-  #
-  # Port, order of precedence: $port parameter, $connect_settings[PGPORT], $postgresql::server::port
-  #
-  if $port {
-    $port_override = $port
-  } elsif 'PGPORT' in $connect_settings {
-    $port_override = undef
-  } else {
-    $port_override = $postgresql::server::port
-  }
-
   if $target_role {
     $_target_role = " FOR ROLE ${target_role}"
     $_check_target_role = "/${target_role}"

manifests/server/extension.pp

@@ -32,7 +32,7 @@
   Optional[String[1]]                                 $version                = undef,
   Enum['present', 'absent']                           $ensure                 = 'present',
   Optional[String[1]]                                 $package_name           = undef,
-  Optional[Stdlib::Port]                              $port                   = undef,
+  Stdlib::Port $port = postgresql::default('port'),
   Hash                                                $connect_settings       = postgresql::default('default_connect_settings'),
   String[1]                                           $database_resource_name = $database,
   String[1]                                           $user                   = postgresql::default('user'),
@@ -76,16 +76,7 @@
     }
   }
 
-  #
-  # Port, order of precedence: $port parameter, $connect_settings[PGPORT], $postgresql::server::port
-  #
-  if $port {
-    $port_override = $port
-  } elsif 'PGPORT' in $connect_settings {
-    $port_override = undef
-  } else {
-    $port_override = $postgresql::server::port
-  }
+  $port_override = pick($connect_settings['PGPORT'], $port)
 
   postgresql_psql { "${database}: ${command}":
     psql_user        => $user,