Merge branch '294232-refactor-variables-merging' into 'master'

Refactor variables merging for job rules

See merge request gitlab-org/gitlab!52305

lib/gitlab/ci/build/rules.rb

@@ -7,30 +7,17 @@ module Gitlab
         include ::Gitlab::Utils::StrongMemoize
 
         Result = Struct.new(:when, :start_in, :allow_failure, :variables) do
-          def build_attributes(seed_attributes = {})
+          def build_attributes
             {
               when: self.when,
               options: { start_in: start_in }.compact,
-              allow_failure: allow_failure,
-              yaml_variables: yaml_variables(seed_attributes[:yaml_variables])
+              allow_failure: allow_failure
             }.compact
           end
 
           def pass?
             self.when != 'never'
           end
-
-          private
-
-          def yaml_variables(seed_variables)
-            return unless variables && seed_variables
-
-            indexed_seed_variables = seed_variables.deep_dup.index_by { |var| var[:key] }
-
-            variables.each_with_object(indexed_seed_variables) do |var, hash|
-              hash[var[0].to_s] = { key: var[0].to_s, value: var[1], public: true }
-            end.values
-          end
         end
 
         def initialize(rule_hashes, default_when:)

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

@@ -159,7 +159,11 @@ module Gitlab
               next {} unless @using_rules
 
               if ::Gitlab::Ci::Features.rules_variables_enabled?(@pipeline.project)
-                rules_result.build_attributes(@seed_attributes)
+                rules_variables_result = ::Gitlab::Ci::Variables::Helpers.merge_variables(
+                  @seed_attributes[:yaml_variables], rules_result.variables
+                )
+
+                rules_result.build_attributes.merge(yaml_variables: rules_variables_result)
               else
                 rules_result.build_attributes
               end

lib/gitlab/ci/variables/helpers.rb

+# frozen_string_literal: true
+
+module Gitlab
+  module Ci
+    module Variables
+      module Helpers
+        class << self
+          def merge_variables(current_vars, new_vars)
+            current_vars = transform_from_yaml_variables(current_vars)
+            new_vars = transform_from_yaml_variables(new_vars)
+
+            transform_to_yaml_variables(
+              current_vars.merge(new_vars)
+            )
+          end
+
+          def transform_to_yaml_variables(vars)
+            vars.to_h.map do |key, value|
+              { key: key.to_s, value: value, public: true }
+            end
+          end
+
+          def transform_from_yaml_variables(vars)
+            return vars.stringify_keys if vars.is_a?(Hash)
+
+            vars.to_a.map { |var| [var[:key].to_s, var[:value]] }.to_h
+          end
+        end
+      end
+    end
+  end
+end

lib/gitlab/ci/yaml_processor/result.rb

@@ -123,9 +123,7 @@ module Gitlab
         end
 
         def transform_to_yaml_variables(variables)
-          variables.to_h.map do |key, value|
-            { key: key.to_s, value: value, public: true }
-          end
+          ::Gitlab::Ci::Variables::Helpers.transform_to_yaml_variables(variables)
         end
       end
     end

spec/lib/gitlab/ci/build/rules_spec.rb

@@ -201,40 +201,13 @@ RSpec.describe Gitlab::Ci::Build::Rules do
     end
 
     describe '#build_attributes' do
-      let(:seed_attributes) { {} }
-
       subject(:build_attributes) do
-        result.build_attributes(seed_attributes)
+        result.build_attributes
       end
 
       it 'compacts nil values' do
         is_expected.to eq(options: {}, when: 'on_success')
       end
-
-      context 'when there are variables in rules' do
-        let(:variables) { { VAR1: 'new var 1', VAR3: 'var 3' } }
-
-        context 'when there are seed variables' do
-          let(:seed_attributes) do
-            { yaml_variables: [{ key: 'VAR1', value: 'var 1', public: true },
-                               { key: 'VAR2', value: 'var 2', public: true }] }
-          end
-
-          it 'returns yaml_variables with override' do
-            is_expected.to include(
-              yaml_variables: [{ key: 'VAR1', value: 'new var 1', public: true },
-                               { key: 'VAR2', value: 'var 2', public: true },
-                               { key: 'VAR3', value: 'var 3', public: true }]
-            )
-          end
-        end
-
-        context 'when there is not seed variables' do
-          it 'does not return yaml_variables' do
-            is_expected.not_to have_key(:yaml_variables)
-          end
-        end
-      end
     end
 
     describe '#pass?' do

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

+# frozen_string_literal: true
+
+require 'fast_spec_helper'
+
+RSpec.describe Gitlab::Ci::Variables::Helpers do
+  describe '.merge_variables' do
+    let(:current_variables) do
+      [{ key: 'key1', value: 'value1' },
+       { key: 'key2', value: 'value2' }]
+    end
+
+    let(:new_variables) do
+      [{ key: 'key2', value: 'value22' },
+       { key: 'key3', value: 'value3' }]
+    end
+
+    let(:result) do
+      [{ key: 'key1', value: 'value1', public: true },
+       { key: 'key2', value: 'value22', public: true },
+       { key: 'key3', value: 'value3', public: true }]
+    end
+
+    subject { described_class.merge_variables(current_variables, new_variables) }
+
+    it { is_expected.to eq(result) }
+
+    context 'when new variables is a hash' do
+      let(:new_variables) do
+        { 'key2' => 'value22', 'key3' => 'value3' }
+      end
+
+      it { is_expected.to eq(result) }
+    end
+
+    context 'when new variables is a hash with symbol keys' do
+      let(:new_variables) do
+        { key2: 'value22', key3: 'value3' }
+      end
+
+      it { is_expected.to eq(result) }
+    end
+
+    context 'when new variables is nil' do
+      let(:new_variables) {}
+      let(:result) do
+        [{ key: 'key1', value: 'value1', public: true },
+         { key: 'key2', value: 'value2', public: true }]
+      end
+
+      it { is_expected.to eq(result) }
+    end
+  end
+
+  describe '.transform_to_yaml_variables' do
+    let(:variables) do
+      { 'key1' => 'value1', 'key2' => 'value2' }
+    end
+
+    let(:result) do
+      [{ key: 'key1', value: 'value1', public: true },
+       { key: 'key2', value: 'value2', public: true }]
+    end
+
+    subject { described_class.transform_to_yaml_variables(variables) }
+
+    it { is_expected.to eq(result) }
+
+    context 'when variables is nil' do
+      let(:variables) {}
+
+      it { is_expected.to eq([]) }
+    end
+  end
+
+  describe '.transform_from_yaml_variables' do
+    let(:variables) do
+      [{ key: 'key1', value: 'value1', public: true },
+       { key: 'key2', value: 'value2', public: true }]
+    end
+
+    let(:result) do
+      { 'key1' => 'value1', 'key2' => 'value2' }
+    end
+
+    subject { described_class.transform_from_yaml_variables(variables) }
+
+    it { is_expected.to eq(result) }
+
+    context 'when variables is nil' do
+      let(:variables) {}
+
+      it { is_expected.to eq({}) }
+    end
+
+    context 'when variables is a hash' do
+      let(:variables) do
+        { key1: 'value1', 'key2' => 'value2' }
+      end
+
+      it { is_expected.to eq(result) }
+    end
+  end
+end