Skip to content

(GH-1426) - Update value to accept array #1434

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
May 9, 2023
Merged

(GH-1426) - Update value to accept array #1434

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
6 changes: 3 additions & 3 deletions manifests/server/config_entry.pp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,9 +5,9 @@
# @param path Path for postgresql.conf
#
define postgresql::server::config_entry (
Enum['present', 'absent'] $ensure = 'present',
Optional[Variant[String[1], Numeric]] $value = undef,
Variant[Boolean, String[1]] $path = false
Enum['present', 'absent'] $ensure = 'present',
Optional[Variant[String[1], Numeric, Array[String[1]]]] $value = undef,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Shouldn't this also an array of Numeric?

Copy link
Contributor Author

@jordanbreen28 jordanbreen28 May 9, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

@ekohl I think that Array[String[1]] is suitable in the case of passing config values for listen_addresses (i understand value is used in numerous cases, but we are still trying to keep some form of security on what is passed), curious on your thoughts of updating this to accept Array[Numeric] values as well.

I think continuing to update this is a bit out of scope, as initially this was implemented as a quick bug fix to ensure that existing manifests with values for listen_addresses did not break after the syntax update carried out on this module. but I'm open to your suggestions. 💯

Copy link
Collaborator

Choose a reason for hiding this comment

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

It's probably fine, since I don't know how many array values there are where it is numeric. You can indeed use the following in YAML as a workaround:

---
value:
  - '1'
  - '2'

But very often you see some consistency, like Variant[X, Y, Array[Variant[X, Y]]] so that's something that always goes through my head. Still, it's not always true. For example, the current type can be written as Variant[Undef, String[1], Numeric, Array[String[1]]] (which is actually shorter than Optional[]) and you don't want to accept Array[Undef].

That's at least how I look at these data types and assess if it fits what's needed.

The config entry is supposed to reflect a setting, so https://www.postgresql.org/docs/current/config-setting.html is a good resource. That states:

All parameter names are case-insensitive. Every parameter takes a value of one of five types: boolean, string, integer, floating point, or enumerated (enum).

So String and Numeric certainly are good. Enum is covered by String. Then for booleans there's:

Boolean: Values can be written as on, off, true, false, yes, no, 1, 0 (all case-insensitive) or any unambiguous prefix of one of these.

So accepting Boolean is also good.

There's no real array type, but there are types which are lists. https://www.postgresql.org/docs/current/runtime-config.html is quite long. Most are comma joined with ssl_ciphers as an exception. We can ignore that one and require the user to specify it as a single string.

In conclusion: I think the current type will work.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ekohl as always, thanks for your great work and investigation into keeping our modules in top condition, it is appreciated!!

Completely understand your reasoning now thanks to all that info, I'll add a ticket to our backlog to further investigate this and implement it if required/we think it will be nice (which I must admit I do now..).

So thanks again 💯

Copy link
Collaborator

Choose a reason for hiding this comment

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

Thinking further on this, you could apply even stricter validations. Completely untested, but something that might work:

$validations = {
  'port' => Stdlib::Port,
  'listen_addresses' => Variant[Enum['*', 'localhost', '::'], Stdlib::IP::Address::Nosubnet, Array[Variant[Enum['localhost', '::'], Stdlib::IP::Address::Nosubnet]]],
}
$validation = $validations[$name]
if $validation && $value !~ $validation {
  fail("[Postgresql::Server::Config_entry[$name] doesn't match $validation")
}

That doesn't require you to specify all possible config entries, but for some you can have stricter validation preventing misconfigurations.

You may also be able to write it as a Struct type, but I think that would get awkward. Benefit would be better visibility in REFERENCE.md so something to think about.

Variant[Boolean, String[1]] $path = false
) {
$postgresql_conf_path = $postgresql::server::postgresql_conf_path

Expand Down