Merge branch 'fix-child-parent-pipeline-for-mr' into 'master'

Fix merge request child pipelines

See merge request gitlab-org/gitlab!23884

app/models/ci/bridge.rb

@@ -186,7 +186,8 @@ module Ci
         },
         execute_params: {
           ignore_skip_ci: true,
-          bridge: self
+          bridge: self,
+          merge_request: parent_pipeline.merge_request
         }
       }
     end

app/models/ci/pipeline.rb

@@ -77,9 +77,7 @@ module Ci
 
     validates :sha, presence: { unless: :importing? }
     validates :ref, presence: { unless: :importing? }
-    validates :merge_request, presence: { if: :merge_request_event? }
-    validates :merge_request, absence: { unless: :merge_request_event? }
-    validates :tag, inclusion: { in: [false], if: :merge_request_event? }
+    validates :tag, inclusion: { in: [false], if: :merge_request? }
 
     validates :external_pull_request, presence: { if: :external_pull_request_event? }
     validates :external_pull_request, absence: { unless: :external_pull_request_event? }
@@ -662,7 +660,7 @@ module Ci
 
         variables.concat(predefined_commit_variables)
 
-        if merge_request_event? && merge_request
+        if merge_request?
           variables.append(key: 'CI_MERGE_REQUEST_EVENT_TYPE', value: merge_request_event_type.to_s)
           variables.append(key: 'CI_MERGE_REQUEST_SOURCE_BRANCH_SHA', value: source_sha.to_s)
           variables.append(key: 'CI_MERGE_REQUEST_TARGET_BRANCH_SHA', value: target_sha.to_s)
@@ -720,7 +718,7 @@ module Ci
     # All the merge requests for which the current pipeline runs/ran against
     def all_merge_requests
       @all_merge_requests ||=
-        if merge_request_event?
+        if merge_request?
           MergeRequest.where(id: merge_request_id)
         else
           MergeRequest.where(source_project_id: project_id, source_branch: ref)
@@ -812,7 +810,7 @@ module Ci
     # * nil: Modified path can not be evaluated
     def modified_paths
       strong_memoize(:modified_paths) do
-        if merge_request_event?
+        if merge_request?
           merge_request.modified_paths
         elsif branch_updated?
           push_details.modified_paths
@@ -836,12 +834,12 @@ module Ci
       ref == project.default_branch
     end
 
-    def triggered_by_merge_request?
-      merge_request_event? && merge_request_id.present?
+    def merge_request?
+      merge_request_id.present?
     end
 
     def detached_merge_request_pipeline?
-      triggered_by_merge_request? && target_sha.nil?
+      merge_request? && target_sha.nil?
     end
 
     def legacy_detached_merge_request_pipeline?
@@ -849,7 +847,7 @@ module Ci
     end
 
     def merge_request_pipeline?
-      triggered_by_merge_request? && target_sha.present?
+      merge_request? && target_sha.present?
     end
 
     def merge_request_ref?
@@ -865,7 +863,7 @@ module Ci
     end
 
     def source_ref
-      if triggered_by_merge_request?
+      if merge_request?
         merge_request.source_branch
       else
         ref
@@ -885,7 +883,7 @@ module Ci
     end
 
     def merge_request_event_type
-      return unless merge_request_event?
+      return unless merge_request?
 
       strong_memoize(:merge_request_event_type) do
         if merge_request_pipeline?
@@ -918,7 +916,7 @@ module Ci
 
     def git_ref
       strong_memoize(:git_ref) do
-        if merge_request_event?
+        if merge_request?
           ##
           # In the future, we're going to change this ref to
           # merge request's merged reference, such as "refs/merge-requests/:iid/merge".

app/models/concerns/ci/pipeline_delegator.rb

@@ -11,7 +11,7 @@ module Ci
     extend ActiveSupport::Concern
 
     included do
-      delegate :merge_request_event?,
+      delegate :merge_request?,
                :merge_request_ref?,
                :legacy_detached_merge_request_pipeline?,
                :merge_train_pipeline?, to: :pipeline

app/models/concerns/has_ref.rb

@@ -7,7 +7,7 @@ module HasRef
   extend ActiveSupport::Concern
 
   def branch?
-    !tag? && !merge_request_event?
+    !tag? && !merge_request?
   end
 
   def git_ref

app/models/merge_request.rb

@@ -1163,7 +1163,7 @@ class MergeRequest < ApplicationRecord
   # Since deployments run on a merge request ref (e.g. `refs/merge-requests/:iid/head`),
   # we cannot look up environments with source branch name.
   def environments
-    return Environment.none unless actual_head_pipeline&.triggered_by_merge_request?
+    return Environment.none unless actual_head_pipeline&.merge_request?
 
     actual_head_pipeline.environments
   end

app/models/merge_request/pipelines.rb

@@ -61,6 +61,8 @@ class MergeRequest::Pipelines
     pipelines.joins(shas_table)
   end
 
+  # NOTE: this method returns only parent merge request pipelines.
+  # Child merge request pipelines have a different source.
   def triggered_by_merge_request
     source_project.ci_pipelines
       .where(source: :merge_request_event, merge_request: merge_request)

app/presenters/ci/pipeline_presenter.rb

@@ -116,7 +116,7 @@ module Ci
 
     def merge_request_presenter
       strong_memoize(:merge_request_presenter) do
-        if pipeline.triggered_by_merge_request?
+        if pipeline.merge_request?
           pipeline.merge_request.present(current_user: current_user)
         end
       end

app/serializers/pipeline_entity.rb

@@ -25,7 +25,7 @@ class PipelineEntity < Grape::Entity
   expose :flags do
     expose :stuck?, as: :stuck
     expose :auto_devops_source?, as: :auto_devops
-    expose :merge_request_event?, as: :merge_request
+    expose :merge_request?, as: :merge_request
     expose :has_yaml_errors?, as: :yaml_errors
     expose :can_retry?, as: :retryable
     expose :can_cancel?, as: :cancelable
@@ -59,11 +59,11 @@ class PipelineEntity < Grape::Entity
 
     expose :tag?, as: :tag
     expose :branch?, as: :branch
-    expose :merge_request_event?, as: :merge_request
+    expose :merge_request?, as: :merge_request
   end
 
   expose :commit, using: CommitEntity
-  expose :merge_request_event_type, if: -> (pipeline, _) { pipeline.merge_request_event? }
+  expose :merge_request_event_type, if: -> (pipeline, _) { pipeline.merge_request? }
   expose :source_sha, if: -> (pipeline, _) { pipeline.merge_request_pipeline? }
   expose :target_sha, if: -> (pipeline, _) { pipeline.merge_request_pipeline? }
   expose :yaml_errors, if: -> (pipeline, _) { pipeline.has_yaml_errors? }
@@ -104,7 +104,7 @@ class PipelineEntity < Grape::Entity
   end
 
   def has_presentable_merge_request?
-    pipeline.triggered_by_merge_request? &&
+    pipeline.merge_request? &&
       can?(request.current_user, :read_merge_request, pipeline.merge_request)
   end
 

app/services/merge_requests/create_pipeline_service.rb

@@ -24,7 +24,7 @@ module MergeRequests
       ##
       # UpdateMergeRequestsWorker could be retried by an exception.
       # pipelines for merge request should not be recreated in such case.
-      return false if !allow_duplicate && merge_request.find_actual_head_pipeline&.triggered_by_merge_request?
+      return false if !allow_duplicate && merge_request.find_actual_head_pipeline&.merge_request?
       return false if merge_request.has_no_commits?
 
       true

changelogs/unreleased/fix-child-parent-pipeline-for-mr.yml

+---
+title: Allow running child pipelines as merge request pipelines
+merge_request: 23884
+author:
+type: fixed

ee/app/models/ee/ci/pipeline.rb

@@ -132,7 +132,7 @@ module EE
 
       override :merge_request_event_type
       def merge_request_event_type
-        return unless merge_request_event?
+        return unless merge_request?
 
         strong_memoize(:merge_request_event_type) do
           merge_train_pipeline? ? :merge_train : super

ee/spec/services/ee/merge_requests/create_service_spec.rb

@@ -36,7 +36,7 @@ describe MergeRequests::CreateService do
 
     context 'report approvers' do
       let(:sha) { project.repository.commits(opts[:source_branch], limit: 1).first.id }
-      let(:pipeline) { instance_double(Ci::Pipeline, id: 42, project_id: project.id, triggered_by_merge_request?: true) }
+      let(:pipeline) { instance_double(Ci::Pipeline, id: 42, project_id: project.id, merge_request?: true) }
 
       it 'refreshes report approvers for the merge request' do
         expect_next_instance_of(::MergeRequests::SyncReportApproverApprovalRules) do |service|

spec/models/ci/build_spec.rb

@@ -33,7 +33,7 @@ describe Ci::Build do
   it { is_expected.to respond_to(:has_trace?) }
   it { is_expected.to respond_to(:trace) }
 
-  it { is_expected.to delegate_method(:merge_request_event?).to(:pipeline) }
+  it { is_expected.to delegate_method(:merge_request?).to(:pipeline) }
   it { is_expected.to delegate_method(:merge_request_ref?).to(:pipeline) }
   it { is_expected.to delegate_method(:legacy_detached_merge_request_pipeline?).to(:pipeline) }
 

spec/models/ci/pipeline_spec.rb

@@ -162,6 +162,23 @@ describe Ci::Pipeline, :mailer do
     end
   end
 
+  describe '#merge_request?' do
+    let(:pipeline) { create(:ci_pipeline, merge_request: merge_request) }
+    let(:merge_request) { create(:merge_request) }
+
+    it 'returns true' do
+      expect(pipeline).to be_merge_request
+    end
+
+    context 'when merge request is nil' do
+      let(:merge_request) { nil }
+
+      it 'returns false' do
+        expect(pipeline).not_to be_merge_request
+      end
+    end
+  end
+
   describe '#detached_merge_request_pipeline?' do
     subject { pipeline.detached_merge_request_pipeline? }
 
@@ -367,48 +384,6 @@ describe Ci::Pipeline, :mailer do
     end
   end
 
-  describe 'Validations for merge request pipelines' do
-    let(:pipeline) do
-      build(:ci_pipeline, source: source, merge_request: merge_request)
-    end
-
-    let(:merge_request) do
-      create(:merge_request,
-        source_project: project,
-        source_branch:  'feature',
-        target_project: project,
-        target_branch:  'master')
-    end
-
-    context 'when source is merge request' do
-      let(:source) { :merge_request_event }
-
-      context 'when merge request is specified' do
-        it { expect(pipeline).to be_valid }
-      end
-
-      context 'when merge request is empty' do
-        let(:merge_request) { nil }
-
-        it { expect(pipeline).not_to be_valid }
-      end
-    end
-
-    context 'when source is web' do
-      let(:source) { :web }
-
-      context 'when merge request is specified' do
-        it { expect(pipeline).not_to be_valid }
-      end
-
-      context 'when merge request is empty' do
-        let(:merge_request) { nil }
-
-        it { expect(pipeline).to be_valid }
-      end
-    end
-  end
-
   describe 'modules' do
     it_behaves_like 'AtomicInternalId', validate_presence: false do
       let(:internal_id_attribute) { :iid }
@@ -612,9 +587,9 @@ describe Ci::Pipeline, :mailer do
       ]
     end
 
-    context 'when source is merge request' do
+    context 'when pipeline is merge request' do
       let(:pipeline) do
-        create(:ci_pipeline, source: :merge_request_event, merge_request: merge_request)
+        create(:ci_pipeline, merge_request: merge_request)
       end
 
       let(:merge_request) do
@@ -651,7 +626,7 @@ describe Ci::Pipeline, :mailer do
             'CI_MERGE_REQUEST_TITLE' => merge_request.title,
             'CI_MERGE_REQUEST_ASSIGNEES' => merge_request.assignee_username_list,
             'CI_MERGE_REQUEST_MILESTONE' => milestone.title,
-            'CI_MERGE_REQUEST_LABELS' => labels.map(&:title).join(','),
+            'CI_MERGE_REQUEST_LABELS' => labels.map(&:title).sort.join(','),
             'CI_MERGE_REQUEST_EVENT_TYPE' => pipeline.merge_request_event_type.to_s)
       end
 
@@ -1263,9 +1238,9 @@ describe Ci::Pipeline, :mailer do
         is_expected.to be_truthy
       end
 
-      context 'when source is merge request' do
+      context 'when pipeline is merge request' do
         let(:pipeline) do
-          create(:ci_pipeline, source: :merge_request_event, merge_request: merge_request)
+          create(:ci_pipeline, merge_request: merge_request)
         end
 
         let(:merge_request) do

spec/services/ci/create_cross_project_pipeline_service_spec.rb

@@ -148,6 +148,12 @@ describe Ci::CreateCrossProjectPipelineService, '#execute' do
       end
 
       context 'when "include" is provided' do
+        let(:file_content) do
+          YAML.dump(
+            rspec: { script: 'rspec' },
+            echo: { script: 'echo' })
+        end
+
         shared_examples 'creates a child pipeline' do
           it 'creates only one new pipeline' do
             expect { service.execute(bridge) }
@@ -189,9 +195,6 @@ describe Ci::CreateCrossProjectPipelineService, '#execute' do
         end
 
         before do
-          file_content = YAML.dump(
-            rspec: { script: 'rspec' },
-            echo: { script: 'echo' })
           upstream_project.repository.create_file(
             user, 'child-pipeline.yml', file_content, message: 'message', branch_name: 'master')
 
@@ -218,6 +221,29 @@ describe Ci::CreateCrossProjectPipelineService, '#execute' do
           it_behaves_like 'creates a child pipeline'
         end
 
+        context 'when the parent is a merge request pipeline' do
+          let(:merge_request) { create(:merge_request, source_project: bridge.project, target_project: bridge.project) }
+          let(:file_content) do
+            YAML.dump(
+              workflow: { rules: [{ if: '$CI_MERGE_REQUEST_ID' }] },
+              rspec: { script: 'rspec' },
+              echo: { script: 'echo' })
+          end
+
+          before do
+            bridge.pipeline.update!(source: :merge_request_event, merge_request: merge_request)
+          end
+
+          it_behaves_like 'creates a child pipeline'
+
+          it 'propagates the merge request to the child pipeline' do
+            pipeline = service.execute(bridge)
+
+            expect(pipeline.merge_request).to eq(merge_request)
+            expect(pipeline).to be_merge_request
+          end
+        end
+
         context 'when upstream pipeline is a child pipeline' do
           let!(:pipeline_source) do
             create(:ci_sources_pipeline,

spec/services/ci/create_pipeline_service_spec.rb

@@ -1473,15 +1473,6 @@ describe Ci::CreatePipelineService do
               end
             end
           end
-
-          context 'when merge request is not specified' do
-            let(:merge_request) { nil }
-
-            it 'does not create a detached merge request pipeline' do
-              expect(pipeline).not_to be_persisted
-              expect(pipeline.errors[:merge_request]).to eq(["can't be blank"])
-            end
-          end
         end
 
         context "when config does not have merge_requests keywords" do
@@ -1518,17 +1509,6 @@ describe Ci::CreatePipelineService do
                 .to eq(['No stages / jobs for this pipeline.'])
             end
           end
-
-          context 'when merge request is not specified' do
-            let(:merge_request) { nil }
-
-            it 'does not create a detached merge request pipeline' do
-              expect(pipeline).not_to be_persisted
-
-              expect(pipeline.errors[:base])
-                .to eq(['No stages / jobs for this pipeline.'])
-            end
-          end
         end
 
         context "when config uses regular expression for only keyword" do
@@ -1623,6 +1603,7 @@ describe Ci::CreatePipelineService do
 
       context 'when source is web' do
         let(:source) { :web }
+        let(:merge_request) { nil }
 
         context "when config has merge_requests keywords" do
           let(:config) do
@@ -1644,30 +1625,11 @@ describe Ci::CreatePipelineService do
             }
           end
 
-          context 'when merge request is specified' do
-            let(:merge_request) do
-              create(:merge_request,
-                source_project: project,
-                source_branch: Gitlab::Git.ref_name(ref_name),
-                target_project: project,
-                target_branch: 'master')
-            end
-
-            it 'does not create a merge request pipeline' do
-              expect(pipeline).not_to be_persisted
-              expect(pipeline.errors[:merge_request]).to eq(["must be blank"])
-            end
-          end
-
-          context 'when merge request is not specified' do
-            let(:merge_request) { nil }
-
-            it 'creates a branch pipeline' do
-              expect(pipeline).to be_persisted
-              expect(pipeline).to be_web
-              expect(pipeline.merge_request).to be_nil
-              expect(pipeline.builds.order(:stage_id).pluck(:name)).to eq(%w[build pages])
-            end
+          it 'creates a branch pipeline' do
+            expect(pipeline).to be_persisted
+            expect(pipeline).to be_web
+            expect(pipeline.merge_request).to be_nil
+            expect(pipeline.builds.order(:stage_id).pluck(:name)).to eq(%w[build pages])
           end
         end
       end