Address MR review comments

lib/expand_variables.rb

@@ -34,11 +34,7 @@ module ExpandVariables
     end
 
     def match_or_blank_value(variables, last_match)
-      ref_var_name = last_match[1] || last_match[2]
-      ref_var = variables[ref_var_name]
-      return ref_var if ref_var.is_a?(String) # if entry is a simple "key" => "value" hash
-
-      ref_var[:value] if ref_var
+      variables[last_match[1] || last_match[2]]
     end
 
     def match_or_original_value(variables, last_match)
@@ -52,10 +48,10 @@ module ExpandVariables
       # Convert Collection to variables
       variables = variables.to_hash if variables.is_a?(Gitlab::Ci::Variables::Collection)
 
-      # Convert hash array to hash of variables
+      # Convert hash array to variables
       if variables.is_a?(Array)
         variables = variables.reduce({}) do |hash, variable|
-          hash[variable[:key]] = variable
+          hash[variable[:key]] = variable[:value]
           hash
         end
       end

lib/gitlab/ci/pipeline/seed/build.rb

@@ -162,7 +162,7 @@ module Gitlab
           end
 
           def variable_expansion_errors
-            expanded_collection = evaluate_context.variables.expand_all(@pipeline.project)
+            expanded_collection = evaluate_context.variables.sort_and_expand_all(@pipeline.project)
             errors = expanded_collection.errors
             ["#{name}: #{errors}"] if errors
           end

lib/gitlab/ci/variables/collection.rb

@@ -72,7 +72,7 @@ module Gitlab
           end
         end
 
-        def expand_all(project, keep_undefined: false)
+        def sort_and_expand_all(project, keep_undefined: false)
           return self if Feature.disabled?(:variable_inside_variable, project)
 
           sorted = Sort.new(self)

spec/lib/gitlab/ci/variables/collection/sort_spec.rb

@@ -29,7 +29,7 @@ RSpec.describe Gitlab::Ci::Variables::Collection::Sort do
         {
           "empty array": {
             variables: [],
-            validation_result: nil
+            expected_errors: nil
           },
           "simple expansions": {
             variables: [
@@ -37,7 +37,7 @@ RSpec.describe Gitlab::Ci::Variables::Collection::Sort do
               { key: 'variable2', value: 'result' },
               { key: 'variable3', value: 'key$variable$variable2' }
             ],
-            validation_result: nil
+            expected_errors: nil
           },
           "cyclic dependency": {
             variables: [
@@ -45,7 +45,7 @@ RSpec.describe Gitlab::Ci::Variables::Collection::Sort do
               { key: 'variable2', value: '$variable3' },
               { key: 'variable3', value: 'key$variable$variable2' }
             ],
-            validation_result: 'circular variable reference detected: ["variable", "variable2", "variable3"]'
+            expected_errors: 'circular variable reference detected: ["variable", "variable2", "variable3"]'
           },
           "array with raw variable": {
             variables: [
@@ -53,7 +53,7 @@ RSpec.describe Gitlab::Ci::Variables::Collection::Sort do
               { key: 'variable2', value: '$variable3' },
               { key: 'variable3', value: 'key$variable$variable2', raw: true }
             ],
-            validation_result: nil
+            expected_errors: nil
           },
           "variable containing escaped variable reference": {
             variables: [
@@ -61,7 +61,7 @@ RSpec.describe Gitlab::Ci::Variables::Collection::Sort do
               { key: 'variable_b', value: '$$variable_a' },
               { key: 'variable_c', value: '$variable_b' }
             ],
-            validation_result: nil
+            expected_errors: nil
           }
         }
       end
@@ -71,12 +71,12 @@ RSpec.describe Gitlab::Ci::Variables::Collection::Sort do
 
         subject { Gitlab::Ci::Variables::Collection::Sort.new(collection) }
 
-        it 'errors matches expected validation result' do
-          expect(subject.errors).to eq(validation_result)
+        it 'errors matches expected errors' do
+          expect(subject.errors).to eq(expected_errors)
         end
 
-        it 'valid? matches expected validation result' do
-          expect(subject.valid?).to eq(validation_result.nil?)
+        it 'valid? matches expected errors' do
+          expect(subject.valid?).to eq(expected_errors.nil?)
         end
 
         it 'does not raise' do
@@ -159,7 +159,7 @@ RSpec.describe Gitlab::Ci::Variables::Collection::Sort do
         subject { Gitlab::Ci::Variables::Collection::Sort.new(collection).tsort }
 
         it 'returns correctly sorted variables' do
-          expect(subject.map { |var| var[:key] }).to eq(result)
+          expect(subject.pluck(:key)).to eq(result)
         end
       end
     end

spec/lib/gitlab/ci/variables/collection_spec.rb

@@ -243,7 +243,7 @@ RSpec.describe Gitlab::Ci::Variables::Collection do
     end
   end
 
-  describe '#expand_all' do
+  describe '#sort_and_expand_all' do
     context 'when FF :variable_inside_variable is disabled' do
       let_it_be(:project_with_flag_disabled) { create(:project) }
       let_it_be(:project_with_flag_enabled) { create(:project) }
@@ -305,15 +305,14 @@ RSpec.describe Gitlab::Ci::Variables::Collection do
         with_them do
           let(:collection) { Gitlab::Ci::Variables::Collection.new(variables, keep_undefined: keep_undefined) }
 
-          subject { collection.expand_all(project_with_flag_disabled) }
+          subject { collection.sort_and_expand_all(project_with_flag_disabled) }
 
           it 'returns Collection' do
             is_expected.to be_an_instance_of(Gitlab::Ci::Variables::Collection)
           end
 
           it 'does not expand variables' do
-            var_hash = variables.to_h { |env| [env.fetch(:key), env.fetch(:value)] }
-              .with_indifferent_access
+            var_hash = variables.pluck(:key, :value).to_h
             expect(subject.to_hash).to eq(var_hash)
           end
         end
@@ -481,7 +480,7 @@ RSpec.describe Gitlab::Ci::Variables::Collection do
         with_them do
           let(:collection) { Gitlab::Ci::Variables::Collection.new(variables) }
 
-          subject { collection.expand_all(project_with_flag_enabled, keep_undefined: keep_undefined) }
+          subject { collection.sort_and_expand_all(project_with_flag_enabled, keep_undefined: keep_undefined) }
 
           it 'returns Collection' do
             is_expected.to be_an_instance_of(Gitlab::Ci::Variables::Collection)
@@ -494,11 +493,7 @@ RSpec.describe Gitlab::Ci::Variables::Collection do
           end
 
           it 'preserves raw attribute' do
-            collection.each do |v|
-              k = v[:key]
-              subject_item = subject.find { |sv| sv[:key] == k }
-              expect(subject_item.raw).to eq(v.raw)
-            end
+            expect(subject.pluck(:key, :raw).to_h).to eq(collection.pluck(:key, :raw).to_h)
           end
         end
       end