From fe64fa1a57d0a390ecd7c95c2f64a96afaba1a5d Mon Sep 17 00:00:00 2001
From: Fabio Pitino <fpitino@gitlab.com>
Date: Thu, 29 Oct 2020 12:39:53 +0000
Subject: [PATCH 1/2] List pipeline reports from self and child pipelines

Make the parent pipeline the entry point for listing
all the reports generated within its hierarchy as
long as `strategy:depend` was used to link pipelines.

By surfacing only reports from dependent child pipelines
we ensure that a parent pipeline completes always after
the child pipelines generating reports. This eliminates
race conditions with detached child pipelines.
---
 app/models/ci/pipeline.rb                     |  83 +++++---
 app/models/concerns/ci/metadatable.rb         |  21 ++
 ...reports-to-include-child-pipeline-jobs.yml |   5 +
 ...include_child_pipeline_jobs_in_reports.yml |   7 +
 lib/gitlab/ci/features.rb                     |   4 +
 lib/gitlab/ci/pipeline_object_hierarchy.rb    |  40 +++-
 spec/factories/ci/pipelines.rb                |   5 +-
 .../ci/pipeline_object_hierarchy_spec.rb      |  49 ++++-
 spec/models/ci/pipeline_spec.rb               | 199 ++++++++++++++++--
 .../helpers/ci/source_pipeline_helpers.rb     |   9 +-
 10 files changed, 363 insertions(+), 59 deletions(-)
 create mode 100644 changelogs/unreleased/ci-reports-to-include-child-pipeline-jobs.yml
 create mode 100644 config/feature_flags/development/include_child_pipeline_jobs_in_reports.yml

diff --git a/app/models/ci/pipeline.rb b/app/models/ci/pipeline.rb
index 8083d4ed48a98b..98d95839b44503 100644
--- a/app/models/ci/pipeline.rb
+++ b/app/models/ci/pipeline.rb
@@ -603,30 +603,6 @@ def batch_lookup_report_artifact_for_file_type(file_type)
         .last
     end
 
-    # This batch loads the latest reports for each CI job artifact
-    # type (e.g. sast, dast, etc.) in a single SQL query to eliminate
-    # the need to do N different `job_artifacts.where(file_type:
-    # X).last` calls.
-    #
-    # Return a hash of file type => array of 1 job artifact
-    def latest_report_artifacts
-      ::Gitlab::SafeRequestStore.fetch("pipeline:#{self.id}:latest_report_artifacts") do
-        # Note we use read_attribute(:project_id) to read the project
-        # ID instead of self.project_id. The latter appears to load
-        # the Project model. This extra filter doesn't appear to
-        # affect query plan but included to ensure we don't leak the
-        # wrong informaiton.
-        ::Ci::JobArtifact.where(
-          id: job_artifacts.with_reports
-            .select('max(ci_job_artifacts.id) as id')
-            .where(project_id: self.read_attribute(:project_id))
-            .group(:file_type)
-        )
-          .preload(:job)
-          .group_by(&:file_type)
-      end
-    end
-
     def has_kubernetes_active?
       project.deployment_platform&.active?
     end
@@ -841,15 +817,18 @@ def build_with_artifacts_in_self_and_descendants(name)
         .find_by_name(name)
     end
 
-    def builds_in_self_and_descendants
-      Ci::Build.latest.where(pipeline: self_and_descendants)
+    def builds_in_self_and_descendants(while_dependent: false)
+      Ci::Build.latest
+        .where(pipeline: self_and_descendants(while_dependent: while_dependent))
     end
 
     # Without using `unscoped`, caller scope is also included into the query.
     # Using `unscoped` here will be redundant after Rails 6.1
-    def self_and_descendants
+    def self_and_descendants(while_dependent: false)
       ::Gitlab::Ci::PipelineObjectHierarchy
-        .new(self.class.unscoped.where(id: id), options: { same_project: true })
+        .new(
+          self.class.unscoped.where(id: id),
+          options: { same_project: true, while_dependent: while_dependent })
         .base_and_descendants
     end
 
@@ -891,7 +870,15 @@ def latest_builds_with_artifacts
     end
 
     def latest_report_builds(reports_scope = ::Ci::JobArtifact.with_reports)
-      builds.latest.with_reports(reports_scope)
+      if include_child_pipeline_jobs_in_reports?
+        # list reports also from child pipelines using `strategy:depend`.
+        # we filter other child pipelines because we can't guarantee those
+        # finish before the parent pipeline as they run asynchronously.
+        builds_in_self_and_descendants(while_dependent: true)
+          .with_reports(reports_scope)
+      else
+        builds.latest.with_reports(reports_scope)
+      end
     end
 
     def builds_with_coverage
@@ -1105,6 +1092,44 @@ def reset_ancestor_bridges!
 
     private
 
+    # This batch loads the latest reports for each CI job artifact
+    # type (e.g. sast, dast, etc.) in a single SQL query to eliminate
+    # the need to do N different `job_artifacts.where(file_type:
+    # X).last` calls.
+    #
+    # Return a hash of file type => array of 1 job artifact
+    def latest_report_artifacts
+      ::Gitlab::SafeRequestStore.fetch("pipeline:#{self.id}:latest_report_artifacts") do
+        # Note we use read_attribute(:project_id) to read the project
+        # ID instead of self.project_id. The latter appears to load
+        # the Project model. This extra filter doesn't appear to
+        # affect query plan but included to ensure we don't leak the
+        # wrong informaiton.
+        ::Ci::JobArtifact.where(
+          id: job_artifacts_in_self_and_descendants.with_reports
+            .select('max(ci_job_artifacts.id) as id')
+            .where(project_id: self.read_attribute(:project_id))
+            .group(:file_type)
+        )
+          .preload(:job)
+          .group_by(&:file_type)
+      end
+    end
+
+    def job_artifacts_in_self_and_descendants
+      if include_child_pipeline_jobs_in_reports?
+        ::Ci::JobArtifact.where(job: builds_in_self_and_descendants(while_dependent: true))
+      else
+        job_artifacts
+      end
+    end
+
+    def include_child_pipeline_jobs_in_reports?
+      strong_memoize(:include_child_pipeline_jobs_in_reports) do
+        Gitlab::Ci::Features.include_child_pipeline_jobs_in_reports?(project)
+      end
+    end
+
     def add_message(severity, content)
       return unless Gitlab::Ci::Features.store_pipeline_messages?(project)
 
diff --git a/app/models/concerns/ci/metadatable.rb b/app/models/concerns/ci/metadatable.rb
index 26e644646b49d6..9d2cf2d832b40d 100644
--- a/app/models/concerns/ci/metadatable.rb
+++ b/app/models/concerns/ci/metadatable.rb
@@ -23,6 +23,27 @@ module Metadatable
       before_create :ensure_metadata
     end
 
+    class_methods do
+      # Tables to join in Arel statements when querying for the options
+      def tables_from_builds_to_options
+        if Feature.enabled?(:ci_build_metadata_config)
+          [::Ci::Build.arel_table, ::Ci::BuildMetadata.arel_table]
+        else
+          [::Ci::Build.arel_table]
+        end
+      end
+
+      def arel_with_strategy_depend_condition
+        if Feature.enabled?(:ci_build_metadata_config)
+          ::Ci::Build.arel_table[:id]
+            .eq(::Ci::BuildMetadata.arel_table[:build_id])
+            .and(Arel.sql("(ci_builds_metadata.config_options->'trigger'->>'strategy' = 'depend')"))
+        else
+          Arel.sql('"ci_builds"."options" LIKE \'%strategy: depend%\'')
+        end
+      end
+    end
+
     def ensure_metadata
       metadata || build_metadata(project: project)
     end
diff --git a/changelogs/unreleased/ci-reports-to-include-child-pipeline-jobs.yml b/changelogs/unreleased/ci-reports-to-include-child-pipeline-jobs.yml
new file mode 100644
index 00000000000000..56531f6fb190b7
--- /dev/null
+++ b/changelogs/unreleased/ci-reports-to-include-child-pipeline-jobs.yml
@@ -0,0 +1,5 @@
+---
+title: Make parent pipelines list reports from child pipelines in MR widgets
+merge_request: 46576
+author:
+type: added
diff --git a/config/feature_flags/development/include_child_pipeline_jobs_in_reports.yml b/config/feature_flags/development/include_child_pipeline_jobs_in_reports.yml
new file mode 100644
index 00000000000000..e00cd22227f388
--- /dev/null
+++ b/config/feature_flags/development/include_child_pipeline_jobs_in_reports.yml
@@ -0,0 +1,7 @@
+---
+name: include_child_pipeline_jobs_in_reports
+introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/4657
+rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/273200
+type: development
+group: group::continuous integration
+default_enabled: false
diff --git a/lib/gitlab/ci/features.rb b/lib/gitlab/ci/features.rb
index 94291f7d9e6a90..7d72b71f2e4b7f 100644
--- a/lib/gitlab/ci/features.rb
+++ b/lib/gitlab/ci/features.rb
@@ -74,6 +74,10 @@ def self.seed_block_run_before_workflow_rules_enabled?(project)
       def self.ci_pipeline_editor_page_enabled?(project)
         ::Feature.enabled?(:ci_pipeline_editor_page, project, default_enabled: false)
       end
+
+      def self.include_child_pipeline_jobs_in_reports?(project)
+        ::Feature.enabled?(:include_child_pipeline_jobs_in_reports, project, default_enabled: false)
+      end
     end
   end
 end
diff --git a/lib/gitlab/ci/pipeline_object_hierarchy.rb b/lib/gitlab/ci/pipeline_object_hierarchy.rb
index de3262b10e06ff..dada3eeac3c830 100644
--- a/lib/gitlab/ci/pipeline_object_hierarchy.rb
+++ b/lib/gitlab/ci/pipeline_object_hierarchy.rb
@@ -1,5 +1,25 @@
 # frozen_string_literal: true
 
+# This class allows to traverse a pipeline hierarchy where
+# multiple pipelines are connected with each others through
+# bridge jobs (e.g. multi-project or parent-child pipelines).
+#
+# Available options:
+#  - `same_project: true` to traverse the hierarchy in the
+#     context of parent-child pipelines. Use `false` for all
+#     upstream/downstream pipelines regardless of the project
+#
+#  - `while_dependent: true` to traverse the hierarchy while
+#     pipelines are connected via bridge jobs with `strategy:depend`
+#     recursive search stops as soon as a bridge job doesn't use
+#     `strategy:depend`.
+#
+#     Example: to return `base_and_descendants` of
+#       * A -*> B ->> C -*> D : would return A and B but not C or D
+#       * A ->> B -*> C -*> D : would return only A
+#
+#       NOTE: `-*>` denotes `strategy:depend`
+#             `->>` denotes the default behavior (async execution).
 module Gitlab
   module Ci
     class PipelineObjectHierarchy < ::Gitlab::ObjectHierarchy
@@ -10,7 +30,13 @@ def middle_table
       end
 
       def from_tables(cte)
-        [objects_table, cte.table, middle_table]
+        [objects_table, cte.table, middle_table] + optional_tables
+      end
+
+      def optional_tables
+        return [] unless options[:while_dependent]
+
+        ::Ci::Build.tables_from_builds_to_options
       end
 
       def parent_id_column(_cte)
@@ -30,6 +56,8 @@ def descendant_conditions(cte)
           middle_table[:source_pipeline_id].eq(cte.table[:id])
         ).and(
           same_project_condition
+        ).and(
+          dependent_condition
         )
       end
 
@@ -40,6 +68,16 @@ def same_project_condition
           Arel.sql('TRUE')
         end
       end
+
+      def dependent_condition
+        if options[:while_dependent]
+          middle_table[:source_job_id]
+            .eq(::Ci::Build.arel_table[:id])
+            .and(::Ci::Build.arel_with_strategy_depend_condition)
+        else
+          Arel.sql('TRUE')
+        end
+      end
     end
   end
 end
diff --git a/spec/factories/ci/pipelines.rb b/spec/factories/ci/pipelines.rb
index 14bd0ab1bc6209..71f4f142702045 100644
--- a/spec/factories/ci/pipelines.rb
+++ b/spec/factories/ci/pipelines.rb
@@ -16,6 +16,7 @@
     transient { head_pipeline_of { nil } }
 
     transient { child_of { nil } }
+    transient { strategy_depend { false } }
 
     after(:build) do |pipeline, evaluator|
       if evaluator.child_of
@@ -29,7 +30,9 @@
       merge_request&.update!(head_pipeline: pipeline)
 
       if evaluator.child_of
-        bridge = create(:ci_bridge, pipeline: evaluator.child_of)
+        bridge_traits = evaluator.strategy_depend ? [:strategy_depend] : []
+        bridge = create(:ci_bridge, *bridge_traits, pipeline: evaluator.child_of)
+
         create(:ci_sources_pipeline,
           source_job: bridge,
           pipeline: pipeline)
diff --git a/spec/lib/gitlab/ci/pipeline_object_hierarchy_spec.rb b/spec/lib/gitlab/ci/pipeline_object_hierarchy_spec.rb
index 89602fe79d17c9..9ea27e55ebf52b 100644
--- a/spec/lib/gitlab/ci/pipeline_object_hierarchy_spec.rb
+++ b/spec/lib/gitlab/ci/pipeline_object_hierarchy_spec.rb
@@ -11,14 +11,25 @@
   let_it_be(:child) { create(:ci_pipeline, project: project) }
   let_it_be(:cousin_parent) { create(:ci_pipeline, project: project) }
   let_it_be(:cousin) { create(:ci_pipeline, project: project) }
+  let_it_be(:cousin_child) { create(:ci_pipeline, project: project) }
   let_it_be(:triggered_pipeline) { create(:ci_pipeline) }
 
-  before_all do
-    create_source_pipeline(ancestor, parent)
-    create_source_pipeline(ancestor, cousin_parent)
-    create_source_pipeline(parent, child)
-    create_source_pipeline(cousin_parent, cousin)
-    create_source_pipeline(child, triggered_pipeline)
+  let(:ci_build_metadata_config_status) { false }
+
+  before do
+    # The use of this Feature Flag affects where options are stored and in particular
+    # the `strategy:depend` config used by `while_dependent:true` option.
+    stub_feature_flags(ci_build_metadata_config: ci_build_metadata_config_status)
+
+    # ancestor
+    #   - parent -> child -> triggered_pipeline
+    #   - cousin_parent -> cousin -> cousin_child
+    create_source_pipeline(ancestor, parent, strategy: :depend)
+    create_source_pipeline(ancestor, cousin_parent, strategy: :depend)
+    create_source_pipeline(parent, child, strategy: :depend)
+    create_source_pipeline(cousin_parent, cousin, strategy: nil)
+    create_source_pipeline(cousin, cousin_child, strategy: :depend)
+    create_source_pipeline(child, triggered_pipeline, strategy: nil)
   end
 
   describe '#base_and_ancestors' do
@@ -80,7 +91,8 @@
           parent.id => 2,
           cousin_parent.id => 2,
           child.id => 3,
-          cousin.id => 3
+          cousin.id => 3,
+          cousin_child.id => 4
         }
 
         relation.each do |object|
@@ -88,6 +100,27 @@
         end
       end
     end
+
+    context 'when while_dependent is true' do
+      let(:relation) do
+        described_class.new(
+          ::Ci::Pipeline.where(id: ancestor.id),
+          options: { same_project: true, while_dependent: true }
+        ).base_and_descendants
+      end
+
+      it 'returns pipelines as long as they use strategy:depend' do
+        expect(relation).to contain_exactly(ancestor, parent, child, cousin_parent)
+      end
+
+      context 'when feature flag ci_build_metadata_config is enabled' do
+        let(:ci_build_metadata_config_status) { true }
+
+        it 'returns pipelines as long as they use strategy:depend' do
+          expect(relation).to contain_exactly(ancestor, parent, child, cousin_parent)
+        end
+      end
+    end
   end
 
   describe '#all_objects' do
@@ -105,7 +138,7 @@
         options: { same_project: true }
       ).all_objects
 
-      expect(relation).to contain_exactly(ancestor, parent, cousin_parent, child, cousin)
+      expect(relation).to contain_exactly(ancestor, parent, cousin_parent, child, cousin, cousin_child)
     end
   end
 end
diff --git a/spec/models/ci/pipeline_spec.rb b/spec/models/ci/pipeline_spec.rb
index 88d08f1ec45847..99b957a7e66da0 100644
--- a/spec/models/ci/pipeline_spec.rb
+++ b/spec/models/ci/pipeline_spec.rb
@@ -2988,9 +2988,64 @@ def create_build(name, stage_idx)
     end
   end
 
+  describe '#self_and_descendants' do
+    subject(:pipelines) { pipeline.self_and_descendants(while_dependent: while_dependent) }
+
+    let(:while_dependent) { false }
+
+    let!(:pipeline) { create(:ci_pipeline, project: project) }
+
+    context 'when pipeline is standalone' do
+      it 'contains only itself' do
+        expect(pipelines).to contain_exactly(pipeline)
+      end
+    end
+
+    context 'when pipeline is parent of another pipeline' do
+      let!(:child_pipeline) { create(:ci_pipeline, child_of: pipeline) }
+
+      it 'returns parent and child pipelines' do
+        expect(pipelines).to contain_exactly(pipeline, child_pipeline)
+      end
+
+      context 'when child pipeline is parent of another child pipeline' do
+        let!(:child_of_child_pipeline) { create(:ci_pipeline, child_of: child_pipeline) }
+
+        it 'includes the child of child pipeline' do
+          expect(pipelines).to contain_exactly(pipeline, child_pipeline, child_of_child_pipeline)
+        end
+
+        context 'when `while_dependent: true' do
+          let(:while_dependent) { true }
+
+          let(:dependent_child_pipeline) { create(:ci_pipeline, child_of: pipeline, strategy_depend: true) }
+          let(:dependent_child_of_child_pipeline) { create(:ci_pipeline, child_of: dependent_child_pipeline, strategy_depend: true) }
+
+          it 'returns the pipelines in the hierarchy following dependent child pipelines' do
+            expect(pipelines).to contain_exactly(pipeline, dependent_child_pipeline, dependent_child_of_child_pipeline)
+          end
+
+          it 'does not include pipelines having their parent non-dependent' do
+            expect(pipelines).not_to include(child_pipeline, child_of_child_pipeline)
+          end
+        end
+      end
+    end
+
+    context 'when pipeline has a parent pipeline' do
+      let!(:parent_pipeline) { create(:ci_pipeline, project: project) }
+      let!(:pipeline) { create(:ci_pipeline, child_of: parent_pipeline) }
+
+      it 'does not include the parent' do
+        expect(pipelines).to contain_exactly(pipeline)
+      end
+    end
+  end
+
   describe '#builds_in_self_and_descendants' do
-    subject(:builds) { pipeline.builds_in_self_and_descendants }
+    subject(:builds) { pipeline.builds_in_self_and_descendants(while_dependent: while_dependent) }
 
+    let(:while_dependent) { false }
     let(:pipeline) { create(:ci_pipeline, project: project) }
     let!(:build) { create(:ci_build, pipeline: pipeline) }
 
@@ -3018,6 +3073,27 @@ def create_build(name, stage_idx)
       it 'returns the list of builds' do
         expect(builds).to contain_exactly(build, child_build, child_of_child_build)
       end
+
+      context 'when `while_depend: true`' do
+        let(:while_dependent) { true }
+
+        let(:dependent_child_pipeline) { create(:ci_pipeline, child_of: pipeline, strategy_depend: true) }
+        let!(:dependent_child_build) { create(:ci_build, pipeline: dependent_child_pipeline) }
+
+        let(:dependent_child_of_child_pipeline) { create(:ci_pipeline, child_of: dependent_child_pipeline, strategy_depend: true) }
+        let!(:dependent_child_of_child_build) { create(:ci_build, pipeline: dependent_child_of_child_pipeline) }
+
+        let(:not_reacheable_pipeline) { create(:ci_pipeline, child_of: child_pipeline, strategy_depend: true) }
+        let!(:not_reacheable_build) { create(:ci_build, pipeline: not_reacheable_pipeline) }
+
+        it 'returns the build in the hierarchy following dependent child pipelines' do
+          expect(builds).to contain_exactly(build, dependent_child_build, dependent_child_of_child_build)
+        end
+
+        it 'does not include builds from dependent child pipelines having their parent non-dependent' do
+          expect(builds).not_to include(not_reacheable_build)
+        end
+      end
     end
   end
 
@@ -3078,36 +3154,123 @@ def create_build(name, stage_idx)
   end
 
   describe '#batch_lookup_report_artifact_for_file_type' do
-    context 'with code quality report artifact' do
-      let(:pipeline) { create(:ci_pipeline, :with_codequality_report, project: project) }
+    before do
+      stub_feature_flags(include_child_pipeline_jobs_in_reports: feature_flag_status)
+    end
+
+    subject(:artifact) { pipeline.batch_lookup_report_artifact_for_file_type(:codequality) }
+
+    let!(:pipeline) { create(:ci_pipeline, :with_codequality_report, project: project) }
 
-      it "returns the code quality artifact" do
-        expect(pipeline.batch_lookup_report_artifact_for_file_type(:codequality)).to eq(pipeline.job_artifacts.sample)
+    context 'when feature flag `include_child_pipeline_jobs_in_reports` is enabled' do
+      let(:feature_flag_status) { true }
+
+      context 'with report artifact' do
+        it 'returns the report artifact' do
+          expect(artifact).to eq(pipeline.job_artifacts.sample)
+        end
+      end
+
+      context 'with report artifact in dependent child pipeline' do
+        let!(:child_pipeline) { create(:ci_pipeline, :with_codequality_report, child_of: pipeline, strategy_depend: true) }
+
+        it 'returns the child report artifact' do
+          expect(artifact).to eq(child_pipeline.job_artifacts.sample)
+        end
+      end
+
+      context 'with report artifact in non dependent child pipeline' do
+        let!(:child_pipeline) { create(:ci_pipeline, :with_codequality_report, child_of: pipeline, strategy_depend: false) }
+
+        it 'returns the parent report artifact' do
+          expect(artifact).to eq(pipeline.job_artifacts.sample)
+        end
+      end
+    end
+
+    context 'when feature flag `include_child_pipeline_jobs_in_reports` is disabled' do
+      let(:feature_flag_status) { false }
+
+      context 'with report artifact' do
+        it 'returns the report artifact' do
+          expect(artifact).to eq(pipeline.job_artifacts.sample)
+        end
+      end
+
+      context 'with report artifact in dependent child pipeline' do
+        let!(:child_pipeline) { create(:ci_pipeline, :with_codequality_report, child_of: pipeline, strategy_depend: true) }
+
+        it 'always returns parent report artifact' do
+          expect(artifact).to eq(pipeline.job_artifacts.sample)
+        end
       end
     end
   end
 
   describe '#latest_report_builds' do
-    it 'returns build with test artifacts' do
-      test_build = create(:ci_build, :test_reports, pipeline: pipeline, project: project)
-      coverage_build = create(:ci_build, :coverage_reports, pipeline: pipeline, project: project)
-      create(:ci_build, :artifacts, pipeline: pipeline, project: project)
+    let!(:test_build) { create(:ci_build, :test_reports, pipeline: pipeline) }
+    let!(:coverage_build) { create(:ci_build, :coverage_reports, pipeline: pipeline) }
+    let!(:other_build) { create(:ci_build, :artifacts, pipeline: pipeline) }
+
+    shared_examples 'returns report builds from current pipeline' do
+      it 'returns builds with test artifacts' do
+        expect(pipeline.latest_report_builds).to contain_exactly(test_build, coverage_build)
+      end
 
-      expect(pipeline.latest_report_builds).to contain_exactly(test_build, coverage_build)
+      it 'filters builds by scope' do
+        expect(pipeline.latest_report_builds(Ci::JobArtifact.test_reports)).to contain_exactly(test_build)
+      end
+
+      it 'only returns not retried builds' do
+        create(:ci_build, :test_reports, :retried, pipeline: pipeline)
+
+        expect(pipeline.latest_report_builds).to contain_exactly(test_build, coverage_build)
+      end
     end
 
-    it 'filters builds by scope' do
-      test_build = create(:ci_build, :test_reports, pipeline: pipeline, project: project)
-      create(:ci_build, :coverage_reports, pipeline: pipeline, project: project)
+    context 'when feature flag `include_child_pipeline_jobs_in_reports` is enabled' do
+      before do
+        stub_feature_flags(include_child_pipeline_jobs_in_reports: true)
+      end
+
+      it_behaves_like 'returns report builds from current pipeline'
+
+      context 'when pipeline has reports in child pipelines' do
+        context 'when child pipeline is dependent' do
+          let(:child_pipeline) { create(:ci_pipeline, child_of: pipeline, strategy_depend: true) }
+          let!(:child_build) { create(:ci_build, :test_reports, pipeline: child_pipeline) }
+
+          it 'returns also builds from child pipelines' do
+            expect(pipeline.latest_report_builds).to contain_exactly(test_build, coverage_build, child_build)
+          end
+        end
 
-      expect(pipeline.latest_report_builds(Ci::JobArtifact.test_reports)).to contain_exactly(test_build)
+        context 'when child pipeline is not dependent' do
+          let(:child_pipeline) { create(:ci_pipeline, child_of: pipeline, strategy_depend: false) }
+          let!(:child_build) { create(:ci_build, :test_reports, pipeline: child_pipeline) }
+
+          it 'returns only builds from parent pipeline' do
+            expect(pipeline.latest_report_builds).to contain_exactly(test_build, coverage_build)
+          end
+        end
+      end
     end
 
-    it 'only returns not retried builds' do
-      test_build = create(:ci_build, :test_reports, pipeline: pipeline, project: project)
-      create(:ci_build, :test_reports, :retried, pipeline: pipeline, project: project)
+    context 'when feature flag `include_child_pipeline_jobs_in_reports` is disabled' do
+      before do
+        stub_feature_flags(include_child_pipeline_jobs_in_reports: false)
+      end
+
+      it_behaves_like 'returns report builds from current pipeline'
+
+      context 'when pipeline has reports in child pipelines' do
+        let(:child_pipeline) { create(:ci_pipeline, child_of: pipeline, strategy_depend: true) }
+        let!(:child_build) { create(:ci_build, :test_reports, pipeline: child_pipeline) }
 
-      expect(pipeline.latest_report_builds).to contain_exactly(test_build)
+        it 'returns only builds from parent pipeline' do
+          expect(pipeline.latest_report_builds).to contain_exactly(test_build, coverage_build)
+        end
+      end
     end
   end
 
diff --git a/spec/support/helpers/ci/source_pipeline_helpers.rb b/spec/support/helpers/ci/source_pipeline_helpers.rb
index b99f499cc16f75..04d77eec298d0d 100644
--- a/spec/support/helpers/ci/source_pipeline_helpers.rb
+++ b/spec/support/helpers/ci/source_pipeline_helpers.rb
@@ -2,9 +2,14 @@
 
 module Ci
   module SourcePipelineHelpers
-    def create_source_pipeline(upstream, downstream)
+    def create_source_pipeline(upstream, downstream, strategy: nil)
+      traits = []
+      traits << :strategy_depend if strategy == :depend
+
+      bridge = create(:ci_bridge, *traits, pipeline: upstream)
+
       create(:ci_sources_pipeline,
-             source_job: create(:ci_build, pipeline: upstream),
+             source_bridge: bridge,
              source_project: upstream.project,
              pipeline: downstream,
              project: downstream.project)
-- 
GitLab


From c73bdc0bfca1aa1526dad9c41dcca186b69a8912 Mon Sep 17 00:00:00 2001
From: Fabio Pitino <fpitino@gitlab.com>
Date: Thu, 29 Oct 2020 12:39:53 +0000
Subject: [PATCH 2/2] List pipeline reports from self and child pipelines

Make the parent pipeline the entry point for listing
all the reports generated within its hierarchy as
long as `strategy:depend` was used to link pipelines.

By surfacing only reports from dependent child pipelines
we ensure that a parent pipeline completes always after
the child pipelines generating reports. This eliminates
race conditions with detached child pipelines.
---
 ...daily_build_group_report_result_service.rb |  28 +++--
 .../merge_request_widget_entity_spec.rb       |   1 +
 ..._build_group_report_result_service_spec.rb | 115 ++++++++++++++++++
 3 files changed, 137 insertions(+), 7 deletions(-)

diff --git a/app/services/ci/daily_build_group_report_result_service.rb b/app/services/ci/daily_build_group_report_result_service.rb
index bc966fb9634843..952a4f7d05b7a5 100644
--- a/app/services/ci/daily_build_group_report_result_service.rb
+++ b/app/services/ci/daily_build_group_report_result_service.rb
@@ -3,6 +3,10 @@
 module Ci
   class DailyBuildGroupReportResultService
     def execute(pipeline)
+      # We don't process dangling pipelines (e.g. child pipelines) directly
+      # because they should not affect the ref status.
+      return if include_child_pipeline_reports?(pipeline) && pipeline.dangling?
+
       DailyBuildGroupReportResult.upsert_reports(coverage_reports(pipeline))
     end
 
@@ -17,23 +21,33 @@ def coverage_reports(pipeline)
         default_branch: pipeline.default_branch?
       }
 
-      aggregate(pipeline.builds.with_coverage).map do |group_name, group|
+      builds_with_coverage(pipeline).group_by(&:group_name).map do |group_name, builds|
         base_attrs.merge(
           group_name: group_name,
           data: {
-            'coverage' => average_coverage(group)
+            'coverage' => average_coverage(builds)
           }
         )
       end
     end
 
-    def aggregate(builds)
-      builds.group_by(&:group_name)
+    def builds_with_coverage(pipeline)
+      if include_child_pipeline_reports?(pipeline)
+        # Include any possible builds from dependent child pipelines. Child pipelines
+        # created without `strategy:depend` won't have their reports discovered.
+        pipeline.builds_in_self_and_descendants(while_dependent: true).with_coverage
+      else
+        pipeline.builds_with_coverage
+      end
+    end
+
+    def average_coverage(builds)
+      total_coverage = builds.reduce(0.0) { |sum, build| sum + build.coverage }
+      (total_coverage / builds.size).round(2)
     end
 
-    def average_coverage(group)
-      total_coverage = group.reduce(0.0) { |sum, build| sum + build.coverage }
-      (total_coverage / group.size).round(2)
+    def include_child_pipeline_reports?(pipeline)
+      Gitlab::Ci::Features.include_child_pipeline_jobs_in_reports?(pipeline.project)
     end
   end
 end
diff --git a/ee/spec/serializers/merge_request_widget_entity_spec.rb b/ee/spec/serializers/merge_request_widget_entity_spec.rb
index f9a75e776f53b7..625c43b98d0dee 100644
--- a/ee/spec/serializers/merge_request_widget_entity_spec.rb
+++ b/ee/spec/serializers/merge_request_widget_entity_spec.rb
@@ -31,6 +31,7 @@ def create_all_artifacts
   end
 
   it 'avoids N+1 queries', :request_store do
+    stub_feature_flags(include_child_pipeline_jobs_in_reports: false)
     allow(pipeline).to receive(:available_licensed_report_type?).and_return(true)
     allow(merge_request).to receive_messages(base_pipeline: pipeline, head_pipeline: pipeline)
     create_all_artifacts
diff --git a/spec/services/ci/daily_build_group_report_result_service_spec.rb b/spec/services/ci/daily_build_group_report_result_service_spec.rb
index e54f10cc4f4488..57db0ba574cc6a 100644
--- a/spec/services/ci/daily_build_group_report_result_service_spec.rb
+++ b/spec/services/ci/daily_build_group_report_result_service_spec.rb
@@ -185,4 +185,119 @@
       end
     end
   end
+
+  context 'when pipeline is a child pipeline' do
+    before do
+      pipeline.update!(source: :parent_pipeline)
+    end
+
+    it 'does not do anything' do
+      expect(::Ci::DailyBuildGroupReportResult).not_to receive(:upsert_reports)
+
+      described_class.new.execute(pipeline)
+    end
+  end
+
+  context 'when pipeline has dependent child pipelines containing coverage reports' do
+    let!(:child_pipeline) { create(:ci_pipeline, child_of: pipeline, strategy_depend: true, created_at: '2020-02-06 00:01:10') }
+    let!(:child_rspec_job) { create(:ci_build, pipeline: child_pipeline, name: '2/3 rspec', coverage: 60) }
+    let!(:child_other_job) { create(:ci_build, pipeline: child_pipeline, name: 'other', coverage: 20) }
+
+    it 'finds coverage reports also in its child pipelines' do
+      described_class.new.execute(pipeline)
+
+      Ci::DailyBuildGroupReportResult.find_by(group_name: 'rspec').tap do |coverage|
+        expect(coverage).to have_attributes(
+          project_id: pipeline.project.id,
+          last_pipeline_id: pipeline.id,
+          ref_path: pipeline.source_ref_path,
+          group_name: rspec_job.group_name,
+          data: { 'coverage' => 70 },
+          date: pipeline.created_at.to_date
+        )
+      end
+
+      Ci::DailyBuildGroupReportResult.find_by(group_name: 'other').tap do |coverage|
+        expect(coverage).to have_attributes(
+          project_id: pipeline.project.id,
+          last_pipeline_id: pipeline.id,
+          ref_path: pipeline.source_ref_path,
+          group_name: child_other_job.group_name,
+          data: { 'coverage' => 20 },
+          date: pipeline.created_at.to_date
+        )
+      end
+    end
+  end
+
+  context 'when pipeline has non-dependent child pipelines containing coverage reports' do
+    let!(:child_pipeline) { create(:ci_pipeline, child_of: pipeline, strategy_depend: false, created_at: '2020-02-06 00:01:10') }
+    let!(:child_rspec_job) { create(:ci_build, pipeline: child_pipeline, name: '2/3 rspec', coverage: 60) }
+    let!(:child_other_job) { create(:ci_build, pipeline: child_pipeline, name: 'other', coverage: 20) }
+
+    it 'ignores coverage reports from non-dependent child pipelines' do
+      described_class.new.execute(pipeline)
+
+      Ci::DailyBuildGroupReportResult.find_by(group_name: 'rspec').tap do |coverage|
+        expect(coverage).to have_attributes(
+          project_id: pipeline.project.id,
+          last_pipeline_id: pipeline.id,
+          ref_path: pipeline.source_ref_path,
+          group_name: rspec_job.group_name,
+          data: { 'coverage' => 80 },
+          date: pipeline.created_at.to_date
+        )
+      end
+    end
+  end
+
+  context 'when feature flag `include_child_pipeline_jobs_in_reports` is disabled' do
+    before do
+      stub_feature_flags(include_child_pipeline_jobs_in_reports: false)
+    end
+
+    context 'when pipeline is a child pipeline' do
+      before do
+        pipeline.update!(source: :parent_pipeline)
+      end
+
+      it 'collects the coverage results' do
+        described_class.new.execute(pipeline)
+
+        Ci::DailyBuildGroupReportResult.find_by(group_name: 'rspec').tap do |coverage|
+          expect(coverage).to have_attributes(
+            project_id: pipeline.project.id,
+            last_pipeline_id: pipeline.id,
+            ref_path: pipeline.source_ref_path,
+            group_name: rspec_job.group_name,
+            data: { 'coverage' => 80 },
+            date: pipeline.created_at.to_date
+          )
+        end
+      end
+    end
+
+    context 'when pipeline has dependent child pipelines containing coverage reports' do
+      let!(:child_pipeline) { create(:ci_pipeline, child_of: pipeline, strategy_depend: true, created_at: '2020-02-06 00:01:10') }
+      let!(:child_rspec_job) { create(:ci_build, pipeline: child_pipeline, name: '2/3 rspec', coverage: 60) }
+      let!(:child_other_job) { create(:ci_build, pipeline: child_pipeline, name: 'other', coverage: 20) }
+
+      it 'ignores reports from child pipelines' do
+        described_class.new.execute(pipeline)
+
+        Ci::DailyBuildGroupReportResult.find_by(group_name: 'rspec').tap do |coverage|
+          expect(coverage).to have_attributes(
+            project_id: pipeline.project.id,
+            last_pipeline_id: pipeline.id,
+            ref_path: pipeline.source_ref_path,
+            group_name: rspec_job.group_name,
+            data: { 'coverage' => 80 },
+            date: pipeline.created_at.to_date
+          )
+        end
+
+        expect(Ci::DailyBuildGroupReportResult.find_by(group_name: 'other')).to be_nil
+      end
+    end
+  end
 end
-- 
GitLab