Don't expand CI_ENVIRONMENT_URL so runner would do

And make sure CI_ENVIRONMENT_URL comes last so all
variables would be available whenever the runner is
trying to expand it.

This is an alternative to !12333

3 files changed, 24 insertions, 9 deletions

diff --git a/app/models/ci/build.rb b/app/models/ci/build.rb
index 58758f7ca8a..3acdfb46d39 100644
--- a/app/models/ci/build.rb
+++ b/app/models/ci/build.rb
@@ -138,11 +138,11 @@ module Ci
       ExpandVariables.expand(environment, simple_variables) if environment
     end
 
-    def environment_url
-      return @environment_url if defined?(@environment_url)
+    def expanded_environment_url
+      return @expanded_environment_url if defined?(@expanded_environment_url)
 
-      @environment_url =
-        if unexpanded_url = options&.dig(:environment, :url)
+      @expanded_environment_url =
+        if unexpanded_url = environment_url
           ExpandVariables.expand(unexpanded_url, simple_variables)
         else
           persisted_environment&.external_url
@@ -506,6 +506,10 @@ module Ci
       variables
     end
 
+    def environment_url
+      options&.dig(:environment, :url)
+    end
+
     def build_attributes_from_config
       return {} unless pipeline.config_processor
 
diff --git a/app/services/create_deployment_service.rb b/app/services/create_deployment_service.rb
index 46823418bb0..fd8781aa48f 100644
--- a/app/services/create_deployment_service.rb
+++ b/app/services/create_deployment_service.rb
@@ -2,7 +2,7 @@ class CreateDeploymentService
   attr_reader :job
 
   delegate :expanded_environment_name,
-           :environment_url,
+           :expanded_environment_url,
            :project,
            to: :job
 
@@ -14,7 +14,8 @@ class CreateDeploymentService
     return unless executable?
 
     ActiveRecord::Base.transaction do
-      environment.external_url = environment_url if environment_url
+      environment.external_url = expanded_environment_url if
+        expanded_environment_url
       environment.fire_state_event(action)
 
       return unless environment.save
diff --git a/spec/models/ci/build_spec.rb b/spec/models/ci/build_spec.rb
index 3816422fec6..ea6a5fc48c5 100644
--- a/spec/models/ci/build_spec.rb
+++ b/spec/models/ci/build_spec.rb
@@ -451,8 +451,8 @@ describe Ci::Build, :models do
       end
     end
 
-    describe '#environment_url' do
-      subject { job.environment_url }
+    describe '#expanded_environment_url' do
+      subject { job.expanded_environment_url }
 
       context 'when yaml environment uses $CI_COMMIT_REF_NAME' do
         let(:job) do
@@ -1292,10 +1292,20 @@ describe Ci::Build, :models do
 
         context 'when the URL was set from the job' do
           before do
-            build.update(options: { environment: { url: 'http://host/$CI_JOB_NAME' } })
+            build.update(options: { environment: { url: url } })
           end
 
           it_behaves_like 'containing environment variables'
+
+          context 'when variables are used in the URL, it does not expand' do
+            let(:url) { 'http://$CI_PROJECT_NAME-$CI_ENVIRONMENT_SLUG' }
+
+            it_behaves_like 'containing environment variables'
+
+            it 'puts $CI_ENVIRONMENT_URL in the last so all other variables are available to be used when runners are trying to expand it' do
+              expect(subject.last).to eq(environment_variables.last)
+            end
+          end
         end
 
         context 'when the URL was not set from the job, but environment' do