Skip to content

Commit 6c22012

Browse files
bastelfreakbastelfreak
committed
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.
1 parent 1dadd63 commit 6c22012

16 files changed

+64
-112
lines changed

‎README.md

Lines changed: 7 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -175,11 +175,15 @@ class { 'postgresql::server':
175175

176176
### Manage remote users, roles, and permissions
177177

178-
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.
178+
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.
179179

180-
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.
180+
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.
181181

182-
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.
182+
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.
183+
184+
**The $connect_settings hash has priority over the explicit variables like $port and $user**
185+
186+
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.
183187

184188
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.
185189

‎REFERENCE.md

Lines changed: 28 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -1754,19 +1754,19 @@ Default value: `$postgresql::server::group`
17541754

17551755
##### <a name="-postgresql--server--database_grant--connect_settings"></a>`connect_settings`
17561756

1757-
Data type: `Optional[Hash]`
1757+
Data type: `Hash`
17581758

17591759
Specifies a hash of environment variables used when connecting to a remote server.
17601760

1761-
Default value: `undef`
1761+
Default value: `$postgresql::server::default_connect_settings`
17621762

17631763
##### <a name="-postgresql--server--database_grant--port"></a>`port`
17641764

1765-
Data type: `Optional[Stdlib::Port]`
1765+
Data type: `Stdlib::Port`
17661766

17671767
Port to use when connecting.
17681768

1769-
Default value: `undef`
1769+
Default value: `$postgresql::server::port`
17701770

17711771
### <a name="postgresql--server--db"></a>`postgresql::server::db`
17721772

@@ -2116,11 +2116,11 @@ Default value: `undef`
21162116

21172117
##### <a name="-postgresql--server--extension--port"></a>`port`
21182118

2119-
Data type: `Optional[Stdlib::Port]`
2119+
Data type: `Stdlib::Port`
21202120

21212121
Port to use when connecting.
21222122

2123-
Default value: `undef`
2123+
Default value: `postgresql::default('port')`
21242124

21252125
##### <a name="-postgresql--server--extension--connect_settings"></a>`connect_settings`
21262126

@@ -2267,11 +2267,11 @@ Default value: `$postgresql::server::user`
22672267

22682268
##### <a name="-postgresql--server--grant--port"></a>`port`
22692269

2270-
Data type: `Optional[Stdlib::Port]`
2270+
Data type: `Stdlib::Port`
22712271

22722272
Port to use when connecting.
22732273

2274-
Default value: `undef`
2274+
Default value: `$postgresql::server::port`
22752275

22762276
##### <a name="-postgresql--server--grant--onlyif_exists"></a>`onlyif_exists`
22772277

@@ -3554,11 +3554,11 @@ Default value: `$postgresql::server::default_database`
35543554

35553555
##### <a name="-postgresql--server--role--port"></a>`port`
35563556

3557-
Data type: `Optional[Stdlib::Port]`
3557+
Data type: `Stdlib::Port`
35583558

35593559
Port to use when connecting.
35603560

3561-
Default value: `undef`
3561+
Default value: `postgresql::default('port')`
35623562

35633563
##### <a name="-postgresql--server--role--login"></a>`login`
35643564

@@ -3697,6 +3697,7 @@ The following parameters are available in the `postgresql::server::schema` defin
36973697
* [`owner`](#-postgresql--server--schema--owner)
36983698
* [`schema`](#-postgresql--server--schema--schema)
36993699
* [`connect_settings`](#-postgresql--server--schema--connect_settings)
3700+
* [`port`](#-postgresql--server--schema--port)
37003701

37013702
##### <a name="-postgresql--server--schema--db"></a>`db`
37023703

@@ -3730,6 +3731,14 @@ Specifies a hash of environment variables used when connecting to a remote serve
37303731

37313732
Default value: `$postgresql::server::default_connect_settings`
37323733

3734+
##### <a name="-postgresql--server--schema--port"></a>`port`
3735+
3736+
Data type: `Stdlib::Port`
3737+
3738+
the post the postgresql instance is listening on.
3739+
3740+
Default value: `$postgresql::server::port`
3741+
37333742
### <a name="postgresql--server--table_grant"></a>`postgresql::server::table_grant`
37343743

37353744
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
38403849
* [`owner`](#-postgresql--server--tablespace--owner)
38413850
* [`spcname`](#-postgresql--server--tablespace--spcname)
38423851
* [`connect_settings`](#-postgresql--server--tablespace--connect_settings)
3852+
* [`port`](#-postgresql--server--tablespace--port)
38433853

38443854
##### <a name="-postgresql--server--tablespace--location"></a>`location`
38453855

@@ -3879,6 +3889,14 @@ Specifies a hash of environment variables used when connecting to a remote serve
38793889

38803890
Default value: `$postgresql::server::default_connect_settings`
38813891

3892+
##### <a name="-postgresql--server--tablespace--port"></a>`port`
3893+
3894+
Data type: `Stdlib::Port`
3895+
3896+
the port of the postgresql instance that sould be used.
3897+
3898+
Default value: `$postgresql::server::port`
3899+
38823900
## Resource types
38833901

38843902
### <a name="postgresql_conf"></a>`postgresql_conf`

‎manifests/server/database.pp

Lines changed: 2 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -30,20 +30,8 @@
3030
String[1] $default_db = $postgresql::server::default_database,
3131
Stdlib::Port $port = $postgresql::server::port
3232
) {
33-
# If possible use the version of the remote database, otherwise
34-
# fallback to our local DB version
35-
if 'DBVERSION' in $connect_settings {
36-
$version = $connect_settings['DBVERSION']
37-
} else {
38-
$version = $postgresql::server::_version
39-
}
40-
41-
# If the connection settings do not contain a port, then use the local server port
42-
if 'PGPORT' in $connect_settings {
43-
$port_override = undef
44-
} else {
45-
$port_override = $port
46-
}
33+
$version = pick($connect_settings['DBVERSION'], $postgresql::server::_version)
34+
$port_override = pick($connect_settings['PGPORT'], $port)
4735

4836
# Set the defaults for the postgresql_psql resource
4937
Postgresql_psql {

‎manifests/server/database_grant.pp

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -16,9 +16,9 @@
1616
Optional[Enum['present', 'absent']] $ensure = undef,
1717
Optional[String[1]] $psql_db = undef,
1818
String[1] $psql_user = $postgresql::server::user,
19-
Optional[Hash] $connect_settings = undef,
19+
Hash $connect_settings = $postgresql::server::default_connect_settings,
2020
String[1] $psql_group = $postgresql::server::group,
21-
Optional[Stdlib::Port] $port = undef,
21+
Stdlib::Port $port = $postgresql::server::port,
2222
) {
2323
postgresql::server::grant { "database:${name}":
2424
ensure => $ensure,

‎manifests/server/default_privileges.pp

Lines changed: 2 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -35,13 +35,8 @@
3535
Stdlib::Absolutepath $psql_path = $postgresql::server::psql_path,
3636
Optional[String] $target_role = undef,
3737
) {
38-
# If possible use the version of the remote database, otherwise
39-
# fallback to our local DB version
40-
if 'DBVERSION' in $connect_settings {
41-
$version = $connect_settings['DBVERSION']
42-
} else {
43-
$version = $postgresql::server::_version
44-
}
38+
$version = pick($connect_settings['DBVERSION'],postgresql::default('version'))
39+
$port_override = pick($connect_settings['PGPORT'], $port)
4540

4641
if (versioncmp($version, '9.6') == -1) {
4742
fail 'Default_privileges is only useable with PostgreSQL >= 9.6'
@@ -59,17 +54,6 @@
5954
}
6055
}
6156

62-
#
63-
# Port, order of precedence: $port parameter, $connect_settings[PGPORT], $postgresql::server::port
64-
#
65-
if $port {
66-
$port_override = $port
67-
} elsif 'PGPORT' in $connect_settings {
68-
$port_override = undef
69-
} else {
70-
$port_override = $postgresql::server::port
71-
}
72-
7357
if $target_role {
7458
$_target_role = " FOR ROLE ${target_role}"
7559
$_check_target_role = "/${target_role}"

‎manifests/server/extension.pp

Lines changed: 2 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -32,7 +32,7 @@
3232
Optional[String[1]] $version = undef,
3333
Enum['present', 'absent'] $ensure = 'present',
3434
Optional[String[1]] $package_name = undef,
35-
Optional[Stdlib::Port] $port = undef,
35+
Stdlib::Port $port = postgresql::default('port'),
3636
Hash $connect_settings = postgresql::default('default_connect_settings'),
3737
String[1] $database_resource_name = $database,
3838
String[1] $user = postgresql::default('user'),
@@ -76,16 +76,7 @@
7676
}
7777
}
7878

79-
#
80-
# Port, order of precedence: $port parameter, $connect_settings[PGPORT], $postgresql::server::port
81-
#
82-
if $port {
83-
$port_override = $port
84-
} elsif 'PGPORT' in $connect_settings {
85-
$port_override = undef
86-
} else {
87-
$port_override = $postgresql::server::port
88-
}
79+
$port_override = pick($connect_settings['PGPORT'], $port)
8980

9081
postgresql_psql { "${database}: ${command}":
9182
psql_user => $user,

‎manifests/server/grant.pp

Lines changed: 2 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -41,7 +41,7 @@
4141
Array[String[1],0] $object_arguments = [],
4242
String $psql_db = $postgresql::server::default_database,
4343
String $psql_user = $postgresql::server::user,
44-
Optional[Stdlib::Port] $port = undef,
44+
Stdlib::Port $port = $postgresql::server::port,
4545
Boolean $onlyif_exists = false,
4646
Hash $connect_settings = $postgresql::server::default_connect_settings,
4747
Enum['present', 'absent'] $ensure = 'present',
@@ -74,16 +74,7 @@
7474
$_object_name = $object_name
7575
}
7676

77-
#
78-
# Port, order of precedence: $port parameter, $connect_settings[PGPORT], $postgresql::server::port
79-
# We don't use pick() here because that would introduce a hard dependency to the postgresql::server class
80-
if $port {
81-
$port_override = $port
82-
} elsif 'PGPORT' in $connect_settings {
83-
$port_override = undef
84-
} else {
85-
$port_override = $postgresql::server::port
86-
}
77+
$port_override = pick($connect_settings['PGPORT'], $port)
8778

8879
## Munge the input values
8980
$_object_type = upcase($object_type)

‎manifests/server/reassign_owned_by.pp

Lines changed: 1 addition & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -21,11 +21,7 @@
2121
$group = $postgresql::server::group
2222
$psql_path = $postgresql::server::psql_path
2323

24-
if 'PGPORT' in $connect_settings {
25-
$port_override = undef
26-
} else {
27-
$port_override = $port
28-
}
24+
$port_override = pick($connect_settings['PGPORT'], $port)
2925

3026
$onlyif = "SELECT tablename FROM pg_catalog.pg_tables WHERE
3127
schemaname NOT IN ('pg_catalog', 'information_schema') AND

‎manifests/server/role.pp

Lines changed: 3 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -29,7 +29,7 @@
2929
Boolean $createdb = false,
3030
Boolean $createrole = false,
3131
String[1] $db = $postgresql::server::default_database,
32-
Optional[Stdlib::Port] $port = undef,
32+
Stdlib::Port $port = postgresql::default('port'),
3333
Boolean $login = true,
3434
Boolean $inherit = true,
3535
Boolean $superuser = false,
@@ -50,24 +50,8 @@
5050
} else {
5151
$password_hash
5252
}
53-
#
54-
# Port, order of precedence: $port parameter, $connect_settings[PGPORT], $postgresql::server::port
55-
#
56-
if $port {
57-
$port_override = $port
58-
} elsif 'PGPORT' in $connect_settings {
59-
$port_override = undef
60-
} else {
61-
$port_override = $postgresql::server::port
62-
}
63-
64-
# If possible use the version of the remote database, otherwise
65-
# fallback to our local DB version
66-
if 'DBVERSION' in $connect_settings {
67-
$version = $connect_settings['DBVERSION']
68-
} else {
69-
$version = $postgresql::server::_version
70-
}
53+
$port_override = pick($connect_settings['PGPORT'], $port)
54+
$version = pick($connect_settings['DBVERSION'], postgresql::default('version'))
7155

7256
Postgresql_psql {
7357
db => $db,

‎manifests/server/schema.pp

Lines changed: 4 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,7 @@
88
# @param owner Sets the default owner of the schema.
99
# @param schema Sets the name of the schema.
1010
# @param connect_settings Specifies a hash of environment variables used when connecting to a remote server.
11+
# @param port the post the postgresql instance is listening on.
1112
# @example
1213
# postgresql::server::schema {'private':
1314
# db => 'template1',
@@ -17,6 +18,7 @@
1718
Optional[String[1]] $owner = undef,
1819
String[1] $schema = $title,
1920
Hash $connect_settings = $postgresql::server::default_connect_settings,
21+
Stdlib::Port $port = $postgresql::server::port,
2022
) {
2123
$user = $postgresql::server::user
2224
$group = $postgresql::server::group
@@ -27,18 +29,14 @@
2729
Postgresql::Server::Db <| dbname == $db |> -> Postgresql::Server::Schema[$name]
2830

2931
# If the connection settings do not contain a port, then use the local server port
30-
if 'PGPORT' in $connect_settings {
31-
$port = undef
32-
} else {
33-
$port = $postgresql::server::port
34-
}
32+
$port_override = pick($connect_settings['PGPORT'], $port)
3533

3634
Postgresql_psql {
3735
db => $db,
3836
psql_user => $user,
3937
psql_group => $group,
4038
psql_path => $psql_path,
41-
port => $port,
39+
port => $port_override,
4240
cwd => $module_workdir,
4341
connect_settings => $connect_settings,
4442
}

‎manifests/server/tablespace.pp

Lines changed: 4 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -5,30 +5,28 @@
55
# @param owner Specifies the default owner of the tablespace.
66
# @param spcname Specifies the name of the tablespace.
77
# @param connect_settings Specifies a hash of environment variables used when connecting to a remote server.
8+
# @param port the port of the postgresql instance that sould be used.
89
define postgresql::server::tablespace (
910
String[1] $location,
1011
Boolean $manage_location = true,
1112
Optional[String[1]] $owner = undef,
1213
String[1] $spcname = $title,
1314
Hash $connect_settings = $postgresql::server::default_connect_settings,
15+
Stdlib::Port $port = $postgresql::server::port,
1416
) {
1517
$user = $postgresql::server::user
1618
$group = $postgresql::server::group
1719
$psql_path = $postgresql::server::psql_path
1820
$module_workdir = $postgresql::server::module_workdir
1921

2022
# If the connection settings do not contain a port, then use the local server port
21-
if 'PGPORT' in $connect_settings {
22-
$port = undef
23-
} else {
24-
$port = $postgresql::server::port
25-
}
23+
$port_override = pick($connect_settings['PGPORT'], $port)
2624

2725
Postgresql_psql {
2826
psql_user => $user,
2927
psql_group => $group,
3028
psql_path => $psql_path,
31-
port => $port,
29+
port => $port_override,
3230
connect_settings => $connect_settings,
3331
cwd => $module_workdir,
3432
}

‎spec/defines/server/database_grant_spec.rb

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -24,7 +24,7 @@
2424

2525
it { is_expected.to compile.with_all_deps }
2626
it { is_expected.to contain_postgresql__server__database_grant('test') }
27-
it { is_expected.to contain_postgresql__server__grant('database:test').with_psql_user('postgres').without_port.with_group('postgres') }
27+
it { is_expected.to contain_postgresql__server__grant('database:test').with_psql_user('postgres').with_port(5432).with_group('postgres') }
2828
end
2929

3030
context 'with different user/group/port' do

0 commit comments

Comments
 (0)