Merge branch 'define-ci-dangling-pipelines' into 'master'

Define dangling pipelines to be excluded from Project#ci_pipelines

See merge request gitlab-org/gitlab!40512

app/models/ci/pipeline.rb

@@ -262,7 +262,7 @@ module Ci
 
     scope :internal, -> { where(source: internal_sources) }
     scope :no_child, -> { where.not(source: :parent_pipeline) }
-    scope :ci_sources, -> { where(config_source: Enums::Ci::Pipeline.ci_config_sources_values) }
+    scope :ci_sources, -> { where(source: Enums::Ci::Pipeline.ci_sources.values) }
     scope :for_user, -> (user) { where(user: user) }
     scope :for_sha, -> (sha) { where(sha: sha) }
     scope :for_source_sha, -> (source_sha) { where(source_sha: source_sha) }
@@ -1033,7 +1033,11 @@ module Ci
     end
 
     def cacheable?
-      Enums::Ci::Pipeline.ci_config_sources.key?(config_source.to_sym)
+      !dangling?
+    end
+
+    def dangling?
+      Enums::Ci::Pipeline.dangling_sources.key?(source.to_sym)
     end
 
     def source_ref_path

app/models/concerns/enums/ci/pipeline.rb

@@ -36,6 +36,23 @@ module Enums
         }
       end
 
+      # Dangling sources are those events that generate pipelines for which
+      # we don't want to directly affect the ref CI status.
+      # - when a webide pipeline fails it does not change the ref CI status to failed
+      # - when a child pipeline (from parent_pipeline source) fails it affects its
+      #   parent pipeline. It's up to the parent to affect the ref CI status
+      # - when an ondemand_dast_scan pipeline runs it is for testing purpose and should
+      #   not affect the ref CI status.
+      def self.dangling_sources
+        sources.slice(:webide, :parent_pipeline, :ondemand_dast_scan)
+      end
+
+      # CI sources are those pipeline events that affect the CI status of the ref
+      # they run for. By definition it excludes dangling pipelines.
+      def self.ci_sources
+        sources.except(*dangling_sources.keys)
+      end
+
       # Returns the `Hash` to use for creating the `config_sources` enum for
       # `Ci::Pipeline`.
       def self.config_sources
@@ -50,24 +67,6 @@ module Enums
           parameter_source: 7
         }
       end
-
-      def self.ci_config_sources
-        config_sources.slice(
-          :unknown_source,
-          :repository_source,
-          :auto_devops_source,
-          :remote_source,
-          :external_project_source
-        )
-      end
-
-      def self.ci_config_sources_values
-        ci_config_sources.values
-      end
-
-      def self.non_ci_config_source_values
-        config_sources.values - ci_config_sources.values
-      end
     end
   end
 end

app/models/project.rb

@@ -280,10 +280,9 @@ class Project < ApplicationRecord
   # The relation :all_pipelines is intended to be used when we want to get the
   # whole list of pipelines associated to the project
   has_many :all_pipelines, class_name: 'Ci::Pipeline', inverse_of: :project
-  # The relation :ci_pipelines is intended to be used when we want to get only
-  # those pipeline which are directly related to CI. There are
-  # other pipelines, like webide ones, that we won't retrieve
-  # if we use this relation.
+  # The relation :ci_pipelines includes all those that directly contribute to the
+  # latest status of a ref. This does not include dangling pipelines such as those
+  # from webide, child pipelines, etc.
   has_many :ci_pipelines,
           -> { ci_sources },
           class_name: 'Ci::Pipeline',

ee/spec/models/ci/pipeline_spec.rb

@@ -24,29 +24,6 @@ RSpec.describe Ci::Pipeline do
     end
   end
 
-  describe '.ci_sources' do
-    subject { described_class.ci_sources }
-
-    let(:all_config_sources) { described_class.config_sources }
-
-    before do
-      all_config_sources.each do |source, _value|
-        create(:ci_pipeline, config_source: source)
-      end
-    end
-
-    it 'contains pipelines having CI only config sources' do
-      expect(subject.map(&:config_source)).to contain_exactly(
-        'auto_devops_source',
-        'external_project_source',
-        'remote_source',
-        'repository_source',
-        'unknown_source'
-      )
-      expect(subject.size).to be < all_config_sources.size
-    end
-  end
-
   describe '#with_vulnerabilities scope' do
     let!(:pipeline_1) { create(:ci_pipeline, project: project) }
     let!(:pipeline_2) { create(:ci_pipeline, project: project) }
@@ -376,7 +353,7 @@ RSpec.describe Ci::Pipeline do
 
     context 'when pipeline is web terminal triggered' do
       before do
-        pipeline.config_source = 'webide_source'
+        pipeline.source = 'webide'
       end
 
       it 'does not schedule the pipeline cache worker' do

spec/graphql/resolvers/project_pipeline_resolver_spec.rb

@@ -34,12 +34,10 @@ RSpec.describe Resolvers::ProjectPipelineResolver do
     expect { resolve_pipeline(project, {}) }.to raise_error(ArgumentError)
   end
 
-  context 'when the pipeline is not a ci_config_source' do
+  context 'when the pipeline is a dangling pipeline' do
     let(:pipeline) do
-      config_source_value = ::Enums::Ci::Pipeline.non_ci_config_source_values.first
-      config_source = ::Enums::Ci::Pipeline.config_sources.key(config_source_value)
-
-      create(:ci_pipeline, config_source: config_source, project: project)
+      dangling_source = ::Enums::Ci::Pipeline.dangling_sources.each_value.first
+      create(:ci_pipeline, source: dangling_source, project: project)
     end
 
     it 'resolves pipeline for the passed iid' do

spec/models/ci/pipeline_spec.rb

@@ -221,6 +221,26 @@ RSpec.describe Ci::Pipeline, :mailer, factory_default: :keep do
     end
   end
 
+  describe '.ci_sources' do
+    subject { described_class.ci_sources }
+
+    let!(:push_pipeline)   { create(:ci_pipeline, source: :push) }
+    let!(:web_pipeline)    { create(:ci_pipeline, source: :web) }
+    let!(:api_pipeline)    { create(:ci_pipeline, source: :api) }
+    let!(:webide_pipeline) { create(:ci_pipeline, source: :webide) }
+    let!(:child_pipeline)  { create(:ci_pipeline, source: :parent_pipeline) }
+
+    it 'contains pipelines having CI only sources' do
+      expect(subject).to contain_exactly(push_pipeline, web_pipeline, api_pipeline)
+    end
+
+    it 'filters on expected sources' do
+      expect(::Enums::Ci::Pipeline.ci_sources.keys).to contain_exactly(
+        *%i[unknown push web trigger schedule api external pipeline chat
+            merge_request_event external_pull_request_event])
+    end
+  end
+
   describe '.outside_pipeline_family' do
     subject(:outside_pipeline_family) { described_class.outside_pipeline_family(upstream_pipeline) }
 
@@ -1239,14 +1259,42 @@ RSpec.describe Ci::Pipeline, :mailer, factory_default: :keep do
     end
 
     describe 'pipeline caching' do
-      before do
-        pipeline.config_source = 'repository_source'
+      context 'if pipeline is cacheable' do
+        before do
+          pipeline.source = 'push'
+        end
+
+        it 'performs ExpirePipelinesCacheWorker' do
+          expect(ExpirePipelineCacheWorker).to receive(:perform_async).with(pipeline.id)
+
+          pipeline.cancel
+        end
       end
 
-      it 'performs ExpirePipelinesCacheWorker' do
-        expect(ExpirePipelineCacheWorker).to receive(:perform_async).with(pipeline.id)
+      context 'if pipeline is not cacheable' do
+        before do
+          pipeline.source = 'webide'
+        end
 
-        pipeline.cancel
+        it 'deos not perform ExpirePipelinesCacheWorker' do
+          expect(ExpirePipelineCacheWorker).not_to receive(:perform_async)
+
+          pipeline.cancel
+        end
+      end
+    end
+
+    describe '#dangling?' do
+      it 'returns true if pipeline comes from any dangling sources' do
+        pipeline.source = Enums::Ci::Pipeline.dangling_sources.each_key.first
+
+        expect(pipeline).to be_dangling
+      end
+
+      it 'returns true if pipeline comes from any CI sources' do
+        pipeline.source = Enums::Ci::Pipeline.ci_sources.each_key.first
+
+        expect(pipeline).not_to be_dangling
       end
     end
 
@@ -1959,12 +2007,12 @@ RSpec.describe Ci::Pipeline, :mailer, factory_default: :keep do
   describe '.last_finished_for_ref_id' do
     let(:branch) { project.default_branch }
     let(:ref) { project.ci_refs.take }
-    let(:config_source) { Enums::Ci::Pipeline.config_sources[:parameter_source] }
+    let(:dangling_source) { Enums::Ci::Pipeline.sources[:ondemand_dast_scan] }
     let!(:pipeline1) { create(:ci_pipeline, :success, project: project, ref: branch) }
     let!(:pipeline2) { create(:ci_pipeline, :success, project: project, ref: branch) }
     let!(:pipeline3) { create(:ci_pipeline, :failed, project: project, ref: branch) }
     let!(:pipeline4) { create(:ci_pipeline, :success, project: project, ref: branch) }
-    let!(:pipeline5) { create(:ci_pipeline, :success, project: project, ref: branch, config_source: config_source) }
+    let!(:pipeline5) { create(:ci_pipeline, :success, project: project, ref: branch, source: dangling_source) }
 
     it 'returns the expected pipeline' do
       result = described_class.last_finished_for_ref_id(ref.id)

spec/models/ci/ref_spec.rb

@@ -107,8 +107,8 @@ RSpec.describe Ci::Ref do
 
   describe '#last_finished_pipeline_id' do
     let(:pipeline_status) { :running }
-    let(:config_source) { Enums::Ci::Pipeline.config_sources[:repository_source] }
-    let(:pipeline) { create(:ci_pipeline, pipeline_status, config_source: config_source) }
+    let(:pipeline_source) { Enums::Ci::Pipeline.sources[:push] }
+    let(:pipeline) { create(:ci_pipeline, pipeline_status, source: pipeline_source) }
     let(:ci_ref) { pipeline.ci_ref }
 
     context 'when there are no finished pipelines' do
@@ -124,8 +124,8 @@ RSpec.describe Ci::Ref do
         expect(ci_ref.last_finished_pipeline_id).to eq(pipeline.id)
       end
 
-      context 'when the pipeline is not a ci_source' do
-        let(:config_source) { Enums::Ci::Pipeline.config_sources[:parameter_source] }
+      context 'when the pipeline a dangling pipeline' do
+        let(:pipeline_source) { Enums::Ci::Pipeline.sources[:ondemand_dast_scan] }
 
         it 'returns nil' do
           expect(ci_ref.last_finished_pipeline_id).to be_nil

spec/models/project_spec.rb

@@ -522,9 +522,10 @@ RSpec.describe Project do
     before do
       create(:ci_pipeline, project: project, ref: 'master', source: :web)
       create(:ci_pipeline, project: project, ref: 'master', source: :external)
+      create(:ci_pipeline, project: project, ref: 'master', source: :webide)
     end
 
-    it 'has ci pipelines' do
+    it 'excludes dangling pipelines such as :webide' do
       expect(project.ci_pipelines.size).to eq(2)
     end
 

spec/requests/api/ci/pipelines_spec.rb

@@ -476,17 +476,18 @@ RSpec.describe API::Ci::Pipelines do
       end
     end
 
-    context 'when config source is not ci' do
-      let(:non_ci_config_source) { Enums::Ci::Pipeline.non_ci_config_source_values.first }
-      let(:pipeline_not_ci) do
-        create(:ci_pipeline, config_source: non_ci_config_source, project: project)
+    context 'when pipeline is a dangling pipeline' do
+      let(:dangling_source) { Enums::Ci::Pipeline.dangling_sources.each_value.first }
+
+      let(:dangling_pipeline) do
+        create(:ci_pipeline, source: dangling_source, project: project)
       end
 
       it 'returns the specified pipeline' do
-        get api("/projects/#{project.id}/pipelines/#{pipeline_not_ci.id}", user)
+        get api("/projects/#{project.id}/pipelines/#{dangling_pipeline.id}", user)
 
         expect(response).to have_gitlab_http_status(:ok)
-        expect(json_response['sha']).to eq(pipeline_not_ci.sha)
+        expect(json_response['sha']).to eq(dangling_pipeline.sha)
       end
     end
   end

spec/requests/api/jobs_spec.rb

@@ -239,13 +239,13 @@ RSpec.describe API::Jobs do
         end
       end
 
-      context 'when config source not ci' do
-        let(:non_ci_config_source) { Enums::Ci::Pipeline.non_ci_config_source_values.first }
+      context 'when jobs belong to a dangling pipeline' do
+        let(:dangling_source) { Enums::Ci::Pipeline.dangling_sources.each_value.first }
         let(:pipeline) do
-          create(:ci_pipeline, config_source: non_ci_config_source, project: project)
+          create(:ci_pipeline, source: dangling_source, project: project)
         end
 
-        it 'returns the specified pipeline' do
+        it 'returns pipeline jobs' do
           expect(response).to have_gitlab_http_status(:ok)
           expect(json_response[0]['pipeline']['sha']).to eq(pipeline.sha.to_s)
         end