Merge branch '34008-fix-CI_ENVIRONMENT_URL-2' into 'master'

Don't expand CI_ENVIRONMENT_URL so runner would do

Closes #34008

See merge request !12344

app/models/ci/build.rb

@@ -142,17 +142,6 @@ module Ci
       ExpandVariables.expand(environment, simple_variables) if environment
     end
 
-    def environment_url
-      return @environment_url if defined?(@environment_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?
       environment.present?
     end
@@ -196,27 +185,29 @@ module Ci
       slugified.gsub(/[^a-z0-9]/, '-')[0..62]
     end
 
-    # Variables whose value does not depend on other variables
+    # Variables whose value does not depend on environment
     def simple_variables
-      variables(with_environment: false)
-    end
-
-    # All variables, including those dependent on other variables
-    def variables(with_environment: true)
       variables = predefined_variables
       variables += project.predefined_variables
       variables += pipeline.predefined_variables
       variables += runner.predefined_variables if runner
       variables += project.container_registry_variables
       variables += project.deployment_variables if has_environment?
-      variables += persisted_environment_variables if with_environment
       variables += yaml_variables
       variables += user_variables
-      variables += secret_variables(with_environment: with_environment)
+      variables += project.secret_variables_for(
+                     ref: ref, environment: persisted_environment)
+                       .map(&:to_runner_variable)
       variables += trigger_request.user_variables if trigger_request
       variables
     end
 
+    # All variables, including those dependent on environment, which could
+    # contain unexpanded variables.
+    def variables
+      simple_variables.concat(persisted_environment_variables)
+    end
+
     def merge_request
       return @merge_request if defined?(@merge_request)
 
@@ -513,9 +504,10 @@ module Ci
 
       variables = persisted_environment.predefined_variables
 
-      if url = environment_url
-        variables << { key: 'CI_ENVIRONMENT_URL', value: url, public: true }
-      end
+      # Here we're passing unexpanded environment_url for runner to expand,
+      # and we need to make sure that CI_ENVIRONMENT_NAME and
+      # CI_ENVIRONMENT_SLUG so on are available for the URL be expanded.
+      variables << { key: 'CI_ENVIRONMENT_URL', value: environment_url, public: true } if environment_url
 
       variables
     end
@@ -538,6 +530,10 @@ module Ci
       variables
     end
 
+    def environment_url
+      options&.dig(:environment, :url) || persisted_environment&.external_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,
-           :environment_url,
+           :variables,
            :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
@@ -49,6 +50,17 @@ class CreateDeploymentService
     @environment_options ||= job.options&.dig(:environment) || {}
   end
 
+  def expanded_environment_url
+    return @expanded_environment_url if defined?(@expanded_environment_url)
+
+    @expanded_environment_url =
+      ExpandVariables.expand(environment_url, variables) if environment_url
+  end
+
+  def environment_url
+    environment_options[:url]
+  end
+
   def on_stop
     environment_options[:on_stop]
   end

changelogs/unreleased/34008-fix-CI_ENVIRONMENT_URL-2.yml

+---
+title: Fix passing CI_ENVIRONMENT_NAME and CI_ENVIRONMENT_SLUG for CI_ENVIRONMENT_URL
+merge_request: 12344
+author:

spec/models/ci/build_spec.rb

@@ -496,42 +496,6 @@ describe Ci::Build, :models do
       end
     end
 
-    describe '#environment_url' do
-      subject { job.environment_url }
-
-      context 'when yaml environment uses $CI_COMMIT_REF_NAME' do
-        let(:job) do
-          create(:ci_build,
-                 ref: 'master',
-                 options: { environment: { url: 'http://review/$CI_COMMIT_REF_NAME' } })
-        end
-
-        it { is_expected.to eq('http://review/master') }
-      end
-
-      context 'when yaml environment uses yaml_variables containing symbol keys' do
-        let(:job) do
-          create(:ci_build,
-                 yaml_variables: [{ key: :APP_HOST, value: 'host' }],
-                 options: { environment: { url: 'http://review/$APP_HOST' } })
-        end
-
-        it { is_expected.to eq('http://review/host') }
-      end
-
-      context 'when yaml environment does not have url' do
-        let(:job) { create(:ci_build, environment: 'staging') }
-
-        let!(:environment) do
-          create(:environment, project: job.project, name: job.environment)
-        end
-
-        it 'returns the external_url from persisted environment' do
-          is_expected.to eq(environment.external_url)
-        end
-      end
-    end
-
     describe '#starts_environment?' do
       subject { build.starts_environment? }
 
@@ -1337,10 +1301,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

spec/services/create_deployment_service_spec.rb

@@ -122,6 +122,61 @@ describe CreateDeploymentService, services: true do
     end
   end
 
+  describe '#expanded_environment_url' do
+    subject { service.send(:expanded_environment_url) }
+
+    context 'when yaml environment uses $CI_COMMIT_REF_NAME' do
+      let(:job) do
+        create(:ci_build,
+               ref: 'master',
+               options: { environment: { url: 'http://review/$CI_COMMIT_REF_NAME' } })
+      end
+
+      it { is_expected.to eq('http://review/master') }
+    end
+
+    context 'when yaml environment uses $CI_ENVIRONMENT_SLUG' do
+      let(:job) do
+        create(:ci_build,
+               ref: 'master',
+               environment: 'production',
+               options: { environment: { url: 'http://review/$CI_ENVIRONMENT_SLUG' } })
+      end
+
+      let!(:environment) do
+        create(:environment,
+          project: job.project,
+          name: 'production',
+          slug: 'prod-slug',
+          external_url: 'http://review/old')
+      end
+
+      it { is_expected.to eq('http://review/prod-slug') }
+    end
+
+    context 'when yaml environment uses yaml_variables containing symbol keys' do
+      let(:job) do
+        create(:ci_build,
+               yaml_variables: [{ key: :APP_HOST, value: 'host' }],
+               options: { environment: { url: 'http://review/$APP_HOST' } })
+      end
+
+      it { is_expected.to eq('http://review/host') }
+    end
+
+    context 'when yaml environment does not have url' do
+      let(:job) { create(:ci_build, environment: 'staging') }
+
+      let!(:environment) do
+        create(:environment, project: job.project, name: job.environment)
+      end
+
+      it 'returns the external_url from persisted environment' do
+        is_expected.to be_nil
+      end
+    end
+  end
+
   describe 'processing of builds' do
     shared_examples 'does not create deployment' do
       it 'does not create a new deployment' do