theforeman / puppet-foreman

Puppet module for Foreman
GNU General Public License v3.0
104 stars 271 forks source link

Allow more database.yml settings #1194

Open dmaes opened 6 days ago

dmaes commented 6 days ago

We need to be able to set the target_session_attrs parameter in database.yml (which is a connection parameter for libpq), but this is currently not supported. I'm open to making a PR, but unsure if there is a preference for adding a String $db_target_session_attrs parameter, or just a generic Hash[String, String] $db_extra_options parameter, which we can then use to set any option. I am aware that target_session_attrs is not documented in foreman or rails docs, but from https://github.com/rails/rails/blob/main/activerecord/lib/active_record/connection_adapters/postgresql_adapter.rb#L47 , any parameter not documented there, will be passed as libpq connection parameter.

ekohl commented 6 days ago

target_session_attrs

For my own interest: https://www.postgresql.org/docs/current/libpq-connect.html#LIBPQ-CONNECT-TARGET-SESSION-ATTRS

I'm open to making a PR, but unsure if there is a preference for adding a String $db_target_session_attrs parameter, or just a generic Hash[String, String] $db_extra_options parameter, which we can then use to set any option.

Something I've been thinking about recently is just exposing a database URL parameter instead of each component. That gives a lot more freedom for these things. However, it would have a lot more consequences like needing to write migrations and properly test it so I haven't started it.

Any thoughts on this matter?

dmaes commented 6 days ago

I think parameter for the "default" things (host,port,username,password,etc...) definitely has it's worth for simplicity's sake for most setups. Hence why I suggested a Hash $db_extra_options, which could be somehting like the following in epp:

<% $extra_options.each |$k, $v| { -%>
<%= $k %>: <%= $v %>
<% } -%>

or even just a String $db_extra_fragment to just <%= $extra_fragment %> at the end of the template, although the first option is probably a bit cleaner, maybe even with a to_yaml and some indentation prefixing, instead of a .each.

ekohl commented 6 days ago

I'm slightly more of a fan of Hash $db_extra_options so a PR to add that is IMHO a good addition.

dmaes commented 6 days ago

Added a PR with Hash[String, String] $db_extra_options.