Simplify CreateDeploymentService so that it uses

methods directly from job, avoid duplicating the works.

4 files changed, 73 insertions, 111 deletions

diff --git a/app/models/ci/build.rb b/app/models/ci/build.rb
index 85a7d64c804..3c7a3a1ccab 100644
--- a/app/models/ci/build.rb
+++ b/app/models/ci/build.rb
@@ -138,6 +138,11 @@ module Ci
       ExpandVariables.expand(environment, simple_variables) if environment
     end
 
+    def expanded_environment_url
+      ExpandVariables.expand(environment_url, simple_variables) if
+        environment_url
+    end
+
     def ci_environment_url
       expanded_environment_url || persisted_environment&.external_url
     end
@@ -526,11 +531,6 @@ module Ci
       variables
     end
 
-    def expanded_environment_url
-      ExpandVariables.expand(environment_url, simple_variables) if
-        environment_url
-    end
-
     def environment_url
       options.dig(:environment, :url)
     end
diff --git a/app/services/create_deployment_service.rb b/app/services/create_deployment_service.rb
index 47f9b2c621c..75f729e838d 100644
--- a/app/services/create_deployment_service.rb
+++ b/app/services/create_deployment_service.rb
@@ -1,71 +1,61 @@
-class CreateDeploymentService < BaseService
-  def execute(deployable = nil)
+class CreateDeploymentService
+  attr_reader :job
+
+  delegate :expanded_environment_name,
+           :expanded_environment_url,
+           :project,
+           to: :job
+
+  def initialize(job)
+    @job = job
+  end
+
+  def execute
     return unless executable?
 
     ActiveRecord::Base.transaction do
-      @deployable = deployable
+      environment.external_url = expanded_environment_url if
+        expanded_environment_url
+      environment.fire_state_event(action)
 
-      @environment = environment
-      @environment.external_url = expanded_url if expanded_url
-      @environment.fire_state_event(action)
+      return unless environment.save
+      return if environment.stopped?
 
-      return unless @environment.save
-      return if @environment.stopped?
-
-      deploy.tap do |deployment|
-        deployment.update_merge_request_metrics!
-      end
+      deploy.tap(&:update_merge_request_metrics!)
     end
   end
 
   private
 
   def executable?
-    project && name.present?
+    project && job.environment.present?
   end
 
   def deploy
     project.deployments.create(
-      environment: @environment,
-      ref: params[:ref],
-      tag: params[:tag],
-      sha: params[:sha],
-      user: current_user,
-      deployable: @deployable,
-      on_stop: options[:on_stop])
+      environment: environment,
+      ref: job.ref,
+      tag: job.tag,
+      sha: job.sha,
+      user: job.user,
+      deployable: job,
+      on_stop: on_stop)
   end
 
   def environment
-    @environment ||= project.environments.find_or_create_by(name: expanded_name)
-  end
-
-  def expanded_name
-    ExpandVariables.expand(name, variables)
-  end
-
-  def expanded_url
-    return unless url
-
-    @expanded_url ||= ExpandVariables.expand(url, variables)
-  end
-
-  def name
-    params[:environment]
-  end
-
-  def url
-    options[:url]
+    @environment ||=
+      project.environments.find_or_create_by(name: expanded_environment_name)
   end
 
-  def options
-    params[:options] || {}
+  def environment_options
+    @environment_options ||= job.options[:environment] || {}
   end
 
-  def variables
-    params[:variables] || []
+  def on_stop
+    environment_options[:on_stop]
   end
 
   def action
-    options[:action] || 'start'
+    environment_options[:action] || 'start'
   end
 end
diff --git a/app/workers/build_success_worker.rb b/app/workers/build_success_worker.rb
index e17add7421f..bf009dfab0f 100644
--- a/app/workers/build_success_worker.rb
+++ b/app/workers/build_success_worker.rb
@@ -11,15 +11,6 @@ class BuildSuccessWorker
   private
 
   def create_deployment(build)
-    service = CreateDeploymentService.new(
-      build.project, build.user,
-      environment: build.environment,
-      sha: build.sha,
-      ref: build.ref,
-      tag: build.tag,
-      options: build.options.to_h[:environment],
-      variables: build.variables)
-
-    service.execute(build)
+    CreateDeploymentService.new(build).execute
   end
 end
diff --git a/spec/services/create_deployment_service_spec.rb b/spec/services/create_deployment_service_spec.rb
index f35d7a33548..9c0420be545 100644
--- a/spec/services/create_deployment_service_spec.rb
+++ b/spec/services/create_deployment_service_spec.rb
@@ -1,22 +1,22 @@
 require 'spec_helper'
 
 describe CreateDeploymentService, services: true do
-  let(:project) { create(:empty_project) }
   let(:user) { create(:user) }
+  let(:options) { nil }
+
+  let(:job) do
+    create(:ci_build,
+      ref: 'master',
+      tag: false,
+      environment: 'production',
+      options: { environment: options })
+  end
+
+  let(:project) { job.project }
 
-  let(:service) { described_class.new(project, user, params) }
+  let(:service) { described_class.new(job) }
 
   describe '#execute' do
-    let(:options) { nil }
-    let(:params) do
-      {
-        environment: 'production',
-        ref: 'master',
-        tag: false,
-        sha: '97de212e80737a608d939f648d959671fb0a0142',
-        options: options
-      }
-    end
 
     subject { service.execute }
 
@@ -83,13 +83,8 @@ describe CreateDeploymentService, services: true do
     end
 
     context 'for environment with invalid name' do
-      let(:params) do
-        {
-          environment: 'name,with,commas',
-          ref: 'master',
-          tag: false,
-          sha: '97de212e80737a608d939f648d959671fb0a0142'
-        }
+      before do
+        job.update(environment: 'name,with,commas')
       end
 
       it 'does not create a new environment' do
@@ -102,27 +97,20 @@ describe CreateDeploymentService, services: true do
     end
 
     context 'when variables are used' do
-      let(:params) do
-        {
-          environment: 'review-apps/$CI_COMMIT_REF_NAME',
-          ref: 'master',
-          tag: false,
-          sha: '97de212e80737a608d939f648d959671fb0a0142',
-          options: {
-            name: 'review-apps/$CI_COMMIT_REF_NAME',
-            url: 'http://$CI_COMMIT_REF_NAME.review-apps.gitlab.com'
-          },
-          variables: [
-            { key: 'CI_COMMIT_REF_NAME', value: 'feature-review-apps' }
-          ]
-        }
+      let(:options) do
+        { name: 'review-apps/$CI_COMMIT_REF_NAME',
+          url: 'http://$CI_COMMIT_REF_NAME.review-apps.gitlab.com' }
+      end
+
+      before do
+        job.update(environment: 'review-apps/$CI_COMMIT_REF_NAME')
       end
 
       it 'does create a new environment' do
         expect { subject }.to change { Environment.count }.by(1)
 
-        expect(subject.environment.name).to eq('review-apps/feature-review-apps')
-        expect(subject.environment.external_url).to eq('http://feature-review-apps.review-apps.gitlab.com')
+        expect(subject.environment.name).to eq('review-apps/master')
+        expect(subject.environment.external_url).to eq('http://master.review-apps.gitlab.com')
       end
 
       it 'does create a new deployment' do
@@ -130,7 +118,7 @@ describe CreateDeploymentService, services: true do
       end
 
       context 'and environment exist' do
-        let!(:environment) { create(:environment, project: project, name: 'review-apps/feature-review-apps') }
+        let!(:environment) { create(:environment, project: project, name: 'review-apps/master') }
 
         it 'does not create a new environment' do
           expect { subject }.not_to change { Environment.count }
@@ -139,8 +127,8 @@ describe CreateDeploymentService, services: true do
         it 'updates external url' do
           subject
 
-          expect(subject.environment.name).to eq('review-apps/feature-review-apps')
-          expect(subject.environment.external_url).to eq('http://feature-review-apps.review-apps.gitlab.com')
+          expect(subject.environment.name).to eq('review-apps/master')
+          expect(subject.environment.external_url).to eq('http://master.review-apps.gitlab.com')
         end
 
         it 'does create a new deployment' do
@@ -150,7 +138,9 @@ describe CreateDeploymentService, services: true do
     end
 
     context 'when project was removed' do
-      let(:project) { nil }
+      before do
+        job.update(project: nil)
+      end
 
       it 'does not create deployment or environment' do
         expect { subject }.not_to raise_error
@@ -250,15 +240,6 @@ describe CreateDeploymentService, services: true do
   end
 
   describe "merge request metrics" do
-    let(:params) do
-      {
-        environment: 'production',
-        ref: 'master',
-        tag: false,
-        sha: '97de212e80737a608d939f648d959671fb0a0142b'
-      }
-    end
-
     let(:merge_request) { create(:merge_request, target_branch: 'master', source_branch: 'feature', source_project: project) }
 
     context "while updating the 'first_deployed_to_production_at' time" do
@@ -273,8 +254,8 @@ describe CreateDeploymentService, services: true do
         end
 
         it "doesn't set the time if the deploy's environment is not 'production'" do
-          staging_params = params.merge(environment: 'staging')
-          service = described_class.new(project, user, staging_params)
+          job.update(environment: 'staging')
+          service = described_class.new(job)
           service.execute
 
           expect(merge_request.reload.metrics.first_deployed_to_production_at).to be_nil
@@ -298,7 +279,7 @@ describe CreateDeploymentService, services: true do
             expect(merge_request.reload.metrics.first_deployed_to_production_at).to be_like_time(time)
 
             # Current deploy
-            service = described_class.new(project, user, params)
+            service = described_class.new(job)
             Timecop.freeze(time + 12.hours) { service.execute }
 
             expect(merge_request.reload.metrics.first_deployed_to_production_at).to be_like_time(time)
@@ -318,7 +299,7 @@ describe CreateDeploymentService, services: true do
             expect(merge_request.reload.metrics.first_deployed_to_production_at).to be_nil
 
             # Current deploy
-            service = described_class.new(project, user, params)
+            service = described_class.new(job)
             Timecop.freeze(time + 12.hours) { service.execute }
 
             expect(merge_request.reload.metrics.first_deployed_to_production_at).to be_nil