diff --git a/app/models/ci/pipeline.rb b/app/models/ci/pipeline.rb index 3fee6c18770..4294a10e9e3 100644 --- a/app/models/ci/pipeline.rb +++ b/app/models/ci/pipeline.rb @@ -161,23 +161,27 @@ module Ci end def retryable? - builds.latest.any? do |build| - (build.failed? || build.canceled?) && build.retryable? - end + builds.latest.failed_or_canceled.any?(&:retryable?) end def cancelable? - builds.running_or_pending.any? + statuses.cancelable.any? end def cancel_running - builds.running_or_pending.each(&:cancel) + Gitlab::OptimisticLocking.retry_lock( + statuses.cancelable) do |cancelable| + cancelable.each(&:cancel) + end end def retry_failed(user) - builds.latest.failed.select(&:retryable?).each do |build| - Ci::Build.retry(build, user) - end + Gitlab::OptimisticLocking.retry_lock( + builds.latest.failed_or_canceled) do |failed_or_canceled| + failed_or_canceled.select(&:retryable?).each do |build| + Ci::Build.retry(build, user) + end + end end def mark_as_processable_after_stage(stage_idx) diff --git a/app/models/concerns/has_status.rb b/app/models/concerns/has_status.rb index ef3e73a4072..2f5aa91a964 100644 --- a/app/models/concerns/has_status.rb +++ b/app/models/concerns/has_status.rb @@ -73,6 +73,11 @@ module HasStatus scope :skipped, -> { where(status: 'skipped') } 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]) + end end def started? diff --git a/changelogs/unreleased/fix-cancelling-pipelines.yml b/changelogs/unreleased/fix-cancelling-pipelines.yml new file mode 100644 index 00000000000..c21e663093a --- /dev/null +++ b/changelogs/unreleased/fix-cancelling-pipelines.yml @@ -0,0 +1,4 @@ +--- +title: Fix cancelling created or external pipelines +merge_request: 7508 +author: diff --git a/spec/factories/ci/builds.rb b/spec/factories/ci/builds.rb index eb20bd7dd58..c443af09075 100644 --- a/spec/factories/ci/builds.rb +++ b/spec/factories/ci/builds.rb @@ -38,6 +38,10 @@ FactoryGirl.define do status 'canceled' end + trait :skipped do + status 'skipped' + end + trait :running do status 'running' end diff --git a/spec/factories/commit_statuses.rb b/spec/factories/commit_statuses.rb index 995f2080f10..756b341ecba 100644 --- a/spec/factories/commit_statuses.rb +++ b/spec/factories/commit_statuses.rb @@ -19,6 +19,10 @@ FactoryGirl.define do status 'canceled' end + trait :skipped do + status 'skipped' + end + trait :running do status 'running' end diff --git a/spec/features/projects/pipelines_spec.rb b/spec/features/projects/pipelines_spec.rb index 002c6f6b359..10e5466fc85 100644 --- a/spec/features/projects/pipelines_spec.rb +++ b/spec/features/projects/pipelines_spec.rb @@ -90,13 +90,20 @@ describe "Pipelines" do visit namespace_project_pipelines_path(project.namespace, project) end - it 'is not cancelable' do - expect(page).not_to have_link('Cancel') + it 'is cancelable' do + expect(page).to have_link('Cancel') end it 'has pipeline running' do expect(page).to have_selector('.ci-running') end + + context 'when canceling' do + before { click_link('Cancel') } + + it { expect(page).not_to have_link('Cancel') } + it { expect(page).to have_selector('.ci-canceled') } + end end context 'when failed' do diff --git a/spec/models/ci/pipeline_spec.rb b/spec/models/ci/pipeline_spec.rb index ea022e03608..3b12f16b4db 100644 --- a/spec/models/ci/pipeline_spec.rb +++ b/spec/models/ci/pipeline_spec.rb @@ -402,6 +402,160 @@ describe Ci::Pipeline, models: true do end end + describe '#cancelable?' do + %i[created running pending].each do |status0| + context "when there is a build #{status0}" do + before do + create(:ci_build, status0, pipeline: pipeline) + end + + it 'is cancelable' do + expect(pipeline.cancelable?).to be_truthy + end + end + + context "when there is an external job #{status0}" do + before do + create(:generic_commit_status, status0, pipeline: pipeline) + end + + it 'is cancelable' do + expect(pipeline.cancelable?).to be_truthy + end + end + + %i[success failed canceled].each do |status1| + context "when there are generic_commit_status jobs for #{status0} and #{status1}" do + before do + create(:generic_commit_status, status0, pipeline: pipeline) + create(:generic_commit_status, status1, pipeline: pipeline) + end + + it 'is cancelable' do + expect(pipeline.cancelable?).to be_truthy + end + end + + context "when there are generic_commit_status and ci_build jobs for #{status0} and #{status1}" do + before do + create(:generic_commit_status, status0, pipeline: pipeline) + create(:ci_build, status1, pipeline: pipeline) + end + + it 'is cancelable' do + expect(pipeline.cancelable?).to be_truthy + end + end + + context "when there are ci_build jobs for #{status0} and #{status1}" do + before do + create(:ci_build, status0, pipeline: pipeline) + create(:ci_build, status1, pipeline: pipeline) + end + + it 'is cancelable' do + expect(pipeline.cancelable?).to be_truthy + end + end + end + end + + %i[success failed canceled].each do |status| + context "when there is a build #{status}" do + before do + create(:ci_build, status, pipeline: pipeline) + end + + it 'is not cancelable' do + expect(pipeline.cancelable?).to be_falsey + end + end + + context "when there is an external job #{status}" do + before do + create(:generic_commit_status, status, pipeline: pipeline) + end + + it 'is not cancelable' do + expect(pipeline.cancelable?).to be_falsey + end + end + end + end + + describe '#cancel_running' do + let(:latest_status) { pipeline.statuses.pluck(:status) } + + context 'when there is a running external job and created build' do + before do + create(:ci_build, :running, pipeline: pipeline) + create(:generic_commit_status, :running, pipeline: pipeline) + + pipeline.cancel_running + end + + it 'cancels both jobs' do + expect(latest_status).to contain_exactly('canceled', 'canceled') + end + end + + context 'when builds are in different stages' do + before do + create(:ci_build, :running, stage_idx: 0, pipeline: pipeline) + create(:ci_build, :running, stage_idx: 1, pipeline: pipeline) + + pipeline.cancel_running + end + + it 'cancels both jobs' do + expect(latest_status).to contain_exactly('canceled', 'canceled') + end + end + end + + describe '#retry_failed' do + let(:latest_status) { pipeline.statuses.latest.pluck(:status) } + + context 'when there is a failed build and failed external status' do + before do + create(:ci_build, :failed, name: 'build', pipeline: pipeline) + create(:generic_commit_status, :failed, name: 'jenkins', pipeline: pipeline) + + pipeline.retry_failed(create(:user)) + end + + it 'retries only build' do + expect(latest_status).to contain_exactly('pending', 'failed') + end + end + + context 'when builds are in different stages' do + before do + create(:ci_build, :failed, name: 'build', stage_idx: 0, pipeline: pipeline) + create(:ci_build, :failed, name: 'jenkins', stage_idx: 1, pipeline: pipeline) + + pipeline.retry_failed(create(:user)) + end + + it 'retries both builds' do + expect(latest_status).to contain_exactly('pending', 'pending') + end + end + + context 'when there are canceled and failed' do + before do + create(:ci_build, :failed, name: 'build', stage_idx: 0, pipeline: pipeline) + create(:ci_build, :canceled, name: 'jenkins', stage_idx: 1, pipeline: pipeline) + + pipeline.retry_failed(create(:user)) + end + + it 'retries both builds' do + expect(latest_status).to contain_exactly('pending', 'pending') + end + end + end + describe '#execute_hooks' do let!(:build_a) { create_build('a', 0) } let!(:build_b) { create_build('b', 1) } diff --git a/spec/models/concerns/has_status_spec.rb b/spec/models/concerns/has_status_spec.rb index 87bffbdc54e..9defb17dc92 100644 --- a/spec/models/concerns/has_status_spec.rb +++ b/spec/models/concerns/has_status_spec.rb @@ -123,4 +123,100 @@ describe HasStatus do it_behaves_like 'build status summary' end end + + context 'for scope with one status' do + shared_examples 'having a job' do |status| + %i[ci_build generic_commit_status].each do |type| + context "when it's #{status} #{type} job" do + let!(:job) { create(type, status) } + + describe ".#{status}" do + it 'contains the job' do + expect(CommitStatus.public_send(status).all). + to contain_exactly(job) + end + end + + describe '.relevant' do + if status == :created + it 'contains nothing' do + expect(CommitStatus.relevant.all).to be_empty + end + else + it 'contains the job' do + expect(CommitStatus.relevant.all).to contain_exactly(job) + end + end + end + end + end + end + + %i[created running pending success + failed canceled skipped].each do |status| + it_behaves_like 'having a job', status + end + end + + context 'for scope with more statuses' do + shared_examples 'containing the job' do |status| + %i[ci_build generic_commit_status].each do |type| + context "when it's #{status} #{type} job" do + let!(:job) { create(type, status) } + + it 'contains the job' do + is_expected.to contain_exactly(job) + end + end + end + end + + shared_examples 'not containing the job' do |status| + %i[ci_build generic_commit_status].each do |type| + context "when it's #{status} #{type} job" do + let!(:job) { create(type, status) } + + it 'contains nothing' do + is_expected.to be_empty + end + end + end + end + + describe '.running_or_pending' do + subject { CommitStatus.running_or_pending } + + %i[running pending].each do |status| + it_behaves_like 'containing the job', status + end + + %i[created failed success].each do |status| + it_behaves_like 'not containing the job', status + end + end + + describe '.finished' do + subject { CommitStatus.finished } + + %i[success failed canceled].each do |status| + it_behaves_like 'containing the job', status + end + + %i[created running pending].each do |status| + it_behaves_like 'not containing the job', status + end + end + + describe '.cancelable' do + subject { CommitStatus.cancelable } + + %i[running pending created].each do |status| + it_behaves_like 'containing the job', status + end + + %i[failed success skipped canceled].each do |status| + it_behaves_like 'not containing the job', status + end + end + end end