Provide a default for config_entry's path and enforce absolute path #1490
Conversation
SimonHoenscheid
requested review from
alexjfisher,
bastelfreak,
deric,
ekohl,
smortex and
a team
as code owners
SimonHoenscheid
force-pushed
the
config_entry_path_parameter
branch
from
9391806
to
9ce7649
Compare
There was a problem hiding this comment.
Technically this doesn't make it configurable because it already was. It just changes the default.
Looking at the old implementation I think it would have been clearer if you used Optional[String[1]] $path = undefand thepick($path, $postgresql::server::postgresql_conf_path)` to get the actual value. Booleans for an empty value just feel awkward.
manifests/server/config_entry.pp
Outdated
| @@ -9,15 +9,8 @@ | |||
| Enum['present', 'absent'] $ensure = 'present', | |||
| String[1] $key = $name, | |||
| Optional[Variant[String[1], Numeric, Array[String[1]]]] $value = undef, | |||
| Variant[Boolean, String[1]] $path = false | |||
| Variant[String[1], Stdlib::Absolutepath] $path = $postgresql::server::postgresql_conf_path | |||
There was a problem hiding this comment.
Why Stdlib::Absolutepath if String[1] is also accepted? Then I'd simplify it to just String[1]. Having said that: does a relative path ever work or should it be simplified to an absolute path?
There was a problem hiding this comment.
Agreed. Thinking about it Stdlib::Absolutepath probably makes more sense. The pick() idea is nice, but I think the current implementation is easier to read.
There was a problem hiding this comment.
The
pick()idea is nice, but I think the current implementation is easier to read.
In theory it allows you to stop caring about the compile order, if you also add include postgresql::server but that's not there today either.
There was a problem hiding this comment.
Regarding the Data Type I just took the Type from the postgresql::server class. I would suggest changing this in the whole module is part of #1467. I am happy to create a PR for that.
There was a problem hiding this comment.
PR ist open #1491
SimonHoenscheid
force-pushed
the
config_entry_path_parameter
branch
2 times, most recently
from
1e5f1d8
to
6d05d74
Compare
SimonHoenscheid
force-pushed
the
config_entry_path_parameter
branch
from
6d05d74
to
cd8dcea
Compare
SimonHoenscheid
force-pushed
the
config_entry_path_parameter
branch
from
cd8dcea
to
ffcdc07
Compare
There was a problem hiding this comment.
This PR should be titled "Provide a default for config_entry's path and enforce absolute path", but maybe a bit shorter.
Technically this already enforces $postgresql::server::postgresql_conf_path to be an absolute path, so I think you should change the data type to Stdlib::Absolutepath here if you do so. Because it will be impossible to set it to a relative path. On the other hand, that never really was valid anyway. But I may be a bit pedantic here.
SimonHoenscheid
changed the title
No description provided.