Merge branch 'fix/gb/ensure-that-job-belongs-to-stage' into 'master'

Make sure that every job has a stage assigned

Closes #37979

See merge request gitlab-org/gitlab-ce!14724

6 files changed, 124 insertions, 4 deletions

diff --git a/app/models/commit_status.rb b/app/models/commit_status.rb
index f3888528940..6b07dbdf3ea 100644
--- a/app/models/commit_status.rb
+++ b/app/models/commit_status.rb
@@ -14,7 +14,6 @@ class CommitStatus < ActiveRecord::Base
   delegate :sha, :short_sha, to: :pipeline
 
   validates :pipeline, presence: true, unless: :importing?
-
   validates :name, presence: true, unless: :importing?
 
   alias_attribute :author, :user
@@ -46,6 +45,17 @@ class CommitStatus < ActiveRecord::Base
     runner_system_failure: 4
   }
 
+  ##
+  # We still create some CommitStatuses outside of CreatePipelineService.
+  #
+  # These are pages deployments and external statuses.
+  #
+  before_create unless: :importing? do
+    Ci::EnsureStageService.new(project, user).execute(self) do |stage|
+      self.run_after_commit { StageUpdateWorker.perform_async(stage.id) }
+    end
+  end
+
   state_machine :status do
     event :process do
       transition [:skipped, :manual] => :created
diff --git a/app/services/ci/ensure_stage_service.rb b/app/services/ci/ensure_stage_service.rb
new file mode 100644
index 00000000000..dc2f49e8db1
--- /dev/null
+++ b/app/services/ci/ensure_stage_service.rb
@@ -0,0 +1,39 @@
+module Ci
+  ##
+  # We call this service everytime we persist a CI/CD job.
+  #
+  # In most cases a job should already have a stage assigned,  but in cases it
+  # doesn't have we need to either find existing one or create a brand new
+  # stage.
+  #
+  class EnsureStageService < BaseService
+    def execute(build)
+      @build = build
+
+      return if build.stage_id.present?
+      return if build.invalid?
+
+      ensure_stage.tap do |stage|
+        build.stage_id = stage.id
+
+        yield stage if block_given?
+      end
+    end
+
+    private
+
+    def ensure_stage
+      find_stage || create_stage
+    end
+
+    def find_stage
+      @build.pipeline.stages.find_by(name: @build.stage)
+    end
+
+    def create_stage
+      Ci::Stage.create!(name: @build.stage,
+                        pipeline: @build.pipeline,
+                        project: @build.project)
+    end
+  end
+end
diff --git a/spec/factories/commit_statuses.rb b/spec/factories/commit_statuses.rb
index 169590deb8e..abbe37df90e 100644
--- a/spec/factories/commit_statuses.rb
+++ b/spec/factories/commit_statuses.rb
@@ -1,6 +1,7 @@
 FactoryGirl.define do
   factory :commit_status, class: CommitStatus do
     name 'default'
+    stage 'test'
     status 'success'
     description 'commit status'
     pipeline factory: :ci_pipeline_with_one_job
diff --git a/spec/models/commit_status_spec.rb b/spec/models/commit_status_spec.rb
index 858ec831200..c536dab2681 100644
--- a/spec/models/commit_status_spec.rb
+++ b/spec/models/commit_status_spec.rb
@@ -1,9 +1,9 @@
 require 'spec_helper'
 
 describe CommitStatus do
-  let(:project) { create(:project, :repository) }
+  set(:project) { create(:project, :repository) }
 
-  let(:pipeline) do
+  set(:pipeline) do
     create(:ci_pipeline, project: project, sha: project.commit.id)
   end
 
@@ -464,4 +464,73 @@ describe CommitStatus do
       it { is_expected.to be_script_failure }
     end
   end
+
+  describe 'ensure stage assignment' do
+    context 'when commit status has a stage_id assigned' do
+      let!(:stage) do
+        create(:ci_stage_entity, project: project, pipeline: pipeline)
+      end
+
+      let(:commit_status) do
+        create(:commit_status, stage_id: stage.id, name: 'rspec', stage: 'test')
+      end
+
+      it 'does not create a new stage' do
+        expect { commit_status }.not_to change { Ci::Stage.count }
+        expect(commit_status.stage_id).to eq stage.id
+      end
+    end
+
+    context 'when commit status does not have a stage_id assigned' do
+      let(:commit_status) do
+        create(:commit_status, name: 'rspec', stage: 'test', status: :success)
+      end
+
+      let(:stage) { Ci::Stage.first }
+
+      it 'creates a new stage' do
+        expect { commit_status }.to change { Ci::Stage.count }.by(1)
+
+        expect(stage.name).to eq 'test'
+        expect(stage.project).to eq commit_status.project
+        expect(stage.pipeline).to eq commit_status.pipeline
+        expect(stage.status).to eq commit_status.status
+        expect(commit_status.stage_id).to eq stage.id
+      end
+    end
+
+    context 'when commit status does not have stage but it exists' do
+      let!(:stage) do
+        create(:ci_stage_entity, project: project,
+                                 pipeline: pipeline,
+                                 name: 'test')
+      end
+
+      let(:commit_status) do
+        create(:commit_status, project: project,
+                               pipeline: pipeline,
+                               name: 'rspec',
+                               stage: 'test',
+                               status: :success)
+      end
+
+      it 'uses existing stage' do
+        expect { commit_status }.not_to change { Ci::Stage.count }
+
+        expect(commit_status.stage_id).to eq stage.id
+        expect(stage.reload.status).to eq commit_status.status
+      end
+    end
+
+    context 'when commit status is being imported' do
+      let(:commit_status) do
+        create(:commit_status, name: 'rspec', stage: 'test', importing: true)
+      end
+
+      it 'does not create a new stage' do
+        expect { commit_status }.not_to change { Ci::Stage.count }
+        expect(commit_status.stage_id).not_to be_present
+      end
+    end
+  end
 end
diff --git a/spec/serializers/pipeline_details_entity_spec.rb b/spec/serializers/pipeline_details_entity_spec.rb
index f60d1843581..45e18086894 100644
--- a/spec/serializers/pipeline_details_entity_spec.rb
+++ b/spec/serializers/pipeline_details_entity_spec.rb
@@ -107,7 +107,7 @@ describe PipelineDetailsEntity do
       it 'contains stages' do
         expect(subject).to include(:details)
         expect(subject[:details]).to include(:stages)
-        expect(subject[:details][:stages].first).to include(name: 'external')
+        expect(subject[:details][:stages].first).to include(name: 'test')
       end
     end
 
diff --git a/spec/support/cycle_analytics_helpers.rb b/spec/support/cycle_analytics_helpers.rb
index 934b4557ba2..26fd271ce31 100644
--- a/spec/support/cycle_analytics_helpers.rb
+++ b/spec/support/cycle_analytics_helpers.rb
@@ -94,6 +94,7 @@ module CycleAnalyticsHelpers
       ref: 'master',
       tag: false,
       name: 'dummy',
+      stage: 'dummy',
       pipeline: dummy_pipeline,
       protected: false)
   end