Merge branch 'all-skipped-equals-success' into 'master'

Skipped jobs should be considered successful

## What does this MR do?

If all jobs in previous stage are all skipped, the next stage should consider previous stage succeeded.

## Why was this MR needed?

Since for now we consider all manual jobs if skipped, should not block the next stage from running.

Closes #22598 

See also #20342 (because this merge request conflicts with it)

See merge request !6604

CHANGELOG

@@ -23,6 +23,7 @@ v 8.13.0 (unreleased)
   - Fix todos page mobile viewport layout (ClemMakesApps)
   - Fix robots.txt disallowing access to groups starting with "s" (Matt Harrison)
   - Close open merge request without source project (Katarzyna Kobierska Ula Budziszewska)
+  - Fix that manual jobs would no longer block jobs in the next stage. !6604
   - Add configurable email subject suffix (Fu Xu)
   - Use a ConnectionPool for Rails.cache on Sidekiq servers
   - Replace `alias_method_chain` with `Module#prepend`

app/models/ci/pipeline.rb

@@ -196,7 +196,7 @@ module Ci
     end
 
     def has_warnings?
-      builds.latest.ignored.any?
+      builds.latest.failed_but_allowed.any?
     end
 
     def config_processor

app/models/commit_status.rb

@@ -24,7 +24,22 @@ class CommitStatus < ActiveRecord::Base
 
   scope :retried, -> { where.not(id: latest) }
   scope :ordered, -> { order(:name) }
-  scope :ignored, -> { where(allow_failure: true, status: [:failed, :canceled]) }
+
+  scope :failed_but_allowed, -> do
+    where(allow_failure: true, status: [:failed, :canceled])
+  end
+
+  scope :exclude_ignored, -> do
+    quoted_when = connection.quote_column_name('when')
+    # We want to ignore failed_but_allowed jobs
+    where("allow_failure = ? OR status IN (?)",
+      false, all_state_names - [:failed, :canceled]).
+      # We want to ignore skipped manual jobs
+      where("#{quoted_when} <> ? OR status <> ?", 'manual', 'skipped').
+      # We want to ignore skipped on_failure
+      where("#{quoted_when} <> ? OR status <> ?", 'on_failure', 'skipped')
+  end
+
   scope :latest_ci_stages, -> { latest.ordered.includes(project: :namespace) }
   scope :retried_ci_stages, -> { retried.ordered.includes(project: :namespace) }
 
@@ -111,7 +126,7 @@ class CommitStatus < ActiveRecord::Base
     end
   end
 
-  def ignored?
+  def failed_but_allowed?
     allow_failure? && (failed? || canceled?)
   end
 

app/models/concerns/has_status.rb

@@ -8,32 +8,32 @@ module HasStatus
 
   class_methods do
     def status_sql
-      scope = all
+      scope = if respond_to?(:exclude_ignored)
+                exclude_ignored
+              else
+                all
+              end
       builds = scope.select('count(*)').to_sql
       created = scope.created.select('count(*)').to_sql
       success = scope.success.select('count(*)').to_sql
-      ignored = scope.ignored.select('count(*)').to_sql if scope.respond_to?(:ignored)
-      ignored ||= '0'
       pending = scope.pending.select('count(*)').to_sql
       running = scope.running.select('count(*)').to_sql
-      canceled = scope.canceled.select('count(*)').to_sql
       skipped = scope.skipped.select('count(*)').to_sql
+      canceled = scope.canceled.select('count(*)').to_sql
 
-      deduce_status = "(CASE
+      "(CASE
+        WHEN (#{builds})=(#{success}) THEN 'success'
         WHEN (#{builds})=(#{created}) THEN 'created'
-        WHEN (#{builds})=(#{skipped}) THEN 'skipped'
-        WHEN (#{builds})=(#{success})+(#{ignored})+(#{skipped}) THEN 'success'
-        WHEN (#{builds})=(#{created})+(#{pending})+(#{skipped}) THEN 'pending'
-        WHEN (#{builds})=(#{canceled})+(#{success})+(#{ignored})+(#{skipped}) THEN 'canceled'
+        WHEN (#{builds})=(#{success})+(#{skipped}) THEN 'skipped'
+        WHEN (#{builds})=(#{success})+(#{skipped})+(#{canceled}) THEN 'canceled'
+        WHEN (#{builds})=(#{created})+(#{skipped})+(#{pending}) THEN 'pending'
         WHEN (#{running})+(#{pending})+(#{created})>0 THEN 'running'
         ELSE 'failed'
       END)"
-
-      deduce_status
     end
 
     def status
-      all.pluck(self.status_sql).first
+      all.pluck(status_sql).first
     end
 
     def started_at
@@ -43,6 +43,10 @@ module HasStatus
     def finished_at
       all.maximum(:finished_at)
     end
+
+    def all_state_names
+      state_machines.values.flat_map(&:states).flat_map { |s| s.map(&:name) }
+    end
   end
 
   included do

spec/models/build_spec.rb

@@ -39,8 +39,8 @@ describe Ci::Build, models: true do
     end
   end
 
-  describe '#ignored?' do
-    subject { build.ignored? }
+  describe '#failed_but_allowed?' do
+    subject { build.failed_but_allowed? }
 
     context 'when build is not allowed to fail' do
       before do

spec/models/commit_status_spec.rb

@@ -7,7 +7,11 @@ describe CommitStatus, models: true do
     create(:ci_pipeline, project: project, sha: project.commit.id)
   end
 
-  let(:commit_status) { create(:commit_status, pipeline: pipeline) }
+  let(:commit_status) { create_status }
+
+  def create_status(args = {})
+    create(:commit_status, args.merge(pipeline: pipeline))
+  end
 
   it { is_expected.to belong_to(:pipeline) }
   it { is_expected.to belong_to(:user) }
@@ -125,32 +129,53 @@ describe CommitStatus, models: true do
   describe '.latest' do
     subject { CommitStatus.latest.order(:id) }
 
-    before do
-      @commit1 = FactoryGirl.create :commit_status, pipeline: pipeline, name: 'aa', ref: 'bb', status: 'running'
-      @commit2 = FactoryGirl.create :commit_status, pipeline: pipeline, name: 'cc', ref: 'cc', status: 'pending'
-      @commit3 = FactoryGirl.create :commit_status, pipeline: pipeline, name: 'aa', ref: 'cc', status: 'success'
-      @commit4 = FactoryGirl.create :commit_status, pipeline: pipeline, name: 'cc', ref: 'bb', status: 'success'
-      @commit5 = FactoryGirl.create :commit_status, pipeline: pipeline, name: 'aa', ref: 'bb', status: 'success'
+    let(:statuses) do
+      [create_status(name: 'aa', ref: 'bb', status: 'running'),
+       create_status(name: 'cc', ref: 'cc', status: 'pending'),
+       create_status(name: 'aa', ref: 'cc', status: 'success'),
+       create_status(name: 'cc', ref: 'bb', status: 'success'),
+       create_status(name: 'aa', ref: 'bb', status: 'success')]
     end
 
     it 'returns unique statuses' do
-      is_expected.to eq([@commit4, @commit5])
+      is_expected.to eq(statuses.values_at(3, 4))
     end
   end
 
   describe '.running_or_pending' do
     subject { CommitStatus.running_or_pending.order(:id) }
 
-    before do
-      @commit1 = FactoryGirl.create :commit_status, pipeline: pipeline, name: 'aa', ref: 'bb', status: 'running'
-      @commit2 = FactoryGirl.create :commit_status, pipeline: pipeline, name: 'cc', ref: 'cc', status: 'pending'
-      @commit3 = FactoryGirl.create :commit_status, pipeline: pipeline, name: 'aa', ref: nil, status: 'success'
-      @commit4 = FactoryGirl.create :commit_status, pipeline: pipeline, name: 'dd', ref: nil, status: 'failed'
-      @commit5 = FactoryGirl.create :commit_status, pipeline: pipeline, name: 'ee', ref: nil, status: 'canceled'
+    let(:statuses) do
+      [create_status(name: 'aa', ref: 'bb', status: 'running'),
+       create_status(name: 'cc', ref: 'cc', status: 'pending'),
+       create_status(name: 'aa', ref: nil, status: 'success'),
+       create_status(name: 'dd', ref: nil, status: 'failed'),
+       create_status(name: 'ee', ref: nil, status: 'canceled')]
     end
 
     it 'returns statuses that are running or pending' do
-      is_expected.to eq([@commit1, @commit2])
+      is_expected.to eq(statuses.values_at(0, 1))
+    end
+  end
+
+  describe '.exclude_ignored' do
+    subject { CommitStatus.exclude_ignored.order(:id) }
+
+    let(:statuses) do
+      [create_status(when: 'manual', status: 'skipped'),
+       create_status(when: 'manual', status: 'success'),
+       create_status(when: 'manual', status: 'failed'),
+       create_status(when: 'on_failure', status: 'skipped'),
+       create_status(when: 'on_failure', status: 'success'),
+       create_status(when: 'on_failure', status: 'failed'),
+       create_status(allow_failure: true, status: 'success'),
+       create_status(allow_failure: true, status: 'failed'),
+       create_status(allow_failure: false, status: 'success'),
+       create_status(allow_failure: false, status: 'failed')]
+    end
+
+    it 'returns statuses without what we want to ignore' do
+      is_expected.to eq(statuses.values_at(1, 2, 4, 5, 6, 8, 9))
     end
   end
 

spec/models/concerns/has_status_spec.rb

 require 'spec_helper'
 
 describe HasStatus do
-  before do
-    @object = Object.new
-    @object.extend(HasStatus::ClassMethods)
-  end
-
   describe '.status' do
-    before do
-      allow(@object).to receive(:all).and_return(CommitStatus.where(id: statuses))
-    end
-
-    subject { @object.status }
+    subject { CommitStatus.status }
 
     shared_examples 'build status summary' do
       context 'all successful' do
-        let(:statuses) { Array.new(2) { create(type, status: :success) } }
+        let!(:statuses) { Array.new(2) { create(type, status: :success) } }
         it { is_expected.to eq 'success' }
       end
 
       context 'at least one failed' do
-        let(:statuses) do
+        let!(:statuses) do
           [create(type, status: :success), create(type, status: :failed)]
         end
 
@@ -28,7 +19,7 @@ describe HasStatus do
       end
 
       context 'at least one running' do
-        let(:statuses) do
+        let!(:statuses) do
           [create(type, status: :success), create(type, status: :running)]
         end
 
@@ -36,7 +27,7 @@ describe HasStatus do
       end
 
       context 'at least one pending' do
-        let(:statuses) do
+        let!(:statuses) do
           [create(type, status: :success), create(type, status: :pending)]
         end
 
@@ -44,7 +35,7 @@ describe HasStatus do
       end
 
       context 'success and failed but allowed to fail' do
-        let(:statuses) do
+        let!(:statuses) do
           [create(type, status: :success),
            create(type, status: :failed, allow_failure: true)]
         end
@@ -53,12 +44,15 @@ describe HasStatus do
       end
 
       context 'one failed but allowed to fail' do
-        let(:statuses) { [create(type, status: :failed, allow_failure: true)] }
+        let!(:statuses) do
+          [create(type, status: :failed, allow_failure: true)]
+        end
+
         it { is_expected.to eq 'success' }
       end
 
       context 'success and canceled' do
-        let(:statuses) do
+        let!(:statuses) do
           [create(type, status: :success), create(type, status: :canceled)]
         end
 
@@ -66,7 +60,7 @@ describe HasStatus do
       end
 
       context 'one failed and one canceled' do
-        let(:statuses) do
+        let!(:statuses) do
           [create(type, status: :failed), create(type, status: :canceled)]
         end
 
@@ -74,7 +68,7 @@ describe HasStatus do
       end
 
       context 'one failed but allowed to fail and one canceled' do
-        let(:statuses) do
+        let!(:statuses) do
           [create(type, status: :failed, allow_failure: true),
            create(type, status: :canceled)]
         end
@@ -83,7 +77,7 @@ describe HasStatus do
       end
 
       context 'one running one canceled' do
-        let(:statuses) do
+        let!(:statuses) do
           [create(type, status: :running), create(type, status: :canceled)]
         end
 
@@ -91,14 +85,15 @@ describe HasStatus do
       end
 
       context 'all canceled' do
-        let(:statuses) do
+        let!(:statuses) do
           [create(type, status: :canceled), create(type, status: :canceled)]
         end
+
         it { is_expected.to eq 'canceled' }
       end
 
       context 'success and canceled but allowed to fail' do
-        let(:statuses) do
+        let!(:statuses) do
           [create(type, status: :success),
            create(type, status: :canceled, allow_failure: true)]
         end
@@ -107,7 +102,7 @@ describe HasStatus do
       end
 
       context 'one finished and second running but allowed to fail' do
-        let(:statuses) do
+        let!(:statuses) do
           [create(type, status: :success),
            create(type, status: :running, allow_failure: true)]
         end
@@ -118,11 +113,13 @@ describe HasStatus do
 
     context 'ci build statuses' do
       let(:type) { :ci_build }
+
       it_behaves_like 'build status summary'
     end
 
     context 'generic commit statuses' do
       let(:type) { :generic_commit_status }
+
       it_behaves_like 'build status summary'
     end
   end

spec/services/ci/process_pipeline_service_spec.rb

@@ -18,7 +18,7 @@ describe Ci::ProcessPipelineService, services: true do
       all_builds.where.not(status: [:created, :skipped])
     end
 
-    def create_builds
+    def process_pipeline
       described_class.new(pipeline.project, user).execute(pipeline)
     end
 
@@ -36,26 +36,26 @@ describe Ci::ProcessPipelineService, services: true do
       end
 
       it 'processes a pipeline' do
-        expect(create_builds).to be_truthy
+        expect(process_pipeline).to be_truthy
         succeed_pending
         expect(builds.success.count).to eq(2)
 
-        expect(create_builds).to be_truthy
+        expect(process_pipeline).to be_truthy
         succeed_pending
         expect(builds.success.count).to eq(4)
 
-        expect(create_builds).to be_truthy
+        expect(process_pipeline).to be_truthy
         succeed_pending
         expect(builds.success.count).to eq(5)
 
-        expect(create_builds).to be_falsey
+        expect(process_pipeline).to be_falsey
       end
 
       it 'does not process pipeline if existing stage is running' do
-        expect(create_builds).to be_truthy
+        expect(process_pipeline).to be_truthy
         expect(builds.pending.count).to eq(2)
 
-        expect(create_builds).to be_falsey
+        expect(process_pipeline).to be_falsey
         expect(builds.pending.count).to eq(2)
       end
     end
@@ -67,7 +67,7 @@ describe Ci::ProcessPipelineService, services: true do
       end
 
       it 'automatically triggers a next stage when build finishes' do
-        expect(create_builds).to be_truthy
+        expect(process_pipeline).to be_truthy
         expect(builds.pluck(:status)).to contain_exactly('pending')
 
         pipeline.builds.running_or_pending.each(&:drop)
@@ -88,7 +88,7 @@ describe Ci::ProcessPipelineService, services: true do
 
       context 'when builds are successful' do
         it 'properly creates builds' do
-          expect(create_builds).to be_truthy
+          expect(process_pipeline).to be_truthy
           expect(builds.pluck(:name)).to contain_exactly('build')
           expect(builds.pluck(:status)).to contain_exactly('pending')
           pipeline.builds.running_or_pending.each(&:success)
@@ -113,7 +113,7 @@ describe Ci::ProcessPipelineService, services: true do
 
       context 'when test job fails' do
         it 'properly creates builds' do
-          expect(create_builds).to be_truthy
+          expect(process_pipeline).to be_truthy
           expect(builds.pluck(:name)).to contain_exactly('build')
           expect(builds.pluck(:status)).to contain_exactly('pending')
           pipeline.builds.running_or_pending.each(&:success)
@@ -138,7 +138,7 @@ describe Ci::ProcessPipelineService, services: true do
 
       context 'when test and test_failure jobs fail' do
         it 'properly creates builds' do
-          expect(create_builds).to be_truthy
+          expect(process_pipeline).to be_truthy
           expect(builds.pluck(:name)).to contain_exactly('build')
           expect(builds.pluck(:status)).to contain_exactly('pending')
           pipeline.builds.running_or_pending.each(&:success)
@@ -164,7 +164,7 @@ describe Ci::ProcessPipelineService, services: true do
 
       context 'when deploy job fails' do
         it 'properly creates builds' do
-          expect(create_builds).to be_truthy
+          expect(process_pipeline).to be_truthy
           expect(builds.pluck(:name)).to contain_exactly('build')
           expect(builds.pluck(:status)).to contain_exactly('pending')
           pipeline.builds.running_or_pending.each(&:success)
@@ -189,7 +189,7 @@ describe Ci::ProcessPipelineService, services: true do
 
       context 'when build is canceled in the second stage' do
         it 'does not schedule builds after build has been canceled' do
-          expect(create_builds).to be_truthy
+          expect(process_pipeline).to be_truthy
           expect(builds.pluck(:name)).to contain_exactly('build')
           expect(builds.pluck(:status)).to contain_exactly('pending')
           pipeline.builds.running_or_pending.each(&:success)
@@ -208,7 +208,7 @@ describe Ci::ProcessPipelineService, services: true do
       context 'when listing manual actions' do
         it 'returns only for skipped builds' do
           # currently all builds are created
-          expect(create_builds).to be_truthy
+          expect(process_pipeline).to be_truthy
           expect(manual_actions).to be_empty
 
           # succeed stage build
@@ -230,6 +230,69 @@ describe Ci::ProcessPipelineService, services: true do
       end
     end
 
+    context 'when there are manual/on_failure jobs in earlier stages' do
+      before do
+        builds
+        process_pipeline
+        builds.each(&:reload)
+      end
+
+      context 'when first stage has only manual jobs' do
+        let(:builds) do
+          [create_build('build', 0, 'manual'),
+           create_build('check', 1),
+           create_build('test', 2)]
+        end
+
+        it 'starts from the second stage' do
+          expect(builds.map(&:status)).to eq(%w[skipped pending created])
+        end
+      end
+
+      context 'when second stage has only manual jobs' do
+        let(:builds) do
+          [create_build('check', 0),
+           create_build('build', 1, 'manual'),
+           create_build('test', 2)]
+        end
+
+        it 'skips second stage and continues on third stage' do
+          expect(builds.map(&:status)).to eq(%w[pending created created])
+
+          builds.first.success
+          builds.each(&:reload)
+
+          expect(builds.map(&:status)).to eq(%w[success skipped pending])
+        end
+      end
+
+      context 'when second stage has only on_failure jobs' do
+        let(:builds) do
+          [create_build('check', 0),
+           create_build('build', 1, 'on_failure'),
+           create_build('test', 2)]
+        end
+
+        it 'skips second stage and continues on third stage' do
+          expect(builds.map(&:status)).to eq(%w[pending created created])
+
+          builds.first.success
+          builds.each(&:reload)
+
+          expect(builds.map(&:status)).to eq(%w[success skipped pending])
+        end
+      end
+
+      def create_build(name, stage_idx, when_value = nil)
+        create(:ci_build,
+               :created,
+               pipeline: pipeline,
+               name: name,
+               stage_idx: stage_idx,
+               when: when_value)
+      end
+    end
+
     context 'when failed build in the middle stage is retried' do
       context 'when failed build is the only unsuccessful build in the stage' do
         before do
@@ -242,7 +305,7 @@ describe Ci::ProcessPipelineService, services: true do
         end
 
         it 'does trigger builds in the next stage' do
-          expect(create_builds).to be_truthy
+          expect(process_pipeline).to be_truthy
           expect(builds.pluck(:name)).to contain_exactly('build:1', 'build:2')
 
           pipeline.builds.running_or_pending.each(&:success)
@@ -297,14 +360,14 @@ describe Ci::ProcessPipelineService, services: true do
         expect(all_builds.count).to eq(2)
 
         # Create builds will mark the created as pending
-        expect(create_builds).to be_truthy
+        expect(process_pipeline).to be_truthy
         expect(builds.count).to eq(2)
         expect(all_builds.count).to eq(2)
 
         # When we builds succeed we will create a rest of pipeline from .gitlab-ci.yml
         # We will have 2 succeeded, 2 pending (from stage test), total 5 (one more build from deploy)
         succeed_pending
-        expect(create_builds).to be_truthy
+        expect(process_pipeline).to be_truthy
         expect(builds.success.count).to eq(2)
         expect(builds.pending.count).to eq(2)
         expect(all_builds.count).to eq(5)
@@ -312,14 +375,14 @@ describe Ci::ProcessPipelineService, services: true do
         # When we succeed the 2 pending from stage test,
         # We will queue a deploy stage, no new builds will be created
         succeed_pending
-        expect(create_builds).to be_truthy
+        expect(process_pipeline).to be_truthy
         expect(builds.pending.count).to eq(1)
         expect(builds.success.count).to eq(4)
         expect(all_builds.count).to eq(5)
 
         # When we succeed last pending build, we will have a total of 5 succeeded builds, no new builds will be created
         succeed_pending
-        expect(create_builds).to be_falsey
+        expect(process_pipeline).to be_falsey
         expect(builds.success.count).to eq(5)
         expect(all_builds.count).to eq(5)
       end