Prefer $connect_settings over explicit parameter

This is the outcome of a large refactoring in puppetlabs#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

@@ -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.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/database_grant.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/default_privileges.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,

manifests/server/extension.pp

@@ -41,7 +41,7 @@
   Array[String[1],0]                             $object_arguments  = [],
   String                                         $psql_db           = $postgresql::server::default_database,
   String                                         $psql_user         = $postgresql::server::user,
-  Optional[Stdlib::Port] $port = undef,
+  Stdlib::Port $port = $postgresql::server::port,
   Boolean                                        $onlyif_exists     = false,
   Hash                                           $connect_settings  = $postgresql::server::default_connect_settings,
   Enum['present', 'absent']                      $ensure            = 'present',
@@ -74,16 +74,7 @@
     $_object_name = $object_name
   }
 
-  #
-  # Port, order of precedence: $port parameter, $connect_settings[PGPORT], $postgresql::server::port
-  # We don't use pick() here because that would introduce a hard dependency to the postgresql::server class
-  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)
 
   ## Munge the input values
   $_object_type = upcase($object_type)

manifests/server/grant.pp

@@ -21,11 +21,7 @@
   $group     = $postgresql::server::group
   $psql_path = $postgresql::server::psql_path
 
-  if 'PGPORT' in $connect_settings {
-    $port_override = undef
-  } else {
-    $port_override = $port
-  }
+  $port_override = pick($connect_settings['PGPORT'], $port)
 
   $onlyif = "SELECT tablename FROM pg_catalog.pg_tables WHERE
                schemaname NOT IN ('pg_catalog', 'information_schema') AND

manifests/server/reassign_owned_by.pp

@@ -29,7 +29,7 @@
   Boolean                                     $createdb         = false,
   Boolean                                     $createrole       = false,
   String[1]                                   $db               = $postgresql::server::default_database,
-  Optional[Stdlib::Port]                      $port             = undef,
+  Stdlib::Port $port = postgresql::default('port'),
   Boolean                                     $login            = true,
   Boolean                                     $inherit          = true,
   Boolean                                     $superuser        = false,
@@ -50,24 +50,8 @@
   } else {
     $password_hash
   }
-  #
-  # 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 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
-  }
+  $port_override = pick($connect_settings['PGPORT'], $port)
+  $version = pick($connect_settings['DBVERSION'], postgresql::default('version'))
 
   Postgresql_psql {
     db         => $db,

manifests/server/role.pp

@@ -8,6 +8,7 @@
 # @param owner Sets the default owner of the schema.
 # @param schema Sets the name of the schema.
 # @param connect_settings Specifies a hash of environment variables used when connecting to a remote server.
+# @param port the post the postgresql instance is listening on.
 # @example
 #   postgresql::server::schema {'private':
 #       db => 'template1',
@@ -17,6 +18,7 @@
   Optional[String[1]] $owner            = undef,
   String[1]           $schema           = $title,
   Hash                $connect_settings = $postgresql::server::default_connect_settings,
+  Stdlib::Port $port = $postgresql::server::port,
 ) {
   $user           = $postgresql::server::user
   $group          = $postgresql::server::group
@@ -27,18 +29,14 @@
   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 'PGPORT' in $connect_settings {
-    $port = undef
-  } else {
-    $port = $postgresql::server::port
-  }
+  $port_override = pick($connect_settings['PGPORT'], $port)
 
   Postgresql_psql {
     db         => $db,
     psql_user  => $user,
     psql_group => $group,
     psql_path  => $psql_path,
-    port       => $port,
+    port       => $port_override,
     cwd        => $module_workdir,
     connect_settings => $connect_settings,
   }

manifests/server/schema.pp

@@ -5,30 +5,28 @@
 # @param owner Specifies the default owner of the tablespace.
 # @param spcname Specifies the name of the tablespace.
 # @param connect_settings Specifies a hash of environment variables used when connecting to a remote server.
+# @param port the port of the postgresql instance that sould be used.
 define postgresql::server::tablespace (
   String[1]           $location,
   Boolean             $manage_location = true,
   Optional[String[1]] $owner   = undef,
   String[1]           $spcname = $title,
   Hash                $connect_settings = $postgresql::server::default_connect_settings,
+  Stdlib::Port $port = $postgresql::server::port,
 ) {
   $user           = $postgresql::server::user
   $group          = $postgresql::server::group
   $psql_path      = $postgresql::server::psql_path
   $module_workdir = $postgresql::server::module_workdir
 
   # If the connection settings do not contain a port, then use the local server port
-  if 'PGPORT' in $connect_settings {
-    $port = undef
-  } else {
-    $port = $postgresql::server::port
-  }
+  $port_override = pick($connect_settings['PGPORT'], $port)
 
   Postgresql_psql {
     psql_user        => $user,
     psql_group       => $group,
     psql_path        => $psql_path,
-    port             => $port,
+    port             => $port_override,
     connect_settings => $connect_settings,
     cwd              => $module_workdir,
   }

manifests/server/tablespace.pp

@@ -24,7 +24,7 @@
 
     it { is_expected.to compile.with_all_deps }
     it { is_expected.to contain_postgresql__server__database_grant('test') }
-    it { is_expected.to contain_postgresql__server__grant('database:test').with_psql_user('postgres').without_port.with_group('postgres') }
+    it { is_expected.to contain_postgresql__server__grant('database:test').with_psql_user('postgres').with_port(5432).with_group('postgres') }
   end
 
   context 'with different user/group/port' do

spec/defines/server/database_grant_spec.rb

@@ -47,7 +47,7 @@ class {'postgresql::server':}"
                             'PGPORT' => '1234' } }
     end
 
-    it { is_expected.to contain_postgresql_psql('CREATE DATABASE "test"').with_connect_settings('PGHOST' => 'postgres-db-server', 'DBVERSION' => '9.1', 'PGPORT' => '1234').with_port(nil) }
+    it { is_expected.to contain_postgresql_psql('CREATE DATABASE "test"').with_connect_settings('PGHOST' => 'postgres-db-server', 'DBVERSION' => '9.1', 'PGPORT' => '1234').with_port(1234) }
   end
 
   context 'with global db connection settings - including port' do
@@ -61,7 +61,7 @@ class {'postgresql::server':}"
        class {'postgresql::server':}"
     end
 
-    it { is_expected.to contain_postgresql_psql('CREATE DATABASE "test"').with_connect_settings('PGHOST' => 'postgres-db-server', 'DBVERSION' => '9.2', 'PGPORT' => '1234').with_port(nil) }
+    it { is_expected.to contain_postgresql_psql('CREATE DATABASE "test"').with_connect_settings('PGHOST' => 'postgres-db-server', 'DBVERSION' => '9.2', 'PGPORT' => '1234').with_port(1234) }
   end
 
   context 'with different owner' do

spec/defines/server/database_spec.rb

@@ -22,8 +22,8 @@
     it { is_expected.to compile.with_all_deps }
     it { is_expected.to contain_postgresql__server__db('testdb').without_port.with_user('foo').with_psql_user('postgres').with_psql_group('postgres') }
     it { is_expected.to contain_postgresql__server__database('testdb').without_owner.with_user('postgres').with_group('postgres') }
-    it { is_expected.to contain_postgresql__server__role('foo').that_comes_before('Postgresql::Server::Database[testdb]').without_port.with_psql_user('postgres').with_psql_group('postgres') }
-    it { is_expected.to contain_postgresql__server__database_grant('GRANT foo - ALL - testdb').without_port.with_psql_user('postgres').with_psql_group('postgres') }
+    it { is_expected.to contain_postgresql__server__role('foo').that_comes_before('Postgresql::Server::Database[testdb]').with_port(5432).with_psql_user('postgres').with_psql_group('postgres') }
+    it { is_expected.to contain_postgresql__server__database_grant('GRANT foo - ALL - testdb').with_port(5432).with_psql_user('postgres').with_psql_group('postgres') }
   end
 
   context 'without dbname param' do

spec/defines/server/db_spec.rb

@@ -257,7 +257,7 @@ class { 'postgresql::server': }
 
     it { is_expected.to compile.with_all_deps }
     it { is_expected.to contain_postgresql__server__default_privileges('test') }
-    it { is_expected.to contain_postgresql_psql('default_privileges:test').with_connect_settings('PGHOST' => 'postgres-db-server', 'DBVERSION' => '9.6', 'PGPORT' => '1234').with_port('5678') }
+    it { is_expected.to contain_postgresql_psql('default_privileges:test').with_connect_settings('PGHOST' => 'postgres-db-server', 'DBVERSION' => '9.6', 'PGPORT' => '1234').with_port('1234') }
   end
 
   context 'with specific schema name' do

spec/defines/server/default_privileges_spec.rb

@@ -211,7 +211,7 @@
     it {
       expect(subject).to contain_postgresql_psql('postgres: CREATE EXTENSION "pg_repack"')
         .with_connect_settings('PGHOST' => 'postgres-db-server', 'DBVERSION' => '9.1', 'PGPORT' => '1234')
-        .with_port(nil)
+        .with_port(1234)
     }
   end
 
@@ -236,7 +236,7 @@
     it {
       expect(subject).to contain_postgresql_psql('postgres: CREATE EXTENSION "pg_repack"')
         .with_connect_settings('PGHOST' => 'postgres-db-server', 'DBVERSION' => '9.1', 'PGPORT' => '1234')
-        .with_port('5678')
+        .with_port('1234')
     }
   end
 end

spec/defines/server/extension_spec.rb

@@ -155,7 +155,7 @@
 
     it { is_expected.to compile.with_all_deps }
     it { is_expected.to contain_postgresql__server__grant('test') }
-    it { is_expected.to contain_postgresql_psql('grant:test').with_connect_settings('PGHOST' => 'postgres-db-server', 'DBVERSION' => '9.1', 'PGPORT' => '1234').with_port('5678') }
+    it { is_expected.to contain_postgresql_psql('grant:test').with_connect_settings('PGHOST' => 'postgres-db-server', 'DBVERSION' => '9.1', 'PGPORT' => '1234').with_port('1234') }
   end
 
   context 'with specific schema name' do