From 4930dfc2b917433e041762b65bd503d8e4b090b6 Mon Sep 17 00:00:00 2001
From: Krasimir Angelov <kangelov@gitlab.com>
Date: Wed, 19 Jun 2019 13:47:48 +1200
Subject: Delay creating build's environment and deployment

Instead of creating environment and deployment on build creation add
this as a new prerequisite for builds. This way we are not going to
unnecessary create records (e.g. for builds with manual start).

Fix specs that assume environment and deployment will be created
immediately when build is created.

Related to https://gitlab.com/gitlab-org/gitlab-ce/issues/27644.
---
 app/models/concerns/deployable.rb                  |  2 -
 lib/gitlab/ci/build/prerequisite/deployment.rb     | 21 +++++++++
 lib/gitlab/ci/build/prerequisite/factory.rb        |  5 +-
 .../ci/build/prerequisite/deployment_spec.rb       | 54 ++++++++++++++++++++++
 .../gitlab/ci/build/prerequisite/factory_spec.rb   | 37 +++++++++------
 spec/models/ci/build_spec.rb                       | 45 ++++++++----------
 spec/models/concerns/deployable_spec.rb            | 49 +++++++++-----------
 spec/models/environment_status_spec.rb             |  4 ++
 spec/models/merge_request_spec.rb                  | 14 ++++--
 spec/services/ci/stop_environments_service_spec.rb |  1 +
 spec/services/update_deployment_service_spec.rb    |  1 +
 spec/workers/build_success_worker_spec.rb          |  4 +-
 12 files changed, 160 insertions(+), 77 deletions(-)
 create mode 100644 lib/gitlab/ci/build/prerequisite/deployment.rb
 create mode 100644 spec/lib/gitlab/ci/build/prerequisite/deployment_spec.rb

diff --git a/app/models/concerns/deployable.rb b/app/models/concerns/deployable.rb
index bc12b06b5af..2da5be696b0 100644
--- a/app/models/concerns/deployable.rb
+++ b/app/models/concerns/deployable.rb
@@ -4,8 +4,6 @@ module Deployable
   extend ActiveSupport::Concern
 
   included do
-    after_create :create_deployment
-
     def create_deployment
       return unless starts_environment? && !has_deployment?
 
diff --git a/lib/gitlab/ci/build/prerequisite/deployment.rb b/lib/gitlab/ci/build/prerequisite/deployment.rb
new file mode 100644
index 00000000000..0442fda1dda
--- /dev/null
+++ b/lib/gitlab/ci/build/prerequisite/deployment.rb
@@ -0,0 +1,21 @@
+# frozen_string_literal: true
+
+module Gitlab
+  module Ci
+    module Build
+      module Prerequisite
+        class Deployment < Base
+          def unmet?
+            build.starts_environment? && !build.has_deployment?
+          end
+
+          def complete!
+            return unless unmet?
+
+            build.create_deployment
+          end
+        end
+      end
+    end
+  end
+end
diff --git a/lib/gitlab/ci/build/prerequisite/factory.rb b/lib/gitlab/ci/build/prerequisite/factory.rb
index 60cdf7af418..19c0e1518f3 100644
--- a/lib/gitlab/ci/build/prerequisite/factory.rb
+++ b/lib/gitlab/ci/build/prerequisite/factory.rb
@@ -8,7 +8,10 @@ module Gitlab
           attr_reader :build
 
           def self.prerequisites
-            [KubernetesNamespace]
+            [
+              Gitlab::Ci::Build::Prerequisite::KubernetesNamespace,
+              Gitlab::Ci::Build::Prerequisite::Deployment
+            ]
           end
 
           def initialize(build)
diff --git a/spec/lib/gitlab/ci/build/prerequisite/deployment_spec.rb b/spec/lib/gitlab/ci/build/prerequisite/deployment_spec.rb
new file mode 100644
index 00000000000..0c6175be1a2
--- /dev/null
+++ b/spec/lib/gitlab/ci/build/prerequisite/deployment_spec.rb
@@ -0,0 +1,54 @@
+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe Gitlab::Ci::Build::Prerequisite::Deployment do
+  describe '#unmet?' do
+    context 'when the build starts an environment' do
+      it 'returns true if there is no deployment' do
+        build = instance_double(Ci::Build, starts_environment?: true, has_deployment?: false)
+        prerequisite = described_class.new(build)
+
+        expect(prerequisite).to be_unmet
+      end
+
+      it 'returns false if there is a deployment' do
+        build = instance_double(Ci::Build, starts_environment?: true, has_deployment?: true)
+        prerequisite = described_class.new(build)
+
+        expect(prerequisite).not_to be_unmet
+      end
+    end
+
+    it 'returns false if the build does not start an environment' do
+      build = instance_double(Ci::Build, starts_environment?: false)
+      prerequisite = described_class.new(build)
+
+      expect(prerequisite).not_to be_unmet
+    end
+  end
+
+  describe '#complete!' do
+    context 'when prerequisite is unmet' do
+      it 'creates a deployment and environment' do
+        build = instance_double(Ci::Build, starts_environment?: true, has_deployment?: false)
+        prerequisite = described_class.new(build)
+
+        expect(build).to receive(:create_deployment)
+
+        prerequisite.complete!
+      end
+    end
+
+    context 'when prerequisite is met' do
+      it 'creates a deployment and environment' do
+        build = instance_double(Ci::Build, starts_environment?: true, has_deployment?: true)
+        prerequisite = described_class.new(build)
+
+        expect(build).not_to receive(:create_deployment)
+
+        prerequisite.complete!
+      end
+    end
+  end
+end
diff --git a/spec/lib/gitlab/ci/build/prerequisite/factory_spec.rb b/spec/lib/gitlab/ci/build/prerequisite/factory_spec.rb
index 5187f99a441..ff9d29f43a4 100644
--- a/spec/lib/gitlab/ci/build/prerequisite/factory_spec.rb
+++ b/spec/lib/gitlab/ci/build/prerequisite/factory_spec.rb
@@ -3,32 +3,39 @@
 require 'spec_helper'
 
 describe Gitlab::Ci::Build::Prerequisite::Factory do
-  let(:build) { create(:ci_build) }
+  let(:build) { instance_double(Ci::Build) }
 
-  describe '.for_build' do
+  describe '.prerequisites' do
+    it 'returns KubernetesNamespace and Deployment' do
+      expect(described_class.prerequisites).to contain_exactly(
+        Gitlab::Ci::Build::Prerequisite::KubernetesNamespace,
+        Gitlab::Ci::Build::Prerequisite::Deployment
+      )
+    end
+  end
+
+  describe '#unmet' do
     let(:kubernetes_namespace) do
       instance_double(
         Gitlab::Ci::Build::Prerequisite::KubernetesNamespace,
-        unmet?: unmet)
+        unmet?: true)
     end
 
-    subject { described_class.new(build).unmet }
+    let(:deployment) do
+      instance_double(
+        Gitlab::Ci::Build::Prerequisite::Deployment,
+        unmet?: false)
+    end
 
-    before do
+    it 'returns only unmet prerequisites' do
       expect(Gitlab::Ci::Build::Prerequisite::KubernetesNamespace)
         .to receive(:new).with(build).and_return(kubernetes_namespace)
-    end
-
-    context 'prerequisite is unmet' do
-      let(:unmet) { true }
-
-      it { is_expected.to eq [kubernetes_namespace] }
-    end
+      expect(Gitlab::Ci::Build::Prerequisite::Deployment)
+        .to receive(:new).with(build).and_return(deployment)
 
-    context 'prerequisite is met' do
-      let(:unmet) { false }
+      unmet = described_class.new(build).unmet
 
-      it { is_expected.to be_empty }
+      expect(unmet).to contain_exactly(kubernetes_namespace)
     end
   end
 end
diff --git a/spec/models/ci/build_spec.rb b/spec/models/ci/build_spec.rb
index d98db024f73..94256a00c0b 100644
--- a/spec/models/ci/build_spec.rb
+++ b/spec/models/ci/build_spec.rb
@@ -182,33 +182,22 @@ describe Ci::Build do
   end
 
   describe '#enqueue' do
-    let(:build) { create(:ci_build, :created) }
-
-    subject { build.enqueue }
-
-    before do
-      allow(build).to receive(:any_unmet_prerequisites?).and_return(has_prerequisites)
-      allow(Ci::PrepareBuildService).to receive(:perform_async)
-    end
-
-    context 'build has unmet prerequisites' do
-      let(:has_prerequisites) { true }
+    it 'transitions to preparing if build has unmet prerequisites' do
+      build = create(:ci_build, :start_review_app, status: 'created')
+      expect(Ci::BuildPrepareWorker).to receive(:perform_async).with(build.id)
 
-      it 'transitions to preparing' do
-        subject
+      build.enqueue
 
-        expect(build).to be_preparing
-      end
+      expect(build).to be_preparing
     end
 
-    context 'build has no prerequisites' do
-      let(:has_prerequisites) { false }
+    it 'transitions to pending if build has no unmet prerequisites' do
+      build = create(:ci_build, :created)
+      expect(Ci::BuildPrepareWorker).not_to receive(:perform_async).with(build.id)
 
-      it 'transitions to pending' do
-        subject
+      build.enqueue
 
-        expect(build).to be_pending
-      end
+      expect(build).to be_pending
     end
   end
 
@@ -374,6 +363,7 @@ describe Ci::Build do
       context 'build has unmet prerequisites' do
         before do
           allow(build).to receive(:prerequisites).and_return([double])
+          expect(Ci::BuildPrepareWorker).to receive(:perform_async).with(build.id)
         end
 
         it 'transits to preparing' do
@@ -791,13 +781,9 @@ describe Ci::Build do
       allow(Deployments::FinishedWorker).to receive(:perform_async)
     end
 
-    it 'has deployments record with created status' do
-      expect(deployment).to be_created
-      expect(environment.name).to eq('review/master')
-    end
-
     context 'when transits to running' do
       before do
+        build.create_deployment
         build.run!
       end
 
@@ -809,6 +795,7 @@ describe Ci::Build do
     context 'when transits to success' do
       before do
         allow(Deployments::SuccessWorker).to receive(:perform_async)
+        build.create_deployment
         build.success!
       end
 
@@ -819,6 +806,7 @@ describe Ci::Build do
 
     context 'when transits to failed' do
       before do
+        build.create_deployment
         build.drop!
       end
 
@@ -829,6 +817,7 @@ describe Ci::Build do
 
     context 'when transits to skipped' do
       before do
+        build.create_deployment
         build.skip!
       end
 
@@ -839,6 +828,7 @@ describe Ci::Build do
 
     context 'when transits to canceled' do
       before do
+        build.create_deployment
         build.cancel!
       end
 
@@ -1781,7 +1771,8 @@ describe Ci::Build do
     end
 
     context 'when build has a start environment' do
-      let(:build) { create(:ci_build, :deploy_to_production, pipeline: pipeline) }
+      let(:deployment) { create(:deployment) }
+      let(:build) { create(:ci_build, :deploy_to_production, pipeline: pipeline, deployment: deployment) }
 
       it 'does not expand environment name' do
         expect(build).not_to receive(:expanded_environment_name)
diff --git a/spec/models/concerns/deployable_spec.rb b/spec/models/concerns/deployable_spec.rb
index 42bed9434f5..f4a1d510f9f 100644
--- a/spec/models/concerns/deployable_spec.rb
+++ b/spec/models/concerns/deployable_spec.rb
@@ -4,51 +4,47 @@ require 'rails_helper'
 
 describe Deployable do
   describe '#create_deployment' do
-    let(:deployment) { job.deployment }
-    let(:environment) { deployment&.environment }
-
-    before do
-      job.reload
-    end
-
     context 'when the deployable object will deploy to production' do
-      let!(:job) { create(:ci_build, :start_review_app) }
+      let(:job) { create(:ci_build, :start_review_app) }
 
       it 'creates a deployment and environment record' do
-        expect(deployment.project).to eq(job.project)
-        expect(deployment.ref).to eq(job.ref)
-        expect(deployment.tag).to eq(job.tag)
-        expect(deployment.sha).to eq(job.sha)
-        expect(deployment.user).to eq(job.user)
-        expect(deployment.deployable).to eq(job)
-        expect(deployment.on_stop).to eq('stop_review_app')
-        expect(environment.name).to eq('review/master')
+        job.create_deployment
+
+        expect(job.deployment.project).to eq(job.project)
+        expect(job.deployment.ref).to eq(job.ref)
+        expect(job.deployment.tag).to eq(job.tag)
+        expect(job.deployment.sha).to eq(job.sha)
+        expect(job.deployment.user).to eq(job.user)
+        expect(job.deployment.deployable).to eq(job)
+        expect(job.deployment.on_stop).to eq('stop_review_app')
+        expect(job.deployment.environment.name).to eq('review/master')
       end
     end
 
     context 'when the deployable object will stop an environment' do
-      let!(:job) { create(:ci_build, :stop_review_app) }
+      let(:job) { create(:ci_build, :stop_review_app) }
 
       it 'does not create a deployment record' do
-        expect(deployment).to be_nil
+        expect { job.create_deployment }.not_to change { job.reload.deployment }.from(nil)
       end
     end
 
     context 'when the deployable object has already had a deployment' do
-      let!(:job) { create(:ci_build, :start_review_app, deployment: race_deployment) }
-      let!(:race_deployment) { create(:deployment, :success) }
+      let(:job) { create(:ci_build, :start_review_app, deployment: race_deployment) }
+      let(:race_deployment) { create(:deployment, :success) }
 
       it 'does not create a new deployment' do
-        expect(deployment).to eq(race_deployment)
+        expect { job.create_deployment }.not_to change { job.reload.deployment }.from(race_deployment)
       end
     end
 
     context 'when the deployable object will not deploy' do
-      let!(:job) { create(:ci_build) }
+      let(:job) { create(:ci_build) }
 
       it 'does not create a deployment and environment record' do
-        expect(deployment).to be_nil
-        expect(environment).to be_nil
+        expect { job.create_deployment }.not_to change { job.reload.deployment }.from(nil)
+
+        expect(job.persisted_environment).to be_nil
       end
     end
 
@@ -68,8 +64,9 @@ describe Deployable do
       end
 
       it 'does not create a deployment and environment record' do
-        expect(deployment).to be_nil
-        expect(environment).to be_nil
+        expect { job.create_deployment }.not_to change { job.reload.deployment }.from(nil)
+
+        expect(job.persisted_environment).to be_nil
       end
     end
   end
diff --git a/spec/models/environment_status_spec.rb b/spec/models/environment_status_spec.rb
index c503c35305f..7440be2f0a7 100644
--- a/spec/models/environment_status_spec.rb
+++ b/spec/models/environment_status_spec.rb
@@ -100,6 +100,10 @@ describe EnvironmentStatus do
     let(:environment) { build.deployment.environment }
     let(:user) { project.owner }
 
+    before do
+      build.create_deployment
+    end
+
     context 'when environment is created on a forked project' do
       let(:project) { create(:project, :repository) }
       let(:forked) { fork_project(project, user, repository: true) }
diff --git a/spec/models/merge_request_spec.rb b/spec/models/merge_request_spec.rb
index c6251326c22..acf8bc557c7 100644
--- a/spec/models/merge_request_spec.rb
+++ b/spec/models/merge_request_spec.rb
@@ -2333,18 +2333,22 @@ describe MergeRequest do
         merge_requests_as_head_pipeline: [merge_request])
     end
 
-    let!(:job) { create(:ci_build, :start_review_app, pipeline: pipeline, project: project) }
+    let(:job) { create(:ci_build, :start_review_app, pipeline: pipeline, project: project) }
 
     it 'returns environments' do
-      is_expected.to eq(pipeline.environments)
-      expect(subject.count).to be(1)
+      job.create_deployment
+
+      expect(subject).to eq(pipeline.environments)
+      expect(subject.count).to eq(1)
     end
 
     context 'when pipeline is not associated with environments' do
-      let!(:job) { create(:ci_build, pipeline: pipeline, project: project) }
+      let(:job) { create(:ci_build, pipeline: pipeline, project: project) }
 
       it 'returns empty array' do
-        is_expected.to be_empty
+        job.create_deployment
+
+        expect(subject).to be_empty
       end
     end
 
diff --git a/spec/services/ci/stop_environments_service_spec.rb b/spec/services/ci/stop_environments_service_spec.rb
index 890fa5bc009..65fdc270605 100644
--- a/spec/services/ci/stop_environments_service_spec.rb
+++ b/spec/services/ci/stop_environments_service_spec.rb
@@ -125,6 +125,7 @@ describe Ci::StopEnvironmentsService do
     let!(:stop_review_job) { create(:ci_build, :stop_review_app, :manual, pipeline: pipeline, project: project) }
 
     before do
+      review_job.create_deployment
       review_job.deployment.success!
     end
 
diff --git a/spec/services/update_deployment_service_spec.rb b/spec/services/update_deployment_service_spec.rb
index 7dc52f6816a..9edf9a6660f 100644
--- a/spec/services/update_deployment_service_spec.rb
+++ b/spec/services/update_deployment_service_spec.rb
@@ -23,6 +23,7 @@ describe UpdateDeploymentService do
 
   before do
     allow(Deployments::FinishedWorker).to receive(:perform_async)
+    job.create_deployment
     job.success! # Create/Succeed deployment
   end
 
diff --git a/spec/workers/build_success_worker_spec.rb b/spec/workers/build_success_worker_spec.rb
index ffe8796ded9..530f3eb4b9b 100644
--- a/spec/workers/build_success_worker_spec.rb
+++ b/spec/workers/build_success_worker_spec.rb
@@ -31,10 +31,12 @@ describe BuildSuccessWorker do
         end
       end
 
-      context 'when deployment was created with the build creation' do # Counter part of the above edge case
+      context 'when deployment was already created' do # Counter part of the above edge case
         let!(:build) { create(:ci_build, :deploy_to_production) }
 
         it 'does not create a new deployment' do
+          build.create_deployment
+
           expect(build).to be_has_deployment
 
           expect { subject }.not_to change { Deployment.count }
-- 
cgit v1.2.3