Refactor plugins feature and make some doc improvements

app/workers/plugin_worker.rb

@@ -4,6 +4,12 @@ class PluginWorker
   sidekiq_options retry: false
 
   def perform(file_name, data)
-    Gitlab::Plugin.execute(file_name, data)
+    success, message = Gitlab::Plugin.execute(file_name, data)
+
+    unless success
+      Gitlab::PluginLogger.error("Plugin Error => #{file_name}: #{message}")
+    end
+
+    true
   end
 end

doc/administration/plugins.md

@@ -11,7 +11,7 @@ A plugin will run on each event so it's up to you to filter events or projects w
 
 ## Setup
 
-Plugins must be placed directly into `plugins` directory, subdirectories will be ignored. There is an `example` directory insider `plugins` where you can find some basic examples. 
+Plugins must be placed directly into `plugins` directory, subdirectories will be ignored. There is an `example` directory inside `plugins` where you can find some basic examples. 
 
 Follow the steps below to set up a custom hook:
 
@@ -20,11 +20,12 @@ Follow the steps below to set up a custom hook:
    `/home/git/gitlab/plugins/`. For Omnibus installs the path is
    usually `/opt/gitlab/embedded/service/gitlab-rails/plugins`.
 1. Inside the `plugins` directory, create a file with a name of your choice, but without spaces or special characters.
-1. Make the hook file executable and make sure it's owned by git.
+1. Make the hook file executable and make sure it's owned by the git user.
 1. Write the code to make the plugin function as expected. Plugin can be
    in any language. Ensure the 'shebang' at the top properly reflects the language
    type. For example, if the script is in Ruby the shebang will probably be
    `#!/usr/bin/env ruby`.
+1. The data to the plugin will be provided as JSON on STDIN. It will be exactly same as one for [system hooks]
 
 That's it! Assuming the plugin code is properly implemented the hook will fire
 as appropriate. Plugins file list is updated for each event. There is no need to restart GitLab to apply a new plugin.

lib/gitlab/plugin.rb

@@ -18,15 +18,9 @@ module Gitlab
       end
 
       exit_status = result.status&.exitstatus
-
-      unless exit_status.zero?
-        Rails.logger.error("Plugin Error => #{file}: #{result.stderr}")
-      end
-
-      exit_status.zero?
+      [exit_status.zero?, result.stderr]
     rescue => e
-      Rails.logger.error("Plugin Error => #{file}: #{e.message}")
-      false
+      [false, e.message]
     end
   end
 end

lib/gitlab/plugin_logger.rb

+module Gitlab
+  class PluginLogger < Gitlab::Logger
+    def self.file_name_noext
+      'plugin'
+    end
+  end
+end
+

lib/tasks/plugins.rake

@@ -4,12 +4,12 @@ namespace :plugins do
     puts 'Validating plugins from /plugins directory'
 
     Gitlab::Plugin.files.each do |file|
-      result = Gitlab::Plugin.execute(file, Gitlab::DataBuilder::Push::SAMPLE_DATA)
+      success, message = Gitlab::Plugin.execute(file, Gitlab::DataBuilder::Push::SAMPLE_DATA)
 
-      if result
-        puts "* #{file} succeed (zero exit code)"
+      if success
+        puts "* #{file} succeed (zero exit code)."
       else
-        puts "* #{file} failure (non-zero exit code)"
+        puts "* #{file} failure (non-zero exit code). #{message}"
       end
     end
   end

spec/lib/gitlab/plugin_spec.rb

@@ -5,6 +5,9 @@ describe Gitlab::Plugin do
     let(:data) { Gitlab::DataBuilder::Push::SAMPLE_DATA }
     let(:plugin) { Rails.root.join('plugins', 'test.rb') }
     let(:tmp_file) { Tempfile.new('plugin-dump') }
+    let(:result) { described_class.execute(plugin.to_s, data) }
+    let(:success) { result.first }
+    let(:message) { result.last }
 
     let(:plugin_source) do
       <<~EOS
@@ -22,8 +25,6 @@ describe Gitlab::Plugin do
       FileUtils.rm(plugin)
     end
 
-    subject { described_class.execute(plugin.to_s, data) }
-
     context 'successful execution' do
       before do
         File.chmod(0o777, plugin)
@@ -33,17 +34,19 @@ describe Gitlab::Plugin do
         tmp_file.close!
       end
 
-      it { is_expected.to be true }
+      it { expect(success).to be true }
+      it { expect(message).to be_empty }
 
       it 'ensures plugin received data via stdin' do
-        subject
+        result
 
         expect(File.read(tmp_file.path)).to eq(data.to_json)
       end
     end
 
     context 'non-executable' do
-      it { is_expected.to be false }
+      it { expect(success).to be false }
+      it { expect(message).to include('Permission denied') }
     end
 
     context 'non-zero exit' do
@@ -58,7 +61,8 @@ describe Gitlab::Plugin do
         File.chmod(0o777, plugin)
       end
 
-      it { is_expected.to be false }
+      it { expect(success).to be false }
+      it { expect(message).to be_empty }
     end
   end
 end

spec/workers/plugin_worker_spec.rb

@@ -3,16 +3,22 @@ require 'spec_helper'
 describe PluginWorker do
   include RepoHelpers
 
-  subject { described_class.new }
-
   let(:filename) { 'my_plugin.rb' }
+  let(:data) { { 'event_name' => 'project_create' } }
+
+  subject { described_class.new }
 
   describe '#perform' do
     it 'executes Gitlab::Plugin with expected values' do
-      data = { 'event_name' => 'project_create' }
+      allow(Gitlab::Plugin).to receive(:execute).with(filename, data).and_return([true, ''])
+
+      expect(subject.perform(filename, data)).to be_truthy
+    end
 
-      allow(Gitlab::Plugin).to receive(:execute).with(filename, data).and_return(true)
+    it 'logs message in case of plugin execution failure' do
+      allow(Gitlab::Plugin).to receive(:execute).with(filename, data).and_return([false, 'permission denied'])
 
+      expect(Gitlab::PluginLogger).to receive(:error)
       expect(subject.perform(filename, data)).to be_truthy
     end
   end