Merge branch '218536-removing-ci_composite_status' into 'master'

Remove FF ci_composite_status and related codes

See merge request gitlab-org/gitlab!39498

app/models/ci/group.rb

@@ -24,15 +24,9 @@ module Ci
 
     def status
       strong_memoize(:status) do
-        if ::Gitlab::Ci::Features.composite_status?(project)
         Gitlab::Ci::Status::Composite
           .new(@jobs)
           .status
-        else
-          CommitStatus
-            .where(id: @jobs)
-            .legacy_status
-        end
       end
     end
 

app/models/ci/legacy_stage.rb

@@ -32,7 +32,7 @@ module Ci
     end
 
     def status
-      @status ||= statuses.latest.slow_composite_status(project: project)
+      @status ||= statuses.latest.composite_status
     end
 
     def detailed_status(current_user)

app/models/ci/pipeline.rb

@@ -418,24 +418,6 @@ module Ci
       false
     end
 
-    def legacy_stages_using_sql
-      # TODO, this needs refactoring, see gitlab-foss#26481.
-      stages_query = statuses
-        .group('stage').select(:stage).order('max(stage_idx)')
-
-      status_sql = statuses.latest.where('stage=sg.stage').legacy_status_sql
-
-      warnings_sql = statuses.latest.select('COUNT(*)')
-        .where('stage=sg.stage').failed_but_allowed.to_sql
-
-      stages_with_statuses = CommitStatus.from(stages_query, :sg)
-        .pluck('sg.stage', Arel.sql(status_sql), Arel.sql("(#{warnings_sql})"))
-
-      stages_with_statuses.map do |stage|
-        Ci::LegacyStage.new(self, Hash[%i[name status warnings].zip(stage)])
-      end
-    end
-
     def legacy_stages_using_composite_status
       stages = latest_statuses_ordered_by_stage.group_by(&:stage)
 
@@ -456,11 +438,7 @@ module Ci
 
     # TODO: Remove usage of this method in templates
     def legacy_stages
-      if ::Gitlab::Ci::Features.composite_status?(project)
       legacy_stages_using_composite_status
-      else
-        legacy_stages_using_sql
-      end
     end
 
     def valid_commit_sha

app/models/ci/stage.rb

@@ -138,7 +138,7 @@ module Ci
     end
 
     def latest_stage_status
-      statuses.latest.slow_composite_status(project: project) || 'skipped'
+      statuses.latest.composite_status || 'skipped'
     end
   end
 end

app/models/concerns/ci/has_status.rb

@@ -20,60 +20,10 @@ module Ci
     UnknownStatusError = Class.new(StandardError)
 
     class_methods do
-      def legacy_status_sql
-        scope_relevant = respond_to?(:exclude_ignored) ? exclude_ignored : all
-        scope_warnings = respond_to?(:failed_but_allowed) ? failed_but_allowed : none
-
-        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
-        scheduled = scope_relevant.scheduled.select('count(*)').to_sql
-        preparing = scope_relevant.preparing.select('count(*)').to_sql
-        waiting_for_resource = scope_relevant.waiting_for_resource.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'
-
-        Arel.sql(
-          "(CASE
-            WHEN (#{builds})=(#{skipped}) AND (#{warnings}) THEN 'success'
-            WHEN (#{builds})=(#{skipped}) THEN 'skipped'
-            WHEN (#{builds})=(#{success}) THEN 'success'
-            WHEN (#{builds})=(#{created}) THEN 'created'
-            WHEN (#{builds})=(#{preparing}) THEN 'preparing'
-            WHEN (#{builds})=(#{success})+(#{skipped}) THEN 'success'
-            WHEN (#{builds})=(#{success})+(#{skipped})+(#{canceled}) THEN 'canceled'
-            WHEN (#{builds})=(#{created})+(#{skipped})+(#{pending}) THEN 'pending'
-            WHEN (#{running})+(#{pending})>0 THEN 'running'
-            WHEN (#{waiting_for_resource})>0 THEN 'waiting_for_resource'
-            WHEN (#{manual})>0 THEN 'manual'
-            WHEN (#{scheduled})>0 THEN 'scheduled'
-            WHEN (#{preparing})>0 THEN 'preparing'
-            WHEN (#{created})>0 THEN 'running'
-            ELSE 'failed'
-          END)"
-        )
-      end
-
-      def legacy_status
-        all.pluck(legacy_status_sql).first
-      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(project:)
-        if ::Gitlab::Ci::Features.composite_status?(project)
+      def composite_status
         Gitlab::Ci::Status::Composite
           .new(all, with_allow_failure: columns_hash.key?('allow_failure'))
           .status
-        else
-          legacy_status
-        end
       end
 
       def started_at

changelogs/unreleased/218536-removing-ci_composite_status.yml

+---
+title: Remove FF ci_composite_status and related codes
+merge_request: 39498
+author:
+type: other

lib/gitlab/ci/features.rb

@@ -18,10 +18,6 @@ module Gitlab
         ::Feature.enabled?(:ci_instance_variables_ui, default_enabled: true)
       end
 
-      def self.composite_status?(project)
-        ::Feature.enabled?(:ci_composite_status, project, default_enabled: true)
-      end
-
       def self.pipeline_latest?
         ::Feature.enabled?(:ci_pipeline_latest, default_enabled: true)
       end

spec/lib/gitlab/ci/status/composite_spec.rb

@@ -16,48 +16,61 @@ RSpec.describe Gitlab::Ci::Status::Composite do
   end
 
   describe '#status' do
-    shared_examples 'compares composite with SQL status' do
-      it 'returns exactly the same result' do
-        builds = Ci::Build.where(id: all_statuses)
+    using RSpec::Parameterized::TableSyntax
 
-        expect(composite_status.status).to eq(builds.legacy_status)
-        expect(composite_status.warnings?).to eq(builds.failed_but_allowed.any?)
-      end
+    shared_examples 'compares status and warnings' do
+      let(:composite_status) do
+        described_class.new(all_statuses)
       end
 
-    shared_examples 'validate all combinations' do |perms|
-      Ci::HasStatus::STATUSES_ENUM.keys.combination(perms).each do |statuses|
-        context "with #{statuses.join(",")}" do
-          it_behaves_like 'compares composite with SQL status' do
-            let(:all_statuses) do
-              statuses.map { |status| @statuses[status] }
+      it 'returns status and warnings?' do
+        expect(composite_status.status).to eq(result)
+        expect(composite_status.warnings?).to eq(has_warnings)
       end
-
-            let(:composite_status) do
-              described_class.new(all_statuses)
     end
+
+    context 'allow_failure: false' do
+      where(:build_statuses, :result, :has_warnings) do
+        %i(skipped) | 'skipped' | false
+        %i(skipped success) | 'success' | false
+        %i(created) | 'created' | false
+        %i(preparing) | 'preparing' | false
+        %i(canceled success skipped) | 'canceled' | false
+        %i(pending created skipped) | 'pending' | false
+        %i(pending created skipped success) | 'running' | false
+        %i(running created skipped success) | 'running' | false
+        %i(success waiting_for_resource) | 'waiting_for_resource' | false
+        %i(success manual) | 'manual' | false
+        %i(success scheduled) | 'scheduled' | false
+        %i(created preparing) | 'preparing' | false
+        %i(created success pending) | 'running' | false
+        %i(skipped success failed) | 'failed' | false
       end
 
-          Ci::HasStatus::STATUSES_ENUM.each do |allow_failure_status, _|
-            context "and allow_failure #{allow_failure_status}" do
-              it_behaves_like 'compares composite with SQL status' do
+      with_them do
         let(:all_statuses) do
-                  statuses.map { |status| @statuses[status] } +
-                    [@statuses_with_allow_failure[allow_failure_status]]
+          build_statuses.map { |status| @statuses[status] }
         end
 
-                let(:composite_status) do
-                  described_class.new(all_statuses)
-                end
+        it_behaves_like 'compares status and warnings'
       end
     end
+
+    context 'allow_failure: true' do
+      where(:build_statuses, :result, :has_warnings) do
+        %i(manual) | 'skipped' | false
+        %i(skipped failed) | 'success' | true
+        %i(created failed) | 'created' | true
+        %i(preparing manual) | 'preparing' | false
       end
+
+      with_them do
+        let(:all_statuses) do
+          build_statuses.map { |status| @statuses_with_allow_failure[status] }
         end
+
+        it_behaves_like 'compares status and warnings'
       end
     end
-
-    it_behaves_like 'validate all combinations', 0
-    it_behaves_like 'validate all combinations', 1
-    it_behaves_like 'validate all combinations', 2
   end
 end

spec/models/ci/group_spec.rb

@@ -29,26 +29,10 @@ RSpec.describe Ci::Group do
       [create(:ci_build, :failed)]
     end
 
-    context 'when ci_composite_status is enabled' do
-      before do
-        stub_feature_flags(ci_composite_status: true)
-      end
-
-      it 'returns a failed status' do
-        expect(subject.status).to eq('failed')
-      end
-    end
-
-    context 'when ci_composite_status is disabled' do
-      before do
-        stub_feature_flags(ci_composite_status: false)
-      end
-
     it 'returns a failed status' do
       expect(subject.status).to eq('failed')
     end
   end
-  end
 
   describe '#detailed_status' do
     context 'when there is only one item in the group' do

spec/models/ci/pipeline_spec.rb

@@ -939,15 +939,6 @@ RSpec.describe Ci::Pipeline, :mailer do
 
         subject { pipeline.legacy_stages }
 
-        where(:ci_composite_status) do
-          [false, true]
-        end
-
-        with_them do
-          before do
-            stub_feature_flags(ci_composite_status: ci_composite_status)
-          end
-
         context 'stages list' do
           it 'returns ordered list of stages' do
             expect(subject.map(&:name)).to eq(%w[build test deploy])
@@ -1004,7 +995,6 @@ RSpec.describe Ci::Pipeline, :mailer do
           end
         end
       end
-      end
 
       describe '#stages_count' do
         it 'returns a valid number of stages' do

spec/models/commit_status_spec.rb

@@ -422,7 +422,7 @@ RSpec.describe CommitStatus do
       end
 
       it 'returns a correct compound status' do
-        expect(described_class.all.slow_composite_status(project: project)).to eq 'running'
+        expect(described_class.all.composite_status).to eq 'running'
       end
     end
 
@@ -432,7 +432,7 @@ RSpec.describe CommitStatus do
       end
 
       it 'returns status that indicates success' do
-        expect(described_class.all.slow_composite_status(project: project)).to eq 'success'
+        expect(described_class.all.composite_status).to eq 'success'
       end
     end
 
@@ -443,7 +443,7 @@ RSpec.describe CommitStatus do
       end
 
       it 'returns status according to the scope' do
-        expect(described_class.latest.slow_composite_status(project: project)).to eq 'success'
+        expect(described_class.latest.composite_status).to eq 'success'
       end
     end
   end

spec/models/concerns/ci/has_status_spec.rb

@@ -3,10 +3,10 @@
 require 'spec_helper'
 
 RSpec.describe Ci::HasStatus do
-  describe '.slow_composite_status' do
+  describe '.composite_status' do
     using RSpec::Parameterized::TableSyntax
 
-    subject { CommitStatus.slow_composite_status(project: nil) }
+    subject { CommitStatus.composite_status }
 
     shared_examples 'build status summary' do
       context 'all successful' do
@@ -184,15 +184,6 @@ RSpec.describe Ci::HasStatus do
       end
     end
 
-    where(:ci_composite_status) do
-      [false, true]
-    end
-
-    with_them do
-      before do
-        stub_feature_flags(ci_composite_status: ci_composite_status)
-      end
-
     context 'ci build statuses' do
       let(:type) { :ci_build }
 
@@ -205,7 +196,6 @@ RSpec.describe Ci::HasStatus do
       it_behaves_like 'build status summary'
     end
   end
-  end
 
   context 'for scope with one status' do
     shared_examples 'having a job' do |status|
@@ -400,12 +390,4 @@ RSpec.describe Ci::HasStatus do
       end
     end
   end
-
-  describe '.legacy_status_sql' do
-    subject { Ci::Build.legacy_status_sql }
-
-    it 'returns SQL' do
-      puts subject
-    end
-  end
 end