diff options
| -rw-r--r-- | app/workers/plugin_worker.rb | 2 | ||||
| -rw-r--r-- | lib/gitlab/plugin.rb | 11 | ||||
| -rw-r--r-- | spec/lib/gitlab/plugin_spec.rb | 53 | ||||
| -rw-r--r-- | spec/workers/plugin_worker_spec.rb | 8 |
4 files changed, 49 insertions, 25 deletions
diff --git a/app/workers/plugin_worker.rb b/app/workers/plugin_worker.rb index cebdf8d0017..34a3c8d62ac 100644 --- a/app/workers/plugin_worker.rb +++ b/app/workers/plugin_worker.rb @@ -5,7 +5,5 @@ class PluginWorker def perform(file_name, data) Gitlab::Plugin.execute(file_name, data) - rescue => e - Rails.logger.error("#{self.class}: #{e.message}") end end diff --git a/lib/gitlab/plugin.rb b/lib/gitlab/plugin.rb index 3e82f5cb0d8..96332362a4c 100644 --- a/lib/gitlab/plugin.rb +++ b/lib/gitlab/plugin.rb @@ -13,11 +13,20 @@ module Gitlab end def self.execute(file, data) - _output, exit_status = Gitlab::Popen.popen([file]) do |stdin| + result = Gitlab::Popen.popen_with_detail([file]) do |stdin| stdin.write(data.to_json) end + exit_status = result.status&.exitstatus + + unless exit_status.zero? + Rails.logger.error("Plugin Error => #{file}: #{result.stderr}") + end + exit_status.zero? + rescue => e + Rails.logger.error("Plugin Error => #{file}: #{e.message}") + false end end end diff --git a/spec/lib/gitlab/plugin_spec.rb b/spec/lib/gitlab/plugin_spec.rb index 065b18858be..a01e1383e3b 100644 --- a/spec/lib/gitlab/plugin_spec.rb +++ b/spec/lib/gitlab/plugin_spec.rb @@ -6,34 +6,59 @@ describe Gitlab::Plugin do let(:plugin) { Rails.root.join('plugins', 'test.rb') } let(:tmp_file) { Tempfile.new('plugin-dump') } + let(:plugin_source) do + <<~EOS + #!/usr/bin/env ruby + x = STDIN.read + File.write('#{tmp_file.path}', x) + EOS + end + before do File.write(plugin, plugin_source) - File.chmod(0o777, plugin) end after do FileUtils.rm(plugin) - tmp_file.close! end subject { described_class.execute(plugin.to_s, data) } - it { is_expected.to be true } + context 'successful execution' do + before do + File.chmod(0o777, plugin) + end + + after do + tmp_file.close! + end - it 'ensures plugin received data via stdin' do - subject + it { is_expected.to be true } - expect(File.read(tmp_file.path)).to eq(data.to_json) + it 'ensures plugin received data via stdin' do + subject + + expect(File.read(tmp_file.path)).to eq(data.to_json) + end end - end - private + context 'non-executable' do + it { is_expected.to be false } + end + + context 'non-zero exit' do + let(:plugin_source) do + <<~EOS + #!/usr/bin/env ruby + exit 1 + EOS + end - def plugin_source - <<~EOS - #!/usr/bin/env ruby - x = STDIN.read - File.write('#{tmp_file.path}', x) - EOS + before do + File.chmod(0o777, plugin) + end + + it { is_expected.to be false } + end end end diff --git a/spec/workers/plugin_worker_spec.rb b/spec/workers/plugin_worker_spec.rb index d64de05f211..60631def8f3 100644 --- a/spec/workers/plugin_worker_spec.rb +++ b/spec/workers/plugin_worker_spec.rb @@ -15,13 +15,5 @@ describe PluginWorker do expect(subject.perform(filename, data)).to be_truthy end - - it 'handles exception well' do - data = { 'event_name' => 'project_create' } - - allow(Gitlab::Plugin).to receive(:execute).with(filename, data).and_raise('Permission denied') - - expect { subject.perform(filename, data) }.not_to raise_error - end end end |