From fa50543b8644841fc097ffb4e6895ed76524ddd9 Mon Sep 17 00:00:00 2001 From: Luke Duncalfe Date: Tue, 26 Mar 2019 16:00:36 +1300 Subject: [PATCH 1/4] Custom hooks return stdout + stderr on success Previously custom hooks would return the stdout of the executed script, or if that was blank, then the stderr. Now custom hooks will return the combined stdout and stderr messages. Note, that the behaviour of these message only being returned when the execute script exits with a success status code has been preserved. A documentation change to Rails is in https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/25625 Issue https://gitlab.com/gitlab-org/gitlab-ce/issues/48132 Additionally, this brings it closer into line with the behaviour of git-receive-pack, which we're trying to emulate. From: https://git-scm.com/docs/githooks: > Both standard output and standard error output are forwarded to git > send-pack on the other end, so you can simply echo messages for the > user. --- ruby/lib/gitlab/git/hook.rb | 55 ++++++++------------------- ruby/spec/lib/gitlab/git/hook_spec.rb | 42 +++++++++++++++++--- 2 files changed, 52 insertions(+), 45 deletions(-) diff --git a/ruby/lib/gitlab/git/hook.rb b/ruby/lib/gitlab/git/hook.rb index 222c93d3bb3..68cb76f7d17 100644 --- a/ruby/lib/gitlab/git/hook.rb +++ b/ruby/lib/gitlab/git/hook.rb @@ -44,60 +44,35 @@ module Gitlab private 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 - - 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 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) + + exit_message = retrieve_output(stdout, stderr, exit_status) - stdout, stderr, status = Open3.capture3(vars, path, *args, options) - [status.success?, stderr.presence || stdout] + [exit_status.success?, exit_message] end - def retrieve_error_message(stderr, stdout) - err_message = stderr.read - err_message = err_message.blank? ? stdout.read : err_message - err_message + def retrieve_output(stdout, stderr, exit_status) + return if exit_status.success? + + (stdout + stderr).strip 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 2e1551ba106..0260d5b4086 100644 --- a/ruby/spec/lib/gitlab/git/hook_spec.rb +++ b/ruby/spec/lib/gitlab/git/hook_spec.rb @@ -68,27 +68,59 @@ describe Gitlab::Git::Hook do 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| 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| + 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 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| 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 eq(false) + end + end + + it 'returns all stdout and stderr messages' 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.first).to be(false) + expect(trigger_result.last).to eq("msg to STDOUT\nmsg to STDERR") end end end -- GitLab From a25e32390fad86cb4d3daadd42d36dbeecdd9fa7 Mon Sep 17 00:00:00 2001 From: Luke Duncalfe Date: Tue, 26 Mar 2019 16:14:38 +1300 Subject: [PATCH 2/4] Log custom hook stderr output --- ruby/lib/gitlab/git/hook.rb | 21 +++++++++++++ ruby/spec/lib/gitlab/git/hook_spec.rb | 45 +++++++++++++++++++++++++++ 2 files changed, 66 insertions(+) diff --git a/ruby/lib/gitlab/git/hook.rb b/ruby/lib/gitlab/git/hook.rb index 68cb76f7d17..88f4868ec6c 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) @@ -52,6 +54,8 @@ module Gitlab stdout, stderr, exit_status = Open3.capture3(vars, path, options) + log_errors(stderr) + exit_message = retrieve_output(stdout, stderr, exit_status) [exit_status.success?, exit_message] @@ -64,6 +68,8 @@ module Gitlab 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] @@ -75,6 +81,21 @@ module Gitlab (stdout + stderr).strip end + 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) { 'GITLAB_SHELL_DIR' => Gitlab.config.gitlab_shell.path, diff --git a/ruby/spec/lib/gitlab/git/hook_spec.rb b/ruby/spec/lib/gitlab/git/hook_spec.rb index 0260d5b4086..5b6e4f1cad5 100644 --- a/ruby/spec/lib/gitlab/git/hook_spec.rb +++ b/ruby/spec/lib/gitlab/git/hook_spec.rb @@ -79,6 +79,8 @@ describe Gitlab::Git::Hook do 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') @@ -88,12 +90,29 @@ describe Gitlab::Git::Hook do 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 + + 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 end context 'when the hooks fail' do @@ -108,6 +127,8 @@ describe Gitlab::Git::Hook do it 'returns false' 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') @@ -117,12 +138,36 @@ describe Gitlab::Git::Hook do 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 + + 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 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 -- GitLab From 9be1737551753c7226980ed971e8aeff58759ca8 Mon Sep 17 00:00:00 2001 From: Luke Duncalfe Date: Tue, 26 Mar 2019 16:54:10 +1300 Subject: [PATCH 3/4] Add tests for hooks receiving args and stdin --- ruby/lib/gitlab/git/hook.rb | 5 ++ ruby/spec/lib/gitlab/git/hook_spec.rb | 88 ++++++++++++++++++--------- 2 files changed, 65 insertions(+), 28 deletions(-) diff --git a/ruby/lib/gitlab/git/hook.rb b/ruby/lib/gitlab/git/hook.rb index 88f4868ec6c..ef02f4fadfd 100644 --- a/ruby/lib/gitlab/git/hook.rb +++ b/ruby/lib/gitlab/git/hook.rb @@ -45,6 +45,9 @@ 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) vars = env_base_vars(gl_id, gl_username) options = { @@ -61,6 +64,8 @@ module Gitlab [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) vars = env_base_vars(gl_id, gl_username) args = [ref, oldrev, newrev] diff --git a/ruby/spec/lib/gitlab/git/hook_spec.rb b/ruby/spec/lib/gitlab/git/hook_spec.rb index 5b6e4f1cad5..e21072e3761 100644 --- a/ruby/spec/lib/gitlab/git/hook_spec.rb +++ b/ruby/spec/lib/gitlab/git/hook_spec.rb @@ -67,6 +67,64 @@ 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) do <<-SCRIPT @@ -99,20 +157,7 @@ describe Gitlab::Git::Hook do end end - 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 + include_examples 'when the hooks fail or are successful' end context 'when the hooks fail' do @@ -147,20 +192,7 @@ describe Gitlab::Git::Hook do end end - 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 + include_examples 'when the hooks fail or are successful' end end -- GitLab From c32b77dd7c8c04820e89e2a8bc2bb4af803905c1 Mon Sep 17 00:00:00 2001 From: Luke Duncalfe Date: Wed, 27 Mar 2019 12:26:33 +1300 Subject: [PATCH 4/4] Add changelog entry --- ...2-custom-hook-return-all-messages-plus-error-logging.yml | 6 ++++++ 1 file changed, 6 insertions(+) create mode 100644 changelogs/unreleased/48132-custom-hook-return-all-messages-plus-error-logging.yml 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 00000000000..f97d5c8f801 --- /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 -- GitLab