(GH-1426) - Update value to accept array #1434
Conversation
jordanbreen28
force-pushed
the
gh-1426-fix_mismatched_data_types
branch
from
00943a8 to
36674b3
Compare
| 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, |
There was a problem hiding this comment.
Shouldn't this also an array of Numeric?
There was a problem hiding this comment.
@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. 💯
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
@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 💯
There was a problem hiding this comment.
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.
Fixes #1426