From 8f47d484dab12df982655c3c05305bce7624914d Mon Sep 17 00:00:00 2001
From: Grzegorz Bizon <grzesiek.bizon@gmail.com>
Date: Mon, 25 Sep 2017 16:22:00 +0200
Subject: Extract pipeline chain builder classes from service

---
 app/services/ci/create_pipeline_service.rb         | 118 +++++----------------
 lib/gitlab/ci/pipeline/chain/base.rb               |  27 +++++
 lib/gitlab/ci/pipeline/chain/skip.rb               |  33 ++++++
 lib/gitlab/ci/pipeline/chain/validate.rb           | 107 +++++++++++++++++++
 spec/lib/gitlab/ci/pipeline/chain/validate_spec.rb | 110 +++++++++++++++++++
 spec/services/ci/create_pipeline_service_spec.rb   |  99 -----------------
 6 files changed, 301 insertions(+), 193 deletions(-)
 create mode 100644 lib/gitlab/ci/pipeline/chain/base.rb
 create mode 100644 lib/gitlab/ci/pipeline/chain/skip.rb
 create mode 100644 lib/gitlab/ci/pipeline/chain/validate.rb
 create mode 100644 spec/lib/gitlab/ci/pipeline/chain/validate_spec.rb

diff --git a/app/services/ci/create_pipeline_service.rb b/app/services/ci/create_pipeline_service.rb
index d20de9b16a4..1a5bf7142e0 100644
--- a/app/services/ci/create_pipeline_service.rb
+++ b/app/services/ci/create_pipeline_service.rb
@@ -2,6 +2,9 @@ module Ci
   class CreatePipelineService < BaseService
     attr_reader :pipeline
 
+    SEQUENCE = [Gitlab::Ci::Pipeline::Chain::Validate,
+                Gitlab::Ci::Pipeline::Chain::Skip]
+
     def execute(source, ignore_skip_ci: false, save_on_errors: true, trigger_request: nil, schedule: nil)
       @pipeline = Ci::Pipeline.new(
         source: source,
@@ -16,11 +19,23 @@ module Ci
         protected: project.protected_for?(ref)
       )
 
-      result = validate_project_and_git_items ||
-        validate_pipeline(ignore_skip_ci: ignore_skip_ci,
-                          save_on_errors: save_on_errors)
+      command = OpenStruct.new(ignore_skip_ci: ignore_skip_ci,
+                               save_incompleted: save_on_errors,
+                               trigger_request: trigger_request,
+                               schedule: schedule,
+                               project: project,
+                               current_user: current_user)
+
+      sequence = SEQUENCE.map { |chain| chain.new(pipeline, command) }
+
+      done = sequence.any? do |chain|
+        chain.perform!
+        chain.break?
+      end
 
-      return result if result
+      update_merge_requests_head_pipeline if pipeline.persisted?
+
+      return pipeline if done
 
       begin
         Ci::Pipeline.transaction do
@@ -36,10 +51,8 @@ module Ci
         return error("Failed to persist the pipeline: #{e}")
       end
 
-      update_merge_requests_head_pipeline
-
+      update_merge_requests_head_pipeline if pipeline.persisted?
       cancel_pending_pipelines if project.auto_cancel_pending_pipelines?
-
       pipeline_created_counter.increment(source: source)
 
       pipeline.tap(&:process!)
@@ -47,65 +60,12 @@ module Ci
 
     private
 
-    def validate_project_and_git_items
-      unless project.builds_enabled?
-        return error('Pipeline is disabled')
-      end
-
-      unless allowed_to_trigger_pipeline?
-        if can?(current_user, :create_pipeline, project)
-          return error("Insufficient permissions for protected ref '#{ref}'")
-        else
-          return error('Insufficient permissions to create a new pipeline')
-        end
-      end
-
-      unless branch? || tag?
-        return error('Reference not found')
-      end
-
-      unless commit
-        return error('Commit not found')
-      end
-    end
-
-    def validate_pipeline(ignore_skip_ci:, save_on_errors:)
-      unless pipeline.config_processor
-        unless pipeline.ci_yaml_file
-          return error("Missing #{pipeline.ci_yaml_file_path} file")
-        end
-        return error(pipeline.yaml_errors, save: save_on_errors)
-      end
-
-      if !ignore_skip_ci && skip_ci?
-        pipeline.skip if save_on_errors
-        return pipeline
-      end
-
-      unless pipeline.has_stage_seeds?
-        return error('No stages / jobs for this pipeline.')
-      end
-    end
-
-    def allowed_to_trigger_pipeline?
-      if current_user
-        allowed_to_create?
-      else # legacy triggers don't have a corresponding user
-        !project.protected_for?(ref)
-      end
+    def commit
+      @commit ||= project.commit(origin_sha || origin_ref)
     end
 
-    def allowed_to_create?
-      return unless can?(current_user, :create_pipeline, project)
-
-      access = Gitlab::UserAccess.new(current_user, project: project)
-      if branch?
-        access.can_update_branch?(ref)
-      elsif tag?
-        access.can_create_tag?(ref)
-      else
-        true # Allow it for now and we'll reject when we check ref existence
-      end
+    def sha
+      commit.try(:id)
     end
 
     def update_merge_requests_head_pipeline
@@ -115,11 +75,6 @@ module Ci
         .update_all(head_pipeline_id: @pipeline.id)
     end
 
-    def skip_ci?
-      return false unless pipeline.git_commit_message
-      pipeline.git_commit_message =~ /\[(ci[ _-]skip|skip[ _-]ci)\]/i
-    end
-
     def cancel_pending_pipelines
       Gitlab::OptimisticLocking.retry_lock(auto_cancelable_pipelines) do |cancelables|
         cancelables.find_each do |cancelable|
@@ -136,13 +91,6 @@ module Ci
         .created_or_pending
     end
 
-    def commit
-      @commit ||= project.commit(origin_sha || origin_ref)
-    end
-
-    def sha
-      commit.try(:id)
-    end
 
     def before_sha
       params[:checkout_sha] || params[:before] || Gitlab::Git::BLANK_SHA
@@ -156,13 +104,6 @@ module Ci
       params[:ref]
     end
 
-    def branch?
-      return @is_branch if defined?(@is_branch)
-
-      @is_branch =
-        project.repository.ref_exists?(Gitlab::Git::BRANCH_REF_PREFIX + ref)
-    end
-
     def tag?
       return @is_tag if defined?(@is_tag)
 
@@ -178,17 +119,6 @@ module Ci
       origin_sha && origin_sha != Gitlab::Git::BLANK_SHA
     end
 
-    def error(message, save: false)
-      pipeline.tap do
-        pipeline.errors.add(:base, message)
-
-        if save
-          pipeline.drop
-          update_merge_requests_head_pipeline
-        end
-      end
-    end
-
     def pipeline_created_counter
       @pipeline_created_counter ||= Gitlab::Metrics.counter(:pipelines_created_total, "Counter of pipelines created")
     end
diff --git a/lib/gitlab/ci/pipeline/chain/base.rb b/lib/gitlab/ci/pipeline/chain/base.rb
new file mode 100644
index 00000000000..8d82e1b288d
--- /dev/null
+++ b/lib/gitlab/ci/pipeline/chain/base.rb
@@ -0,0 +1,27 @@
+module Gitlab
+  module Ci
+    module Pipeline
+      module Chain
+        class Base
+          attr_reader :pipeline, :project, :current_user
+
+          def initialize(pipeline, command)
+            @pipeline = pipeline
+            @command = command
+
+            @project = command.project
+            @current_user = command.current_user
+          end
+
+          def perform!
+            raise NotImplementedError
+          end
+
+          def break?
+            raise NotImplementedError
+          end
+        end
+      end
+    end
+  end
+end
diff --git a/lib/gitlab/ci/pipeline/chain/skip.rb b/lib/gitlab/ci/pipeline/chain/skip.rb
new file mode 100644
index 00000000000..3f86275ae15
--- /dev/null
+++ b/lib/gitlab/ci/pipeline/chain/skip.rb
@@ -0,0 +1,33 @@
+module Gitlab
+  module Ci
+    module Pipeline
+      module Chain
+        class Skip < Chain::Base
+          SKIP_PATTERN = /\[(ci[ _-]skip|skip[ _-]ci)\]/i
+
+          def perform!
+            if skipped?
+              @pipeline.skip if @command.save_incompleted
+            end
+          end
+
+          def skipped?
+            !@command.ignore_skip_ci && commit_message_skips_ci?
+          end
+
+          def break?
+            skipped?
+          end
+
+          private
+
+          def commit_message_skips_ci?
+            return false unless @pipeline.git_commit_message
+
+            @skipped ||= @pipeline.git_commit_message =~ SKIP_PATTERN
+          end
+        end
+      end
+    end
+  end
+end
diff --git a/lib/gitlab/ci/pipeline/chain/validate.rb b/lib/gitlab/ci/pipeline/chain/validate.rb
new file mode 100644
index 00000000000..e7109425d6c
--- /dev/null
+++ b/lib/gitlab/ci/pipeline/chain/validate.rb
@@ -0,0 +1,107 @@
+module Gitlab
+  module Ci
+    module Pipeline
+      module Chain
+        class Validate < Chain::Base
+          include Gitlab::Allowable
+
+          def perform!
+            validate_project! || validate_repository! || validate_pipeline!
+          end
+
+          def break?
+           @pipeline.errors.any? || @pipeline.persisted?
+          end
+
+          def allowed_to_trigger_pipeline?
+            if current_user
+              allowed_to_create?
+            else # legacy triggers don't have a corresponding user
+              !project.protected_for?(@pipeline.ref)
+            end
+          end
+
+          def allowed_to_create?
+            return unless can?(current_user, :create_pipeline, project)
+
+            access = Gitlab::UserAccess.new(current_user, project: project)
+
+            if branch?
+              access.can_update_branch?(@pipeline.ref)
+            elsif tag?
+              access.can_create_tag?(@pipeline.ref)
+            else
+              true # Allow it for now and we'll reject when we check ref existence
+            end
+          end
+
+          private
+
+          def validate_project!
+            unless project.builds_enabled?
+              return error('Pipeline is disabled')
+            end
+
+            unless allowed_to_trigger_pipeline?
+              if can?(current_user, :create_pipeline, project)
+                return error("Insufficient permissions for protected ref '#{pipeline.ref}'")
+              else
+                return error('Insufficient permissions to create a new pipeline')
+              end
+            end
+          end
+
+          def validate_repository!
+            unless branch? || tag?
+              return error('Reference not found')
+            end
+
+            unless pipeline.sha
+              return error('Commit not found')
+            end
+          end
+
+          def validate_pipeline!
+            unless @pipeline.config_processor
+              unless @pipeline.ci_yaml_file
+                return error("Missing #{@pipeline.ci_yaml_file_path} file")
+              end
+
+              if @command.save_incompleted && @pipeline.has_yaml_errors?
+                @pipeline.drop
+              end
+
+              return error(@pipeline.yaml_errors)
+            end
+
+            unless @pipeline.has_stage_seeds?
+              return error('No stages / jobs for this pipeline.')
+            end
+          end
+
+          ## TODO, move to Pipeline as `branch_exists?`
+          #
+          def branch?
+            return @is_branch if defined?(@is_branch)
+
+            @is_branch = project.repository
+              .ref_exists?(Gitlab::Git::BRANCH_REF_PREFIX + pipeline.ref)
+          end
+
+          ## TODO, move to pipeline as `tag_exists?`
+          #
+          def tag?
+            return @is_tag if defined?(@is_tag)
+
+            @is_tag = project.repository
+              .ref_exists?(Gitlab::Git::TAG_REF_PREFIX + pipeline.ref)
+          end
+
+          def error(message)
+            pipeline.errors.add(:base, message)
+          end
+        end
+      end
+    end
+  end
+end
diff --git a/spec/lib/gitlab/ci/pipeline/chain/validate_spec.rb b/spec/lib/gitlab/ci/pipeline/chain/validate_spec.rb
new file mode 100644
index 00000000000..117f55fd8a7
--- /dev/null
+++ b/spec/lib/gitlab/ci/pipeline/chain/validate_spec.rb
@@ -0,0 +1,110 @@
+require 'spec_helper'
+
+describe Gitlab::Ci::Pipeline::Chain::Validate do
+  describe '#allowed_to_create?' do
+    let(:user) { create(:user) }
+    let(:project) { create(:project, :repository) }
+    let(:ref) { 'master' }
+
+    let(:pipeline) do
+      build_stubbed(:ci_pipeline, ref: ref, project: project)
+    end
+
+    let(:command) do
+      double('command', project: project, current_user: user)
+    end
+
+    subject do
+      described_class.new(pipeline, command).allowed_to_create?
+    end
+
+    context 'when user is a developer' do
+      before do
+        project.add_developer(user)
+      end
+
+      it { is_expected.to be_truthy }
+
+      context 'when the branch is protected' do
+        let!(:protected_branch) do
+          create(:protected_branch, project: project, name: ref)
+        end
+
+        it { is_expected.to be_falsey }
+
+        context 'when developers are allowed to merge' do
+          let!(:protected_branch) do
+            create(:protected_branch,
+                   :developers_can_merge,
+                   project: project,
+                   name: ref)
+          end
+
+          it { is_expected.to be_truthy }
+        end
+      end
+
+      context 'when the tag is protected' do
+        let(:ref) { 'v1.0.0' }
+
+        let!(:protected_tag) do
+          create(:protected_tag, project: project, name: ref)
+        end
+
+        it { is_expected.to be_falsey }
+
+        context 'when developers are allowed to create the tag' do
+          let!(:protected_tag) do
+            create(:protected_tag,
+                   :developers_can_create,
+                   project: project,
+                   name: ref)
+          end
+
+          it { is_expected.to be_truthy }
+        end
+      end
+    end
+
+    context 'when user is a master' do
+      before do
+        project.add_master(user)
+      end
+
+      it { is_expected.to be_truthy }
+
+      context 'when the branch is protected' do
+        let!(:protected_branch) do
+          create(:protected_branch, project: project, name: ref)
+        end
+
+        it { is_expected.to be_truthy }
+      end
+
+      context 'when the tag is protected' do
+        let(:ref) { 'v1.0.0' }
+
+        let!(:protected_tag) do
+          create(:protected_tag, project: project, name: ref)
+        end
+
+        it { is_expected.to be_truthy }
+
+        context 'when no one can create the tag' do
+          let!(:protected_tag) do
+            create(:protected_tag,
+                   :no_one_can_create,
+                   project: project,
+                   name: ref)
+          end
+
+          it { is_expected.to be_falsey }
+        end
+      end
+    end
+
+    context 'when owner cannot create pipeline' do
+      it { is_expected.to be_falsey }
+    end
+  end
+end
diff --git a/spec/services/ci/create_pipeline_service_spec.rb b/spec/services/ci/create_pipeline_service_spec.rb
index 4c2ff08039c..5b959b9a142 100644
--- a/spec/services/ci/create_pipeline_service_spec.rb
+++ b/spec/services/ci/create_pipeline_service_spec.rb
@@ -482,103 +482,4 @@ describe Ci::CreatePipelineService do
     end
   end
 
-  describe '#allowed_to_create?' do
-    let(:user) { create(:user) }
-    let(:project) { create(:project, :repository) }
-    let(:ref) { 'master' }
-
-    subject do
-      described_class.new(project, user, ref: ref)
-        .send(:allowed_to_create?)
-    end
-
-    context 'when user is a developer' do
-      before do
-        project.add_developer(user)
-      end
-
-      it { is_expected.to be_truthy }
-
-      context 'when the branch is protected' do
-        let!(:protected_branch) do
-          create(:protected_branch, project: project, name: ref)
-        end
-
-        it { is_expected.to be_falsey }
-
-        context 'when developers are allowed to merge' do
-          let!(:protected_branch) do
-            create(:protected_branch,
-                   :developers_can_merge,
-                   project: project,
-                   name: ref)
-          end
-
-          it { is_expected.to be_truthy }
-        end
-      end
-
-      context 'when the tag is protected' do
-        let(:ref) { 'v1.0.0' }
-
-        let!(:protected_tag) do
-          create(:protected_tag, project: project, name: ref)
-        end
-
-        it { is_expected.to be_falsey }
-
-        context 'when developers are allowed to create the tag' do
-          let!(:protected_tag) do
-            create(:protected_tag,
-                   :developers_can_create,
-                   project: project,
-                   name: ref)
-          end
-
-          it { is_expected.to be_truthy }
-        end
-      end
-    end
-
-    context 'when user is a master' do
-      before do
-        project.add_master(user)
-      end
-
-      it { is_expected.to be_truthy }
-
-      context 'when the branch is protected' do
-        let!(:protected_branch) do
-          create(:protected_branch, project: project, name: ref)
-        end
-
-        it { is_expected.to be_truthy }
-      end
-
-      context 'when the tag is protected' do
-        let(:ref) { 'v1.0.0' }
-
-        let!(:protected_tag) do
-          create(:protected_tag, project: project, name: ref)
-        end
-
-        it { is_expected.to be_truthy }
-
-        context 'when no one can create the tag' do
-          let!(:protected_tag) do
-            create(:protected_tag,
-                   :no_one_can_create,
-                   project: project,
-                   name: ref)
-          end
-
-          it { is_expected.to be_falsey }
-        end
-      end
-    end
-
-    context 'when owner cannot create pipeline' do
-      it { is_expected.to be_falsey }
-    end
-  end
 end
-- 
cgit v1.2.1