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

Prefer to use virtual statuses

parent 4372795d
...@@ -23,7 +23,7 @@ module Ci ...@@ -23,7 +23,7 @@ module Ci
def status def status
strong_memoize(:status) do strong_memoize(:status) do
if Feature.enabled?(:ci_composite_status, default_enabled: true) if Feature.enabled?(:ci_composite_status, default_enabled: false)
all_statuses = @jobs.pluck(:status, :allow_failure) all_statuses = @jobs.pluck(:status, :allow_failure)
Gitlab::Ci::Status::Composite Gitlab::Ci::Status::Composite
...@@ -51,11 +51,5 @@ module Ci ...@@ -51,11 +51,5 @@ module Ci
self.new(stage, name: group_name, jobs: grouped_statuses) self.new(stage, name: group_name, jobs: grouped_statuses)
end end
end end
private
def commit_statuses
@commit_statuses ||= CommitStatus.where(id: jobs.map(&:id))
end
end end
end end
...@@ -425,7 +425,7 @@ module Ci ...@@ -425,7 +425,7 @@ module Ci
end end
def legacy_stages def legacy_stages
if Feature.enabled?(:ci_composite_status, default_enabled: true) if Feature.enabled?(:ci_composite_status, default_enabled: false)
legacy_stages_using_composite_status legacy_stages_using_composite_status
else else
legacy_stages_using_sql legacy_stages_using_sql
......
...@@ -59,8 +59,12 @@ module HasStatus ...@@ -59,8 +59,12 @@ module HasStatus
all.pluck(legacy_status_sql).first all.pluck(legacy_status_sql).first
end end
# This method should not be used.
# This method performs expensive calculation of status:
# 1. By plucking all related objects,
# 2. Or executes expensive SQL query
def slow_composite_status def slow_composite_status
if Feature.enabled?(:ci_composite_status, default_enabled: true) if Feature.enabled?(:ci_composite_status, default_enabled: false)
columns = [:status] columns = [:status]
columns << :allow_failure if column_names.include?('allow_failure') columns << :allow_failure if column_names.include?('allow_failure')
......
...@@ -6,21 +6,17 @@ module Gitlab ...@@ -6,21 +6,17 @@ module Gitlab
class Composite class Composite
include Gitlab::Utils::StrongMemoize include Gitlab::Utils::StrongMemoize
attr_reader :warnings
# This class accepts an array of arrays # This class accepts an array of arrays
# The `status_key` and `allow_failure_key` define an index # The `status_key` and `allow_failure_key` define an index
# or key in each entry # or key in each entry
def initialize(all_statuses, status_key:, allow_failure_key:) def initialize(all_statuses, status_key:, allow_failure_key:)
raise ArgumentError, "all_statuses needs to be an Array" unless all_statuses.is_a?(Array) raise ArgumentError, "all_statuses needs to be an Array" unless all_statuses.is_a?(Array)
@count = 0
@warnings = 0
@status_set = Set.new @status_set = Set.new
@status_key = status_key @status_key = status_key
@allow_failure_key = allow_failure_key @allow_failure_key = allow_failure_key
build_status_set(all_statuses) consume_all_statuses(all_statuses)
end end
# The status calculation is order dependent, # The status calculation is order dependent,
...@@ -30,20 +26,20 @@ module Gitlab ...@@ -30,20 +26,20 @@ module Gitlab
# based on what statuses are no longer valid based on the # based on what statuses are no longer valid based on the
# data set that we have # data set that we have
def status def status
strong_memoize(:status) do return if none?
next if @count.zero?
if none? || only_of?(:skipped) strong_memoize(:status) do
warnings? ? 'success' : 'skipped' if only_of?(:skipped, :ignored)
elsif only_of?(:success, :skipped) 'skipped'
elsif only_of?(:success, :skipped, :success_with_warnings, :ignored)
'success' 'success'
elsif only_of?(:created) elsif only_of?(:created, :success_with_warnings, :ignored)
'created' 'created'
elsif only_of?(:preparing) elsif only_of?(:preparing, :success_with_warnings, :ignored)
'preparing' 'preparing'
elsif only_of?(:canceled, :success, :skipped) elsif only_of?(:canceled, :success, :skipped, :success_with_warnings, :ignored)
'canceled' 'canceled'
elsif only_of?(:pending, :created, :skipped) elsif only_of?(:pending, :created, :skipped, :success_with_warnings, :ignored)
'pending' 'pending'
elsif any_of?(:running, :pending) elsif any_of?(:running, :pending)
'running' 'running'
...@@ -62,7 +58,7 @@ module Gitlab ...@@ -62,7 +58,7 @@ module Gitlab
end end
def warnings? def warnings?
@warnings > 0 @status_set.include?(:success_with_warnings)
end end
private private
...@@ -77,28 +73,37 @@ module Gitlab ...@@ -77,28 +73,37 @@ module Gitlab
def only_of?(*names) def only_of?(*names)
matching = names.count { |name| @status_set.include?(name) } matching = names.count { |name| @status_set.include?(name) }
matching == @status_set.size matching > 0 && \
matching == @status_set.size
end
def consume_all_statuses(all_statuses)
all_statuses.each(&method(:consume_status))
end end
def build_status_set(all_statuses) def consume_status(description)
all_statuses.each do |status| # convert `"status"` into `["status"]`
status = Array(status) description = Array(description)
@count += 1 status =
@warnings += 1 if count_as_warning?(status) if success_with_warnings?(description)
next if exclude_from_calculation?(status) :success_with_warnings
elsif ignored_status?(description)
:ignored
else
description[@status_key].to_sym
end
@status_set.add(status[@status_key].to_sym) @status_set.add(status)
end
end end
def count_as_warning?(status) def success_with_warnings?(status)
@allow_failure_key && @allow_failure_key &&
status[@allow_failure_key] && status[@allow_failure_key] &&
HasStatus::PASSED_WITH_WARNINGS_STATUSES.include?(status[@status_key]) HasStatus::PASSED_WITH_WARNINGS_STATUSES.include?(status[@status_key])
end end
def exclude_from_calculation?(status) def ignored_status?(status)
@allow_failure_key && @allow_failure_key &&
status[@allow_failure_key] && status[@allow_failure_key] &&
HasStatus::EXCLUDE_IGNORED_STATUSES.include?(status[@status_key]) HasStatus::EXCLUDE_IGNORED_STATUSES.include?(status[@status_key])
......
...@@ -270,6 +270,29 @@ describe CommitStatus do ...@@ -270,6 +270,29 @@ describe CommitStatus do
end end
end end
describe '.exclude_ignored' do
subject { described_class.exclude_ignored.order(:id) }
let(:statuses) do
[create_status(when: 'manual', status: 'skipped'),
create_status(when: 'manual', status: 'success'),
create_status(when: 'manual', status: 'failed'),
create_status(when: 'on_failure', status: 'skipped'),
create_status(when: 'on_failure', status: 'success'),
create_status(when: 'on_failure', status: 'failed'),
create_status(allow_failure: true, status: 'success'),
create_status(allow_failure: true, status: 'failed'),
create_status(allow_failure: false, status: 'success'),
create_status(allow_failure: false, status: 'failed'),
create_status(allow_failure: true, status: 'manual'),
create_status(allow_failure: false, status: 'manual')]
end
it 'returns statuses without what we want to ignore' do
is_expected.to eq(statuses.values_at(0, 1, 2, 3, 4, 5, 6, 8, 9, 11))
end
end
describe '.failed_but_allowed' do describe '.failed_but_allowed' do
subject { described_class.failed_but_allowed.order(:id) } subject { described_class.failed_but_allowed.order(:id) }
......
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