Integration variables collections with expressions

9 files changed, 71 insertions, 45 deletions

diff --git a/lib/gitlab/ci/build/policy/variables.rb b/lib/gitlab/ci/build/policy/variables.rb
index ec6c64d9547..e5fec4469b3 100644
--- a/lib/gitlab/ci/build/policy/variables.rb
+++ b/lib/gitlab/ci/build/policy/variables.rb
@@ -8,9 +8,13 @@ module Gitlab
           end
 
           def satisfied_by?(pipeline, build)
+            variables = Gitlab::Ci::Variables::Collection
+              .new(build.simple_variables)
+              .to_hash
+
             statements = @expressions.map do |statement|
               ::Gitlab::Ci::Pipeline::Expression::Statement
-                .new(statement, pipeline)
+                .new(statement, variables)
             end
 
             statements.any?(&:truthful?)
diff --git a/lib/gitlab/ci/pipeline/expression/lexeme/variable.rb b/lib/gitlab/ci/pipeline/expression/lexeme/variable.rb
index b781c15fd67..37643c8ef53 100644
--- a/lib/gitlab/ci/pipeline/expression/lexeme/variable.rb
+++ b/lib/gitlab/ci/pipeline/expression/lexeme/variable.rb
@@ -11,7 +11,7 @@ module Gitlab
             end
 
             def evaluate(variables = {})
-              HashWithIndifferentAccess.new(variables).fetch(@name, nil)
+              variables.with_indifferent_access.fetch(@name, nil)
             end
 
             def self.build(string)
diff --git a/lib/gitlab/ci/pipeline/expression/statement.rb b/lib/gitlab/ci/pipeline/expression/statement.rb
index f0f3d760b70..b04576f7978 100644
--- a/lib/gitlab/ci/pipeline/expression/statement.rb
+++ b/lib/gitlab/ci/pipeline/expression/statement.rb
@@ -14,24 +14,9 @@ module Gitlab
             %w[variable]
           ].freeze
 
-          def initialize(statement, pipeline = nil)
+          def initialize(statement, variables = {})
             @lexer = Expression::Lexer.new(statement)
-
-            return if pipeline.nil?
-
-            # REFACTORING, temporary refactoring stubs
-            #
-            @variables = pipeline.project.predefined_variables.map do |variable|
-              [variable[:key], variable[:value]]
-            end
-
-            @variables += pipeline.variables.map do |variable|
-              [variable.key, variable.value]
-            end
-
-            @variables += pipeline.predefined_variables.map do |variable|
-              [variable[:key], variable[:value]]
-            end
+            @variables = variables.with_indifferent_access
           end
 
           def parse_tree
diff --git a/lib/gitlab/ci/variables/collection.rb b/lib/gitlab/ci/variables/collection.rb
index 0deca55fe8f..ad30b3f427c 100644
--- a/lib/gitlab/ci/variables/collection.rb
+++ b/lib/gitlab/ci/variables/collection.rb
@@ -30,7 +30,13 @@ module Gitlab
         end
 
         def to_runner_variables
-          self.map(&:to_hash)
+          self.map(&:to_runner_variable)
+        end
+
+        def to_hash
+          self.to_runner_variables
+            .map { |env| [env.fetch(:key), env.fetch(:value)] }
+            .to_h.with_indifferent_access
         end
       end
     end
diff --git a/lib/gitlab/ci/variables/collection/item.rb b/lib/gitlab/ci/variables/collection/item.rb
index 939912981e6..23ed71db8b0 100644
--- a/lib/gitlab/ci/variables/collection/item.rb
+++ b/lib/gitlab/ci/variables/collection/item.rb
@@ -17,7 +17,7 @@ module Gitlab
           end
 
           def ==(other)
-            to_hash == self.class.fabricate(other).to_hash
+            to_runner_variable == self.class.fabricate(other).to_runner_variable
           end
 
           ##
@@ -25,7 +25,7 @@ module Gitlab
           # don't expose `file` attribute at all (stems from what the runner
           # expects).
           #
-          def to_hash
+          def to_runner_variable
             @variable.reject do |hash_key, hash_value|
               hash_key == :file && hash_value == false
             end
diff --git a/spec/lib/gitlab/ci/build/policy/variables_spec.rb b/spec/lib/gitlab/ci/build/policy/variables_spec.rb
index ce229ed256a..ac5d05e0583 100644
--- a/spec/lib/gitlab/ci/build/policy/variables_spec.rb
+++ b/spec/lib/gitlab/ci/build/policy/variables_spec.rb
@@ -1,42 +1,58 @@
 require 'spec_helper'
 
 describe Gitlab::Ci::Build::Policy::Variables do
-  let(:pipeline) { build_stubbed(:ci_empty_pipeline, ref: 'master') }
-  let(:build) { build_stubbed(:ci_build, pipeline: pipeline, ref: 'master') }
+  set(:project) { create(:project) }
+
+  let(:ci_pipeline) do
+    build(:ci_empty_pipeline, project: project, ref: 'master')
+  end
+
+  let(:ci_build) do
+    build(:ci_build, pipeline: ci_pipeline, project: project, ref: 'master')
+  end
 
   before do
-    pipeline.variables.build(key: 'CI_PROJECT_NAME', value: '')
+    ci_pipeline.variables.build(key: 'CI_PROJECT_NAME', value: '')
   end
 
   describe '#satisfied_by?' do
     it 'is satisfied by a defined and existing variable' do
       policy = described_class.new(['$CI_PROJECT_ID', '$UNDEFINED'])
 
-      expect(policy).to be_satisfied_by(pipeline, build)
+      expect(policy).to be_satisfied_by(ci_pipeline, ci_build)
     end
 
     it 'is not satisfied by an overriden empty variable' do
       policy = described_class.new(['$CI_PROJECT_NAME'])
 
-      expect(policy).not_to be_satisfied_by(pipeline, build)
+      expect(policy).not_to be_satisfied_by(ci_pipeline, ci_build)
     end
 
     it 'is satisfied by a truthy pipeline expression' do
-      policy = described_class.new([%($CI_PIPELINE_SOURCE == "#{pipeline.source}")])
+      policy = described_class.new([%($CI_PIPELINE_SOURCE == "#{ci_pipeline.source}")])
 
-      expect(policy).to be_satisfied_by(pipeline, build)
+      expect(policy).to be_satisfied_by(ci_pipeline, ci_build)
     end
 
     it 'is not satisfied by a falsy pipeline expression' do
       policy = described_class.new([%($CI_PIPELINE_SOURCE == "invalid source")])
 
-      expect(policy).not_to be_satisfied_by(pipeline, build)
+      expect(policy).not_to be_satisfied_by(ci_pipeline, ci_build)
     end
 
     it 'is satisfied by a truthy expression using undefined variable' do
       policy = described_class.new(['$UNDEFINED', '$UNDEFINED == null'])
 
-      expect(policy).to be_satisfied_by(pipeline, build)
+      expect(policy).to be_satisfied_by(ci_pipeline, ci_build)
     end
+
+    it 'does not persist neither pipeline nor build' do
+      described_class.new('$VAR').satisfied_by?(ci_pipeline, ci_build)
+
+      expect(ci_pipeline).not_to be_persisted
+      expect(ci_build).not_to be_persisted
+    end
+
+    pending 'test for secret variables'
   end
 end
diff --git a/spec/lib/gitlab/ci/pipeline/expression/statement_spec.rb b/spec/lib/gitlab/ci/pipeline/expression/statement_spec.rb
index 475ac7afcb3..6685bf5385b 100644
--- a/spec/lib/gitlab/ci/pipeline/expression/statement_spec.rb
+++ b/spec/lib/gitlab/ci/pipeline/expression/statement_spec.rb
@@ -1,22 +1,18 @@
 require 'spec_helper'
 
 describe Gitlab::Ci::Pipeline::Expression::Statement do
-  let(:pipeline) { build(:ci_pipeline) }
-
   subject do
-    described_class.new(text, pipeline)
+    described_class.new(text, variables)
   end
 
-  before do
-    variables = [{ key: 'PRESENT_VARIABLE', value: 'my variable' },
-                 { key: 'EMPTY_VARIABLE', value: '' }]
-
-    pipeline.variables.build(variables)
+  let(:variables) do
+    { 'PRESENT_VARIABLE' => 'my variable',
+      EMPTY_VARIABLE: '' }
   end
 
   describe '.new' do
-    context 'when pipeline is not provided' do
-      it 'allows to properly initialize the statement' do
+    context 'when variables are not provided' do
+      it 'allows to properly initializes the statement' do
         statement = described_class.new('$PRESENT_VARIABLE')
 
         expect(statement.evaluate).to be_nil
diff --git a/spec/lib/gitlab/ci/variables/collection/item_spec.rb b/spec/lib/gitlab/ci/variables/collection/item_spec.rb
index cc1257484d2..bf9208f1ff4 100644
--- a/spec/lib/gitlab/ci/variables/collection/item_spec.rb
+++ b/spec/lib/gitlab/ci/variables/collection/item_spec.rb
@@ -46,9 +46,13 @@ describe Gitlab::Ci::Variables::Collection::Item do
     end
   end
 
-  describe '#to_hash' do
-    it 'returns a hash representation of a collection item' do
-      expect(described_class.new(**variable).to_hash).to eq variable
+  describe '#to_runner_variable' do
+    it 'returns a runner-compatible hash representation' do
+      runner_variable = described_class
+        .new(**variable)
+        .to_runner_variable
+
+      expect(runner_variable).to eq variable
     end
   end
 end
diff --git a/spec/lib/gitlab/ci/variables/collection_spec.rb b/spec/lib/gitlab/ci/variables/collection_spec.rb
index 90b6e178242..cb2f7718c9c 100644
--- a/spec/lib/gitlab/ci/variables/collection_spec.rb
+++ b/spec/lib/gitlab/ci/variables/collection_spec.rb
@@ -7,7 +7,7 @@ describe Gitlab::Ci::Variables::Collection do
 
       collection = described_class.new([variable])
 
-      expect(collection.first.to_hash).to eq variable
+      expect(collection.first.to_runner_variable).to eq variable
     end
 
     it 'can be initialized without an argument' do
@@ -96,4 +96,19 @@ describe Gitlab::Ci::Variables::Collection do
         .to eq [{ key: 'TEST', value: 1, public: true }]
     end
   end
+
+  describe '#to_hash' do
+    it 'returns regular hash in valid order without duplicates' do
+      collection = described_class.new
+        .append(key: 'TEST1', value: 'test-1')
+        .append(key: 'TEST2', value: 'test-2')
+        .append(key: 'TEST1', value: 'test-3')
+
+      expect(collection.to_hash).to eq('TEST1' => 'test-3',
+                                       'TEST2' => 'test-2')
+
+      expect(collection.to_hash).to include(TEST1: 'test-3')
+      expect(collection.to_hash).not_to include(TEST1: 'test-1')
+    end
+  end
 end