Build deploy hook payload in-request

This change builds the deploy web hook payload in the request, rather
than delayed through `Deployments::HooksWorker`.

Once the payload is built, it is sent to the queue via `WebHookWorker`
and the webhook executes on the queue, as normal.

This change is behind a feature flag to allow us to test whether the
change will fix a problem for a particular customer who has some webhook
payloads containing a "status" of "running" when the deployment has
finished, when the status should be "succeeded".

https://gitlab.com/gitlab-org/gitlab/-/issues/355903

app/models/deployment.rb

@@ -109,7 +109,11 @@ class Deployment < ApplicationRecord
     after_transition any => :running do |deployment|
       deployment.run_after_commit do
-        Deployments::HooksWorker.perform_async(deployment_id: id, status_changed_at: Time.current)
+        if Feature.enabled?(:deployment_hooks_skip_worker, deployment.project)
+          deployment.execute_hooks(Time.current)
+        else
+          Deployments::HooksWorker.perform_async(deployment_id: id, status_changed_at: Time.current)
+        end
       end
     end
@@ -123,7 +127,11 @@ class Deployment < ApplicationRecord
     after_transition any => FINISHED_STATUSES do |deployment|
       deployment.run_after_commit do
-        Deployments::HooksWorker.perform_async(deployment_id: id, status_changed_at: Time.current)
+        if Feature.enabled?(:deployment_hooks_skip_worker, deployment.project)
+          deployment.execute_hooks(Time.current)
+        else
+          Deployments::HooksWorker.perform_async(deployment_id: id, status_changed_at: Time.current)
+        end
       end
     end

config/feature_flags/development/deployment_hooks_skip_worker.yml

+---
+name: deployment_hooks_skip_worker
+introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/83351
+rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/356468
+milestone: '14.10'
+type: development
+group: group::integrations
+default_enabled: false

ee/spec/services/ci/process_build_service_spec.rb

@@ -34,7 +34,6 @@
       context 'when user does not have access to the environment' do
         it 'fails the build' do
           allow(Deployments::LinkMergeRequestWorker).to receive(:perform_async)
-          allow(Deployments::HooksWorker).to receive(:perform_async)
           subject
           expect(ci_build.failed?).to be_truthy

spec/models/ci/build_spec.rb

@@ -1367,7 +1367,7 @@
     before do
       allow(Deployments::LinkMergeRequestWorker).to receive(:perform_async)
-      allow(Deployments::HooksWorker).to receive(:perform_async)
+      allow(deployment).to receive(:execute_hooks)
     end
     it 'has deployments record with created status' do
@@ -1423,7 +1423,7 @@
       before do
         allow(Deployments::UpdateEnvironmentWorker).to receive(:perform_async)
-        allow(Deployments::HooksWorker).to receive(:perform_async)
+        allow(deployment).to receive(:execute_hooks)
       end
       it_behaves_like 'avoid deadlock'
@@ -1509,14 +1509,28 @@
         it 'transitions to running and calls webhook' do
           freeze_time do
-            expect(Deployments::HooksWorker)
-            .to receive(:perform_async).with(deployment_id: deployment.id, status_changed_at: Time.current)
+            expect(deployment).to receive(:execute_hooks).with(Time.current)
             subject
           end
           expect(deployment).to be_running
         end
+
+        context 'when `deployment_hooks_skip_worker` flag is disabled' do
+          before do
+            stub_feature_flags(deployment_hooks_skip_worker: false)
+          end
+
+          it 'executes Deployments::HooksWorker asynchronously' do
+            freeze_time do
+              expect(Deployments::HooksWorker)
+                .to receive(:perform_async).with(deployment_id: deployment.id, status_changed_at: Time.current)
+
+              subject
+            end
+          end
+        end
       end
     end
   end

spec/models/deployment_spec.rb

@@ -137,15 +137,29 @@
         end
       end
-      it 'executes Deployments::HooksWorker asynchronously' do
+      it 'executes deployment hooks' do
         freeze_time do
-          expect(Deployments::HooksWorker)
-            .to receive(:perform_async).with(deployment_id: deployment.id, status_changed_at: Time.current)
+          expect(deployment).to receive(:execute_hooks).with(Time.current)
           deployment.run!
         end
       end
+      context 'when `deployment_hooks_skip_worker` flag is disabled' do
+        before do
+          stub_feature_flags(deployment_hooks_skip_worker: false)
+        end
+
+        it 'executes Deployments::HooksWorker asynchronously' do
+          freeze_time do
+            expect(Deployments::HooksWorker)
+              .to receive(:perform_async).with(deployment_id: deployment.id, status_changed_at: Time.current)
+
+            deployment.run!
+          end
+        end
+      end
+
       it 'executes Deployments::DropOlderDeploymentsWorker asynchronously' do
         expect(Deployments::DropOlderDeploymentsWorker)
             .to receive(:perform_async).once.with(deployment.id)
@@ -173,14 +187,28 @@
         deployment.succeed!
       end
-      it 'executes Deployments::HooksWorker asynchronously' do
+      it 'executes deployment hooks' do
         freeze_time do
-          expect(Deployments::HooksWorker)
-          .to receive(:perform_async).with(deployment_id: deployment.id, status_changed_at: Time.current)
+          expect(deployment).to receive(:execute_hooks).with(Time.current)
           deployment.succeed!
         end
       end
+
+      context 'when `deployment_hooks_skip_worker` flag is disabled' do
+        before do
+          stub_feature_flags(deployment_hooks_skip_worker: false)
+        end
+
+        it 'executes Deployments::HooksWorker asynchronously' do
+          freeze_time do
+            expect(Deployments::HooksWorker)
+            .to receive(:perform_async).with(deployment_id: deployment.id, status_changed_at: Time.current)
+
+            deployment.succeed!
+          end
+        end
+      end
     end
     context 'when deployment failed' do
@@ -202,14 +230,28 @@
         deployment.drop!
       end
-      it 'executes Deployments::HooksWorker asynchronously' do
+      it 'executes deployment hooks' do
         freeze_time do
-          expect(Deployments::HooksWorker)
-            .to receive(:perform_async).with(deployment_id: deployment.id, status_changed_at: Time.current)
+          expect(deployment).to receive(:execute_hooks).with(Time.current)
           deployment.drop!
         end
       end
+
+      context 'when `deployment_hooks_skip_worker` flag is disabled' do
+        before do
+          stub_feature_flags(deployment_hooks_skip_worker: false)
+        end
+
+        it 'executes Deployments::HooksWorker asynchronously' do
+          freeze_time do
+            expect(Deployments::HooksWorker)
+              .to receive(:perform_async).with(deployment_id: deployment.id, status_changed_at: Time.current)
+
+            deployment.drop!
+          end
+        end
+      end
     end
     context 'when deployment was canceled' do
@@ -231,14 +273,28 @@
         deployment.cancel!
       end
-      it 'executes Deployments::HooksWorker asynchronously' do
+      it 'executes deployment hooks' do
         freeze_time do
-          expect(Deployments::HooksWorker)
-            .to receive(:perform_async).with(deployment_id: deployment.id, status_changed_at: Time.current)
+          expect(deployment).to receive(:execute_hooks).with(Time.current)
           deployment.cancel!
         end
       end
+
+      context 'when `deployment_hooks_skip_worker` flag is disabled' do
+        before do
+          stub_feature_flags(deployment_hooks_skip_worker: false)
+        end
+
+        it 'executes Deployments::HooksWorker asynchronously' do
+          freeze_time do
+            expect(Deployments::HooksWorker)
+              .to receive(:perform_async).with(deployment_id: deployment.id, status_changed_at: Time.current)
+
+            deployment.cancel!
+          end
+        end
+      end
     end
     context 'when deployment was skipped' do
@@ -266,6 +322,12 @@
           deployment.skip!
         end
       end
+
+      it 'does not execute deployment hooks' do
+        expect(deployment).not_to receive(:execute_hooks)
+
+        deployment.skip!
+      end
     end
     context 'when deployment is blocked' do
@@ -289,6 +351,12 @@
         deployment.block!
       end
+
+      it 'does not execute deployment hooks' do
+        expect(deployment).not_to receive(:execute_hooks)
+
+        deployment.block!
+      end
     end
     describe 'synching status to Jira' do
@@ -845,11 +913,30 @@
       expect(Deployments::UpdateEnvironmentWorker).to receive(:perform_async)
       expect(Deployments::LinkMergeRequestWorker).to receive(:perform_async)
       expect(Deployments::ArchiveInProjectWorker).to receive(:perform_async)
-      expect(Deployments::HooksWorker).to receive(:perform_async)
       expect(deploy.update_status('success')).to eq(true)
     end
+    context 'when `deployment_hooks_skip_worker` flag is disabled' do
+      before do
+        stub_feature_flags(deployment_hooks_skip_worker: false)
+      end
+
+      it 'schedules `Deployments::HooksWorker` when finishing a deploy' do
+        expect(Deployments::HooksWorker).to receive(:perform_async)
+
+        deploy.update_status('success')
+      end
+    end
+
+    it 'executes deployment hooks when finishing a deploy' do
+      freeze_time do
+        expect(deploy).to receive(:execute_hooks).with(Time.current)
+
+        deploy.update_status('success')
+      end
+    end
+
     it 'updates finished_at when transitioning to a finished status' do
       freeze_time do
         deploy.update_status('success')

spec/services/deployments/create_service_spec.rb

@@ -21,11 +21,34 @@
       expect(Deployments::UpdateEnvironmentWorker).to receive(:perform_async)
       expect(Deployments::LinkMergeRequestWorker).to receive(:perform_async)
-      expect(Deployments::HooksWorker).to receive(:perform_async)
+      expect_next_instance_of(Deployment) do |deployment|
+        expect(deployment).to receive(:execute_hooks)
+      end
       expect(service.execute).to be_persisted
     end
+    context 'when `deployment_hooks_skip_worker` flag is disabled' do
+      before do
+        stub_feature_flags(deployment_hooks_skip_worker: false)
+      end
+
+      it 'executes Deployments::HooksWorker asynchronously' do
+        service = described_class.new(
+          environment,
+          user,
+          sha: 'b83d6e391c22777fca1ed3012fce84f633d7fed0',
+          ref: 'master',
+          tag: false,
+          status: 'success'
+        )
+
+        expect(Deployments::HooksWorker).to receive(:perform_async)
+
+        service.execute
+      end
+    end
+
     it 'does not change the status if no status is given' do
       service = described_class.new(
         environment,
@@ -37,7 +60,9 @@
       expect(Deployments::UpdateEnvironmentWorker).not_to receive(:perform_async)
       expect(Deployments::LinkMergeRequestWorker).not_to receive(:perform_async)
-      expect(Deployments::HooksWorker).not_to receive(:perform_async)
+      expect_next_instance_of(Deployment) do |deployment|
+        expect(deployment).not_to receive(:execute_hooks)
+      end
       expect(service.execute).to be_persisted
     end
@@ -55,11 +80,9 @@
       it 'does not create a new deployment' do
         described_class.new(environment, user, params).execute
-        expect(Deployments::UpdateEnvironmentWorker).not_to receive(:perform_async)
-        expect(Deployments::LinkMergeRequestWorker).not_to receive(:perform_async)
-        expect(Deployments::HooksWorker).not_to receive(:perform_async)
-
-        described_class.new(environment.reload, user, params).execute
+        expect do
+          described_class.new(environment.reload, user, params).execute
+        end.not_to change { Deployment.count }
       end
     end
   end

spec/services/deployments/update_environment_service_spec.rb

@@ -33,7 +33,7 @@
   before do
     allow(Deployments::LinkMergeRequestWorker).to receive(:perform_async)
-    allow(Deployments::HooksWorker).to receive(:perform_async)
+    allow(deployment).to receive(:execute_hooks)
     job.success! # Create/Succeed deployment
   end