Skip to content
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.

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

Prefer $connect_settings over explicit parameters #1498

Merged
merged 1 commit into from Sep 5, 2023

Conversation

bastelfreak
Copy link
Collaborator

@bastelfreak bastelfreak commented Aug 31, 2023

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:

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

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:

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

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.

@bastelfreak
Copy link
Collaborator Author

bastelfreak commented Aug 31, 2023

I raised this to explain the current situation. This PR doesn't solve all the problems. If we agree on preferring $connect_settings I will update this PR and fix the other code pieces.

Edit: I updated all related code pieces.

@bastelfreak bastelfreak force-pushed the connect_setting branch 8 times, most recently from 6c22012 to 7a579ae Compare August 31, 2023 19:18
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.
REFERENCE.md Show resolved Hide resolved
Copy link
Contributor

@Ramesh7 Ramesh7 left a comment

Choose a reason for hiding this comment

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

LGTM

@Ramesh7 Ramesh7 merged commit b7ad2fd into puppetlabs:main Sep 5, 2023
38 checks passed
@bastelfreak bastelfreak deleted the connect_setting branch September 5, 2023 11:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants