Only link merge requests to successful deployments

Linking merge requests to failed deployments prevents them from being
linked to future deployments, as we only link a merge request once per
environment. In addition, linking to failed deployments isn't very
useful, as an MR technically isn't deployed (because the deploy failed).

This commit changes our MR linking logic so we don't link merge requests
for deployments that did not succeed.

See https://gitlab.com/gitlab-com/gl-infra/delivery/-/issues/1659 for
more information.

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(