Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Pass all PG settings as env vars #990

Merged
merged 3 commits into from
Oct 23, 2024
+24 −18
Merged

Pass all PG settings as env vars #990

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
03fdb97
Refs #37940 - allow passing extra_env when calling commands
evgeni Oct 18, 2024
e627ceb
Fixes #37940 - pass PGPASSWORD via environment variable
evgeni Oct 18, 2024
150b2b3
Refs #37940 - pass all PostgreSQL settings as environment variables
evgeni Oct 18, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion Gemfile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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', '>= 7.6', '< 8'
gem 'librarian-puppet', '>= 3.0'
gem 'puppet', ENV.key?('PUPPET_VERSION') ? "~> #{ENV['PUPPET_VERSION']}" : '~> 7.0'
gem 'facter', '>= 3.0', '!= 4.0.52'
Expand Down
16 changes: 8 additions & 8 deletions hooks/boot/01-kafo-hook-extensions.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -101,8 +101,8 @@ def log_and_say(level, message, do_say = true, do_log = true)
Kafo::KafoConfigure.logger.send(level, message) if do_log
end

def execute!(command, do_say = true, do_log = true)
stdout_stderr, status = execute_command(command, do_say, do_log)
def execute!(command, do_say = true, do_log = true, extra_env = {})
stdout_stderr, status = execute_command(command, do_say, do_log, extra_env)

if stdout_stderr.nil?
log_and_say(:error, "Command #{command} not found", do_say, do_log)
Expand All @@ -115,21 +115,21 @@ def execute!(command, do_say = true, do_log = true)
end
end

def execute_as!(user, command, do_say = true, do_log = true)
def execute_as!(user, command, do_say = true, do_log = true, extra_env = {})
runuser_command = "runuser -l #{user} -c '#{command}'"
execute!(runuser_command, do_say, do_log)
execute!(runuser_command, do_say, do_log, extra_env)
end

def execute(command, do_say, do_log)
_stdout_stderr, status = execute_command(command, do_say, do_log)
def execute(command, do_say, do_log, extra_env = {})
_stdout_stderr, status = execute_command(command, do_say, do_log, extra_env)
status
end

def execute_command(command, do_say, do_log)
def execute_command(command, do_say, do_log, extra_env = {})
log_and_say(:debug, "Executing: #{command}", do_say, do_log)

begin
stdout_stderr, status = Open3.capture2e(*Kafo::PuppetCommand.format_command(command))
stdout_stderr, status = Open3.capture2e(*Kafo::PuppetCommand.format_command(command, extra_env))
rescue Errno::ENOENT
return [nil, false]
end
Expand Down
16 changes: 11 additions & 5 deletions hooks/pre/10-reset_data.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,18 +56,24 @@ def reset_candlepin
empty_db_in_postgresql('candlepin')
end

def pg_command_base(config, command, args)
"PGPASSWORD='#{config[:password]}' #{command} -U #{config[:username]} -h #{config[:host]} -p #{config[:port]} #{args}"
def pg_env(config)
{
'PGHOST' => config.fetch(:host, 'localhost'),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need a default or can we trust psql to figure it out?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No idea, copied from your fm patch ;)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think keeping a default for localhost is OK for now then.

'PGPORT' => config.fetch(:port, '5432').to_s,
'PGUSER' => config[:username],
'PGPASSWORD' => config[:password],
'PGDATABASE' => config[:database],
}
end

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}\""
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd be tempted to pass in the statement via stdin but wonder if that works well with runuser (if we do that)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

runuser is only used for local sql, and there we don't need statements.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

that said, our execute helpers don't do stdin right now, so it'd need more refactoring.

end

# WARNING: deletes all the data owned by the user. No warnings. No confirmations.
def empty_database!(config)
delete_statement = 'DROP OWNED BY CURRENT_USER CASCADE;'
execute!(pg_sql_statement(config, delete_statement), false, true)
execute!(pg_sql_statement(delete_statement), false, true, pg_env(config))
end

def clear_pulpcore_content(content_dir)
Expand Down
4 changes: 2 additions & 2 deletions spec/hook_context_extension_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -153,7 +153,7 @@

it 'executes a command' do
expect(subject).to be_nil
expect(context).to have_received(:execute_command).with(command, true, true)
expect(context).to have_received(:execute_command).with(command, true, true, {})
end
end

Expand All @@ -168,7 +168,7 @@

it 'executes a command' do
expect(subject).to be_nil
expect(context).to have_received(:execute!).with("runuser -l postgres -c 'uptime'", true, true)
expect(context).to have_received(:execute!).with("runuser -l postgres -c 'uptime'", true, true, {})
end
end
end
Expand Down
4 changes: 2 additions & 2 deletions spec/postgresql_upgrade_hook_context_extension_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,12 +70,12 @@

it 'runs postgresql-setup --upgrade' do
expect(subject).to be_nil
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, {})
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

runuser -l clears the environment, so that's why I did not use the env stuff here too.

end

it 'runs vacuumdb --all --analyze-in-stages' do
expect(subject).to be_nil
expect(context).to have_received(:'execute!').with("runuser -l postgres -c 'vacuumdb --all --analyze-in-stages'", false, true)
expect(context).to have_received(:'execute!').with("runuser -l postgres -c 'vacuumdb --all --analyze-in-stages'", false, true, {})
end
end
end