Merge branch 'link-mrs-failed-deploys' into 'master'

Only link merge requests to successful deployments

See merge request gitlab-org/gitlab!58072

app/models/deployment.rb

@@ -94,11 +94,6 @@ class Deployment < ApplicationRecord
     after_transition any => :success do |deployment|
       deployment.run_after_commit do
         Deployments::UpdateEnvironmentWorker.perform_async(id)
-      end
-    end
-
-    after_transition any => FINISHED_STATUSES do |deployment|
-      deployment.run_after_commit do
         Deployments::LinkMergeRequestWorker.perform_async(id)
       end
     end

app/services/deployments/link_merge_requests_service.rb

@@ -18,6 +18,21 @@ module Deployments
       # app deployments, as this is not useful.
       return if deployment.environment.environment_type
 
+      # This service is triggered by a Sidekiq worker, which only runs when a
+      # deployment is successful. We add an extra check here in case we ever
+      # call this service elsewhere and forget to check the status there.
+      #
+      # The reason we only want to link successful deployments is as follows:
+      # when we link a merge request, we don't link it to future deployments for
+      # the same environment. If we were to link an MR to a failed deploy, we
+      # wouldn't be able to later on link it to a successful deploy (e.g. after
+      # the deploy is retried).
+      #
+      # In addition, showing failed deploys in the UI of a merge request isn't
+      # useful to users, as they can't act upon the information in any
+      # meaningful way (i.e. they can't just retry the deploy themselves).
+      return unless deployment.success?
+
       if (prev = deployment.previous_environment_deployment)
         link_merge_requests_for_range(prev.sha, deployment.sha)
       else

changelogs/unreleased/link-mrs-failed-deploys.yml

+---
+title: Only link merge requests to successful deployments
+merge_request: 58072
+author:
+type: fixed

spec/models/deployment_spec.rb

@@ -161,9 +161,9 @@ RSpec.describe Deployment do
         end
       end
 
-      it 'executes Deployments::LinkMergeRequestWorker asynchronously' do
+      it 'does not execute Deployments::LinkMergeRequestWorker' do
         expect(Deployments::LinkMergeRequestWorker)
-          .to receive(:perform_async).with(deployment.id)
+          .not_to receive(:perform_async).with(deployment.id)
 
         deployment.drop!
       end
@@ -188,9 +188,9 @@ RSpec.describe Deployment do
         end
       end
 
-      it 'executes Deployments::LinkMergeRequestWorker asynchronously' do
+      it 'does not execute Deployments::LinkMergeRequestWorker' do
         expect(Deployments::LinkMergeRequestWorker)
-          .to receive(:perform_async).with(deployment.id)
+          .not_to receive(:perform_async).with(deployment.id)
 
         deployment.cancel!
       end

spec/services/deployments/link_merge_requests_service_spec.rb

@@ -32,6 +32,17 @@ RSpec.describe Deployments::LinkMergeRequestsService do
       end
     end
 
+    context 'when the deployment failed' do
+      it 'does nothing' do
+        environment = create(:environment, name: 'foo')
+        deploy = create(:deployment, :failed, environment: environment)
+
+        expect(deploy).not_to receive(:link_merge_requests)
+
+        described_class.new(deploy).execute
+      end
+    end
+
     context 'when there is a previous deployment' do
       it 'links all merge requests merged since the previous deployment' do
         deploy1 = create(