From 64e09a1b35cc1897df7e46e73989c6e7013f6196 Mon Sep 17 00:00:00 2001 From: Grzegorz Bizon Date: Mon, 8 May 2017 12:47:21 +0200 Subject: [PATCH 1/6] Fix pipeline status when allowed to fail jobs present --- app/models/concerns/has_status.rb | 4 +++- spec/services/ci/process_pipeline_service_spec.rb | 4 ++++ 2 files changed, 7 insertions(+), 1 deletion(-) diff --git a/app/models/concerns/has_status.rb b/app/models/concerns/has_status.rb index 3c9c6584e02..692bd1f9317 100644 --- a/app/models/concerns/has_status.rb +++ b/app/models/concerns/has_status.rb @@ -37,7 +37,9 @@ module HasStatus end def status - all.pluck(status_sql).first + all.pluck(status_sql).first.tap do |status| + return 'success' if status == 'skipped' && all.failed_but_allowed.any? + end end def started_at diff --git a/spec/services/ci/process_pipeline_service_spec.rb b/spec/services/ci/process_pipeline_service_spec.rb index 1557cb3c938..efcaccc254e 100644 --- a/spec/services/ci/process_pipeline_service_spec.rb +++ b/spec/services/ci/process_pipeline_service_spec.rb @@ -62,6 +62,10 @@ describe Ci::ProcessPipelineService, '#execute', :services do fail_running_or_pending expect(builds_statuses).to eq %w(failed pending) + + fail_running_or_pending + + expect(pipeline.reload).to be_success end end From 21f3e9ce2617b8869583bdae60cc619bcd0a29bd Mon Sep 17 00:00:00 2001 From: Grzegorz Bizon Date: Mon, 8 May 2017 14:26:08 +0200 Subject: [PATCH 2/6] Move custom compound status method to commit status --- app/models/commit_status.rb | 6 ++++++ app/models/concerns/has_status.rb | 4 +--- spec/models/commit_status_spec.rb | 24 ++++++++++++++++++++++++ 3 files changed, 31 insertions(+), 3 deletions(-) diff --git a/app/models/commit_status.rb b/app/models/commit_status.rb index 07cec63b939..500d05fd840 100644 --- a/app/models/commit_status.rb +++ b/app/models/commit_status.rb @@ -103,6 +103,12 @@ class CommitStatus < ActiveRecord::Base end end + def self.status + super.tap do |status| + return 'success' if status == 'skipped' && all.failed_but_allowed.any? + end + end + def locking_enabled? status_changed? end diff --git a/app/models/concerns/has_status.rb b/app/models/concerns/has_status.rb index 692bd1f9317..3c9c6584e02 100644 --- a/app/models/concerns/has_status.rb +++ b/app/models/concerns/has_status.rb @@ -37,9 +37,7 @@ module HasStatus end def status - all.pluck(status_sql).first.tap do |status| - return 'success' if status == 'skipped' && all.failed_but_allowed.any? - end + all.pluck(status_sql).first end def started_at diff --git a/spec/models/commit_status_spec.rb b/spec/models/commit_status_spec.rb index 9262ce08987..2ee9620228c 100644 --- a/spec/models/commit_status_spec.rb +++ b/spec/models/commit_status_spec.rb @@ -284,6 +284,30 @@ describe CommitStatus, :models do end end + describe '.status' do + context 'when there are multiple statuses present' do + before do + create_status(status: 'running') + create_status(status: 'success') + create_status(allow_failure: true, status: 'failed') + end + + it 'returns a correct compound status' do + expect(described_class.all.status).to eq 'running' + end + end + + context 'when there are only allowed to fail commit statuses present' do + before do + create_status(allow_failure: true, status: 'failed') + end + + it 'returns status that indicates success' do + expect(described_class.all.status).to eq 'success' + end + end + end + describe '#before_sha' do subject { commit_status.before_sha } From 26092b5158d61f353b66a38d5034f3cb6e567ffa Mon Sep 17 00:00:00 2001 From: Grzegorz Bizon Date: Mon, 8 May 2017 14:28:01 +0200 Subject: [PATCH 3/6] Add changelog entry for pipeline status fix --- .../fix-gb-fix-skipped-pipeline-with-allowed-to-fail-jobs.yml | 4 ++++ 1 file changed, 4 insertions(+) create mode 100644 changelogs/unreleased/fix-gb-fix-skipped-pipeline-with-allowed-to-fail-jobs.yml diff --git a/changelogs/unreleased/fix-gb-fix-skipped-pipeline-with-allowed-to-fail-jobs.yml b/changelogs/unreleased/fix-gb-fix-skipped-pipeline-with-allowed-to-fail-jobs.yml new file mode 100644 index 00000000000..f59c6ecd90c --- /dev/null +++ b/changelogs/unreleased/fix-gb-fix-skipped-pipeline-with-allowed-to-fail-jobs.yml @@ -0,0 +1,4 @@ +--- +title: Fix CI/CD status in case there are only allowed to failed jobs in the pipeline +merge_request: 11166 +author: From e538963d80872e3844ba345967cfa0bf3821c82a Mon Sep 17 00:00:00 2001 From: Grzegorz Bizon Date: Mon, 8 May 2017 14:32:14 +0200 Subject: [PATCH 4/6] Add test for using overridden status method with scopes --- spec/models/commit_status_spec.rb | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/spec/models/commit_status_spec.rb b/spec/models/commit_status_spec.rb index 2ee9620228c..dc348f6cd33 100644 --- a/spec/models/commit_status_spec.rb +++ b/spec/models/commit_status_spec.rb @@ -306,6 +306,17 @@ describe CommitStatus, :models do expect(described_class.all.status).to eq 'success' end end + + context 'when using a scope to select latest statuses' do + before do + create_status(name: 'test', status: 'failed') + create_status(allow_failure: true, name: 'test', status: 'failed') + end + + it 'returns status according to the scope' do + expect(described_class.latest.status).to eq 'success' + end + end end describe '#before_sha' do From 51ce9a6caa15b5619b486b8ea81d834ec1206561 Mon Sep 17 00:00:00 2001 From: Grzegorz Bizon Date: Mon, 8 May 2017 15:14:41 +0200 Subject: [PATCH 5/6] Fix specs for a concern that implements CI/CD statuses --- spec/models/concerns/has_status_spec.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/spec/models/concerns/has_status_spec.rb b/spec/models/concerns/has_status_spec.rb index 67dae7cf4c0..bcee293d876 100644 --- a/spec/models/concerns/has_status_spec.rb +++ b/spec/models/concerns/has_status_spec.rb @@ -48,7 +48,7 @@ describe HasStatus do [create(type, status: :failed, allow_failure: true)] end - it { is_expected.to eq 'skipped' } + it { is_expected.to eq 'success' } end context 'success and canceled' do From caf6b9918e3f7a79c9ffcffd1880f29422d50eb5 Mon Sep 17 00:00:00 2001 From: Grzegorz Bizon Date: Thu, 15 Jun 2017 14:43:47 +0200 Subject: [PATCH 6/6] Check warnings when building compound status SQL query --- app/models/commit_status.rb | 6 ------ app/models/concerns/has_status.rb | 21 ++++++++++++--------- spec/models/commit_status_spec.rb | 2 +- 3 files changed, 13 insertions(+), 16 deletions(-) diff --git a/app/models/commit_status.rb b/app/models/commit_status.rb index 500d05fd840..07cec63b939 100644 --- a/app/models/commit_status.rb +++ b/app/models/commit_status.rb @@ -103,12 +103,6 @@ class CommitStatus < ActiveRecord::Base end end - def self.status - super.tap do |status| - return 'success' if status == 'skipped' && all.failed_but_allowed.any? - end - end - def locking_enabled? status_changed? end diff --git a/app/models/concerns/has_status.rb b/app/models/concerns/has_status.rb index 3c9c6584e02..32af5566135 100644 --- a/app/models/concerns/has_status.rb +++ b/app/models/concerns/has_status.rb @@ -11,18 +11,21 @@ module HasStatus class_methods do def status_sql - scope = respond_to?(:exclude_ignored) ? exclude_ignored : all + scope_relevant = respond_to?(:exclude_ignored) ? exclude_ignored : all + scope_warnings = respond_to?(:failed_but_allowed) ? failed_but_allowed : none - builds = scope.select('count(*)').to_sql - created = scope.created.select('count(*)').to_sql - success = scope.success.select('count(*)').to_sql - manual = scope.manual.select('count(*)').to_sql - pending = scope.pending.select('count(*)').to_sql - running = scope.running.select('count(*)').to_sql - skipped = scope.skipped.select('count(*)').to_sql - canceled = scope.canceled.select('count(*)').to_sql + builds = scope_relevant.select('count(*)').to_sql + created = scope_relevant.created.select('count(*)').to_sql + success = scope_relevant.success.select('count(*)').to_sql + manual = scope_relevant.manual.select('count(*)').to_sql + pending = scope_relevant.pending.select('count(*)').to_sql + running = scope_relevant.running.select('count(*)').to_sql + skipped = scope_relevant.skipped.select('count(*)').to_sql + canceled = scope_relevant.canceled.select('count(*)').to_sql + warnings = scope_warnings.select('count(*) > 0').to_sql.presence || 'false' "(CASE + WHEN (#{builds})=(#{skipped}) AND (#{warnings}) THEN 'success' WHEN (#{builds})=(#{skipped}) THEN 'skipped' WHEN (#{builds})=(#{success}) THEN 'success' WHEN (#{builds})=(#{created}) THEN 'created' diff --git a/spec/models/commit_status_spec.rb b/spec/models/commit_status_spec.rb index dc348f6cd33..1e074c7ad26 100644 --- a/spec/models/commit_status_spec.rb +++ b/spec/models/commit_status_spec.rb @@ -309,7 +309,7 @@ describe CommitStatus, :models do context 'when using a scope to select latest statuses' do before do - create_status(name: 'test', status: 'failed') + create_status(name: 'test', retried: true, status: 'failed') create_status(allow_failure: true, name: 'test', status: 'failed') end