move PGPASSWORD to env #990
Conversation
| def pg_sql_statement(config, statement) | ||
| pg_command_base(config, 'psql', "-d #{config[:database]} -t -c \"#{statement}\"") | ||
| def pg_sql_statement(statement) | ||
| "psql -t -c \"#{statement}\"" |
There was a problem hiding this comment.
I'd be tempted to pass in the statement via stdin but wonder if that works well with runuser (if we do that)
There was a problem hiding this comment.
runuser is only used for local sql, and there we don't need statements.
There was a problem hiding this comment.
that said, our execute helpers don't do stdin right now, so it'd need more refactoring.
| "PGPASSWORD='#{config[:password]}' #{command} -U #{config[:username]} -h #{config[:host]} -p #{config[:port]} #{args}" | ||
| def pg_env(config) | ||
| { | ||
| 'PGHOST' => config.fetch(:host, 'localhost'), |
There was a problem hiding this comment.
Do we need a default or can we trust psql to figure it out?
There was a problem hiding this comment.
No idea, copied from your fm patch ;)
There was a problem hiding this comment.
There I had it because local? compares to localhost and I didn't want to change that. Perhaps we should make it more robust so it can parse any postgresql url. I'd like to be able to use unix sockets and ident auth but now we'd break foreman-maintain
There was a problem hiding this comment.
Here we have remote_host?(hostname) which does something very similar.
The old code just assumed config[:host] is set (the command would be wrong if it were unset), which we can do here too?
Yeah, but our helpers incorporate the escaping and I like helpers ;) |
|
You can pass an array and avoid any escaping |
I meant the "escaping from Puppet ruby env". My point is:
So if we can drop the "puppet-env-escaping" all together, cool (but I think we can't, as we call f-m and f-rake, which need system ruby), but scope-creep. Here I just want to pass in PG things via env :) |
|
/packit build |
| expect(context).to have_received(:'execute!').with("runuser -l postgres -c 'PGSETUP_INITDB_OPTIONS=\"--lc-collate=en_US.UTF-8 --lc-ctype=en_US.UTF-8 --locale=en_US.UTF-8\" postgresql-setup --upgrade'", false, true) | ||
| expect(context).to have_received(:'execute!').with("runuser -l postgres -c 'PGSETUP_INITDB_OPTIONS=\"--lc-collate=en_US.UTF-8 --lc-ctype=en_US.UTF-8 --locale=en_US.UTF-8\" postgresql-setup --upgrade'", false, true, {}) |
There was a problem hiding this comment.
runuser -l clears the environment, so that's why I did not use the env stuff here too.
| @@ -4,7 +4,7 @@ source 'https://rubygems.org' | |||
| # We don't want to list psych since that updates the bundled version | |||
| gem 'rdoc', '< 6.4' | |||
|
|
|||
| gem 'kafo', '>= 7.3', '< 8' | |||
| gem 'kafo', github: 'theforeman/kafo' | |||
There was a problem hiding this comment.
This needs updating once Kafo 7.6 is released with the feature (see theforeman/kafo#380), but other than that this is ready for review
needs theforeman/kafo#378