Refactor plugins feature and make some doc improvements

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

7 files changed, 44 insertions, 25 deletions

diff --git a/app/workers/plugin_worker.rb b/app/workers/plugin_worker.rb
index 34a3c8d62ac..bfcc683d99a 100644
--- a/app/workers/plugin_worker.rb
+++ b/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
diff --git a/doc/administration/plugins.md b/doc/administration/plugins.md
index 89ab0b9c8ea..ed1a3480ffc 100644
--- a/doc/administration/plugins.md
+++ b/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.
diff --git a/lib/gitlab/plugin.rb b/lib/gitlab/plugin.rb
index 96332362a4c..0d1cb16b378 100644
--- a/lib/gitlab/plugin.rb
+++ b/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
diff --git a/lib/gitlab/plugin_logger.rb b/lib/gitlab/plugin_logger.rb
new file mode 100644
index 00000000000..a106a2677ed
--- /dev/null
+++ b/lib/gitlab/plugin_logger.rb
@@ -0,0 +1,8 @@
+module Gitlab
+  class PluginLogger < Gitlab::Logger
+    def self.file_name_noext
+      'plugin'
+    end
+  end
+end
+
diff --git a/lib/tasks/plugins.rake b/lib/tasks/plugins.rake
index 11c41f13614..7a9de3afbec 100644
--- a/lib/tasks/plugins.rake
+++ b/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
diff --git a/spec/lib/gitlab/plugin_spec.rb b/spec/lib/gitlab/plugin_spec.rb
index a01e1383e3b..33dd4f79130 100644
--- a/spec/lib/gitlab/plugin_spec.rb
+++ b/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
diff --git a/spec/workers/plugin_worker_spec.rb b/spec/workers/plugin_worker_spec.rb
index 60631def8f3..9238a8199bc 100644
--- a/spec/workers/plugin_worker_spec.rb
+++ b/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