From 1209f4f671c290ce6c577c0ff16ad7f9ea8b6271 Mon Sep 17 00:00:00 2001
From: Grzegorz Bizon <grzesiek.bizon@gmail.com>
Date: Mon, 25 Sep 2017 13:22:04 +0200
Subject: [PATCH 01/20] Move related pipeline class to new pipeline module

---
 app/models/ci/pipeline.rb                     |   2 +-
 lib/gitlab/ci/pipeline/duration.rb            | 143 ++++++++++++++++++
 lib/gitlab/ci/pipeline_duration.rb            | 141 -----------------
 .../duration_spec.rb}                         |   6 +-
 4 files changed, 147 insertions(+), 145 deletions(-)
 create mode 100644 lib/gitlab/ci/pipeline/duration.rb
 delete mode 100644 lib/gitlab/ci/pipeline_duration.rb
 rename spec/lib/gitlab/ci/{pipeline_duration_spec.rb => pipeline/duration_spec.rb} (90%)

diff --git a/app/models/ci/pipeline.rb b/app/models/ci/pipeline.rb
index acaa028eaa2..3d5acc00f8f 100644
--- a/app/models/ci/pipeline.rb
+++ b/app/models/ci/pipeline.rb
@@ -434,7 +434,7 @@ module Ci
     def update_duration
       return unless started_at
 
-      self.duration = Gitlab::Ci::PipelineDuration.from_pipeline(self)
+      self.duration = Gitlab::Ci::Pipeline::Duration.from_pipeline(self)
     end
 
     def execute_hooks
diff --git a/lib/gitlab/ci/pipeline/duration.rb b/lib/gitlab/ci/pipeline/duration.rb
new file mode 100644
index 00000000000..469fc094cc8
--- /dev/null
+++ b/lib/gitlab/ci/pipeline/duration.rb
@@ -0,0 +1,143 @@
+module Gitlab
+  module Ci
+    module Pipeline
+      # # Introduction - total running time
+      #
+      # The problem this module is trying to solve is finding the total running
+      # time amongst all the jobs, excluding retries and pending (queue) time.
+      # We could reduce this problem down to finding the union of periods.
+      #
+      # So each job would be represented as a `Period`, which consists of
+      # `Period#first` as when the job started and `Period#last` as when the
+      # job was finished. A simple example here would be:
+      #
+      # * A (1, 3)
+      # * B (2, 4)
+      # * C (6, 7)
+      #
+      # Here A begins from 1, and ends to 3. B begins from 2, and ends to 4.
+      # C begins from 6, and ends to 7. Visually it could be viewed as:
+      #
+      #     0  1  2  3  4  5  6  7
+      #        AAAAAAA
+      #           BBBBBBB
+      #                       CCCC
+      #
+      # The union of A, B, and C would be (1, 4) and (6, 7), therefore the
+      # total running time should be:
+      #
+      #     (4 - 1) + (7 - 6) => 4
+      #
+      # # The Algorithm
+      #
+      # The algorithm used here for union would be described as follow.
+      # First we make sure that all periods are sorted by `Period#first`.
+      # Then we try to merge periods by iterating through the first period
+      # to the last period. The goal would be merging all overlapped periods
+      # so that in the end all the periods are discrete. When all periods
+      # are discrete, we're free to just sum all the periods to get real
+      # running time.
+      #
+      # Here we begin from A, and compare it to B. We could find that
+      # before A ends, B already started. That is `B.first <= A.last`
+      # that is `2 <= 3` which means A and B are overlapping!
+      #
+      # When we found that two periods are overlapping, we would need to merge
+      # them into a new period and disregard the old periods. To make a new
+      # period, we take `A.first` as the new first because remember? we sorted
+      # them, so `A.first` must be smaller or equal to `B.first`. And we take
+      # `[A.last, B.last].max` as the new last because we want whoever ended
+      # later. This could be broken into two cases:
+      #
+      #     0  1  2  3  4
+      #        AAAAAAA
+      #           BBBBBBB
+      #
+      # Or:
+      #
+      #     0  1  2  3  4
+      #        AAAAAAAAAA
+      #           BBBB
+      #
+      # So that we need to take whoever ends later. Back to our example,
+      # after merging and discard A and B it could be visually viewed as:
+      #
+      #     0  1  2  3  4  5  6  7
+      #        DDDDDDDDDD
+      #                       CCCC
+      #
+      # Now we could go on and compare the newly created D and the old C.
+      # We could figure out that D and C are not overlapping by checking
+      # `C.first <= D.last` is `false`. Therefore we need to keep both C
+      # and D. The example would end here because there are no more jobs.
+      #
+      # After having the union of all periods, we just need to sum the length
+      # of all periods to get total time.
+      #
+      #     (4 - 1) + (7 - 6) => 4
+      #
+      # That is 4 is the answer in the example.
+      module Duration
+        extend self
+
+        Period = Struct.new(:first, :last) do
+          def duration
+            last - first
+          end
+        end
+
+        def from_pipeline(pipeline)
+          status = %w[success failed running canceled]
+          builds = pipeline.builds.latest
+            .where(status: status).where.not(started_at: nil).order(:started_at)
+
+          from_builds(builds)
+        end
+
+        def from_builds(builds)
+          now = Time.now
+
+          periods = builds.map do |b|
+            Period.new(b.started_at, b.finished_at || now)
+          end
+
+          from_periods(periods)
+        end
+
+        # periods should be sorted by `first`
+        def from_periods(periods)
+          process_duration(process_periods(periods))
+        end
+
+        private
+
+        def process_periods(periods)
+          return periods if periods.empty?
+
+          periods.drop(1).inject([periods.first]) do |result, current|
+            previous = result.last
+
+            if overlap?(previous, current)
+              result[-1] = merge(previous, current)
+              result
+            else
+              result << current
+            end
+          end
+        end
+
+        def overlap?(previous, current)
+          current.first <= previous.last
+        end
+
+        def merge(previous, current)
+          Period.new(previous.first, [previous.last, current.last].max)
+        end
+
+        def process_duration(periods)
+          periods.sum(&:duration)
+        end
+      end
+    end
+  end
+end
diff --git a/lib/gitlab/ci/pipeline_duration.rb b/lib/gitlab/ci/pipeline_duration.rb
deleted file mode 100644
index 3208cc2bef6..00000000000
--- a/lib/gitlab/ci/pipeline_duration.rb
+++ /dev/null
@@ -1,141 +0,0 @@
-module Gitlab
-  module Ci
-    # # Introduction - total running time
-    #
-    # The problem this module is trying to solve is finding the total running
-    # time amongst all the jobs, excluding retries and pending (queue) time.
-    # We could reduce this problem down to finding the union of periods.
-    #
-    # So each job would be represented as a `Period`, which consists of
-    # `Period#first` as when the job started and `Period#last` as when the
-    # job was finished. A simple example here would be:
-    #
-    # * A (1, 3)
-    # * B (2, 4)
-    # * C (6, 7)
-    #
-    # Here A begins from 1, and ends to 3. B begins from 2, and ends to 4.
-    # C begins from 6, and ends to 7. Visually it could be viewed as:
-    #
-    #     0  1  2  3  4  5  6  7
-    #        AAAAAAA
-    #           BBBBBBB
-    #                       CCCC
-    #
-    # The union of A, B, and C would be (1, 4) and (6, 7), therefore the
-    # total running time should be:
-    #
-    #     (4 - 1) + (7 - 6) => 4
-    #
-    # # The Algorithm
-    #
-    # The algorithm used here for union would be described as follow.
-    # First we make sure that all periods are sorted by `Period#first`.
-    # Then we try to merge periods by iterating through the first period
-    # to the last period. The goal would be merging all overlapped periods
-    # so that in the end all the periods are discrete. When all periods
-    # are discrete, we're free to just sum all the periods to get real
-    # running time.
-    #
-    # Here we begin from A, and compare it to B. We could find that
-    # before A ends, B already started. That is `B.first <= A.last`
-    # that is `2 <= 3` which means A and B are overlapping!
-    #
-    # When we found that two periods are overlapping, we would need to merge
-    # them into a new period and disregard the old periods. To make a new
-    # period, we take `A.first` as the new first because remember? we sorted
-    # them, so `A.first` must be smaller or equal to `B.first`. And we take
-    # `[A.last, B.last].max` as the new last because we want whoever ended
-    # later. This could be broken into two cases:
-    #
-    #     0  1  2  3  4
-    #        AAAAAAA
-    #           BBBBBBB
-    #
-    # Or:
-    #
-    #     0  1  2  3  4
-    #        AAAAAAAAAA
-    #           BBBB
-    #
-    # So that we need to take whoever ends later. Back to our example,
-    # after merging and discard A and B it could be visually viewed as:
-    #
-    #     0  1  2  3  4  5  6  7
-    #        DDDDDDDDDD
-    #                       CCCC
-    #
-    # Now we could go on and compare the newly created D and the old C.
-    # We could figure out that D and C are not overlapping by checking
-    # `C.first <= D.last` is `false`. Therefore we need to keep both C
-    # and D. The example would end here because there are no more jobs.
-    #
-    # After having the union of all periods, we just need to sum the length
-    # of all periods to get total time.
-    #
-    #     (4 - 1) + (7 - 6) => 4
-    #
-    # That is 4 is the answer in the example.
-    module PipelineDuration
-      extend self
-
-      Period = Struct.new(:first, :last) do
-        def duration
-          last - first
-        end
-      end
-
-      def from_pipeline(pipeline)
-        status = %w[success failed running canceled]
-        builds = pipeline.builds.latest
-          .where(status: status).where.not(started_at: nil).order(:started_at)
-
-        from_builds(builds)
-      end
-
-      def from_builds(builds)
-        now = Time.now
-
-        periods = builds.map do |b|
-          Period.new(b.started_at, b.finished_at || now)
-        end
-
-        from_periods(periods)
-      end
-
-      # periods should be sorted by `first`
-      def from_periods(periods)
-        process_duration(process_periods(periods))
-      end
-
-      private
-
-      def process_periods(periods)
-        return periods if periods.empty?
-
-        periods.drop(1).inject([periods.first]) do |result, current|
-          previous = result.last
-
-          if overlap?(previous, current)
-            result[-1] = merge(previous, current)
-            result
-          else
-            result << current
-          end
-        end
-      end
-
-      def overlap?(previous, current)
-        current.first <= previous.last
-      end
-
-      def merge(previous, current)
-        Period.new(previous.first, [previous.last, current.last].max)
-      end
-
-      def process_duration(periods)
-        periods.sum(&:duration)
-      end
-    end
-  end
-end
diff --git a/spec/lib/gitlab/ci/pipeline_duration_spec.rb b/spec/lib/gitlab/ci/pipeline/duration_spec.rb
similarity index 90%
rename from spec/lib/gitlab/ci/pipeline_duration_spec.rb
rename to spec/lib/gitlab/ci/pipeline/duration_spec.rb
index b26728a843c..7c9836e2da6 100644
--- a/spec/lib/gitlab/ci/pipeline_duration_spec.rb
+++ b/spec/lib/gitlab/ci/pipeline/duration_spec.rb
@@ -1,6 +1,6 @@
 require 'spec_helper'
 
-describe Gitlab::Ci::PipelineDuration do
+describe Gitlab::Ci::Pipeline::Duration do
   let(:calculated_duration) { calculate(data) }
 
   shared_examples 'calculating duration' do
@@ -107,9 +107,9 @@ describe Gitlab::Ci::PipelineDuration do
 
   def calculate(data)
     periods = data.shuffle.map do |(first, last)|
-      Gitlab::Ci::PipelineDuration::Period.new(first, last)
+      described_class::Period.new(first, last)
     end
 
-    Gitlab::Ci::PipelineDuration.from_periods(periods.sort_by(&:first))
+    described_class.from_periods(periods.sort_by(&:first))
   end
 end

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: [PATCH 02/20] 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 ++++++++++++++++
 .../gitlab/ci/pipeline/chain/validate_spec.rb | 110 ++++++++++++++++
 .../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)
 
-      return result if result
+      sequence = SEQUENCE.map { |chain| chain.new(pipeline, command) }
+
+      done = sequence.any? do |chain|
+        chain.perform!
+        chain.break?
+      end
+
+      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
+    def commit
+      @commit ||= project.commit(origin_sha || origin_ref)
     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
-    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

From 9776dbda4f6d8f0f7e9a32185cc0d493f9fe3c02 Mon Sep 17 00:00:00 2001
From: Grzegorz Bizon <grzesiek.bizon@gmail.com>
Date: Mon, 25 Sep 2017 16:59:05 +0200
Subject: [PATCH 03/20] Use tag/branch methods to check if pipeline ref exists

---
 app/services/ci/create_pipeline_service.rb |  6 +++---
 lib/gitlab/ci/pipeline/chain/validate.rb   | 10 ++--------
 2 files changed, 5 insertions(+), 11 deletions(-)

diff --git a/app/services/ci/create_pipeline_service.rb b/app/services/ci/create_pipeline_service.rb
index 1a5bf7142e0..f96c4741ba4 100644
--- a/app/services/ci/create_pipeline_service.rb
+++ b/app/services/ci/create_pipeline_service.rb
@@ -107,8 +107,7 @@ module Ci
     def tag?
       return @is_tag if defined?(@is_tag)
 
-      @is_tag =
-        project.repository.ref_exists?(Gitlab::Git::TAG_REF_PREFIX + ref)
+      @is_tag = project.repository.tag_exists?(ref)
     end
 
     def ref
@@ -120,7 +119,8 @@ module Ci
     end
 
     def pipeline_created_counter
-      @pipeline_created_counter ||= Gitlab::Metrics.counter(:pipelines_created_total, "Counter of pipelines created")
+      @pipeline_created_counter ||= Gitlab::Metrics
+        .counter(:pipelines_created_total, "Counter of pipelines created")
     end
   end
 end
diff --git a/lib/gitlab/ci/pipeline/chain/validate.rb b/lib/gitlab/ci/pipeline/chain/validate.rb
index e7109425d6c..4c4c8b27be6 100644
--- a/lib/gitlab/ci/pipeline/chain/validate.rb
+++ b/lib/gitlab/ci/pipeline/chain/validate.rb
@@ -79,22 +79,16 @@ module Gitlab
             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)
+            @is_branch = project.repository.branch_exists?(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)
+            @is_tag = project.repository.tag_exists?(pipeline.ref)
           end
 
           def error(message)

From 3420c0f7dc7e9f11ec94a1b35b93ce7adba75bd1 Mon Sep 17 00:00:00 2001
From: Grzegorz Bizon <grzesiek.bizon@gmail.com>
Date: Tue, 26 Sep 2017 09:36:45 +0200
Subject: [PATCH 04/20] Fix post receive specs regarding pipeline creation

---
 spec/workers/post_receive_spec.rb | 13 ++++++++-----
 1 file changed, 8 insertions(+), 5 deletions(-)

diff --git a/spec/workers/post_receive_spec.rb b/spec/workers/post_receive_spec.rb
index d3707a3cc11..05eecf5f0bb 100644
--- a/spec/workers/post_receive_spec.rb
+++ b/spec/workers/post_receive_spec.rb
@@ -70,12 +70,15 @@ describe PostReceive do
 
       context "creates a Ci::Pipeline for every change" do
         before do
-          allow_any_instance_of(Ci::CreatePipelineService).to receive(:commit) do
-            OpenStruct.new(id: '123456')
-          end
-          allow_any_instance_of(Ci::CreatePipelineService).to receive(:branch?).and_return(true)
-          allow_any_instance_of(Repository).to receive(:ref_exists?).and_return(true)
           stub_ci_pipeline_to_return_yaml_file
+
+          # TODO, don't stub private methods
+          #
+          allow_any_instance_of(Ci::CreatePipelineService)
+            .to receive(:commit).and_return(OpenStruct.new(id: '123456'))
+
+          allow_any_instance_of(Repository)
+            .to receive(:branch_exists?).and_return(true)
         end
 
         it { expect { subject }.to change { Ci::Pipeline.count }.by(2) }

From 7cfaccd6edba11e43963bbd4dcb5c2bb3c71d9f4 Mon Sep 17 00:00:00 2001
From: Grzegorz Bizon <grzesiek.bizon@gmail.com>
Date: Tue, 26 Sep 2017 09:38:16 +0200
Subject: [PATCH 05/20] Fix code style offenses in pipeline create services

---
 app/services/ci/create_pipeline_service.rb       | 3 +--
 lib/gitlab/ci/pipeline/chain/validate.rb         | 2 +-
 spec/services/ci/create_pipeline_service_spec.rb | 1 -
 3 files changed, 2 insertions(+), 4 deletions(-)

diff --git a/app/services/ci/create_pipeline_service.rb b/app/services/ci/create_pipeline_service.rb
index f96c4741ba4..c5a5d9fc527 100644
--- a/app/services/ci/create_pipeline_service.rb
+++ b/app/services/ci/create_pipeline_service.rb
@@ -3,7 +3,7 @@ module Ci
     attr_reader :pipeline
 
     SEQUENCE = [Gitlab::Ci::Pipeline::Chain::Validate,
-                Gitlab::Ci::Pipeline::Chain::Skip]
+                Gitlab::Ci::Pipeline::Chain::Skip].freeze
 
     def execute(source, ignore_skip_ci: false, save_on_errors: true, trigger_request: nil, schedule: nil)
       @pipeline = Ci::Pipeline.new(
@@ -91,7 +91,6 @@ module Ci
         .created_or_pending
     end
 
-
     def before_sha
       params[:checkout_sha] || params[:before] || Gitlab::Git::BLANK_SHA
     end
diff --git a/lib/gitlab/ci/pipeline/chain/validate.rb b/lib/gitlab/ci/pipeline/chain/validate.rb
index 4c4c8b27be6..ec06b0f76c5 100644
--- a/lib/gitlab/ci/pipeline/chain/validate.rb
+++ b/lib/gitlab/ci/pipeline/chain/validate.rb
@@ -10,7 +10,7 @@ module Gitlab
           end
 
           def break?
-           @pipeline.errors.any? || @pipeline.persisted?
+            @pipeline.errors.any? || @pipeline.persisted?
           end
 
           def allowed_to_trigger_pipeline?
diff --git a/spec/services/ci/create_pipeline_service_spec.rb b/spec/services/ci/create_pipeline_service_spec.rb
index 5b959b9a142..6ee75b8fc23 100644
--- a/spec/services/ci/create_pipeline_service_spec.rb
+++ b/spec/services/ci/create_pipeline_service_spec.rb
@@ -481,5 +481,4 @@ describe Ci::CreatePipelineService do
       end
     end
   end
-
 end

From 61dc0b7dc7aeffc1b70bdde84864dccd90f7d655 Mon Sep 17 00:00:00 2001
From: Grzegorz Bizon <grzesiek.bizon@gmail.com>
Date: Tue, 26 Sep 2017 09:54:56 +0200
Subject: [PATCH 06/20] Extract pipeline persistence to a separate chain class

---
 app/services/ci/create_pipeline_service.rb | 59 +++++++++-------------
 lib/gitlab/ci/pipeline/chain/create.rb     | 29 +++++++++++
 2 files changed, 53 insertions(+), 35 deletions(-)
 create mode 100644 lib/gitlab/ci/pipeline/chain/create.rb

diff --git a/app/services/ci/create_pipeline_service.rb b/app/services/ci/create_pipeline_service.rb
index c5a5d9fc527..0386c1e8829 100644
--- a/app/services/ci/create_pipeline_service.rb
+++ b/app/services/ci/create_pipeline_service.rb
@@ -3,9 +3,10 @@ module Ci
     attr_reader :pipeline
 
     SEQUENCE = [Gitlab::Ci::Pipeline::Chain::Validate,
-                Gitlab::Ci::Pipeline::Chain::Skip].freeze
+                Gitlab::Ci::Pipeline::Chain::Skip,
+                Gitlab::Ci::Pipeline::Chain::Create].freeze
 
-    def execute(source, ignore_skip_ci: false, save_on_errors: true, trigger_request: nil, schedule: nil)
+    def execute(source, ignore_skip_ci: false, save_on_errors: true, trigger_request: nil, schedule: nil, &block)
       @pipeline = Ci::Pipeline.new(
         source: source,
         project: project,
@@ -19,43 +20,31 @@ module Ci
         protected: project.protected_for?(ref)
       )
 
-      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)
+      @pipeline.tap do |pipeline|
+        command = OpenStruct.new(ignore_skip_ci: ignore_skip_ci,
+                                 save_incompleted: save_on_errors,
+                                 trigger_request: trigger_request,
+                                 schedule: schedule,
+                                 seeds_block: block,
+                                 project: project,
+                                 current_user: current_user)
 
-      sequence = SEQUENCE.map { |chain| chain.new(pipeline, command) }
+        sequence = SEQUENCE.map { |chain| chain.new(pipeline, command) }
 
-      done = sequence.any? do |chain|
-        chain.perform!
-        chain.break?
-      end
-
-      update_merge_requests_head_pipeline if pipeline.persisted?
-
-      return pipeline if done
-
-      begin
-        Ci::Pipeline.transaction do
-          pipeline.save!
-
-          yield(pipeline) if block_given?
-
-          Ci::CreatePipelineStagesService
-            .new(project, current_user)
-            .execute(pipeline)
+        sequence_complete = sequence.none? do |chain|
+          chain.perform!
+          chain.break?
+        end
+
+        update_merge_requests_head_pipeline if pipeline.persisted?
+
+        if sequence_complete
+          cancel_pending_pipelines if project.auto_cancel_pending_pipelines?
+          pipeline_created_counter.increment(source: source)
+
+          pipeline.process!
         end
-      rescue ActiveRecord::RecordInvalid => e
-        return error("Failed to persist the pipeline: #{e}")
       end
-
-      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!)
     end
 
     private
diff --git a/lib/gitlab/ci/pipeline/chain/create.rb b/lib/gitlab/ci/pipeline/chain/create.rb
new file mode 100644
index 00000000000..3b067ca6ace
--- /dev/null
+++ b/lib/gitlab/ci/pipeline/chain/create.rb
@@ -0,0 +1,29 @@
+module Gitlab
+  module Ci
+    module Pipeline
+      module Chain
+        class Create < Chain::Base
+          def perform!
+            ::Ci::Pipeline.transaction do
+              pipeline.save!
+
+              if @command.seeds_block
+                @command.seeds_block.call(pipeline)
+              end
+
+              ::Ci::CreatePipelineStagesService
+                .new(project, current_user)
+                .execute(pipeline)
+            end
+          rescue ActiveRecord::RecordInvalid => e
+            pipeline.erros.add(:base, "Failed to persist the pipeline: #{e}")
+          end
+
+          def break?
+            !pipeline.persisted?
+          end
+        end
+      end
+    end
+  end
+end

From 3e60d62cc39b512fa1ac6b419815e88a6f6e1224 Mon Sep 17 00:00:00 2001
From: Grzegorz Bizon <grzesiek.bizon@gmail.com>
Date: Tue, 26 Sep 2017 11:13:40 +0200
Subject: [PATCH 07/20] Add class that handles pipeline creation sequence

---
 app/services/ci/create_pipeline_service.rb | 26 +++++++--------
 lib/gitlab/ci/pipeline/chain/sequence.rb   | 37 ++++++++++++++++++++++
 2 files changed, 49 insertions(+), 14 deletions(-)
 create mode 100644 lib/gitlab/ci/pipeline/chain/sequence.rb

diff --git a/app/services/ci/create_pipeline_service.rb b/app/services/ci/create_pipeline_service.rb
index 0386c1e8829..b22904fa4f1 100644
--- a/app/services/ci/create_pipeline_service.rb
+++ b/app/services/ci/create_pipeline_service.rb
@@ -20,25 +20,20 @@ module Ci
         protected: project.protected_for?(ref)
       )
 
-      @pipeline.tap do |pipeline|
-        command = OpenStruct.new(ignore_skip_ci: ignore_skip_ci,
-                                 save_incompleted: save_on_errors,
-                                 trigger_request: trigger_request,
-                                 schedule: schedule,
-                                 seeds_block: block,
-                                 project: project,
-                                 current_user: current_user)
+      command = OpenStruct.new(ignore_skip_ci: ignore_skip_ci,
+                               save_incompleted: save_on_errors,
+                               seeds_block: block,
+                               project: project,
+                               current_user: current_user)
 
-        sequence = SEQUENCE.map { |chain| chain.new(pipeline, command) }
 
-        sequence_complete = sequence.none? do |chain|
-          chain.perform!
-          chain.break?
-        end
+      sequence = Gitlab::Ci::Pipeline::Chain::Sequence
+        .new(pipeline, command, SEQUENCE)
 
+      sequence.build! do |pipeline, sequence|
         update_merge_requests_head_pipeline if pipeline.persisted?
 
-        if sequence_complete
+        if sequence.complete?
           cancel_pending_pipelines if project.auto_cancel_pending_pipelines?
           pipeline_created_counter.increment(source: source)
 
@@ -49,6 +44,9 @@ module Ci
 
     private
 
+    def process_pipeline_sequence
+    end
+
     def commit
       @commit ||= project.commit(origin_sha || origin_ref)
     end
diff --git a/lib/gitlab/ci/pipeline/chain/sequence.rb b/lib/gitlab/ci/pipeline/chain/sequence.rb
new file mode 100644
index 00000000000..e8d1ab36883
--- /dev/null
+++ b/lib/gitlab/ci/pipeline/chain/sequence.rb
@@ -0,0 +1,37 @@
+module Gitlab
+  module Ci
+    module Pipeline
+      module Chain
+        class Sequence
+          def initialize(pipeline, command, sequence)
+            @pipeline = pipeline
+            @completed = []
+
+            @sequence = sequence.map do |chain|
+              chain.new(pipeline, command)
+            end
+          end
+
+          def build!
+           @sequence.each do |step|
+              step.perform!
+
+              break if step.break?
+
+              @completed << true
+            end
+
+            @pipeline.tap do
+              yield @pipeline, self if block_given?
+            end
+          end
+
+          def complete?
+            @completed.size == @sequence.size &&
+              @completed.all?
+          end
+        end
+      end
+    end
+  end
+end

From 609fa45f0ed273414eac2798f76daf088e287b30 Mon Sep 17 00:00:00 2001
From: Grzegorz Bizon <grzesiek.bizon@gmail.com>
Date: Tue, 26 Sep 2017 11:50:47 +0200
Subject: [PATCH 08/20] Split pipeline chain builder validation class

---
 app/services/ci/create_pipeline_service.rb    |   4 +-
 lib/gitlab/ci/pipeline/chain/create.rb        |   4 +-
 lib/gitlab/ci/pipeline/chain/helpers.rb       |  25 +++++
 lib/gitlab/ci/pipeline/chain/validate.rb      | 101 ------------------
 .../ci/pipeline/chain/validate_abilities.rb   |  52 +++++++++
 .../ci/pipeline/chain/validate_config.rb      |  33 ++++++
 .../ci/pipeline/chain/validate_repository.rb  |  30 ++++++
 ...ate_spec.rb => validate_abilities_spec.rb} |   2 +-
 8 files changed, 147 insertions(+), 104 deletions(-)
 create mode 100644 lib/gitlab/ci/pipeline/chain/helpers.rb
 delete mode 100644 lib/gitlab/ci/pipeline/chain/validate.rb
 create mode 100644 lib/gitlab/ci/pipeline/chain/validate_abilities.rb
 create mode 100644 lib/gitlab/ci/pipeline/chain/validate_config.rb
 create mode 100644 lib/gitlab/ci/pipeline/chain/validate_repository.rb
 rename spec/lib/gitlab/ci/pipeline/chain/{validate_spec.rb => validate_abilities_spec.rb} (97%)

diff --git a/app/services/ci/create_pipeline_service.rb b/app/services/ci/create_pipeline_service.rb
index b22904fa4f1..ecf9be6600c 100644
--- a/app/services/ci/create_pipeline_service.rb
+++ b/app/services/ci/create_pipeline_service.rb
@@ -2,7 +2,9 @@ module Ci
   class CreatePipelineService < BaseService
     attr_reader :pipeline
 
-    SEQUENCE = [Gitlab::Ci::Pipeline::Chain::Validate,
+    SEQUENCE = [Gitlab::Ci::Pipeline::Chain::ValidateAbilities,
+                Gitlab::Ci::Pipeline::Chain::ValidateRepository,
+                Gitlab::Ci::Pipeline::Chain::ValidateConfig,
                 Gitlab::Ci::Pipeline::Chain::Skip,
                 Gitlab::Ci::Pipeline::Chain::Create].freeze
 
diff --git a/lib/gitlab/ci/pipeline/chain/create.rb b/lib/gitlab/ci/pipeline/chain/create.rb
index 3b067ca6ace..8bd658b3069 100644
--- a/lib/gitlab/ci/pipeline/chain/create.rb
+++ b/lib/gitlab/ci/pipeline/chain/create.rb
@@ -3,6 +3,8 @@ module Gitlab
     module Pipeline
       module Chain
         class Create < Chain::Base
+          include Chain::Helpers
+
           def perform!
             ::Ci::Pipeline.transaction do
               pipeline.save!
@@ -16,7 +18,7 @@ module Gitlab
                 .execute(pipeline)
             end
           rescue ActiveRecord::RecordInvalid => e
-            pipeline.erros.add(:base, "Failed to persist the pipeline: #{e}")
+            error("Failed to persist the pipeline: #{e}")
           end
 
           def break?
diff --git a/lib/gitlab/ci/pipeline/chain/helpers.rb b/lib/gitlab/ci/pipeline/chain/helpers.rb
new file mode 100644
index 00000000000..02d81286f21
--- /dev/null
+++ b/lib/gitlab/ci/pipeline/chain/helpers.rb
@@ -0,0 +1,25 @@
+module Gitlab
+  module Ci
+    module Pipeline
+      module Chain
+        module Helpers
+          def branch_exists?
+            return @is_branch if defined?(@is_branch)
+
+            @is_branch = project.repository.branch_exists?(pipeline.ref)
+          end
+
+          def tag_exists?
+            return @is_tag if defined?(@is_tag)
+
+            @is_tag = project.repository.tag_exists?(pipeline.ref)
+          end
+
+          def error(message)
+            pipeline.errors.add(:base, message)
+          end
+        end
+      end
+    end
+  end
+end
diff --git a/lib/gitlab/ci/pipeline/chain/validate.rb b/lib/gitlab/ci/pipeline/chain/validate.rb
deleted file mode 100644
index ec06b0f76c5..00000000000
--- a/lib/gitlab/ci/pipeline/chain/validate.rb
+++ /dev/null
@@ -1,101 +0,0 @@
-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
-
-          def branch?
-            return @is_branch if defined?(@is_branch)
-
-            @is_branch = project.repository.branch_exists?(pipeline.ref)
-          end
-
-          def tag?
-            return @is_tag if defined?(@is_tag)
-
-            @is_tag = project.repository.tag_exists?(pipeline.ref)
-          end
-
-          def error(message)
-            pipeline.errors.add(:base, message)
-          end
-        end
-      end
-    end
-  end
-end
diff --git a/lib/gitlab/ci/pipeline/chain/validate_abilities.rb b/lib/gitlab/ci/pipeline/chain/validate_abilities.rb
new file mode 100644
index 00000000000..28a9c0ba999
--- /dev/null
+++ b/lib/gitlab/ci/pipeline/chain/validate_abilities.rb
@@ -0,0 +1,52 @@
+module Gitlab
+  module Ci
+    module Pipeline
+      module Chain
+        class ValidateAbilities < Chain::Base
+          include Gitlab::Allowable
+          include Chain::Helpers
+
+          def perform!
+            unless project.builds_enabled?
+              return error('Pipelines are 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 break?
+            @pipeline.errors.any?
+          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_exists?
+              access.can_update_branch?(@pipeline.ref)
+            elsif tag_exists?
+              access.can_create_tag?(@pipeline.ref)
+            else
+              true # Allow it for now and we'll reject when we check ref existence
+            end
+          end
+        end
+      end
+    end
+  end
+end
diff --git a/lib/gitlab/ci/pipeline/chain/validate_config.rb b/lib/gitlab/ci/pipeline/chain/validate_config.rb
new file mode 100644
index 00000000000..0dba8731438
--- /dev/null
+++ b/lib/gitlab/ci/pipeline/chain/validate_config.rb
@@ -0,0 +1,33 @@
+module Gitlab
+  module Ci
+    module Pipeline
+      module Chain
+        class ValidateConfig < Chain::Base
+          include Chain::Helpers
+
+          def perform!
+            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
+
+          def break?
+            @pipeline.errors.any? || @pipeline.persisted?
+          end
+        end
+      end
+    end
+  end
+end
diff --git a/lib/gitlab/ci/pipeline/chain/validate_repository.rb b/lib/gitlab/ci/pipeline/chain/validate_repository.rb
new file mode 100644
index 00000000000..4d1b88a7065
--- /dev/null
+++ b/lib/gitlab/ci/pipeline/chain/validate_repository.rb
@@ -0,0 +1,30 @@
+module Gitlab
+  module Ci
+    module Pipeline
+      module Chain
+        class ValidateRepository < Chain::Base
+          include Chain::Helpers
+
+          def perform!
+            unless branch_exists? || tag_exists?
+              return error('Reference not found')
+            end
+
+            ## TODO, we check commit in the service, that is why
+            # there is no repository access here.
+            #
+            # Should we validate repository before building a pipeline?
+            #
+            unless pipeline.sha
+              return error('Commit not found')
+            end
+          end
+
+          def break?
+            @pipeline.errors.any?
+          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_abilities_spec.rb
similarity index 97%
rename from spec/lib/gitlab/ci/pipeline/chain/validate_spec.rb
rename to spec/lib/gitlab/ci/pipeline/chain/validate_abilities_spec.rb
index 117f55fd8a7..1613df395d1 100644
--- a/spec/lib/gitlab/ci/pipeline/chain/validate_spec.rb
+++ b/spec/lib/gitlab/ci/pipeline/chain/validate_abilities_spec.rb
@@ -1,6 +1,6 @@
 require 'spec_helper'
 
-describe Gitlab::Ci::Pipeline::Chain::Validate do
+describe Gitlab::Ci::Pipeline::Chain::ValidateAbilities do
   describe '#allowed_to_create?' do
     let(:user) { create(:user) }
     let(:project) { create(:project, :repository) }

From fa3fb23fb18ddea694ea54013059178fe7892c91 Mon Sep 17 00:00:00 2001
From: Grzegorz Bizon <grzesiek.bizon@gmail.com>
Date: Tue, 26 Sep 2017 11:53:50 +0200
Subject: [PATCH 09/20] Move pipeline builder validation chain to a module

---
 app/services/ci/create_pipeline_service.rb    |  6 +--
 .../ci/pipeline/chain/validate/abilities.rb   | 54 +++++++++++++++++++
 .../ci/pipeline/chain/validate/config.rb      | 35 ++++++++++++
 .../ci/pipeline/chain/validate/repository.rb  | 32 +++++++++++
 .../ci/pipeline/chain/validate_abilities.rb   | 52 ------------------
 .../ci/pipeline/chain/validate_config.rb      | 33 ------------
 .../ci/pipeline/chain/validate_repository.rb  | 30 -----------
 .../abilities_spec.rb}                        |  2 +-
 8 files changed, 125 insertions(+), 119 deletions(-)
 create mode 100644 lib/gitlab/ci/pipeline/chain/validate/abilities.rb
 create mode 100644 lib/gitlab/ci/pipeline/chain/validate/config.rb
 create mode 100644 lib/gitlab/ci/pipeline/chain/validate/repository.rb
 delete mode 100644 lib/gitlab/ci/pipeline/chain/validate_abilities.rb
 delete mode 100644 lib/gitlab/ci/pipeline/chain/validate_config.rb
 delete mode 100644 lib/gitlab/ci/pipeline/chain/validate_repository.rb
 rename spec/lib/gitlab/ci/pipeline/chain/{validate_abilities_spec.rb => validate/abilities_spec.rb} (97%)

diff --git a/app/services/ci/create_pipeline_service.rb b/app/services/ci/create_pipeline_service.rb
index ecf9be6600c..0bf38776afc 100644
--- a/app/services/ci/create_pipeline_service.rb
+++ b/app/services/ci/create_pipeline_service.rb
@@ -2,9 +2,9 @@ module Ci
   class CreatePipelineService < BaseService
     attr_reader :pipeline
 
-    SEQUENCE = [Gitlab::Ci::Pipeline::Chain::ValidateAbilities,
-                Gitlab::Ci::Pipeline::Chain::ValidateRepository,
-                Gitlab::Ci::Pipeline::Chain::ValidateConfig,
+    SEQUENCE = [Gitlab::Ci::Pipeline::Chain::Validate::Abilities,
+                Gitlab::Ci::Pipeline::Chain::Validate::Repository,
+                Gitlab::Ci::Pipeline::Chain::Validate::Config,
                 Gitlab::Ci::Pipeline::Chain::Skip,
                 Gitlab::Ci::Pipeline::Chain::Create].freeze
 
diff --git a/lib/gitlab/ci/pipeline/chain/validate/abilities.rb b/lib/gitlab/ci/pipeline/chain/validate/abilities.rb
new file mode 100644
index 00000000000..4913a604079
--- /dev/null
+++ b/lib/gitlab/ci/pipeline/chain/validate/abilities.rb
@@ -0,0 +1,54 @@
+module Gitlab
+  module Ci
+    module Pipeline
+      module Chain
+        module Validate
+          class Abilities < Chain::Base
+            include Gitlab::Allowable
+            include Chain::Helpers
+
+            def perform!
+              unless project.builds_enabled?
+                return error('Pipelines are 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 break?
+              @pipeline.errors.any?
+            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_exists?
+                access.can_update_branch?(@pipeline.ref)
+              elsif tag_exists?
+                access.can_create_tag?(@pipeline.ref)
+              else
+                true # Allow it for now and we'll reject when we check ref existence
+              end
+            end
+          end
+        end
+      end
+    end
+  end
+end
diff --git a/lib/gitlab/ci/pipeline/chain/validate/config.rb b/lib/gitlab/ci/pipeline/chain/validate/config.rb
new file mode 100644
index 00000000000..489bcd79655
--- /dev/null
+++ b/lib/gitlab/ci/pipeline/chain/validate/config.rb
@@ -0,0 +1,35 @@
+module Gitlab
+  module Ci
+    module Pipeline
+      module Chain
+        module Validate
+          class Config < Chain::Base
+            include Chain::Helpers
+
+            def perform!
+              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
+
+            def break?
+              @pipeline.errors.any? || @pipeline.persisted?
+            end
+          end
+        end
+      end
+    end
+  end
+end
diff --git a/lib/gitlab/ci/pipeline/chain/validate/repository.rb b/lib/gitlab/ci/pipeline/chain/validate/repository.rb
new file mode 100644
index 00000000000..9d328c9cedb
--- /dev/null
+++ b/lib/gitlab/ci/pipeline/chain/validate/repository.rb
@@ -0,0 +1,32 @@
+module Gitlab
+  module Ci
+    module Pipeline
+      module Chain
+        module Validate
+          class Repository < Chain::Base
+            include Chain::Helpers
+
+            def perform!
+              unless branch_exists? || tag_exists?
+                return error('Reference not found')
+              end
+
+              ## TODO, we check commit in the service, that is why
+              # there is no repository access here.
+              #
+              # Should we validate repository before building a pipeline?
+              #
+              unless pipeline.sha
+                return error('Commit not found')
+              end
+            end
+
+            def break?
+              @pipeline.errors.any?
+            end
+          end
+        end
+      end
+    end
+  end
+end
diff --git a/lib/gitlab/ci/pipeline/chain/validate_abilities.rb b/lib/gitlab/ci/pipeline/chain/validate_abilities.rb
deleted file mode 100644
index 28a9c0ba999..00000000000
--- a/lib/gitlab/ci/pipeline/chain/validate_abilities.rb
+++ /dev/null
@@ -1,52 +0,0 @@
-module Gitlab
-  module Ci
-    module Pipeline
-      module Chain
-        class ValidateAbilities < Chain::Base
-          include Gitlab::Allowable
-          include Chain::Helpers
-
-          def perform!
-            unless project.builds_enabled?
-              return error('Pipelines are 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 break?
-            @pipeline.errors.any?
-          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_exists?
-              access.can_update_branch?(@pipeline.ref)
-            elsif tag_exists?
-              access.can_create_tag?(@pipeline.ref)
-            else
-              true # Allow it for now and we'll reject when we check ref existence
-            end
-          end
-        end
-      end
-    end
-  end
-end
diff --git a/lib/gitlab/ci/pipeline/chain/validate_config.rb b/lib/gitlab/ci/pipeline/chain/validate_config.rb
deleted file mode 100644
index 0dba8731438..00000000000
--- a/lib/gitlab/ci/pipeline/chain/validate_config.rb
+++ /dev/null
@@ -1,33 +0,0 @@
-module Gitlab
-  module Ci
-    module Pipeline
-      module Chain
-        class ValidateConfig < Chain::Base
-          include Chain::Helpers
-
-          def perform!
-            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
-
-          def break?
-            @pipeline.errors.any? || @pipeline.persisted?
-          end
-        end
-      end
-    end
-  end
-end
diff --git a/lib/gitlab/ci/pipeline/chain/validate_repository.rb b/lib/gitlab/ci/pipeline/chain/validate_repository.rb
deleted file mode 100644
index 4d1b88a7065..00000000000
--- a/lib/gitlab/ci/pipeline/chain/validate_repository.rb
+++ /dev/null
@@ -1,30 +0,0 @@
-module Gitlab
-  module Ci
-    module Pipeline
-      module Chain
-        class ValidateRepository < Chain::Base
-          include Chain::Helpers
-
-          def perform!
-            unless branch_exists? || tag_exists?
-              return error('Reference not found')
-            end
-
-            ## TODO, we check commit in the service, that is why
-            # there is no repository access here.
-            #
-            # Should we validate repository before building a pipeline?
-            #
-            unless pipeline.sha
-              return error('Commit not found')
-            end
-          end
-
-          def break?
-            @pipeline.errors.any?
-          end
-        end
-      end
-    end
-  end
-end
diff --git a/spec/lib/gitlab/ci/pipeline/chain/validate_abilities_spec.rb b/spec/lib/gitlab/ci/pipeline/chain/validate/abilities_spec.rb
similarity index 97%
rename from spec/lib/gitlab/ci/pipeline/chain/validate_abilities_spec.rb
rename to spec/lib/gitlab/ci/pipeline/chain/validate/abilities_spec.rb
index 1613df395d1..9e217102886 100644
--- a/spec/lib/gitlab/ci/pipeline/chain/validate_abilities_spec.rb
+++ b/spec/lib/gitlab/ci/pipeline/chain/validate/abilities_spec.rb
@@ -1,6 +1,6 @@
 require 'spec_helper'
 
-describe Gitlab::Ci::Pipeline::Chain::ValidateAbilities do
+describe Gitlab::Ci::Pipeline::Chain::Validate::Abilities do
   describe '#allowed_to_create?' do
     let(:user) { create(:user) }
     let(:project) { create(:project, :repository) }

From da15b38850d8d345a17862754285c34a2fbf08b6 Mon Sep 17 00:00:00 2001
From: Grzegorz Bizon <grzesiek.bizon@gmail.com>
Date: Tue, 26 Sep 2017 13:00:29 +0200
Subject: [PATCH 10/20] Add specs for pipeline builder abilities validator

---
 app/services/ci/create_pipeline_service.rb    |  3 --
 .../pipeline/chain/validate/abilities_spec.rb | 49 +++++++++++++------
 2 files changed, 34 insertions(+), 18 deletions(-)

diff --git a/app/services/ci/create_pipeline_service.rb b/app/services/ci/create_pipeline_service.rb
index 0bf38776afc..af1784fe1dc 100644
--- a/app/services/ci/create_pipeline_service.rb
+++ b/app/services/ci/create_pipeline_service.rb
@@ -46,9 +46,6 @@ module Ci
 
     private
 
-    def process_pipeline_sequence
-    end
-
     def commit
       @commit ||= project.commit(origin_sha || origin_ref)
     end
diff --git a/spec/lib/gitlab/ci/pipeline/chain/validate/abilities_spec.rb b/spec/lib/gitlab/ci/pipeline/chain/validate/abilities_spec.rb
index 9e217102886..e6decde475a 100644
--- a/spec/lib/gitlab/ci/pipeline/chain/validate/abilities_spec.rb
+++ b/spec/lib/gitlab/ci/pipeline/chain/validate/abilities_spec.rb
@@ -1,22 +1,41 @@
 require 'spec_helper'
 
 describe Gitlab::Ci::Pipeline::Chain::Validate::Abilities do
+  set(:project) { create(:project, :repository) }
+  set(:user) { create(:user) }
+
+  let(:pipeline) do
+    build_stubbed(:ci_pipeline, ref: ref, project: project)
+  end
+
+  let(:command) do
+    double('command', project: project, current_user: user)
+  end
+
+  let(:step) { described_class.new(pipeline, command) }
+
+  let(:ref) { 'master' }
+
+  context 'when users has no ability to run a pipeline' do
+    before do
+      step.perform!
+    end
+
+    it 'adds an error about insufficient permissions' do
+      expect(pipeline.errors.to_a)
+        .to include /Insufficient permissions/
+    end
+
+    it 'breaks the pipeline builder chain' do
+      expect(step.break?).to eq true
+    end
+  end
+
+  context 'when user has ability to create a pipeline' do
+  end
+
   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
+    subject { step.allowed_to_create? }
 
     context 'when user is a developer' do
       before do

From 1a8777c8d775b951384fbe4b8cf050b26b247d00 Mon Sep 17 00:00:00 2001
From: Grzegorz Bizon <grzesiek.bizon@gmail.com>
Date: Tue, 26 Sep 2017 13:02:57 +0200
Subject: [PATCH 11/20] Fix coding style offenses in pipeline chain classes

---
 app/services/ci/create_pipeline_service.rb | 1 -
 lib/gitlab/ci/pipeline/chain/create.rb     | 4 +---
 lib/gitlab/ci/pipeline/chain/sequence.rb   | 2 +-
 3 files changed, 2 insertions(+), 5 deletions(-)

diff --git a/app/services/ci/create_pipeline_service.rb b/app/services/ci/create_pipeline_service.rb
index af1784fe1dc..df5b32d97ca 100644
--- a/app/services/ci/create_pipeline_service.rb
+++ b/app/services/ci/create_pipeline_service.rb
@@ -28,7 +28,6 @@ module Ci
                                project: project,
                                current_user: current_user)
 
-
       sequence = Gitlab::Ci::Pipeline::Chain::Sequence
         .new(pipeline, command, SEQUENCE)
 
diff --git a/lib/gitlab/ci/pipeline/chain/create.rb b/lib/gitlab/ci/pipeline/chain/create.rb
index 8bd658b3069..d5e17a123df 100644
--- a/lib/gitlab/ci/pipeline/chain/create.rb
+++ b/lib/gitlab/ci/pipeline/chain/create.rb
@@ -9,9 +9,7 @@ module Gitlab
             ::Ci::Pipeline.transaction do
               pipeline.save!
 
-              if @command.seeds_block
-                @command.seeds_block.call(pipeline)
-              end
+              @command.seeds_block&.call(pipeline)
 
               ::Ci::CreatePipelineStagesService
                 .new(project, current_user)
diff --git a/lib/gitlab/ci/pipeline/chain/sequence.rb b/lib/gitlab/ci/pipeline/chain/sequence.rb
index e8d1ab36883..c80d583939c 100644
--- a/lib/gitlab/ci/pipeline/chain/sequence.rb
+++ b/lib/gitlab/ci/pipeline/chain/sequence.rb
@@ -13,7 +13,7 @@ module Gitlab
           end
 
           def build!
-           @sequence.each do |step|
+            @sequence.each do |step|
               step.perform!
 
               break if step.break?

From 2432d5bd9ec49ce5a6287ee57ef701c740dff7f9 Mon Sep 17 00:00:00 2001
From: Grzegorz Bizon <grzesiek.bizon@gmail.com>
Date: Tue, 26 Sep 2017 13:22:28 +0200
Subject: [PATCH 12/20] Add specs for builder chain step that skipps pipelines

---
 lib/gitlab/ci/pipeline/chain/skip.rb          |  2 +-
 .../lib/gitlab/ci/pipeline/chain/skip_spec.rb | 85 +++++++++++++++++++
 2 files changed, 86 insertions(+), 1 deletion(-)
 create mode 100644 spec/lib/gitlab/ci/pipeline/chain/skip_spec.rb

diff --git a/lib/gitlab/ci/pipeline/chain/skip.rb b/lib/gitlab/ci/pipeline/chain/skip.rb
index 3f86275ae15..9a72de87bab 100644
--- a/lib/gitlab/ci/pipeline/chain/skip.rb
+++ b/lib/gitlab/ci/pipeline/chain/skip.rb
@@ -24,7 +24,7 @@ module Gitlab
           def commit_message_skips_ci?
             return false unless @pipeline.git_commit_message
 
-            @skipped ||= @pipeline.git_commit_message =~ SKIP_PATTERN
+            @skipped ||= !!(@pipeline.git_commit_message =~ SKIP_PATTERN)
           end
         end
       end
diff --git a/spec/lib/gitlab/ci/pipeline/chain/skip_spec.rb b/spec/lib/gitlab/ci/pipeline/chain/skip_spec.rb
new file mode 100644
index 00000000000..32bd5de829b
--- /dev/null
+++ b/spec/lib/gitlab/ci/pipeline/chain/skip_spec.rb
@@ -0,0 +1,85 @@
+require 'spec_helper'
+
+describe Gitlab::Ci::Pipeline::Chain::Skip do
+  set(:project) { create(:project) }
+  set(:user) { create(:user) }
+  set(:pipeline) { create(:ci_pipeline, project: project) }
+
+  let(:command) do
+    double('command', project: project,
+                      current_user: user,
+                      ignore_skip_ci: false,
+                      save_incompleted: true)
+  end
+
+  let(:step) { described_class.new(pipeline, command) }
+
+  context 'when pipeline has been skipped by a user' do
+    before do
+      allow(pipeline).to receive(:git_commit_message)
+        .and_return('commit message [ci skip]')
+
+      step.perform!
+    end
+
+    it 'should break the chain' do
+      expect(step.break?).to be true
+    end
+
+    it 'skips the pipeline' do
+      expect(pipeline.reload).to be_skipped
+    end
+  end
+
+  context 'when pipeline has not been skipped' do
+    before do
+      step.perform!
+    end
+
+    it 'should not break the chain' do
+      expect(step.break?).to be false
+    end
+
+    it 'should not skip a pipeline chain' do
+      expect(pipeline.reload).not_to be_skipped
+    end
+  end
+
+  context 'when [ci skip] should be ignored' do
+    let(:command) do
+      double('command', project: project,
+                        current_user: user,
+                        ignore_skip_ci: true)
+    end
+
+    it 'does not break the chain' do
+      step.perform!
+
+      expect(step.break?).to be false
+    end
+  end
+
+  context 'when pipeline should be skipped but not persisted' do
+    let(:command) do
+      double('command', project: project,
+                        current_user: user,
+                        ignore_skip_ci: false,
+                        save_incompleted: false)
+    end
+
+    before do
+      allow(pipeline).to receive(:git_commit_message)
+        .and_return('commit message [ci skip]')
+
+      step.perform!
+    end
+
+    it 'breaks the chain' do
+      expect(step.break?).to be true
+    end
+
+    it 'does not skip pipeline' do
+      expect(pipeline.reload).not_to be_skipped
+    end
+  end
+end

From 652ecff91be036382ecac6059093bfae80522c81 Mon Sep 17 00:00:00 2001
From: Grzegorz Bizon <grzesiek.bizon@gmail.com>
Date: Tue, 26 Sep 2017 13:34:53 +0200
Subject: [PATCH 13/20] Add specs for builder chain that persist a pipeline

---
 .../gitlab/ci/pipeline/chain/create_spec.rb   | 66 +++++++++++++++++++
 1 file changed, 66 insertions(+)
 create mode 100644 spec/lib/gitlab/ci/pipeline/chain/create_spec.rb

diff --git a/spec/lib/gitlab/ci/pipeline/chain/create_spec.rb b/spec/lib/gitlab/ci/pipeline/chain/create_spec.rb
new file mode 100644
index 00000000000..f54e2326b06
--- /dev/null
+++ b/spec/lib/gitlab/ci/pipeline/chain/create_spec.rb
@@ -0,0 +1,66 @@
+require 'spec_helper'
+
+describe Gitlab::Ci::Pipeline::Chain::Create do
+  set(:project) { create(:project) }
+  set(:user) { create(:user) }
+
+  let(:pipeline) do
+    build(:ci_pipeline_with_one_job, project: project,
+                                     ref: 'master')
+  end
+
+  let(:command) do
+    double('command', project: project,
+                      current_user: user,
+                      seeds_block: nil)
+  end
+
+  let(:step) { described_class.new(pipeline, command) }
+
+  before do
+    step.perform!
+  end
+
+  context 'when pipeline is ready to be saved' do
+    it 'saves a pipeline' do
+      expect(pipeline).to be_persisted
+    end
+
+    it 'does not break the chain' do
+      expect(step.break?).to be false
+    end
+
+    it 'creates stages' do
+      expect(pipeline.reload.stages).to be_one
+    end
+  end
+
+  context 'when pipeline has validation errors' do
+    let(:pipeline) do
+      build(:ci_pipeline, project: project, ref: nil)
+    end
+
+    it 'breaks the chain' do
+      expect(step.break?).to be true
+    end
+
+    it 'appends validation error' do
+      expect(pipeline.errors.to_a)
+        .to include /Failed to persist the pipeline/
+    end
+  end
+
+  context 'when there is a seed block present' do
+    let(:seeds) { spy('pipeline seeds') }
+
+    let(:command) do
+      double('command', project: project,
+                        current_user: user,
+                        seeds_block: seeds)
+    end
+
+    it 'executes the block' do
+      expect(seeds).to have_received(:call).with(pipeline)
+    end
+  end
+end

From 39f05fd85ed53e2045f615a55baa3158f5eb20cc Mon Sep 17 00:00:00 2001
From: Grzegorz Bizon <grzesiek.bizon@gmail.com>
Date: Tue, 26 Sep 2017 13:40:37 +0200
Subject: [PATCH 14/20] Add specs for pipeline builder repository validator

---
 .../chain/validate/repository_spec.rb         | 60 +++++++++++++++++++
 1 file changed, 60 insertions(+)
 create mode 100644 spec/lib/gitlab/ci/pipeline/chain/validate/repository_spec.rb

diff --git a/spec/lib/gitlab/ci/pipeline/chain/validate/repository_spec.rb b/spec/lib/gitlab/ci/pipeline/chain/validate/repository_spec.rb
new file mode 100644
index 00000000000..bb356efe9ad
--- /dev/null
+++ b/spec/lib/gitlab/ci/pipeline/chain/validate/repository_spec.rb
@@ -0,0 +1,60 @@
+require 'spec_helper'
+
+describe Gitlab::Ci::Pipeline::Chain::Validate::Repository do
+  set(:project) { create(:project, :repository) }
+  set(:user) { create(:user) }
+
+  let(:command) do
+    double('command', project: project, current_user: user)
+  end
+
+  let!(:step) { described_class.new(pipeline, command) }
+
+  before do
+    step.perform!
+  end
+
+  context 'when pipeline ref and sha exists' do
+    let(:pipeline) do
+      build_stubbed(:ci_pipeline, ref: 'master', sha: '123', project: project)
+    end
+
+    it 'does not break the chain' do
+      expect(step.break?).to be false
+    end
+
+    it 'does not append pipeline errors' do
+      expect(pipeline.errors).to be_empty
+    end
+  end
+
+  context 'when pipeline ref does not exist' do
+    let(:pipeline) do
+      build_stubbed(:ci_pipeline, ref: 'something', project: project)
+    end
+
+    it 'breaks the chain' do
+      expect(step.break?).to be true
+    end
+
+    it 'adds an error about missing ref' do
+      expect(pipeline.errors.to_a)
+        .to include 'Reference not found'
+    end
+  end
+
+  context 'when pipeline does not have SHA set' do
+    let(:pipeline) do
+      build_stubbed(:ci_pipeline, ref: 'master', sha: nil, project: project)
+    end
+
+    it 'breaks the chain' do
+      expect(step.break?).to be true
+    end
+
+    it 'adds an error about missing SHA' do
+      expect(pipeline.errors.to_a)
+        .to include 'Commit not found'
+    end
+  end
+end

From 6a9cfdde02b33b0a53d9f2b0f1bb3ee0e092a46d Mon Sep 17 00:00:00 2001
From: Grzegorz Bizon <grzesiek.bizon@gmail.com>
Date: Tue, 26 Sep 2017 14:04:28 +0200
Subject: [PATCH 15/20] Add specs for pipeline builder that validates config

---
 .../ci/pipeline/chain/validate/config_spec.rb | 127 ++++++++++++++++++
 1 file changed, 127 insertions(+)
 create mode 100644 spec/lib/gitlab/ci/pipeline/chain/validate/config_spec.rb

diff --git a/spec/lib/gitlab/ci/pipeline/chain/validate/config_spec.rb b/spec/lib/gitlab/ci/pipeline/chain/validate/config_spec.rb
new file mode 100644
index 00000000000..d5e885b7409
--- /dev/null
+++ b/spec/lib/gitlab/ci/pipeline/chain/validate/config_spec.rb
@@ -0,0 +1,127 @@
+require 'spec_helper'
+
+describe Gitlab::Ci::Pipeline::Chain::Validate::Config do
+  set(:project) { create(:project) }
+  set(:user) { create(:user) }
+
+  let(:command) do
+    double('command', project: project,
+                      current_user: user,
+                      save_incompleted: true)
+  end
+
+  let!(:step) { described_class.new(pipeline, command) }
+
+  before do
+    step.perform!
+  end
+
+  context 'when pipeline has no YAML configuration' do
+    let(:pipeline) do
+      build_stubbed(:ci_pipeline, project: project)
+    end
+
+    it 'appends errors about missing configuration' do
+      expect(pipeline.errors.to_a)
+        .to include 'Missing .gitlab-ci.yml file'
+    end
+
+    it 'breaks the chain' do
+      expect(step.break?).to be true
+    end
+  end
+
+  context 'when YAML configuration contains errors' do
+    let(:pipeline) do
+      build(:ci_pipeline, project: project, config: 'invalid YAML')
+    end
+
+    it 'appends errors about YAML errors' do
+      expect(pipeline.errors.to_a)
+        .to include 'Invalid configuration format'
+    end
+
+    it 'breaks the chain' do
+      expect(step.break?).to be true
+    end
+
+    context 'when saving incomplete pipeline is allowed' do
+      let(:command) do
+        double('command', project: project,
+                          current_user: user,
+                          save_incompleted: true)
+      end
+
+      it 'fails the pipeline' do
+        expect(pipeline.reload).to be_failed
+      end
+    end
+
+    context 'when saving incomplete pipeline is not allowed' do
+      let(:command) do
+        double('command', project: project,
+                          current_user: user,
+                          save_incompleted: false)
+      end
+
+      it 'does not drop pipeline' do
+        expect(pipeline).not_to be_failed
+        expect(pipeline).not_to be_persisted
+      end
+    end
+  end
+
+  context 'when pipeline has no stages / jobs' do
+    let(:config) do
+      { rspec: {
+          script: 'ls',
+          only: ['something']
+        }
+      }
+    end
+
+    let(:pipeline) do
+      build(:ci_pipeline, project: project, config: config)
+    end
+
+    it 'appends an error about missing stages' do
+      expect(pipeline.errors.to_a)
+        .to include 'No stages / jobs for this pipeline.'
+    end
+
+    it 'breaks the chain' do
+      expect(step.break?).to be true
+    end
+  end
+
+  context 'when pipeline contains configuration validation errors' do
+    let(:config) { { rspec: { } } }
+
+    let(:pipeline) do
+      build(:ci_pipeline, project: project, config: config)
+    end
+
+    it 'appends configuration validation errors to pipeline errors' do
+      expect(pipeline.errors.to_a)
+        .to include "jobs:rspec config can't be blank"
+    end
+
+    it 'breaks the chain' do
+      expect(step.break?).to be true
+    end
+  end
+
+  context 'when pipeline is correct and complete' do
+    let(:pipeline) do
+      build(:ci_pipeline_with_one_job, project: project)
+    end
+
+    it 'does not invalidate the pipeline' do
+      expect(pipeline).to be_valid
+    end
+
+    it 'does not break the chain' do
+      expect(step.break?).to be false
+    end
+  end
+end

From 53cad500433b69c77628ce020587fd5c9227690c Mon Sep 17 00:00:00 2001
From: Grzegorz Bizon <grzesiek.bizon@gmail.com>
Date: Tue, 26 Sep 2017 14:13:08 +0200
Subject: [PATCH 16/20] Add missing tests for pipeline chain access validator

---
 .../ci/pipeline/chain/validate/abilities_spec.rb    | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/spec/lib/gitlab/ci/pipeline/chain/validate/abilities_spec.rb b/spec/lib/gitlab/ci/pipeline/chain/validate/abilities_spec.rb
index e6decde475a..0bbdd23f4d6 100644
--- a/spec/lib/gitlab/ci/pipeline/chain/validate/abilities_spec.rb
+++ b/spec/lib/gitlab/ci/pipeline/chain/validate/abilities_spec.rb
@@ -32,6 +32,19 @@ describe Gitlab::Ci::Pipeline::Chain::Validate::Abilities do
   end
 
   context 'when user has ability to create a pipeline' do
+    before do
+      project.add_developer(user)
+
+      step.perform!
+    end
+
+    it 'does not invalidate the pipeline' do
+      expect(pipeline).to be_valid
+    end
+
+    it 'does not break the chain' do
+      expect(step.break?).to eq false
+    end
   end
 
   describe '#allowed_to_create?' do

From f6bd832f3fb5a833422c7a08deda844f6e78ab93 Mon Sep 17 00:00:00 2001
From: Grzegorz Bizon <grzesiek.bizon@gmail.com>
Date: Tue, 26 Sep 2017 14:15:05 +0200
Subject: [PATCH 17/20] Fix some code style offenses in pipeline chain classes

---
 spec/lib/gitlab/ci/pipeline/chain/validate/config_spec.rb | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/spec/lib/gitlab/ci/pipeline/chain/validate/config_spec.rb b/spec/lib/gitlab/ci/pipeline/chain/validate/config_spec.rb
index d5e885b7409..3740df88f42 100644
--- a/spec/lib/gitlab/ci/pipeline/chain/validate/config_spec.rb
+++ b/spec/lib/gitlab/ci/pipeline/chain/validate/config_spec.rb
@@ -76,8 +76,7 @@ describe Gitlab::Ci::Pipeline::Chain::Validate::Config do
       { rspec: {
           script: 'ls',
           only: ['something']
-        }
-      }
+      } }
     end
 
     let(:pipeline) do
@@ -95,7 +94,7 @@ describe Gitlab::Ci::Pipeline::Chain::Validate::Config do
   end
 
   context 'when pipeline contains configuration validation errors' do
-    let(:config) { { rspec: { } } }
+    let(:config) { { rspec: {} } }
 
     let(:pipeline) do
       build(:ci_pipeline, project: project, config: config)

From 835bdcb88e3eff70b33d27b6ca42e7d1970eac35 Mon Sep 17 00:00:00 2001
From: Grzegorz Bizon <grzesiek.bizon@gmail.com>
Date: Tue, 26 Sep 2017 14:24:40 +0200
Subject: [PATCH 18/20] Add specs for pipeline chain builder sequence class

---
 .../gitlab/ci/pipeline/chain/sequence_spec.rb | 55 +++++++++++++++++++
 1 file changed, 55 insertions(+)
 create mode 100644 spec/lib/gitlab/ci/pipeline/chain/sequence_spec.rb

diff --git a/spec/lib/gitlab/ci/pipeline/chain/sequence_spec.rb b/spec/lib/gitlab/ci/pipeline/chain/sequence_spec.rb
new file mode 100644
index 00000000000..e165e0fac2a
--- /dev/null
+++ b/spec/lib/gitlab/ci/pipeline/chain/sequence_spec.rb
@@ -0,0 +1,55 @@
+require 'spec_helper'
+
+describe Gitlab::Ci::Pipeline::Chain::Sequence do
+  set(:project) { create(:project) }
+  set(:user) { create(:user) }
+
+  let(:pipeline) { build_stubbed(:ci_pipeline) }
+  let(:command) { double('command' ) }
+  let(:first_step) { spy('first step') }
+  let(:second_step) { spy('second step') }
+  let(:sequence) { [first_step, second_step] }
+
+  subject do
+    described_class.new(pipeline, command, sequence)
+  end
+
+  context 'when one of steps breaks the chain' do
+    before do
+      allow(first_step).to receive(:break?).and_return(true)
+    end
+
+    it 'does not process the second step' do
+      subject.build! do |pipeline, sequence|
+        expect(sequence).not_to be_complete
+      end
+
+      expect(second_step).not_to have_received(:perform!)
+    end
+
+    it 'returns a pipeline object' do
+      expect(subject.build!).to eq pipeline
+    end
+  end
+
+  context 'when all chains are executed correctly' do
+    before do
+      sequence.each do |step|
+        allow(step).to receive(:break?).and_return(false)
+      end
+    end
+
+    it 'iterates through entire sequence' do
+      subject.build! do |pipeline, sequence|
+        expect(sequence).to be_complete
+      end
+
+      expect(first_step).to have_received(:perform!)
+      expect(second_step).to have_received(:perform!)
+    end
+
+    it 'returns a pipeline object' do
+      expect(subject.build!).to eq pipeline
+    end
+  end
+end

From 057a8b709346a89e2ccdfe6e9b352ce5f93e71c7 Mon Sep 17 00:00:00 2001
From: Grzegorz Bizon <grzesiek.bizon@gmail.com>
Date: Tue, 26 Sep 2017 15:01:16 +0200
Subject: [PATCH 19/20] Add test for head pipeline assignment when skipped

Closes gitlab-org/gitlab-ce#34415
---
 .../ci/create_pipeline_service_spec.rb        | 20 +++++++++++++++++++
 1 file changed, 20 insertions(+)

diff --git a/spec/services/ci/create_pipeline_service_spec.rb b/spec/services/ci/create_pipeline_service_spec.rb
index 6ee75b8fc23..eb6e683cc23 100644
--- a/spec/services/ci/create_pipeline_service_spec.rb
+++ b/spec/services/ci/create_pipeline_service_spec.rb
@@ -133,6 +133,26 @@ describe Ci::CreatePipelineService do
             expect(merge_request.reload.head_pipeline).to eq head_pipeline
           end
         end
+
+        context 'when pipeline has been skipped' do
+          before do
+            allow_any_instance_of(Ci::Pipeline)
+              .to receive(:git_commit_message)
+              .and_return('some commit [ci skip]')
+          end
+
+          it 'updates merge request head pipeline' do
+            merge_request = create(:merge_request, source_branch: 'master',
+                                                   target_branch: 'feature',
+                                                   source_project: project)
+
+            head_pipeline = execute_service
+
+            expect(head_pipeline).to be_skipped
+            expect(head_pipeline).to be_persisted
+            expect(merge_request.reload.head_pipeline).to eq head_pipeline
+          end
+        end
       end
 
       context 'auto-cancel enabled' do

From 26e73c2e8fa3d6cdd85ba82628981d3334445aeb Mon Sep 17 00:00:00 2001
From: Grzegorz Bizon <grzesiek.bizon@gmail.com>
Date: Wed, 27 Sep 2017 11:45:16 +0200
Subject: [PATCH 20/20] Add some minor improvements to pipeline creation chain

---
 app/services/ci/create_pipeline_service.rb          | 12 +++---------
 lib/gitlab/ci/pipeline/chain/sequence.rb            |  5 ++---
 lib/gitlab/ci/pipeline/chain/validate/repository.rb |  2 --
 3 files changed, 5 insertions(+), 14 deletions(-)

diff --git a/app/services/ci/create_pipeline_service.rb b/app/services/ci/create_pipeline_service.rb
index df5b32d97ca..31a712ccc1b 100644
--- a/app/services/ci/create_pipeline_service.rb
+++ b/app/services/ci/create_pipeline_service.rb
@@ -15,7 +15,7 @@ module Ci
         ref: ref,
         sha: sha,
         before_sha: before_sha,
-        tag: tag?,
+        tag: tag_exists?,
         trigger_requests: Array(trigger_request),
         user: current_user,
         pipeline_schedule: schedule,
@@ -88,20 +88,14 @@ module Ci
       params[:ref]
     end
 
-    def tag?
-      return @is_tag if defined?(@is_tag)
-
-      @is_tag = project.repository.tag_exists?(ref)
+    def tag_exists?
+      project.repository.tag_exists?(ref)
     end
 
     def ref
       @ref ||= Gitlab::Git.ref_name(origin_ref)
     end
 
-    def valid_sha?
-      origin_sha && origin_sha != Gitlab::Git::BLANK_SHA
-    end
-
     def pipeline_created_counter
       @pipeline_created_counter ||= Gitlab::Metrics
         .counter(:pipelines_created_total, "Counter of pipelines created")
diff --git a/lib/gitlab/ci/pipeline/chain/sequence.rb b/lib/gitlab/ci/pipeline/chain/sequence.rb
index c80d583939c..015f2988327 100644
--- a/lib/gitlab/ci/pipeline/chain/sequence.rb
+++ b/lib/gitlab/ci/pipeline/chain/sequence.rb
@@ -18,7 +18,7 @@ module Gitlab
 
               break if step.break?
 
-              @completed << true
+              @completed << step
             end
 
             @pipeline.tap do
@@ -27,8 +27,7 @@ module Gitlab
           end
 
           def complete?
-            @completed.size == @sequence.size &&
-              @completed.all?
+            @completed.size == @sequence.size
           end
         end
       end
diff --git a/lib/gitlab/ci/pipeline/chain/validate/repository.rb b/lib/gitlab/ci/pipeline/chain/validate/repository.rb
index 9d328c9cedb..70a4cfdbdea 100644
--- a/lib/gitlab/ci/pipeline/chain/validate/repository.rb
+++ b/lib/gitlab/ci/pipeline/chain/validate/repository.rb
@@ -14,8 +14,6 @@ module Gitlab
               ## TODO, we check commit in the service, that is why
               # there is no repository access here.
               #
-              # Should we validate repository before building a pipeline?
-              #
               unless pipeline.sha
                 return error('Commit not found')
               end