Commit aef3a688 authored by Fabio Pitino's avatar Fabio Pitino

Merge branch 'lm-replace-retryable-rename-service' into 'master'

Renames RetryJobService and adds retryable job check

See merge request gitlab-org/gitlab!82096
parents e6a0f5b9 adaee22d
...@@ -78,10 +78,13 @@ class Projects::JobsController < Projects::ApplicationController ...@@ -78,10 +78,13 @@ class Projects::JobsController < Projects::ApplicationController
end end
def retry def retry
return respond_422 unless @build.retryable? response = Ci::RetryJobService.new(project, current_user).execute(@build)
build = Ci::Build.retry(@build, current_user) if response.success?
redirect_to build_path(build) redirect_to build_path(response[:job])
else
respond_422
end
end end
def play def play
......
...@@ -57,11 +57,13 @@ class Projects::WebIdeTerminalsController < Projects::ApplicationController ...@@ -57,11 +57,13 @@ class Projects::WebIdeTerminalsController < Projects::ApplicationController
end end
def retry def retry
return respond_422 unless build.retryable? response = Ci::RetryJobService.new(build.project, current_user).execute(build)
new_build = Ci::Build.retry(build, current_user) if response.success?
render_terminal(response[:job])
render_terminal(new_build) else
respond_422
end
end end
private private
......
...@@ -17,11 +17,19 @@ module Mutations ...@@ -17,11 +17,19 @@ module Mutations
job = authorized_find!(id: id) job = authorized_find!(id: id)
project = job.project project = job.project
::Ci::RetryBuildService.new(project, current_user).execute(job) response = ::Ci::RetryJobService.new(project, current_user).execute(job)
{
job: job, if response.success?
errors: errors_on_object(job) {
} job: response[:job],
errors: []
}
else
{
job: nil,
errors: [response.message]
}
end
end end
end end
end end
......
...@@ -57,10 +57,6 @@ module Ci ...@@ -57,10 +57,6 @@ module Ci
end end
end end
def self.retry(bridge, current_user)
raise NotImplementedError
end
def self.with_preloads def self.with_preloads
preload( preload(
:metadata, :metadata,
...@@ -69,6 +65,10 @@ module Ci ...@@ -69,6 +65,10 @@ module Ci
) )
end end
def retryable?
false
end
def inherit_status_from_downstream!(pipeline) def inherit_status_from_downstream!(pipeline)
case pipeline.status case pipeline.status
when 'success' when 'success'
......
...@@ -218,17 +218,21 @@ module Ci ...@@ -218,17 +218,21 @@ module Ci
pending.unstarted.order('created_at ASC').first pending.unstarted.order('created_at ASC').first
end end
def retry(build, current_user)
# rubocop: disable CodeReuse/ServiceClass
Ci::RetryBuildService
.new(build.project, current_user)
.execute(build)
# rubocop: enable CodeReuse/ServiceClass
end
def with_preloads def with_preloads
preload(:job_artifacts_archive, :job_artifacts, :tags, project: [:namespace]) preload(:job_artifacts_archive, :job_artifacts, :tags, project: [:namespace])
end end
def extra_accessors
[]
end
def clone_accessors
%i[pipeline project ref tag options name
allow_failure stage stage_id stage_idx trigger_request
yaml_variables when environment coverage_regex
description tag_list protected needs_attributes
job_variables_attributes resource_group scheduling_type].freeze
end
end end
state_machine :status do state_machine :status do
...@@ -351,7 +355,9 @@ module Ci ...@@ -351,7 +355,9 @@ module Ci
if build.auto_retry_allowed? if build.auto_retry_allowed?
begin begin
Ci::Build.retry(build, build.user) # rubocop: disable CodeReuse/ServiceClass
Ci::RetryJobService.new(build.project, build.user).execute(build)
# rubocop: enable CodeReuse/ServiceClass
rescue Gitlab::Access::AccessDeniedError => ex rescue Gitlab::Access::AccessDeniedError => ex
Gitlab::AppLogger.error "Unable to auto-retry job #{build.id}: #{ex}" Gitlab::AppLogger.error "Unable to auto-retry job #{build.id}: #{ex}"
end end
...@@ -472,12 +478,6 @@ module Ci ...@@ -472,12 +478,6 @@ module Ci
active? || created? active? || created?
end end
def retryable?
return false if retried? || archived? || deployment_rejected?
success? || failed? || canceled?
end
def retries_count def retries_count
pipeline.builds.retried.where(name: self.name).count pipeline.builds.retried.where(name: self.name).count
end end
......
...@@ -101,6 +101,12 @@ module Ci ...@@ -101,6 +101,12 @@ module Ci
:merge_train_pipeline?, :merge_train_pipeline?,
to: :pipeline to: :pipeline
def retryable?
return false if retried? || archived? || deployment_rejected?
success? || failed? || canceled?
end
def aggregated_needs_names def aggregated_needs_names
read_attribute(:aggregated_needs_names) read_attribute(:aggregated_needs_names)
end end
......
...@@ -14,10 +14,7 @@ module Ci ...@@ -14,10 +14,7 @@ module Ci
AfterRequeueJobService.new(project, current_user).execute(build) AfterRequeueJobService.new(project, current_user).execute(build)
end end
else else
# Retrying in Ci::PlayBuildService is a legacy process that should be removed. Ci::RetryJobService.new(project, current_user).execute(build)[:job]
# Instead, callers should explicitly execute Ci::RetryBuildService.
# See https://gitlab.com/gitlab-org/gitlab/-/issues/347493.
build.retryable? ? Ci::Build.retry(build, current_user) : build
end end
end end
......
# frozen_string_literal: true # frozen_string_literal: true
module Ci module Ci
class RetryBuildService < ::BaseService class RetryJobService < ::BaseService
include Gitlab::Utils::StrongMemoize include Gitlab::Utils::StrongMemoize
def self.clone_accessors def execute(job)
%i[pipeline project ref tag options name if job.retryable?
allow_failure stage stage_id stage_idx trigger_request job.ensure_scheduling_type!
yaml_variables when environment coverage_regex new_job = retry_job(job)
description tag_list protected needs_attributes
job_variables_attributes resource_group scheduling_type].freeze ServiceResponse.success(payload: { job: new_job })
end else
ServiceResponse.error(
def self.extra_accessors message: 'Job cannot be retried',
[] payload: { job: job, reason: :not_retryable }
end )
def execute(build)
build.ensure_scheduling_type!
clone!(build).tap do |new_build|
check_assignable_runners!(new_build)
next if new_build.failed?
Gitlab::OptimisticLocking.retry_lock(new_build, name: 'retry_build', &:enqueue)
AfterRequeueJobService.new(project, current_user).execute(build)
end end
end end
# rubocop: disable CodeReuse/ActiveRecord # rubocop: disable CodeReuse/ActiveRecord
def clone!(build) def clone!(job)
# Cloning a build requires a strict type check to ensure # Cloning a job requires a strict type check to ensure
# the attributes being used for the clone are taken straight # the attributes being used for the clone are taken straight
# from the model and not overridden by other abstractions. # from the model and not overridden by other abstractions.
raise TypeError unless build.instance_of?(Ci::Build) raise TypeError unless job.instance_of?(Ci::Build)
check_access!(build) check_access!(job)
new_build = clone_build(build) new_job = clone_job(job)
new_build.run_after_commit do new_job.run_after_commit do
::Ci::CopyCrossDatabaseAssociationsService.new.execute(build, new_build) ::Ci::CopyCrossDatabaseAssociationsService.new.execute(job, new_job)
::Deployments::CreateForBuildService.new.execute(new_build) ::Deployments::CreateForBuildService.new.execute(new_job)
::MergeRequests::AddTodoWhenBuildFailsService ::MergeRequests::AddTodoWhenBuildFailsService
.new(project: project) .new(project: project)
.close(new_build) .close(new_job)
end end
::Ci::Pipelines::AddJobService.new(build.pipeline).execute!(new_build) do |job| ::Ci::Pipelines::AddJobService.new(job.pipeline).execute!(new_job) do |processable|
BulkInsertableAssociations.with_bulk_insert do BulkInsertableAssociations.with_bulk_insert do
job.save! processable.save!
end end
end end
build.reset # refresh the data to get new values of `retried` and `processed`. job.reset # refresh the data to get new values of `retried` and `processed`.
new_build new_job
end end
# rubocop: enable CodeReuse/ActiveRecord # rubocop: enable CodeReuse/ActiveRecord
private private
def check_access!(build) def retry_job(job)
unless can?(current_user, :update_build, build) clone!(job).tap do |new_job|
check_assignable_runners!(new_job)
next if new_job.failed?
Gitlab::OptimisticLocking.retry_lock(new_job, name: 'retry_build', &:enqueue)
AfterRequeueJobService.new(project, current_user).execute(job)
end
end
def check_access!(job)
unless can?(current_user, :update_build, job)
raise Gitlab::Access::AccessDeniedError, '403 Forbidden' raise Gitlab::Access::AccessDeniedError, '403 Forbidden'
end end
end end
def check_assignable_runners!(build); end def check_assignable_runners!(job); end
def clone_build(build) def clone_job(job)
project.builds.new(build_attributes(build)) project.builds.new(job_attributes(job))
end end
def build_attributes(build) def job_attributes(job)
attributes = self.class.clone_accessors.to_h do |attribute| attributes = job.class.clone_accessors.to_h do |attribute|
[attribute, build.public_send(attribute)] # rubocop:disable GitlabSecurity/PublicSend [attribute, job.public_send(attribute)] # rubocop:disable GitlabSecurity/PublicSend
end end
if build.persisted_environment.present? if job.persisted_environment.present?
attributes[:metadata_attributes] ||= {} attributes[:metadata_attributes] ||= {}
attributes[:metadata_attributes][:expanded_environment_name] = build.expanded_environment_name attributes[:metadata_attributes][:expanded_environment_name] = job.expanded_environment_name
end end
attributes[:user] = current_user attributes[:user] = current_user
...@@ -91,4 +91,4 @@ module Ci ...@@ -91,4 +91,4 @@ module Ci
end end
end end
Ci::RetryBuildService.prepend_mod_with('Ci::RetryBuildService') Ci::RetryJobService.prepend_mod_with('Ci::RetryJobService')
...@@ -13,7 +13,7 @@ module Ci ...@@ -13,7 +13,7 @@ module Ci
builds_relation(pipeline).find_each do |build| builds_relation(pipeline).find_each do |build|
next unless can_be_retried?(build) next unless can_be_retried?(build)
Ci::RetryBuildService.new(project, current_user).clone!(build) Ci::RetryJobService.new(project, current_user).clone!(build)
end end
pipeline.processables.latest.skipped.find_each do |skipped| pipeline.processables.latest.skipped.find_each do |skipped|
......
...@@ -58,6 +58,19 @@ module EE ...@@ -58,6 +58,19 @@ module EE
end end
end end
class_methods do
extend ::Gitlab::Utils::Override
override :clone_accessors
def clone_accessors
(super + extra_accessors).freeze
end
def extra_accessors
(super + %i[secrets]).freeze
end
end
override :variables override :variables
def variables def variables
strong_memoize(:variables) do strong_memoize(:variables) do
......
...@@ -9,8 +9,13 @@ module Deployments ...@@ -9,8 +9,13 @@ module Deployments
deployment = find_rollback_target(environment) deployment = find_rollback_target(environment)
return error('Failed to find a rollback target.') unless deployment return error('Failed to find a rollback target.') unless deployment
new_deployment = rollback_to(deployment) response = rollback_to(deployment)
success(deployment: new_deployment)
if response.success?
success(deployment: response[:job].deployment)
else
error(response.message)
end
end end
private private
...@@ -48,7 +53,7 @@ module Deployments ...@@ -48,7 +53,7 @@ module Deployments
end end
def rollback_to(deployment) def rollback_to(deployment)
Ci::Build.retry(deployment.deployable, deployment.deployed_by).deployment Ci::RetryJobService.new(deployment.deployable.project, deployment.deployed_by).execute(deployment.deployable)
end end
end end
end end
...@@ -2,24 +2,10 @@ ...@@ -2,24 +2,10 @@
module EE module EE
module Ci module Ci
module RetryBuildService module RetryJobService
extend ActiveSupport::Concern extend ActiveSupport::Concern
extend ::Gitlab::Utils::Override extend ::Gitlab::Utils::Override
class_methods do
extend ::Gitlab::Utils::Override
override :clone_accessors
def clone_accessors
(super + extra_accessors).freeze
end
override :extra_accessors
def extra_accessors
%i[secrets].freeze
end
end
private private
override :check_access! override :check_access!
......
...@@ -40,6 +40,12 @@ RSpec.describe Ci::Build, :saas do ...@@ -40,6 +40,12 @@ RSpec.describe Ci::Build, :saas do
end end
end end
describe 'extra_accessors' do
it 'includes the cloneable extra accessors' do
expect(::Ci::Build.extra_accessors).to eq([:secrets])
end
end
describe 'associations' do describe 'associations' do
it { is_expected.to have_many(:security_scans).class_name('Security::Scan') } it { is_expected.to have_many(:security_scans).class_name('Security::Scan') }
it { is_expected.to have_one(:dast_site_profiles_build).class_name('Dast::SiteProfilesBuild').with_foreign_key(:ci_build_id) } it { is_expected.to have_one(:dast_site_profiles_build).class_name('Dast::SiteProfilesBuild').with_foreign_key(:ci_build_id) }
......
...@@ -3,7 +3,9 @@ ...@@ -3,7 +3,9 @@
require 'spec_helper' require 'spec_helper'
RSpec.describe Ci::PlayBuildService, '#execute' do RSpec.describe Ci::PlayBuildService, '#execute' do
it_behaves_like 'restricts access to protected environments' it_behaves_like 'restricts access to protected environments' do
subject { service.execute(build) }
end
it_behaves_like 'prevents playing job when credit card is required' do it_behaves_like 'prevents playing job when credit card is required' do
let(:user) { create(:user, maintainer_projects: [project]) } let(:user) { create(:user, maintainer_projects: [project]) }
......
# frozen_string_literal: true # frozen_string_literal: true
require 'spec_helper' require 'spec_helper'
RSpec.describe Ci::RetryBuildService do RSpec.describe Ci::RetryJobService do
let_it_be(:user) { create(:user) } let_it_be(:user) { create(:user) }
let(:pipeline) { create(:ci_pipeline, project: project) } let(:pipeline) { create(:ci_pipeline, project: project) }
let(:build) { create(:ci_build, pipeline: pipeline) } let(:build) { create(:ci_build, :success, pipeline: pipeline) }
subject(:service) { described_class.new(project, user) } subject(:service) { described_class.new(project, user) }
...@@ -15,14 +15,16 @@ RSpec.describe Ci::RetryBuildService do ...@@ -15,14 +15,16 @@ RSpec.describe Ci::RetryBuildService do
project.add_developer(user) project.add_developer(user)
end end
it_behaves_like 'restricts access to protected environments' it_behaves_like 'restricts access to protected environments' do
subject { service.execute(build)[:job] }
end
describe '#clone!' do describe '#clone!' do
context 'when user has ability to execute build' do context 'when user has ability to execute build' do
let_it_be(:namespace) { create(:namespace) } let_it_be(:namespace) { create(:namespace) }
let_it_be(:project) { create(:project, namespace: namespace, creator: user) } let_it_be(:project) { create(:project, namespace: namespace, creator: user) }
let(:new_build) { service.clone!(build)} let(:new_build) { service.clone!(build) }
context 'dast' do context 'dast' do
let_it_be(:dast_site_profile) { create(:dast_site_profile, project: project) } let_it_be(:dast_site_profile) { create(:dast_site_profile, project: project) }
...@@ -127,7 +129,7 @@ RSpec.describe Ci::RetryBuildService do ...@@ -127,7 +129,7 @@ RSpec.describe Ci::RetryBuildService do
end end
describe '#execute' do describe '#execute' do
let(:new_build) { service.execute(build) } let(:new_build) { service.execute(build)[:job] }
context 'when the CI quota is exceeded' do context 'when the CI quota is exceeded' do
let_it_be(:namespace) { create(:namespace, :with_used_build_minutes_limit) } let_it_be(:namespace) { create(:namespace, :with_used_build_minutes_limit) }
......
...@@ -30,13 +30,25 @@ RSpec.describe Deployments::AutoRollbackService, :clean_gitlab_redis_rate_limiti ...@@ -30,13 +30,25 @@ RSpec.describe Deployments::AutoRollbackService, :clean_gitlab_redis_rate_limiti
commits.reverse_each { |commit| create_deployment(commit.id) } commits.reverse_each { |commit| create_deployment(commit.id) }
end end
it 'successfully roll back a deployment' do it 'successfully rolls back a deployment' do
expect { subject }.to change { Deployment.count }.by(1) expect { subject }.to change { Deployment.count }.by(1)
expect(subject[:status]).to eq(:success) expect(subject[:status]).to eq(:success)
expect(subject[:deployment].sha).to eq(commits[1].id) expect(subject[:deployment].sha).to eq(commits[1].id)
end end
context 'when RetryJobService fails to retry the deployable' do
before do
allow_next_instance_of(::Ci::RetryJobService) do |service|
allow(service).to receive(:execute).and_return(ServiceResponse.error(message: message))
end
end
it_behaves_like 'rollback failure' do
let(:message) { 'Job cannot be retried.' }
end
end
context 'when auto_rollback checkbox is disabled on the project' do context 'when auto_rollback checkbox is disabled on the project' do
before do before do
environment.project.auto_rollback_enabled = false environment.project.auto_rollback_enabled = false
......
...@@ -6,7 +6,7 @@ RSpec.shared_examples 'restricts access to protected environments' do |developer ...@@ -6,7 +6,7 @@ RSpec.shared_examples 'restricts access to protected environments' do |developer
let(:project) { create(:project, :repository) } let(:project) { create(:project, :repository) }
let(:pipeline) { create(:ci_pipeline, project: project) } let(:pipeline) { create(:ci_pipeline, project: project) }
let(:environment) { create(:environment, project: project, name: 'production') } let(:environment) { create(:environment, project: project, name: 'production') }
let(:build) { create(:ci_build, :created, pipeline: pipeline, environment: environment.name, project: project) } let(:build) { create(:ci_build, :success, pipeline: pipeline, environment: environment.name, project: project) }
let(:protected_environment) { create(:protected_environment, name: environment.name, project: project) } let(:protected_environment) { create(:protected_environment, name: environment.name, project: project) }
let(:service) { described_class.new(project, user) } let(:service) { described_class.new(project, user) }
...@@ -19,8 +19,7 @@ RSpec.shared_examples 'restricts access to protected environments' do |developer ...@@ -19,8 +19,7 @@ RSpec.shared_examples 'restricts access to protected environments' do |developer
context 'when user does not have access to the environment' do context 'when user does not have access to the environment' do
it 'raises Gitlab::Access::DeniedError' do it 'raises Gitlab::Access::DeniedError' do
expect { service.execute(build) } expect { subject }.to raise_error Gitlab::Access::AccessDeniedError
.to raise_error Gitlab::Access::AccessDeniedError
end end
end end
...@@ -30,9 +29,7 @@ RSpec.shared_examples 'restricts access to protected environments' do |developer ...@@ -30,9 +29,7 @@ RSpec.shared_examples 'restricts access to protected environments' do |developer
end end
it 'enqueues the build' do it 'enqueues the build' do
build_enqueued = service.execute(build) is_expected.to be_pending
expect(build_enqueued).to be_pending
end end
end end
end end
......
...@@ -114,11 +114,14 @@ module API ...@@ -114,11 +114,14 @@ module API
build = find_build!(params[:job_id]) build = find_build!(params[:job_id])
authorize!(:update_build, build) authorize!(:update_build, build)
break forbidden!('Job is not retryable') unless build.retryable?
build = ::Ci::Build.retry(build, current_user) response = ::Ci::RetryJobService.new(@project, current_user).execute(build)
present build, with: Entities::Ci::Job if response.success?
present response[:job], with: Entities::Ci::Job
else
forbidden!('Job is not retryable')
end
end end
desc 'Erase job (remove artifacts and the trace)' do desc 'Erase job (remove artifacts and the trace)' do
......
...@@ -796,7 +796,7 @@ RSpec.describe Projects::JobsController, :clean_gitlab_redis_shared_state do ...@@ -796,7 +796,7 @@ RSpec.describe Projects::JobsController, :clean_gitlab_redis_shared_state do
retried_build = Ci::Build.last retried_build = Ci::Build.last
Ci::RetryBuildService.clone_accessors.each do |accessor| Ci::Build.clone_accessors.each do |accessor|
expect(job.read_attribute(accessor)) expect(job.read_attribute(accessor))
.to eq(retried_build.read_attribute(accessor)), .to eq(retried_build.read_attribute(accessor)),
"Mismatched attribute on \"#{accessor}\". " \ "Mismatched attribute on \"#{accessor}\". " \
......
...@@ -30,6 +30,12 @@ RSpec.describe Ci::Bridge do ...@@ -30,6 +30,12 @@ RSpec.describe Ci::Bridge do
expect(bridge).to have_one(:downstream_pipeline) expect(bridge).to have_one(:downstream_pipeline)
end end
describe '#retryable?' do
it 'returns false' do
expect(bridge.retryable?).to eq(false)
end
end
describe '#tags' do describe '#tags' do
it 'only has a bridge tag' do it 'only has a bridge tag' do
expect(bridge.tags).to eq [:bridge] expect(bridge.tags).to eq [:bridge]
......
...@@ -14,7 +14,7 @@ RSpec.describe Ci::BuildDependencies do ...@@ -14,7 +14,7 @@ RSpec.describe Ci::BuildDependencies do
end end
let!(:build) { create(:ci_build, pipeline: pipeline, name: 'build', stage_idx: 0, stage: 'build') } let!(:build) { create(:ci_build, pipeline: pipeline, name: 'build', stage_idx: 0, stage: 'build') }
let!(:rspec_test) { create(:ci_build, pipeline: pipeline, name: 'rspec', stage_idx: 1, stage: 'test') } let!(:rspec_test) { create(:ci_build, :success, pipeline: pipeline, name: 'rspec', stage_idx: 1, stage: 'test') }
let!(:rubocop_test) { create(:ci_build, pipeline: pipeline, name: 'rubocop', stage_idx: 1, stage: 'test') } let!(:rubocop_test) { create(:ci_build, pipeline: pipeline, name: 'rubocop', stage_idx: 1, stage: 'test') }
let!(:staging) { create(:ci_build, pipeline: pipeline, name: 'staging', stage_idx: 2, stage: 'deploy') } let!(:staging) { create(:ci_build, pipeline: pipeline, name: 'staging', stage_idx: 2, stage: 'deploy') }
...@@ -48,7 +48,7 @@ RSpec.describe Ci::BuildDependencies do ...@@ -48,7 +48,7 @@ RSpec.describe Ci::BuildDependencies do
project.add_developer(user) project.add_developer(user)
end end
let!(:retried_job) { Ci::Build.retry(rspec_test, user) } let!(:retried_job) { Ci::RetryJobService.new(rspec_test.project, user).execute(rspec_test)[:job] }
it 'contains the retried job instead of the original one' do it 'contains the retried job instead of the original one' do
is_expected.to contain_exactly(build, retried_job, rubocop_test) is_expected.to contain_exactly(build, retried_job, rubocop_test)
......
...@@ -1951,90 +1951,6 @@ RSpec.describe Ci::Build do ...@@ -1951,90 +1951,6 @@ RSpec.describe Ci::Build do
end end
end end
describe '#retryable?' do
subject { build }
context 'when build is retryable' do
context 'when build is successful' do
before do
build.success!
end
it { is_expected.to be_retryable }
end
context 'when build is failed' do
before do
build.drop!
end
it { is_expected.to be_retryable }
end
context 'when build is canceled' do
before do
build.cancel!
end
it { is_expected.to be_retryable }
end
end
context 'when build is not retryable' do
context 'when build is running' do
before do
build.run!
end
it { is_expected.not_to be_retryable }
end
context 'when build is skipped' do
before do
build.skip!
end
it { is_expected.not_to be_retryable }
end
context 'when build is degenerated' do
before do
build.degenerate!
end
it { is_expected.not_to be_retryable }
end
context 'when a canceled build has been retried already' do
before do
project.add_developer(user)
build.cancel!
described_class.retry(build, user)
end
it { is_expected.not_to be_retryable }
end
context 'when deployment is rejected' do
before do
build.drop!(:deployment_rejected)
end
it { is_expected.not_to be_retryable }
end
context 'when build is waiting for deployment approval' do
subject { build_stubbed(:ci_build, :manual, environment: 'production') }
before do
create(:deployment, :blocked, deployable: subject)
end
it { is_expected.not_to be_retryable }
end
end
end
describe '#action?' do describe '#action?' do
before do before do
build.update!(when: value) build.update!(when: value)
...@@ -2358,24 +2274,12 @@ RSpec.describe Ci::Build do ...@@ -2358,24 +2274,12 @@ RSpec.describe Ci::Build do
end end
context 'when build is retried' do context 'when build is retried' do
let!(:new_build) { described_class.retry(build, user) } let!(:new_build) { Ci::RetryJobService.new(project, user).execute(build)[:job] }
it 'does not return any of them' do it 'does not return any of them' do
is_expected.not_to include(build, new_build) is_expected.not_to include(build, new_build)
end end
end end
context 'when other build is retried' do
let!(:retried_build) { described_class.retry(other_build, user) }
before do
retried_build.success
end
it 'returns a retried build' do
is_expected.to contain_exactly(retried_build)
end
end
end end
describe '#other_scheduled_actions' do describe '#other_scheduled_actions' do
...@@ -3962,8 +3866,9 @@ RSpec.describe Ci::Build do ...@@ -3962,8 +3866,9 @@ RSpec.describe Ci::Build do
subject { create(:ci_build, :running, options: { script: ["ls -al"], retry: 3 }, project: project, user: user) } subject { create(:ci_build, :running, options: { script: ["ls -al"], retry: 3 }, project: project, user: user) }
it 'retries build and assigns the same user to it' do it 'retries build and assigns the same user to it' do
expect(described_class).to receive(:retry) expect_next_instance_of(::Ci::RetryJobService) do |service|
.with(subject, user) expect(service).to receive(:execute).with(subject)
end
subject.drop! subject.drop!
end end
...@@ -3977,10 +3882,10 @@ RSpec.describe Ci::Build do ...@@ -3977,10 +3882,10 @@ RSpec.describe Ci::Build do
end end
context 'when retry service raises Gitlab::Access::AccessDeniedError exception' do context 'when retry service raises Gitlab::Access::AccessDeniedError exception' do
let(:retry_service) { Ci::RetryBuildService.new(subject.project, subject.user) } let(:retry_service) { Ci::RetryJobService.new(subject.project, subject.user) }
before do before do
allow_any_instance_of(Ci::RetryBuildService) allow_any_instance_of(Ci::RetryJobService)
.to receive(:execute) .to receive(:execute)
.with(subject) .with(subject)
.and_raise(Gitlab::Access::AccessDeniedError) .and_raise(Gitlab::Access::AccessDeniedError)
......
...@@ -1663,7 +1663,7 @@ RSpec.describe Ci::Pipeline, :mailer, factory_default: :keep do ...@@ -1663,7 +1663,7 @@ RSpec.describe Ci::Pipeline, :mailer, factory_default: :keep do
expect(upstream_pipeline.reload).to be_failed expect(upstream_pipeline.reload).to be_failed
Sidekiq::Testing.inline! do Sidekiq::Testing.inline! do
new_job = Ci::Build.retry(job, project.users.first) new_job = Ci::RetryJobService.new(project, project.users.first).execute(job)[:job]
expect(downstream_pipeline.reload).to be_running expect(downstream_pipeline.reload).to be_running
expect(upstream_pipeline.reload).to be_running expect(upstream_pipeline.reload).to be_running
...@@ -1684,7 +1684,7 @@ RSpec.describe Ci::Pipeline, :mailer, factory_default: :keep do ...@@ -1684,7 +1684,7 @@ RSpec.describe Ci::Pipeline, :mailer, factory_default: :keep do
expect(upstream_pipeline.reload).to be_success expect(upstream_pipeline.reload).to be_success
Sidekiq::Testing.inline! do Sidekiq::Testing.inline! do
new_job = Ci::Build.retry(job, project.users.first) new_job = Ci::RetryJobService.new(project, project.users.first).execute(job)[:job]
expect(downstream_pipeline.reload).to be_running expect(downstream_pipeline.reload).to be_running
expect(upstream_pipeline.reload).to be_running expect(upstream_pipeline.reload).to be_running
...@@ -1715,7 +1715,7 @@ RSpec.describe Ci::Pipeline, :mailer, factory_default: :keep do ...@@ -1715,7 +1715,7 @@ RSpec.describe Ci::Pipeline, :mailer, factory_default: :keep do
expect(upstream_of_upstream_pipeline.reload).to be_failed expect(upstream_of_upstream_pipeline.reload).to be_failed
Sidekiq::Testing.inline! do Sidekiq::Testing.inline! do
new_job = Ci::Build.retry(job, project.users.first) new_job = Ci::RetryJobService.new(project, project.users.first).execute(job)[:job]
expect(downstream_pipeline.reload).to be_running expect(downstream_pipeline.reload).to be_running
expect(upstream_pipeline.reload).to be_running expect(upstream_pipeline.reload).to be_running
...@@ -2583,8 +2583,7 @@ RSpec.describe Ci::Pipeline, :mailer, factory_default: :keep do ...@@ -2583,8 +2583,7 @@ RSpec.describe Ci::Pipeline, :mailer, factory_default: :keep do
build.drop build.drop
project.add_developer(user) project.add_developer(user)
::Ci::RetryJobService.new(project, user).execute(build)[:job]
Ci::Build.retry(build, user)
end end
# We are changing a state: created > failed > running # We are changing a state: created > failed > running
...@@ -4688,7 +4687,7 @@ RSpec.describe Ci::Pipeline, :mailer, factory_default: :keep do ...@@ -4688,7 +4687,7 @@ RSpec.describe Ci::Pipeline, :mailer, factory_default: :keep do
project.add_developer(user) project.add_developer(user)
retried_build.cancel! retried_build.cancel!
::Ci::Build.retry(retried_build, user) Ci::RetryJobService.new(project, user).execute(retried_build)[:job]
end end
it 'does not include retried builds' do it 'does not include retried builds' do
......
...@@ -14,6 +14,100 @@ RSpec.describe Ci::Processable do ...@@ -14,6 +14,100 @@ RSpec.describe Ci::Processable do
it { is_expected.to delegate_method(:legacy_detached_merge_request_pipeline?).to(:pipeline) } it { is_expected.to delegate_method(:legacy_detached_merge_request_pipeline?).to(:pipeline) }
end end
describe '#retryable' do
shared_examples_for 'retryable processable' do
context 'when processable is successful' do
before do
processable.success!
end
it { is_expected.to be_retryable }
end
context 'when processable is failed' do
before do
processable.drop!
end
it { is_expected.to be_retryable }
end
context 'when processable is canceled' do
before do
processable.cancel!
end
it { is_expected.to be_retryable }
end
end
shared_examples_for 'non-retryable processable' do
context 'when processable is skipped' do
before do
processable.skip!
end
it { is_expected.not_to be_retryable }
end
context 'when processable is degenerated' do
before do
processable.degenerate!
end
it { is_expected.not_to be_retryable }
end
context 'when a canceled processable has been retried already' do
before do
project.add_developer(create(:user))
processable.cancel!
processable.update!(retried: true)
end
it { is_expected.not_to be_retryable }
end
end
context 'when the processable is a build' do
subject(:processable) { create(:ci_build, pipeline: pipeline) }
context 'when the processable is retryable' do
it_behaves_like 'retryable processable'
context 'when deployment is rejected' do
before do
processable.drop!(:deployment_rejected)
end
it { is_expected.not_to be_retryable }
end
context 'when build is waiting for deployment approval' do
subject { build_stubbed(:ci_build, :manual, environment: 'production') }
before do
create(:deployment, :blocked, deployable: subject)
end
it { is_expected.not_to be_retryable }
end
end
context 'when the processable is non-retryable' do
it_behaves_like 'non-retryable processable'
context 'when processable is running' do
before do
processable.run!
end
it { is_expected.not_to be_retryable }
end
end
end
end
describe '#aggregated_needs_names' do describe '#aggregated_needs_names' do
let(:with_aggregated_needs) { pipeline.processables.select_with_aggregated_needs(project) } let(:with_aggregated_needs) { pipeline.processables.select_with_aggregated_needs(project) }
......
...@@ -623,6 +623,15 @@ RSpec.describe API::Ci::Jobs do ...@@ -623,6 +623,15 @@ RSpec.describe API::Ci::Jobs do
end end
end end
context 'when a build is not retryable' do
let(:job) { create(:ci_build, :created, pipeline: pipeline) }
it 'responds with unprocessable entity' do
expect(json_response['message']).to eq('403 Forbidden - Job is not retryable')
expect(response).to have_gitlab_http_status(:forbidden)
end
end
context 'user without :update_build permission' do context 'user without :update_build permission' do
let(:api_user) { reporter } let(:api_user) { reporter }
......
...@@ -8,7 +8,8 @@ RSpec.describe 'JobRetry' do ...@@ -8,7 +8,8 @@ RSpec.describe 'JobRetry' do
let_it_be(:user) { create(:user) } let_it_be(:user) { create(:user) }
let_it_be(:project) { create(:project) } let_it_be(:project) { create(:project) }
let_it_be(:pipeline) { create(:ci_pipeline, project: project, user: user) } let_it_be(:pipeline) { create(:ci_pipeline, project: project, user: user) }
let_it_be(:job) { create(:ci_build, :success, pipeline: pipeline, name: 'build') }
let(:job) { create(:ci_build, :success, pipeline: pipeline, name: 'build') }
let(:mutation) do let(:mutation) do
variables = { variables = {
...@@ -37,10 +38,23 @@ RSpec.describe 'JobRetry' do ...@@ -37,10 +38,23 @@ RSpec.describe 'JobRetry' do
end end
it 'retries a job' do it 'retries a job' do
job_id = ::Gitlab::GlobalId.build(job, id: job.id).to_s
post_graphql_mutation(mutation, current_user: user) post_graphql_mutation(mutation, current_user: user)
expect(response).to have_gitlab_http_status(:success) expect(response).to have_gitlab_http_status(:success)
expect(mutation_response['job']['id']).to eq(job_id) new_job_id = GitlabSchema.object_from_id(mutation_response['job']['id']).sync.id
new_job = ::Ci::Build.find(new_job_id)
expect(new_job).not_to be_retried
end
context 'when the job is not retryable' do
let(:job) { create(:ci_build, :retried, pipeline: pipeline) }
it 'returns an error' do
post_graphql_mutation(mutation, current_user: user)
expect(mutation_response['job']).to be(nil)
expect(mutation_response['errors']).to match_array(['Job cannot be retried'])
end
end end
end end
...@@ -85,7 +85,7 @@ RSpec.describe Ci::AfterRequeueJobService, :sidekiq_inline do ...@@ -85,7 +85,7 @@ RSpec.describe Ci::AfterRequeueJobService, :sidekiq_inline do
c2: 'skipped' c2: 'skipped'
) )
new_a1 = Ci::RetryBuildService.new(project, user).clone!(a1) new_a1 = Ci::RetryJobService.new(project, user).clone!(a1)
new_a1.enqueue! new_a1.enqueue!
check_jobs_statuses( check_jobs_statuses(
a1: 'pending', a1: 'pending',
...@@ -172,7 +172,7 @@ RSpec.describe Ci::AfterRequeueJobService, :sidekiq_inline do ...@@ -172,7 +172,7 @@ RSpec.describe Ci::AfterRequeueJobService, :sidekiq_inline do
c2: 'skipped' c2: 'skipped'
) )
new_a1 = Ci::RetryBuildService.new(project, user).clone!(a1) new_a1 = Ci::RetryJobService.new(project, user).clone!(a1)
new_a1.enqueue! new_a1.enqueue!
check_jobs_statuses( check_jobs_statuses(
a1: 'pending', a1: 'pending',
......
...@@ -725,7 +725,7 @@ RSpec.describe Ci::PipelineProcessing::AtomicProcessingService do ...@@ -725,7 +725,7 @@ RSpec.describe Ci::PipelineProcessing::AtomicProcessingService do
expect(builds_names).to eq ['build:1', 'build:2', 'test:1', 'test:2'] expect(builds_names).to eq ['build:1', 'build:2', 'test:1', 'test:2']
Ci::Build.retry(pipeline.builds.find_by(name: 'test:2'), user).reset.success! Ci::RetryJobService.new(pipeline.project, user).execute(pipeline.builds.find_by(name: 'test:2'))[:job].reset.success!
expect(builds_names).to eq ['build:1', 'build:2', 'test:1', 'test:2', expect(builds_names).to eq ['build:1', 'build:2', 'test:1', 'test:2',
'test:2', 'deploy:1', 'deploy:2'] 'test:2', 'deploy:1', 'deploy:2']
...@@ -1111,11 +1111,11 @@ RSpec.describe Ci::PipelineProcessing::AtomicProcessingService do ...@@ -1111,11 +1111,11 @@ RSpec.describe Ci::PipelineProcessing::AtomicProcessingService do
end end
def enqueue_scheduled(name) def enqueue_scheduled(name)
builds.scheduled.find_by(name: name).enqueue_scheduled builds.scheduled.find_by(name: name).enqueue!
end end
def retry_build(name) def retry_build(name)
Ci::Build.retry(builds.find_by(name: name), user) Ci::RetryJobService.new(project, user).execute(builds.find_by(name: name))
end end
def manual_actions def manual_actions
......
...@@ -2,7 +2,7 @@ ...@@ -2,7 +2,7 @@
require 'spec_helper' require 'spec_helper'
RSpec.describe Ci::RetryBuildService do RSpec.describe Ci::RetryJobService do
let_it_be(:reporter) { create(:user) } let_it_be(:reporter) { create(:user) }
let_it_be(:developer) { create(:user) } let_it_be(:developer) { create(:user) }
let_it_be(:project) { create(:project, :repository) } let_it_be(:project) { create(:project, :repository) }
...@@ -17,7 +17,7 @@ RSpec.describe Ci::RetryBuildService do ...@@ -17,7 +17,7 @@ RSpec.describe Ci::RetryBuildService do
name: 'test') name: 'test')
end end
let_it_be_with_refind(:build) { create(:ci_build, pipeline: pipeline, stage_id: stage.id) } let_it_be_with_refind(:build) { create(:ci_build, :success, pipeline: pipeline, stage_id: stage.id) }
let(:user) { developer } let(:user) { developer }
...@@ -30,7 +30,7 @@ RSpec.describe Ci::RetryBuildService do ...@@ -30,7 +30,7 @@ RSpec.describe Ci::RetryBuildService do
project.add_reporter(reporter) project.add_reporter(reporter)
end end
clone_accessors = described_class.clone_accessors.without(described_class.extra_accessors) clone_accessors = ::Ci::Build.clone_accessors.without(::Ci::Build.extra_accessors)
reject_accessors = reject_accessors =
%i[id status user token token_encrypted coverage trace runner %i[id status user token token_encrypted coverage trace runner
...@@ -94,6 +94,10 @@ RSpec.describe Ci::RetryBuildService do ...@@ -94,6 +94,10 @@ RSpec.describe Ci::RetryBuildService do
create(:terraform_state_version, build: build) create(:terraform_state_version, build: build)
end end
before do
build.update!(retried: false, status: :success)
end
describe 'clone accessors' do describe 'clone accessors' do
let(:forbidden_associations) do let(:forbidden_associations) do
Ci::Build.reflect_on_all_associations.each_with_object(Set.new) do |assoc, memo| Ci::Build.reflect_on_all_associations.each_with_object(Set.new) do |assoc, memo|
...@@ -156,8 +160,8 @@ RSpec.describe Ci::RetryBuildService do ...@@ -156,8 +160,8 @@ RSpec.describe Ci::RetryBuildService do
Ci::Build.attribute_aliases.keys.map(&:to_sym) + Ci::Build.attribute_aliases.keys.map(&:to_sym) +
Ci::Build.reflect_on_all_associations.map(&:name) + Ci::Build.reflect_on_all_associations.map(&:name) +
[:tag_list, :needs_attributes, :job_variables_attributes] - [:tag_list, :needs_attributes, :job_variables_attributes] -
# ee-specific accessors should be tested in ee/spec/services/ci/retry_build_service_spec.rb instead # ee-specific accessors should be tested in ee/spec/services/ci/retry_job_service_spec.rb instead
described_class.extra_accessors - Ci::Build.extra_accessors -
[:dast_site_profiles_build, :dast_scanner_profiles_build] # join tables [:dast_site_profiles_build, :dast_scanner_profiles_build] # join tables
current_accessors.uniq! current_accessors.uniq!
...@@ -170,7 +174,7 @@ RSpec.describe Ci::RetryBuildService do ...@@ -170,7 +174,7 @@ RSpec.describe Ci::RetryBuildService do
describe '#execute' do describe '#execute' do
let(:new_build) do let(:new_build) do
travel_to(1.second.from_now) do travel_to(1.second.from_now) do
service.execute(build) service.execute(build)[:job]
end end
end end
...@@ -248,7 +252,7 @@ RSpec.describe Ci::RetryBuildService do ...@@ -248,7 +252,7 @@ RSpec.describe Ci::RetryBuildService do
context 'when build has scheduling_type' do context 'when build has scheduling_type' do
it 'does not call populate_scheduling_type!' do it 'does not call populate_scheduling_type!' do
expect_any_instance_of(Ci::Pipeline).not_to receive(:ensure_scheduling_type!) expect_any_instance_of(Ci::Pipeline).not_to receive(:ensure_scheduling_type!) # rubocop: disable RSpec/AnyInstanceOf
expect(new_build.scheduling_type).to eq('stage') expect(new_build.scheduling_type).to eq('stage')
end end
...@@ -286,6 +290,18 @@ RSpec.describe Ci::RetryBuildService do ...@@ -286,6 +290,18 @@ RSpec.describe Ci::RetryBuildService do
expect { service.execute(build) } expect { service.execute(build) }
.to raise_error Gitlab::Access::AccessDeniedError .to raise_error Gitlab::Access::AccessDeniedError
end end
context 'when the job is not retryable' do
let(:build) { create(:ci_build, :created, pipeline: pipeline) }
it 'returns a ServiceResponse error' do
response = service.execute(build)
expect(response).to be_a(ServiceResponse)
expect(response).to be_error
expect(response.message).to eq("Job cannot be retried")
end
end
end end
end end
......
...@@ -340,7 +340,7 @@ RSpec.describe Ci::RetryPipelineService, '#execute' do ...@@ -340,7 +340,7 @@ RSpec.describe Ci::RetryPipelineService, '#execute' do
context 'when user is not allowed to retry build' do context 'when user is not allowed to retry build' do
before do before do
build = create(:ci_build, pipeline: pipeline, status: :failed) build = create(:ci_build, pipeline: pipeline, status: :failed)
allow_next_instance_of(Ci::RetryBuildService) do |service| allow_next_instance_of(Ci::RetryJobService) do |service|
allow(service).to receive(:can?).with(user, :update_build, build).and_return(false) allow(service).to receive(:can?).with(user, :update_build, build).and_return(false)
end end
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