Refactor code related to pipeline blocking actions

7 files changed, 36 insertions, 30 deletions

diff --git a/app/models/ci/build.rb b/app/models/ci/build.rb
index e2eb1c49975..d9057a7c97b 100644
--- a/app/models/ci/build.rb
+++ b/app/models/ci/build.rb
@@ -64,7 +64,7 @@ module Ci
 
     state_machine :status do
       event :block do
-        transition :created => :manual, if: ->() { self.when == 'manual' }
+        transition :created => :blocked
       end
 
       after_transition any => [:pending] do |build|
@@ -103,11 +103,20 @@ module Ci
     end
 
     def playable?
-      project.builds_enabled? && commands.present? && manual?
+      project.builds_enabled? && has_commands? && manual? &&
+        (skipped? || blocked?)
     end
 
-    def is_blocking?
-      playable? && !allow_failure?
+    def manual?
+      self.when == 'manual'
+    end
+
+    def barrier?
+      manual? && !allow_failure?
+    end
+
+    def has_commands?
+      commands.present?
     end
 
     def play(current_user)
@@ -126,7 +135,7 @@ module Ci
     end
 
     def retryable?
-      project.builds_enabled? && commands.present? &&
+      project.builds_enabled? && has_commands? &&
         (success? || failed? || canceled?)
     end
 
diff --git a/app/models/commit_status.rb b/app/models/commit_status.rb
index bca61151eff..3ef07ffd0da 100644
--- a/app/models/commit_status.rb
+++ b/app/models/commit_status.rb
@@ -25,13 +25,13 @@ class CommitStatus < ActiveRecord::Base
   end
 
   scope :failed_but_allowed, -> do
-    where(allow_failure: true, status: [:failed, :canceled, :manual])
+    where(allow_failure: true, status: [:failed, :canceled, :blocked])
   end
 
   scope :exclude_ignored, -> do
     # We want to ignore failed_but_allowed jobs
     where("allow_failure = ? OR status IN (?)",
-      false, all_state_names - [:failed, :canceled, :manual])
+      false, all_state_names - [:failed, :canceled, :blocked])
   end
 
   scope :retried, -> { where.not(id: latest) }
@@ -42,11 +42,11 @@ class CommitStatus < ActiveRecord::Base
 
   state_machine :status do
     event :enqueue do
-      transition [:created, :skipped, :manual] => :pending
+      transition [:created, :skipped, :blocked] => :pending
     end
 
     event :process do
-      transition [:skipped, :manual] => :created
+      transition [:skipped, :blocked] => :created
     end
 
     event :run do
@@ -66,7 +66,7 @@ class CommitStatus < ActiveRecord::Base
     end
 
     event :cancel do
-      transition [:created, :pending, :running, :manual] => :canceled
+      transition [:created, :pending, :running, :blocked] => :canceled
     end
 
     before_transition created: [:pending, :running] do |commit_status|
diff --git a/app/models/concerns/has_status.rb b/app/models/concerns/has_status.rb
index 62a7e9c0b7f..012407a72d8 100644
--- a/app/models/concerns/has_status.rb
+++ b/app/models/concerns/has_status.rb
@@ -2,23 +2,20 @@ module HasStatus
   extend ActiveSupport::Concern
 
   DEFAULT_STATUS = 'created'.freeze
-  AVAILABLE_STATUSES = %w[created pending running success failed canceled skipped manual].freeze
+  AVAILABLE_STATUSES = %w[created pending running success failed canceled skipped blocked].freeze
   STARTED_STATUSES = %w[running success failed skipped].freeze
-  ACTIVE_STATUSES = %w[pending running manual].freeze
+  ACTIVE_STATUSES = %w[pending running blocked].freeze
   COMPLETED_STATUSES = %w[success failed canceled skipped].freeze
   ORDERED_STATUSES = %w[failed pending running canceled success skipped].freeze
 
   class_methods do
     def status_sql
-      scope = if respond_to?(:exclude_ignored)
-                exclude_ignored
-              else
-                all
-              end
+      scope = respond_to?(:exclude_ignored) ? exclude_ignored : all
+
       builds = scope.select('count(*)').to_sql
       created = scope.created.select('count(*)').to_sql
       success = scope.success.select('count(*)').to_sql
-      manual = scope.manual.select('count(*)').to_sql
+      blocked = scope.blocked.select('count(*)').to_sql
       pending = scope.pending.select('count(*)').to_sql
       running = scope.running.select('count(*)').to_sql
       skipped = scope.skipped.select('count(*)').to_sql
@@ -32,7 +29,7 @@ module HasStatus
         WHEN (#{builds})=(#{success})+(#{skipped})+(#{canceled}) THEN 'canceled'
         WHEN (#{builds})=(#{created})+(#{skipped})+(#{pending}) THEN 'pending'
         WHEN (#{running})+(#{pending})+(#{created})>0 THEN 'running'
-        WHEN (#{manual})>0 THEN 'manual'
+        WHEN (#{blocked})>0 THEN 'blocked'
         ELSE 'failed'
       END)"
     end
@@ -65,7 +62,7 @@ module HasStatus
       state :success, value: 'success'
       state :canceled, value: 'canceled'
       state :skipped, value: 'skipped'
-      state :manual, value: 'manual'
+      state :blocked, value: 'blocked'
     end
 
     scope :created, -> { where(status: 'created') }
@@ -76,13 +73,13 @@ module HasStatus
     scope :failed, -> { where(status: 'failed')  }
     scope :canceled, -> { where(status: 'canceled')  }
     scope :skipped, -> { where(status: 'skipped')  }
-    scope :manual, -> { where(status: 'manual')  }
+    scope :blocked, -> { where(status: 'blocked')  }
     scope :running_or_pending, -> { where(status: [:running, :pending]) }
     scope :finished, -> { where(status: [:success, :failed, :canceled]) }
     scope :failed_or_canceled, -> { where(status: [:failed, :canceled]) }
 
     scope :cancelable, -> do
-      where(status: [:running, :pending, :created, :manual])
+      where(status: [:running, :pending, :created, :blocked])
     end
   end
 
diff --git a/app/services/ci/process_pipeline_service.rb b/app/services/ci/process_pipeline_service.rb
index 0a478f91214..24428d9afae 100644
--- a/app/services/ci/process_pipeline_service.rb
+++ b/app/services/ci/process_pipeline_service.rb
@@ -35,9 +35,9 @@ module Ci
       if valid_statuses_for_when(build.when).include?(current_status)
         build.enqueue
         true
-      elsif build.can_block?
+      elsif build.barrier?
         build.block
-        build.is_blocking?
+        false
       else
         build.skip
         false
diff --git a/lib/gitlab/ci/config/entry/job.rb b/lib/gitlab/ci/config/entry/job.rb
index 69e2a8a2563..7f7662f2776 100644
--- a/lib/gitlab/ci/config/entry/job.rb
+++ b/lib/gitlab/ci/config/entry/job.rb
@@ -104,10 +104,6 @@ module Gitlab
             (before_script_value.to_a + script_value.to_a).join("\n")
           end
 
-          def allow_failure
-            super || self.when == 'manual'
-          end
-
           private
 
           def inherit!(deps)
diff --git a/spec/factories/ci/builds.rb b/spec/factories/ci/builds.rb
index cabe128acf7..52244888e13 100644
--- a/spec/factories/ci/builds.rb
+++ b/spec/factories/ci/builds.rb
@@ -71,6 +71,10 @@ FactoryGirl.define do
       allow_failure true
     end
 
+    trait :ignored do
+      allowed_to_fail
+    end
+
     trait :playable do
       skipped
       manual
diff --git a/spec/services/ci/process_pipeline_service_spec.rb b/spec/services/ci/process_pipeline_service_spec.rb
index de68fb64726..3ce904244b2 100644
--- a/spec/services/ci/process_pipeline_service_spec.rb
+++ b/spec/services/ci/process_pipeline_service_spec.rb
@@ -68,9 +68,9 @@ describe Ci::ProcessPipelineService, :services do
         create(:ci_build, :created, pipeline: pipeline, name: 'test', stage_idx: 1)
         create(:ci_build, :created, pipeline: pipeline, name: 'test_failure', stage_idx: 2, when: 'on_failure')
         create(:ci_build, :created, pipeline: pipeline, name: 'deploy', stage_idx: 3)
-        create(:ci_build, :created, pipeline: pipeline, name: 'production', stage_idx: 3, when: 'manual')
+        create(:ci_build, :created, pipeline: pipeline, name: 'production', stage_idx: 3, when: 'manual', allow_failure: true)
         create(:ci_build, :created, pipeline: pipeline, name: 'cleanup', stage_idx: 4, when: 'always')
-        create(:ci_build, :created, pipeline: pipeline, name: 'clear cache', stage_idx: 4, when: 'manual')
+        create(:ci_build, :created, pipeline: pipeline, name: 'clear cache', stage_idx: 4, when: 'manual', allow_failure: true)
       end
 
       context 'when builds are successful' do