From 66edbc5e5cfe49984069512a9e550df9498497d8 Mon Sep 17 00:00:00 2001
From: Lin Jen-Shin <godfat@godfat.org>
Date: Thu, 1 Jun 2017 19:07:57 +0800
Subject: [PATCH] Introduce ci_environment_url which would fallback

to the external_url from persisted environment,
and make the other utility methods private as
we don't really need to use them outside of the job.
---
 app/models/ci/build.rb       | 28 +++++++++++++---------------
 spec/models/ci/build_spec.rb | 24 ++++++++++++++++++------
 2 files changed, 31 insertions(+), 21 deletions(-)

diff --git a/app/models/ci/build.rb b/app/models/ci/build.rb
index 79edacc02f3..85a7d64c804 100644
--- a/app/models/ci/build.rb
+++ b/app/models/ci/build.rb
@@ -138,13 +138,8 @@ 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
-      options.dig(:environment, :url)
+    def ci_environment_url
+      expanded_environment_url || persisted_environment&.external_url
     end
 
     def has_environment?
@@ -506,14 +501,8 @@ module Ci
 
       variables = persisted_environment.predefined_variables
 
-      if environment_url
-        variables << { key: 'CI_ENVIRONMENT_URL',
-                       value: expanded_environment_url,
-                       public: true }
-      elsif persisted_environment.external_url.present?
-        variables << { key: 'CI_ENVIRONMENT_URL',
-                       value: persisted_environment.external_url,
-                       public: true }
+      if url = ci_environment_url
+        variables << { key: 'CI_ENVIRONMENT_URL', value: url, public: true }
       end
 
       variables
@@ -537,6 +526,15 @@ module Ci
       variables
     end
 
+    def expanded_environment_url
+      ExpandVariables.expand(environment_url, simple_variables) if
+        environment_url
+    end
+
+    def environment_url
+      options.dig(:environment, :url)
+    end
+
     def build_attributes_from_config
       return {} unless pipeline.config_processor
 
diff --git a/spec/models/ci/build_spec.rb b/spec/models/ci/build_spec.rb
index 141a2741ced..0c68ac20c7d 100644
--- a/spec/models/ci/build_spec.rb
+++ b/spec/models/ci/build_spec.rb
@@ -427,11 +427,11 @@ describe Ci::Build, :models do
       end
     end
 
-    describe '#expanded_environment_url' do
-      subject { build.expanded_environment_url }
+    describe '#ci_environment_url' do
+      subject { job.ci_environment_url }
 
-      context 'when environment uses $CI_COMMIT_REF_NAME' do
-        let(:build) do
+      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' } })
@@ -440,8 +440,8 @@ describe Ci::Build, :models do
         it { is_expected.to eq('http://review/master') }
       end
 
-      context 'when environment uses yaml_variables containing symbol keys' do
-        let(:build) do
+      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' } })
@@ -449,6 +449,18 @@ describe Ci::Build, :models do
 
         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
-- 
GitLab