Link merge requests for all non review app deploys

This changes how we link merge requests to deployments so that:

1. We link merge requests for any deployment that is not in the
   "created" state.
2. We do not link merge requests to review app deployments.

We want to link deployments regardless of their status so that we can
also show failed and running deploys in the merge request widget (and
perhaps in other places). We don't want to link review app deployments
since this is not useful, and may lead to deployments to different
review app environments showing up on a merge request page.

This builds on the refactoring of the deployment model introduced in
merge request https://gitlab.com/gitlab-org/gitlab/merge_requests/20474,
and moves the linking of merge requests to Deployments::FinishedWorker.
This worker is scheduled every time a deployment transitions to a
finished state.

As part of these changes we also change Deployment#link_merge_requests
so that it ignores any already linked merge requests. This way if a
deployment changes its status multiple times, we don't error with
duplicate key errors.

This fixes https://gitlab.com/gitlab-org/gitlab/issues/38092.

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