Refactor plugin execution method

Signed-off-by: Dmitriy Zaporozhets <dmitriy.zaporozhets@gmail.com>

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

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

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
 
-    it 'ensures plugin received data via stdin' do
-      subject
+      after do
+        tmp_file.close!
+      end
 
-      expect(File.read(tmp_file.path)).to eq(data.to_json)
+      it { is_expected.to be true }
+
+      it 'ensures plugin received data via stdin' do
+        subject
+
+        expect(File.read(tmp_file.path)).to eq(data.to_json)
+      end
+    end
+
+    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
+
+      before do
+        File.chmod(0o777, plugin)
+      end
+
+      it { is_expected.to be false }
     end
   end
-
-  private
-
-  def plugin_source
-    <<~EOS
-      #!/usr/bin/env ruby
-      x = STDIN.read
-      File.write('#{tmp_file.path}', x)
-    EOS
-  end
 end

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