Verify expandability of variables defined as part of environment

5 files changed, 134 insertions, 13 deletions

diff --git a/app/services/create_deployment_service.rb b/app/services/create_deployment_service.rb
index efa0798f47d..577ba731583 100644
--- a/app/services/create_deployment_service.rb
+++ b/app/services/create_deployment_service.rb
@@ -23,13 +23,13 @@ class CreateDeploymentService < BaseService
   private
 
   def expanded_name
-    name.expand_variables(variables)
+    ExpandVariables.expand(name, variables)
   end
 
   def expanded_url
     return unless url
 
-    @expanded_url ||= url.expand_variables(variables)
+    @expanded_url ||= ExpandVariables.expand(url, variables)
   end
 
   def name
diff --git a/lib/expand_variables.rb b/lib/expand_variables.rb
index 40bf9483eb5..669735cc56c 100644
--- a/lib/expand_variables.rb
+++ b/lib/expand_variables.rb
@@ -1,15 +1,17 @@
-String.instance_eval
-  def expand_variables(variables)
-    # Convert hash array to variables
-    if variables.is_a?(Array)
-      variables = variables.reduce({}) do |hash, variable|
-        hash[variable[:key]] = variable[:value]
-        hash
+module ExpandVariables
+  class << self
+    def expand_variables(value, variables)
+      # Convert hash array to variables
+      if variables.is_a?(Array)
+        variables = variables.reduce({}) do |hash, variable|
+          hash[variable[:key]] = variable[:value]
+          hash
+        end
       end
-    end
 
-    self.gsub(/\$([a-zA-Z_][a-zA-Z0-9_]*)|\${\g<1>}|%\g<1>%/) do
-      variables[$1 || $2]
+      value.gsub(/\$([a-zA-Z_][a-zA-Z0-9_]*)|\${\g<1>}|%\g<1>%/) do
+        variables[$1 || $2]
+      end
     end
   end
 end
diff --git a/spec/lib/expand_variables_spec.rb b/spec/lib/expand_variables_spec.rb
new file mode 100644
index 00000000000..90bc7dad379
--- /dev/null
+++ b/spec/lib/expand_variables_spec.rb
@@ -0,0 +1,73 @@
+require 'spec_helper'
+
+describe ExpandVariables do
+  describe '#expand' do
+    subject { described_class.expand(value, variables) }
+
+    tests = [
+      { value: 'key',
+        result: 'key',
+        variables: []
+      },
+      { value: 'key$variable',
+        result: 'key',
+        variables: []
+      },
+      { value: 'key$variable',
+        result: 'keyvalue',
+        variables: [
+          { key: 'variable', value: 'value' }
+        ]
+      },
+      { value: 'key${variable}',
+        result: 'keyvalue',
+        variables: [
+          { key: 'variable', value: 'value' }
+        ]
+      },
+      { value: 'key$variable$variable2',
+        result: 'keyvalueresult',
+        variables: [
+          { key: 'variable', value: 'value' },
+          { key: 'variable2', value: 'result' },
+        ]
+      },
+      { value: 'key${variable}${variable2}',
+        result: 'keyvalueresult',
+        variables: [
+          { key: 'variable', value: 'value' },
+          { key: 'variable2', value: 'result' }
+        ]
+      },
+      { value: 'key$variable2$variable',
+        result: 'keyresultvalue',
+        variables: [
+          { key: 'variable', value: 'value' },
+          { key: 'variable2', value: 'result' },
+        ]
+      },
+      { value: 'key${variable2}${variable}',
+        result: 'keyresultvalue',
+        variables: [
+          { key: 'variable', value: 'value' },
+          { key: 'variable2', value: 'result' }
+        ]
+      },
+      { value: 'review/$CI_BUILD_REF_NAME',
+        result: 'review/feature/add-review-apps',
+        variables: [
+          { key: 'CI_BUILD_REF_NAME', value: 'feature/add-review-apps' }
+        ]
+      },
+    ]
+
+    tests.each do |test|
+      context "#{test[:value]} resolves to #{test[:result]}" do
+        let(:value) { test[:value] }
+        let(:variables) { test[:variables] }
+
+        it { is_expected.to eq(test[:result]) }
+      end
+    end
+  end
+end
diff --git a/spec/models/environment_spec.rb b/spec/models/environment_spec.rb
index c881897926e..7afc7ec5ca1 100644
--- a/spec/models/environment_spec.rb
+++ b/spec/models/environment_spec.rb
@@ -63,4 +63,20 @@ describe Environment, models: true do
       end
     end
   end
+
+  describe '#environment_type' do
+    subject { environment.environment_type }
+
+    it 'sets a environment type if name has multiple segments' do
+      environment.update(name: 'production/worker.gitlab.com')
+
+      is_expected.to eq('production')
+    end
+
+    it 'nullifies a type if it\'s a simple name' do
+      environment.update(name: 'production')
+
+      is_expected.to be_nil
+    end
+  end
 end
diff --git a/spec/services/create_deployment_service_spec.rb b/spec/services/create_deployment_service_spec.rb
index 8da2a2b3c1b..c8c741c0e88 100644
--- a/spec/services/create_deployment_service_spec.rb
+++ b/spec/services/create_deployment_service_spec.rb
@@ -56,8 +56,38 @@ describe CreateDeploymentService, services: true do
         expect(subject).not_to be_persisted
       end
     end
+
+    context 'when variables are used' do
+      let(:params) do
+        { environment: 'review-apps/$CI_BUILD_REF_NAME',
+          ref: 'master',
+          tag: false,
+          sha: '97de212e80737a608d939f648d959671fb0a0142',
+          options: {
+            environment: {
+              name: 'review-apps/$CI_BUILD_REF_NAME',
+              url: 'http://$CI_BUILD_REF_NAME.review-apps.gitlab.com'
+            }
+          },
+          variables: [
+            { key: 'CI_BUILD_REF_NAME', value: 'feature-review-apps' }
+          ]
+        }
+      end
+
+      it 'does create a new environment' do
+        expect { subject }.to change { Environment.count }.by(1)
+
+        expect(subject.environment.name).to eq('review-apps/feature-review-apps')
+        expect(subject.environment.external_url).to eq('http://feature-review-apps.review-apps.gitlab.com')
+      end
+
+      it 'does create a new deployment' do
+        expect(subject).not_to be_persisted
+      end
+    end
   end
-  
+
   describe 'processing of builds' do
     let(:environment) { nil }