Commit 8ebbe492 authored by Kamil Trzciński's avatar Kamil Trzciński Committed by Kushal Pandya

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
parent 49da5a20
......@@ -138,17 +138,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
......@@ -192,7 +181,7 @@ 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 = predefined_variables
variables += project.predefined_variables
......@@ -207,7 +196,8 @@ module Ci
variables
end
# All variables, including those dependent on other variables
# All variables, including those dependent on environment, which could
# contain unexpanded variables.
def variables
simple_variables.concat(persisted_environment_variables)
end
......@@ -481,9 +471,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
......@@ -506,6 +497,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
......
......@@ -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
......
---
title: Fix passing CI_ENVIRONMENT_NAME and CI_ENVIRONMENT_SLUG for CI_ENVIRONMENT_URL
merge_request: 12344
author:
......@@ -439,42 +439,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? }
......@@ -1276,10 +1240,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
......
......@@ -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
......
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment