Merge branch 'fix-linking-merge-requests' into 'master'

Link merge requests for all non review app deploys

Closes #38000 and #38092

See merge request gitlab-org/gitlab!21254

app/models/deployment.rb

@@ -212,10 +212,14 @@ class Deployment < ApplicationRecord
 
     # We don't use `Gitlab::Database.bulk_insert` here so that we don't need to
     # first pluck lots of IDs into memory.
+    #
+    # We also ignore any duplicates so this method can be called multiple times
+    # for the same deployment, only inserting any missing merge requests.
     DeploymentMergeRequest.connection.execute(<<~SQL)
       INSERT INTO #{DeploymentMergeRequest.table_name}
       (merge_request_id, deployment_id)
       #{select}
+      ON CONFLICT DO NOTHING
     SQL
   end
 

app/services/deployments/after_create_service.rb

@@ -34,21 +34,12 @@ module Deployments
 
         if environment.save && !environment.stopped?
           deployment.update_merge_request_metrics!
-          link_merge_requests(deployment)
         end
       end
     end
 
     private
 
-    def link_merge_requests(deployment)
-      unless Feature.enabled?(:deployment_merge_requests, deployment.project)
-        return
-      end
-
-      LinkMergeRequestsService.new(deployment).execute
-    end
-
     def environment_options
       options&.dig(:environment) || {}
     end

app/services/deployments/link_merge_requests_service.rb

@@ -13,7 +13,10 @@ module Deployments
     end
 
     def execute
-      return unless deployment.success?
+      # Review apps have the environment type set (e.g. to `review`, though the
+      # exact value may differ). We don't want to link merge requests to review
+      # app deployments, as this is not useful.
+      return if deployment.environment.environment_type
 
       if (prev = deployment.previous_environment_deployment)
         link_merge_requests_for_range(prev.sha, deployment.sha)

app/workers/deployments/finished_worker.rb

@@ -9,7 +9,18 @@ module Deployments
     worker_resource_boundary :cpu
 
     def perform(deployment_id)
-      Deployment.find_by_id(deployment_id).try(:execute_hooks)
+      if (deploy = Deployment.find_by_id(deployment_id))
+        link_merge_requests(deploy)
+        deploy.execute_hooks
+      end
+    end
+
+    def link_merge_requests(deployment)
+      unless Feature.enabled?(:deployment_merge_requests, deployment.project)
+        return
+      end
+
+      LinkMergeRequestsService.new(deployment).execute
     end
   end
 end

changelogs/unreleased/fix-linking-merge-requests.yml

+---
+title: Enable the linking of merge requests to all non review app deployments
+merge_request:
+author:
+type: added

spec/models/deployment_spec.rb

@@ -399,6 +399,29 @@ describe Deployment do
 
       expect(deploy.merge_requests).to include(mr1, mr2)
     end
+
+    it 'ignores already linked merge requests' do
+      deploy = create(:deployment)
+      mr1 = create(
+        :merge_request,
+        :merged,
+        target_project: deploy.project,
+        source_project: deploy.project
+      )
+
+      deploy.link_merge_requests(deploy.project.merge_requests)
+
+      mr2 = create(
+        :merge_request,
+        :merged,
+        target_project: deploy.project,
+        source_project: deploy.project
+      )
+
+      deploy.link_merge_requests(deploy.project.merge_requests)
+
+      expect(deploy.merge_requests).to include(mr1, mr2)
+    end
   end
 
   describe '#previous_environment_deployment' do

spec/services/deployments/after_create_service_spec.rb

@@ -61,14 +61,6 @@ describe Deployments::AfterCreateService do
       service.execute
     end
 
-    it 'links merge requests to deployment' do
-      expect_next_instance_of(Deployments::LinkMergeRequestsService, deployment) do |link_mr_service|
-        expect(link_mr_service).to receive(:execute)
-      end
-
-      service.execute
-    end
-
     it 'returns the deployment' do
       expect(subject.execute).to eq(deployment)
     end
@@ -272,30 +264,4 @@ describe Deployments::AfterCreateService do
       end
     end
   end
-
-  describe '#update_environment' do
-    it 'links the merge requests' do
-      double = instance_double(Deployments::LinkMergeRequestsService)
-
-      allow(Deployments::LinkMergeRequestsService)
-        .to receive(:new)
-        .with(deployment)
-        .and_return(double)
-
-      expect(double).to receive(:execute)
-
-      service.update_environment(deployment)
-    end
-
-    context 'when the tracking of merge requests is disabled' do
-      it 'does nothing' do
-        stub_feature_flags(deployment_merge_requests: false)
-
-        expect(Deployments::LinkMergeRequestsService)
-          .not_to receive(:new)
-
-        service.update_environment(deployment)
-      end
-    end
-  end
 end

spec/services/deployments/link_merge_requests_service_spec.rb

@@ -6,9 +6,12 @@ describe Deployments::LinkMergeRequestsService do
   let(:project) { create(:project, :repository) }
 
   describe '#execute' do
-    context 'when the deployment did not succeed' do
+    context 'when the deployment is for a review environment' do
       it 'does nothing' do
-        deploy = create(:deployment, :failed)
+        environment =
+          create(:environment, environment_type: 'review', name: 'review/foo')
+
+        deploy = create(:deployment, :success, environment: environment)
 
         expect(deploy).not_to receive(:link_merge_requests)
 

spec/workers/deployments/finished_worker_spec.rb

@@ -10,6 +10,20 @@ describe Deployments::FinishedWorker do
       allow(ProjectServiceWorker).to receive(:perform_async)
     end
 
+    it 'links merge requests to the deployment' do
+      deployment = create(:deployment)
+      service = instance_double(Deployments::LinkMergeRequestsService)
+
+      expect(Deployments::LinkMergeRequestsService)
+        .to receive(:new)
+        .with(deployment)
+        .and_return(service)
+
+      expect(service).to receive(:execute)
+
+      worker.perform(deployment.id)
+    end
+
     it 'executes project services for deployment_hooks' do
       deployment = create(:deployment)
       project = deployment.project
@@ -35,5 +49,17 @@ describe Deployments::FinishedWorker do
 
       expect(ProjectServiceWorker).not_to have_received(:perform_async)
     end
+
+    context 'when the tracking of merge requests is disabled' do
+      it 'does not track the deployed merge requests' do
+        stub_feature_flags(deployment_merge_requests: false)
+
+        deployment = create(:deployment)
+
+        expect(Deployments::LinkMergeRequestsService).not_to receive(:new)
+
+        worker.perform(deployment.id)
+      end
+    end
   end
 end