Commit ad526918 authored by Kamil Trzciński's avatar Kamil Trzciński

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
parents 5c136f1d 0d8b6955
...@@ -14,7 +14,6 @@ class CommitStatus < ActiveRecord::Base ...@@ -14,7 +14,6 @@ class CommitStatus < ActiveRecord::Base
delegate :sha, :short_sha, to: :pipeline delegate :sha, :short_sha, to: :pipeline
validates :pipeline, presence: true, unless: :importing? validates :pipeline, presence: true, unless: :importing?
validates :name, presence: true, unless: :importing? validates :name, presence: true, unless: :importing?
alias_attribute :author, :user alias_attribute :author, :user
...@@ -46,6 +45,17 @@ class CommitStatus < ActiveRecord::Base ...@@ -46,6 +45,17 @@ class CommitStatus < ActiveRecord::Base
runner_system_failure: 4 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 state_machine :status do
event :process do event :process do
transition [:skipped, :manual] => :created transition [:skipped, :manual] => :created
......
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
FactoryGirl.define do FactoryGirl.define do
factory :commit_status, class: CommitStatus do factory :commit_status, class: CommitStatus do
name 'default' name 'default'
stage 'test'
status 'success' status 'success'
description 'commit status' description 'commit status'
pipeline factory: :ci_pipeline_with_one_job pipeline factory: :ci_pipeline_with_one_job
......
require 'spec_helper' require 'spec_helper'
describe CommitStatus do 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) create(:ci_pipeline, project: project, sha: project.commit.id)
end end
...@@ -464,4 +464,73 @@ describe CommitStatus do ...@@ -464,4 +464,73 @@ describe CommitStatus do
it { is_expected.to be_script_failure } it { is_expected.to be_script_failure }
end end
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 end
...@@ -107,7 +107,7 @@ describe PipelineDetailsEntity do ...@@ -107,7 +107,7 @@ describe PipelineDetailsEntity do
it 'contains stages' do it 'contains stages' do
expect(subject).to include(:details) expect(subject).to include(:details)
expect(subject[:details]).to include(:stages) 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
end end
......
...@@ -94,6 +94,7 @@ module CycleAnalyticsHelpers ...@@ -94,6 +94,7 @@ module CycleAnalyticsHelpers
ref: 'master', ref: 'master',
tag: false, tag: false,
name: 'dummy', name: 'dummy',
stage: 'dummy',
pipeline: dummy_pipeline, pipeline: dummy_pipeline,
protected: false) protected: false)
end end
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment