From 1a193d042a8bff1e5cfefd103baa8286d8a4c9b5 Mon Sep 17 00:00:00 2001 From: Lin Jen-Shin Date: Wed, 21 Jun 2017 18:03:42 +0800 Subject: [PATCH 1/5] 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 --- app/models/ci/build.rb | 12 ++++++++---- app/services/create_deployment_service.rb | 5 +++-- spec/models/ci/build_spec.rb | 16 +++++++++++++--- 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 From df095d7df66aad3e51df5bd07ecdd4758391ced4 Mon Sep 17 00:00:00 2001 From: Lin Jen-Shin Date: Wed, 21 Jun 2017 18:25:27 +0800 Subject: [PATCH 2/5] Add comments to explain some culprit; Add changelog --- app/models/ci/build.rb | 11 +++++++++-- .../unreleased/34008-fix-CI_ENVIRONMENT_URL-2.yml | 4 ++++ 2 files changed, 13 insertions(+), 2 deletions(-) create mode 100644 changelogs/unreleased/34008-fix-CI_ENVIRONMENT_URL-2.yml diff --git a/app/models/ci/build.rb b/app/models/ci/build.rb index 3acdfb46d39..9e38a916a2e 100644 --- a/app/models/ci/build.rb +++ b/app/models/ci/build.rb @@ -192,7 +192,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 +207,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 @@ -482,6 +483,12 @@ module Ci variables = persisted_environment.predefined_variables if url = environment_url + # Note that CI_ENVIRONMENT_URL should be the last variable, because + # here we're passing unexpanded environment_url for runner to expand, + # and the runner would expand in order. In order to make sure that + # CI_ENVIRONMENT_URL has everything available, such as variables + # from Environment#predefined_variables, we need to make sure it's + # the last variable. variables << { key: 'CI_ENVIRONMENT_URL', value: url, public: true } end diff --git a/changelogs/unreleased/34008-fix-CI_ENVIRONMENT_URL-2.yml b/changelogs/unreleased/34008-fix-CI_ENVIRONMENT_URL-2.yml new file mode 100644 index 00000000000..7f4d6e3bc67 --- /dev/null +++ b/changelogs/unreleased/34008-fix-CI_ENVIRONMENT_URL-2.yml @@ -0,0 +1,4 @@ +--- +title: Fix passing CI_ENVIRONMENT_NAME and CI_ENVIRONMENT_SLUG for CI_ENVIRONMENT_URL +merge_request: 12344 +author: From 0d3631acc16b5046ef295bf09204a565cb8580ea Mon Sep 17 00:00:00 2001 From: Lin Jen-Shin Date: Wed, 21 Jun 2017 19:53:19 +0800 Subject: [PATCH 3/5] Move expanded_environment_url to CreateDeploymentService Because that's the only place we need it. --- app/models/ci/build.rb | 30 +++++----------- app/services/create_deployment_service.rb | 13 ++++++- spec/models/ci/build_spec.rb | 36 ------------------- .../create_deployment_service_spec.rb | 36 +++++++++++++++++++ 4 files changed, 57 insertions(+), 58 deletions(-) diff --git a/app/models/ci/build.rb b/app/models/ci/build.rb index 9e38a916a2e..4454057e418 100644 --- a/app/models/ci/build.rb +++ b/app/models/ci/build.rb @@ -138,17 +138,6 @@ module Ci ExpandVariables.expand(environment, simple_variables) if environment end - def expanded_environment_url - return @expanded_environment_url if defined?(@expanded_environment_url) - - @expanded_environment_url = - if unexpanded_url = environment_url - ExpandVariables.expand(unexpanded_url, simple_variables) - else - persisted_environment&.external_url - end - end - def has_environment? environment.present? end @@ -482,15 +471,10 @@ module Ci variables = persisted_environment.predefined_variables - if url = environment_url - # Note that CI_ENVIRONMENT_URL should be the last variable, because - # here we're passing unexpanded environment_url for runner to expand, - # and the runner would expand in order. In order to make sure that - # CI_ENVIRONMENT_URL has everything available, such as variables - # from Environment#predefined_variables, we need to make sure it's - # the last variable. - 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 @@ -514,7 +498,11 @@ module Ci end def environment_url - options&.dig(:environment, :url) + return @environment_url if defined?(@environment_url) + + @environment_url = + options&.dig(:environment, :url) || + persisted_environment&.external_url end def build_attributes_from_config diff --git a/app/services/create_deployment_service.rb b/app/services/create_deployment_service.rb index fd8781aa48f..85500d6c9c9 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, - :expanded_environment_url, + :simple_variables, :project, to: :job @@ -50,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 = + if unexpanded_url = environment_options[:url] + ExpandVariables.expand(unexpanded_url, simple_variables) + else + environment&.external_url + end + end + def on_stop environment_options[:on_stop] end diff --git a/spec/models/ci/build_spec.rb b/spec/models/ci/build_spec.rb index ea6a5fc48c5..77ab9a5c5ab 100644 --- a/spec/models/ci/build_spec.rb +++ b/spec/models/ci/build_spec.rb @@ -451,42 +451,6 @@ describe Ci::Build, :models do end end - describe '#expanded_environment_url' do - subject { job.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 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? } diff --git a/spec/services/create_deployment_service_spec.rb b/spec/services/create_deployment_service_spec.rb index 6cf4342ad4c..790713cf2e4 100644 --- a/spec/services/create_deployment_service_spec.rb +++ b/spec/services/create_deployment_service_spec.rb @@ -122,6 +122,42 @@ 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 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 'processing of builds' do shared_examples 'does not create deployment' do it 'does not create a new deployment' do From 98b60ee29ba99ec28f4c4782a76f52d675eaf083 Mon Sep 17 00:00:00 2001 From: Lin Jen-Shin Date: Wed, 21 Jun 2017 20:22:26 +0800 Subject: [PATCH 4/5] Expand with all the variables so that things like CI_ENVIRONMENT_SLUG is also available. It won't be recursive because we're not putting this value in the variables. --- app/models/ci/build.rb | 12 ++++------- app/services/create_deployment_service.rb | 18 +++++++--------- .../create_deployment_service_spec.rb | 21 ++++++++++++++++++- 3 files changed, 31 insertions(+), 20 deletions(-) diff --git a/app/models/ci/build.rb b/app/models/ci/build.rb index 4454057e418..a300536532b 100644 --- a/app/models/ci/build.rb +++ b/app/models/ci/build.rb @@ -471,9 +471,9 @@ module Ci variables = persisted_environment.predefined_variables - # 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. + # 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 @@ -498,11 +498,7 @@ module Ci end def environment_url - return @environment_url if defined?(@environment_url) - - @environment_url = - options&.dig(:environment, :url) || - persisted_environment&.external_url + options&.dig(:environment, :url) || persisted_environment&.external_url end def build_attributes_from_config diff --git a/app/services/create_deployment_service.rb b/app/services/create_deployment_service.rb index 85500d6c9c9..45f68391df8 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, - :simple_variables, + :variables, :project, to: :job @@ -14,8 +14,10 @@ class CreateDeploymentService return unless executable? ActiveRecord::Base.transaction do - environment.external_url = expanded_environment_url if - expanded_environment_url + if external_url = expanded_environment_url + environment.external_url = external_url + end + environment.fire_state_event(action) return unless environment.save @@ -51,14 +53,8 @@ class CreateDeploymentService end def expanded_environment_url - return @expanded_environment_url if defined?(@expanded_environment_url) - - @expanded_environment_url = - if unexpanded_url = environment_options[:url] - ExpandVariables.expand(unexpanded_url, simple_variables) - else - environment&.external_url - end + ExpandVariables.expand(environment_options[:url], variables) if + environment_options[:url] end def on_stop diff --git a/spec/services/create_deployment_service_spec.rb b/spec/services/create_deployment_service_spec.rb index 790713cf2e4..dfab6ebf372 100644 --- a/spec/services/create_deployment_service_spec.rb +++ b/spec/services/create_deployment_service_spec.rb @@ -135,6 +135,25 @@ describe CreateDeploymentService, services: true do 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, @@ -153,7 +172,7 @@ describe CreateDeploymentService, services: true do end it 'returns the external_url from persisted environment' do - is_expected.to eq(environment.external_url) + is_expected.to be_nil end end end From 2a7f1eec12657310e2f8a24b0a1f0a659b9363d6 Mon Sep 17 00:00:00 2001 From: Lin Jen-Shin Date: Wed, 21 Jun 2017 20:39:05 +0800 Subject: [PATCH 5/5] Memorize the value in methods rather than local --- app/services/create_deployment_service.rb | 16 ++++++++++------ 1 file changed, 10 insertions(+), 6 deletions(-) diff --git a/app/services/create_deployment_service.rb b/app/services/create_deployment_service.rb index 45f68391df8..63b85c3de7d 100644 --- a/app/services/create_deployment_service.rb +++ b/app/services/create_deployment_service.rb @@ -14,10 +14,8 @@ class CreateDeploymentService return unless executable? ActiveRecord::Base.transaction do - if external_url = expanded_environment_url - environment.external_url = external_url - end - + environment.external_url = expanded_environment_url if + expanded_environment_url environment.fire_state_event(action) return unless environment.save @@ -53,8 +51,14 @@ class CreateDeploymentService end def expanded_environment_url - ExpandVariables.expand(environment_options[:url], variables) if - environment_options[: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