diff --git a/changelogs/unreleased/48132-custom-hook-return-all-messages-plus-error-logging.yml b/changelogs/unreleased/48132-custom-hook-return-all-messages-plus-error-logging.yml new file mode 100644 index 0000000000000000000000000000000000000000..f97d5c8f8017147d14b6aadc79b2c852e826639b --- /dev/null +++ b/changelogs/unreleased/48132-custom-hook-return-all-messages-plus-error-logging.yml @@ -0,0 +1,6 @@ +--- +title: pre-receive, post-receive and update hooks now return all stdout and stderr + in its response when script exits with an error code, and will log all stderr +merge_request: 1152 +author: +type: changed diff --git a/ruby/lib/gitlab/git/hook.rb b/ruby/lib/gitlab/git/hook.rb index 222c93d3bb34f43ab19ea0bf697f152302c24748..ef02f4fadfd59617d9d6740d47ca78fd0cc67270 100644 --- a/ruby/lib/gitlab/git/hook.rb +++ b/ruby/lib/gitlab/git/hook.rb @@ -12,6 +12,8 @@ module Gitlab end GL_PROTOCOL = 'web' + ERROR_LOG_FORMAT = '%s hook error in repository %s: %s' + attr_reader :name, :path, :repository def initialize(name, repository) @@ -43,61 +45,60 @@ module Gitlab private + # pre- and post-receive hooks pass the changes as stdin to the + # executable script. + # See https://git-scm.com/docs/githooks#pre-receive def call_receive_hook(gl_id, gl_username, oldrev, newrev, ref) - changes = [oldrev, newrev, ref].join(" ") - - exit_status = false - exit_message = nil - vars = env_base_vars(gl_id, gl_username) - options = { - chdir: repo_path + chdir: repo_path, + stdin_data: [oldrev, newrev, ref].join(' ') } - Open3.popen3(vars, path, options) do |stdin, stdout, stderr, wait_thr| - exit_status = true - stdin.sync = true - - # in git, pre- and post- receive hooks may just exit without - # reading stdin. We catch the exception to avoid a broken pipe - # warning - begin - # inject all the changes as stdin to the hook - changes.lines do |line| - stdin.puts line - end - rescue Errno::EPIPE - end + stdout, stderr, exit_status = Open3.capture3(vars, path, options) - stdin.close + log_errors(stderr) - unless wait_thr.value == 0 - exit_status = false - exit_message = retrieve_error_message(stderr, stdout) - end - end + exit_message = retrieve_output(stdout, stderr, exit_status) - [exit_status, exit_message] + [exit_status.success?, exit_message] end + # Update hooks pass the changes as arguments to the executable script + # See https://git-scm.com/docs/githooks#update def call_update_hook(gl_id, gl_username, oldrev, newrev, ref) - options = { - chdir: repo_path - } - + vars = env_base_vars(gl_id, gl_username) args = [ref, oldrev, newrev] + options = { chdir: repo_path } - vars = env_base_vars(gl_id, gl_username) + stdout, stderr, exit_status = Open3.capture3(vars, path, *args, options) + + log_errors(stderr) + + exit_message = retrieve_output(stdout, stderr, exit_status) + + [exit_status.success?, exit_message] + end + + def retrieve_output(stdout, stderr, exit_status) + return if exit_status.success? - stdout, stderr, status = Open3.capture3(vars, path, *args, options) - [status.success?, stderr.presence || stdout] + (stdout + stderr).strip end - def retrieve_error_message(stderr, stdout) - err_message = stderr.read - err_message = err_message.blank? ? stdout.read : err_message - err_message + def log_errors(errors) + return unless errors.present? + + errors.split("\n").each do |error| + message = format( + ERROR_LOG_FORMAT, + name, + repository.relative_path, + error + ) + + Gitlab::GitLogger.error(message) + end end def env_base_vars(gl_id, gl_username) diff --git a/ruby/spec/lib/gitlab/git/hook_spec.rb b/ruby/spec/lib/gitlab/git/hook_spec.rb index 2e1551ba106285571ff4f72dc67d81c5e71d5f15..e21072e3761298c36cef11b2a1b76e4658b36d40 100644 --- a/ruby/spec/lib/gitlab/git/hook_spec.rb +++ b/ruby/spec/lib/gitlab/git/hook_spec.rb @@ -67,30 +67,139 @@ describe Gitlab::Git::Hook do end end + shared_examples_for 'when the hooks fail or are successful' do + it 'logs all stderr to the error log' do + hook_names.each do |hook| + error_message = format( + Gitlab::Git::Hook::ERROR_LOG_FORMAT, + hook, + repo.relative_path, + 'msg to STDERR' + ) + + expect(Gitlab::GitLogger).to receive(:error).with(error_message) + + described_class.new(hook, repo).trigger('user-456', 'admin', '0' * 40, 'a' * 40, 'master') + end + end + + describe 'hooks passing git changes to the script' do + context 'the pre-receive and post-receive hook' do + let(:hook_names) { %w[pre-receive post-receive] } + let(:script) do + <<-SCRIPT + #!/bin/sh + echo $( "' do + hook_names.each do |hook| + trigger_result = described_class.new(hook, repo) + .trigger('user-456', 'admin', '0' * 40, 'a' * 40, 'master') + + expect(trigger_result.last).to eq('0000000000000000000000000000000000000000 aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa master') + end + end + end + + context 'the update hook' do + let(:script) do + <<-SCRIPT + #!/bin/sh + echo $1; + echo $2; + echo $3; + exit 1; + SCRIPT + end + + it 'receives git changes as args in the order ' do + trigger_result = described_class.new('update', repo) + .trigger('user-456', 'admin', '0' * 40, 'a' * 40, 'master') + + expect(trigger_result.last).to eq("master\n0000000000000000000000000000000000000000\naaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa") + end + end + end + end + context 'when the hooks are successful' do - let(:script) { "#!/bin/sh\nexit 0\n" } + let(:script) do + <<-SCRIPT + #!/bin/sh + echo "msg to STDOUT"; + 1>&2 echo "msg to STDERR"; + exit 0 + SCRIPT + end it 'returns true' do hook_names.each do |hook| + silence_error_log + trigger_result = described_class.new(hook, repo) .trigger('user-456', 'admin', '0' * 40, 'a' * 40, 'master') - expect(trigger_result.first).to be(true) + expect(trigger_result.first).to eq(true) end end + + it 'does not return a message' do + hook_names.each do |hook| + silence_error_log + + trigger_result = described_class.new(hook, repo) + .trigger('user-456', 'admin', '0' * 40, 'a' * 40, 'master') + + expect(trigger_result.last).to eq(nil) + end + end + + include_examples 'when the hooks fail or are successful' end context 'when the hooks fail' do - let(:script) { "#!/bin/sh\nexit 1\n" } + let(:script) do + <<-SCRIPT + #!/bin/sh + echo "msg to STDOUT"; + 1>&2 echo "msg to STDERR"; + exit 1 + SCRIPT + end it 'returns false' do hook_names.each do |hook| + silence_error_log + trigger_result = described_class.new(hook, repo) - .trigger('user-1', 'admin', '0' * 40, 'a' * 40, 'master') + .trigger('user-456', 'admin', '0' * 40, 'a' * 40, 'master') - expect(trigger_result.first).to be(false) + expect(trigger_result.first).to eq(false) end end + + it 'returns all stdout and stderr messages' do + hook_names.each do |hook| + silence_error_log + + trigger_result = described_class.new(hook, repo) + .trigger('user-456', 'admin', '0' * 40, 'a' * 40, 'master') + + expect(trigger_result.last).to eq("msg to STDOUT\nmsg to STDERR") + end + end + + include_examples 'when the hooks fail or are successful' end end + + # Call before tests of scripts that write to stderr, when stderr is + # not a subject of a test. This prevents the error from appearing in + # rspec's output when rspec is running + def silence_error_log + expect(Gitlab::GitLogger).to receive(:error) + end end