Merge all environment url methods, introduce ensure_persisted_environment

To make sure that we're accessing the same instance, so ending up
with `env.external` = `env.external_url` shall be fine.

app/models/ci/build.rb

@@ -19,6 +19,12 @@ module Ci
       )
     end
 
+    def ensure_persisted_environment
+      persisted_environment ||
+        @persisted_environment =
+          project.environments.create(name: expanded_environment_name)
+    end
+
     serialize :options
     serialize :yaml_variables, Gitlab::Serializer::Ci::Variables
 
@@ -138,13 +144,15 @@ 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 environment_url
+      return @environment_url if defined?(@environment_url)
 
-    def ci_environment_url
-      expanded_environment_url || persisted_environment&.external_url
+      @environment_url =
+        if unexpanded_url = options.dig(:environment, :url)
+          ExpandVariables.expand(unexpanded_url, simple_variables)
+        else
+          persisted_environment&.external_url
+        end
     end
 
     def has_environment?
@@ -506,7 +514,7 @@ module Ci
 
       variables = persisted_environment.predefined_variables
 
-      if url = ci_environment_url
+      if url = environment_url
         variables << { key: 'CI_ENVIRONMENT_URL', value: url, public: true }
       end
 
@@ -531,10 +539,6 @@ module Ci
       variables
     end
 
-    def environment_url
-      options.dig(:environment, :url)
-    end
-
     def build_attributes_from_config
       return {} unless pipeline.config_processor
 

app/services/create_deployment_service.rb

@@ -2,7 +2,7 @@ class CreateDeploymentService
   attr_reader :job
 
   delegate :expanded_environment_name,
-           :expanded_environment_url,
+           :environment_url,
            :project,
            to: :job
 
@@ -14,8 +14,7 @@ class CreateDeploymentService
     return unless executable?
 
     ActiveRecord::Base.transaction do
-      environment.external_url = expanded_environment_url if
-        expanded_environment_url
+      environment.external_url = environment_url if environment_url
       environment.fire_state_event(action)
 
       return unless environment.save
@@ -43,8 +42,7 @@ class CreateDeploymentService
   end
 
   def environment
-    @environment ||=
-      project.environments.find_or_create_by(name: expanded_environment_name)
+    @environment ||= job.ensure_persisted_environment
   end
 
   def environment_options

spec/models/ci/build_spec.rb

@@ -427,8 +427,8 @@ describe Ci::Build, :models do
       end
     end
 
-    describe '#expanded_environment_url' do
-      subject { job.expanded_environment_url }
+    describe '#environment_url' do
+      subject { job.environment_url }
 
       context 'when yaml environment uses $CI_COMMIT_REF_NAME' do
         let(:job) do
@@ -453,31 +453,10 @@ describe Ci::Build, :models do
       context 'when yaml environment does not have url' do
         let(:job) { create(:ci_build, environment: 'staging') }
 
-        it { is_expected.to be_nil }
-      end
-    end
-
-    describe '#ci_environment_url' do
-      subject { job.ci_environment_url }
-
-      let!(:environment) do
-        create(:environment, project: job.project, name: job.environment)
-      end
-
-      context 'when yaml environment has url' do
-        let(:job) do
-          create(:ci_build,
-                 ref: 'master',
-                 environment: 'staging',
-                 options: { environment: { url: 'http://review/$CI_COMMIT_REF_NAME' } })
+        let!(:environment) do
+          create(:environment, project: job.project, name: job.environment)
         end
 
-        it { is_expected.to eq('http://review/master') }
-      end
-
-      context 'when yaml environment does not have url' do
-        let(:job) { create(:ci_build, environment: 'staging') }
-
         it 'returns the external_url from persisted environment' do
           is_expected.to eq(environment.external_url)
         end
@@ -975,6 +954,40 @@ describe Ci::Build, :models do
 
       it { is_expected.to eq(environment) }
     end
+
+    context 'when there is not environment' do
+      it { is_expected.to be_nil }
+    end
+  end
+
+  describe '#ensure_persisted_environment' do
+    subject { job.ensure_persisted_environment }
+
+    let(:job) do
+      create(:ci_build,
+        ref: 'master',
+        environment: 'staging/$CI_COMMIT_REF_NAME')
+    end
+
+    context 'when there is no environment' do
+      it 'creates one by the expanded name' do
+        expect do
+          expect(subject.name).to eq('staging/master')
+        end.to change { Environment.count }.by(1)
+      end
+    end
+
+    context 'when there is already an environment' do
+      let!(:environment) do
+        create(:environment, project: job.project, name: 'staging/master')
+      end
+
+      it 'returns the existing environment' do
+        expect do
+          expect(subject).to eq(environment)
+        end.to change { Environment.count }.by(0)
+      end
+    end
   end
 
   describe '#play' do