Verify expandability of variables defined as part of environment

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

lib/expand_variables.rb

-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

spec/lib/expand_variables_spec.rb

+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

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

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 }