Laurenz,
Thank you for the review.
Included with this email are 2 patches.
`0004-Add-ldapservice-connection-parameter.patch` contains all of the
changes you recommended:
- Removed the redundant/unneeded check for the string ldap
- The function has now been moved from parseServiceInfo() to
conninfo_add_defaults().
- I will say though the reason I included this in parseServiceInfo()
was I consider this new LDAP functionality and the existing
pg_service.conf() functionality all different ways of accessing a
postgres "service". I thought it made sense to handle all service
parsing in a single function. Happy to keep it moved it if I was
incorrect about this
- A failed parseServiceInfo() now returns false in conninfo_add_defaults()
- Made documentation a bit more explicit.
Also I added support for specifying PGLDAPPSERVICE in the form of an
env var as well. Also added additional tests.
> For the same reason, I am not entirely happy with the name "ldapservice", but I can't think of anything better.
I have a few suggestions here. Maybe `remoteserviceuri` would be a
better name. This tells the reader that it's a uri and not the name of
a service. LDAP is not mentioned because maybe in the future this
functionality could be expanded to read connection parameters from an
HTTP server, postgres side channel, etc.
Alternatively we could bypass the naming altogether and add this
functionality into the existing pgservice parameter. I have attached a
second patch `0001-Allow-LDAP-lookup-from-pgservice-connection-paramete.patch`
that has an implementation of this. The idea being that if you pass a
string that starts with ldap:// it will look up the service at the
given LDAP uri instead of looking for the corresponding service in the
pg_service.conf file. This would be a breaking change for anyone who
populated their service names as LDAP uris, I would imagine this
would be a very small number of users, if any. It also ends up being a
smaller patch overall.
> I don't have an LDAP server handy, so I couldn't test the patch
On the off chance that it helps, we now have unit tests over the LDAP
service functionality. It should automatically start a slapd server,
etc. If you add a false assertion in the perl code it should fail the
test and also dump a bunch of files into the src/test/ldap directory.
You can copy paste the slapd invocation that is left there. I did have
to fight with App Armor a bit to get it to be okay with a slapd
process reading files in unexpected locations.
Thanks again for the review,
Andrew Jackson
