Convert CI YAML variables keys into strings

So that this would be more consistent with the other variables,
which all of them are string based.

Closes #25554

8 files changed, 69 insertions, 16 deletions

diff --git a/app/models/ci/build.rb b/app/models/ci/build.rb
index e7cf606a7ae..99f3f4711aa 100644
--- a/app/models/ci/build.rb
+++ b/app/models/ci/build.rb
@@ -10,7 +10,7 @@ module Ci
     has_many :deployments, as: :deployable
 
     serialize :options
-    serialize :yaml_variables
+    serialize :yaml_variables, Gitlab::Serialize::YamlVariables
 
     validates :coverage, numericality: true, allow_blank: true
     validates_presence_of :ref
diff --git a/changelogs/unreleased/fix-yaml-variables.yml b/changelogs/unreleased/fix-yaml-variables.yml
new file mode 100644
index 00000000000..3abff1e3b08
--- /dev/null
+++ b/changelogs/unreleased/fix-yaml-variables.yml
@@ -0,0 +1,4 @@
+---
+title: Convert CI YAML variables keys into strings
+merge_request: 8088
+author:
diff --git a/lib/ci/gitlab_ci_yaml_processor.rb b/lib/ci/gitlab_ci_yaml_processor.rb
index fef652cb975..8806a506ffa 100644
--- a/lib/ci/gitlab_ci_yaml_processor.rb
+++ b/lib/ci/gitlab_ci_yaml_processor.rb
@@ -118,7 +118,7 @@ module Ci
         .merge(job_variables(name))
 
       variables.map do |key, value|
-        { key: key, value: value, public: true }
+        { key: key.to_s, value: value.to_s, public: true }
       end
     end
 
diff --git a/lib/gitlab/serialize/yaml_variables.rb b/lib/gitlab/serialize/yaml_variables.rb
new file mode 100644
index 00000000000..ca44acbd906
--- /dev/null
+++ b/lib/gitlab/serialize/yaml_variables.rb
@@ -0,0 +1,30 @@
+
+module Gitlab
+  module Serialize
+    # This serializer could make sure our YAML variables' keys and values
+    # are always strings. This is more for legacy build data because
+    # from now on we convert them into strings before saving to database.
+    module YamlVariables
+      extend self
+
+      def load(string)
+        return unless string
+
+        YAML.load(string).
+          map(&YamlVariables.method(:convert_key_value_to_string))
+      end
+
+      def dump(object)
+        YAML.dump(object)
+      end
+
+      private
+
+      def convert_key_value_to_string(variable)
+        variable[:key] = variable[:key].to_s
+        variable[:value] = variable[:value].to_s
+        variable
+      end
+    end
+  end
+end
diff --git a/spec/factories/ci/builds.rb b/spec/factories/ci/builds.rb
index 62466c06194..0397d5d4001 100644
--- a/spec/factories/ci/builds.rb
+++ b/spec/factories/ci/builds.rb
@@ -22,7 +22,7 @@ FactoryGirl.define do
 
     yaml_variables do
       [
-        { key: :DB_NAME, value: 'postgres', public: true }
+        { key: 'DB_NAME', value: 'postgres', public: true }
       ]
     end
 
diff --git a/spec/lib/ci/gitlab_ci_yaml_processor_spec.rb b/spec/lib/ci/gitlab_ci_yaml_processor_spec.rb
index ff5dcc06ab3..62d68721574 100644
--- a/spec/lib/ci/gitlab_ci_yaml_processor_spec.rb
+++ b/spec/lib/ci/gitlab_ci_yaml_processor_spec.rb
@@ -483,7 +483,7 @@ module Ci
 
       context 'when global variables are defined' do
         let(:variables) do
-          { VAR1: 'value1', VAR2: 'value2' }
+          { 'VAR1' => 'value1', 'VAR2' => 'value2' }
         end
         let(:config) do
           {
@@ -495,18 +495,18 @@ module Ci
 
         it 'returns global variables' do
           expect(subject).to contain_exactly(
-            { key: :VAR1, value: 'value1', public: true },
-            { key: :VAR2, value: 'value2', public: true }
+            { key: 'VAR1', value: 'value1', public: true },
+            { key: 'VAR2', value: 'value2', public: true }
           )
         end
       end
 
       context 'when job and global variables are defined' do
         let(:global_variables) do
-          { VAR1: 'global1', VAR3: 'global3' }
+          { 'VAR1' => 'global1', 'VAR3' => 'global3' }
         end
         let(:job_variables) do
-          { VAR1: 'value1', VAR2: 'value2' }
+          { 'VAR1' => 'value1', 'VAR2' => 'value2' }
         end
         let(:config) do
           {
@@ -518,9 +518,9 @@ module Ci
 
         it 'returns all unique variables' do
           expect(subject).to contain_exactly(
-            { key: :VAR3, value: 'global3', public: true },
-            { key: :VAR1, value: 'value1', public: true },
-            { key: :VAR2, value: 'value2', public: true }
+            { key: 'VAR3', value: 'global3', public: true },
+            { key: 'VAR1', value: 'value1', public: true },
+            { key: 'VAR2', value: 'value2', public: true }
           )
         end
       end
@@ -535,13 +535,13 @@ module Ci
 
         context 'when syntax is correct' do
           let(:variables) do
-            { VAR1: 'value1', VAR2: 'value2' }
+            { 'VAR1' => 'value1', 'VAR2' => 'value2' }
           end
 
           it 'returns job variables' do
             expect(subject).to contain_exactly(
-              { key: :VAR1, value: 'value1', public: true },
-              { key: :VAR2, value: 'value2', public: true }
+              { key: 'VAR1', value: 'value1', public: true },
+              { key: 'VAR2', value: 'value2', public: true }
             )
           end
         end
@@ -549,7 +549,7 @@ module Ci
         context 'when syntax is incorrect' do
           context 'when variables defined but invalid' do
             let(:variables) do
-              [ :VAR1, 'value1', :VAR2, 'value2' ]
+              [ 'VAR1', 'value1', 'VAR2', 'value2' ]
             end
 
             it 'raises error' do
diff --git a/spec/lib/gitlab/serialize/yaml_variables_spec.rb b/spec/lib/gitlab/serialize/yaml_variables_spec.rb
new file mode 100644
index 00000000000..6d74f8c44d6
--- /dev/null
+++ b/spec/lib/gitlab/serialize/yaml_variables_spec.rb
@@ -0,0 +1,19 @@
+require 'spec_helper'
+
+describe Gitlab::Serialize::YamlVariables do
+  subject do
+    Gitlab::Serialize::YamlVariables.load(
+      Gitlab::Serialize::YamlVariables.dump(object))
+  end
+
+  let(:object) do
+    [{ key: :key, value: 'value', public: true },
+     { key: 'wee', value: 1, public: false }]
+  end
+
+  it 'converts key and values into strings' do
+    is_expected.to eq([
+      { key: 'key', value: 'value', public: true },
+      { key: 'wee', value: '1', public: false }])
+  end
+end
diff --git a/spec/models/build_spec.rb b/spec/models/build_spec.rb
index 7f39aff7639..13928695ccb 100644
--- a/spec/models/build_spec.rb
+++ b/spec/models/build_spec.rb
@@ -403,7 +403,7 @@ describe Ci::Build, models: true do
             })
           end
           let(:variables) do
-            [{ key: :KEY, value: 'value', public: true }]
+            [{ key: 'KEY', value: 'value', public: true }]
           end
 
           it { is_expected.to eq(predefined_variables + variables) }