Merge branch 'fix-yaml-variables' into 'master'

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

See merge request !8088

8 files changed, 81 insertions, 18 deletions

diff --git a/app/models/ci/build.rb b/app/models/ci/build.rb
index 591aba6bdc9..cb76cdf5981 100644
--- a/app/models/ci/build.rb
+++ b/app/models/ci/build.rb
@@ -18,7 +18,7 @@ module Ci
     end
 
     serialize :options
-    serialize :yaml_variables
+    serialize :yaml_variables, Gitlab::Serialize::Ci::Variables
 
     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..7463bd719d5 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, public: true }
       end
     end
 
diff --git a/lib/gitlab/serialize/ci/variables.rb b/lib/gitlab/serialize/ci/variables.rb
new file mode 100644
index 00000000000..3a9443bfcd9
--- /dev/null
+++ b/lib/gitlab/serialize/ci/variables.rb
@@ -0,0 +1,27 @@
+module Gitlab
+  module Serialize
+    module Ci
+      # 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 Variables
+        extend self
+
+        def load(string)
+          return unless string
+
+          object = YAML.safe_load(string, [Symbol])
+
+          object.map do |variable|
+            variable[:key] = variable[:key].to_s
+            variable
+          end
+        end
+
+        def dump(object)
+          YAML.dump(object)
+        end
+      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/ci/variables_spec.rb b/spec/lib/gitlab/serialize/ci/variables_spec.rb
new file mode 100644
index 00000000000..7ea74da5252
--- /dev/null
+++ b/spec/lib/gitlab/serialize/ci/variables_spec.rb
@@ -0,0 +1,18 @@
+require 'spec_helper'
+
+describe Gitlab::Serialize::Ci::Variables do
+  subject do
+    described_class.load(described_class.dump(object))
+  end
+
+  let(:object) do
+    [{ key: :key, value: 'value', public: true },
+     { key: 'wee', value: 1, public: false }]
+  end
+
+  it 'converts keys 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 6f1c2ae0fd8..cd3b6d51545 100644
--- a/spec/models/build_spec.rb
+++ b/spec/models/build_spec.rb
@@ -458,7 +458,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) }
@@ -1306,11 +1306,25 @@ describe Ci::Build, models: true do
   describe '#expanded_environment_name' do
     subject { build.expanded_environment_name }
 
-    context 'when environment uses variables' do
-      let(:build) { create(:ci_build, ref: 'master', environment: 'review/$CI_BUILD_REF_NAME') }
+    context 'when environment uses $CI_BUILD_REF_NAME' do
+      let(:build) do
+        create(:ci_build,
+               ref: 'master',
+               environment: 'review/$CI_BUILD_REF_NAME')
+      end
 
       it { is_expected.to eq('review/master') }
     end
+
+    context 'when environment uses yaml_variables containing symbol keys' do
+      let(:build) do
+        create(:ci_build,
+               yaml_variables: [{ key: :APP_HOST, value: 'host' }],
+               environment: 'review/$APP_HOST')
+      end
+
+      it { is_expected.to eq('review/host') }
+    end
   end
 
   describe '#detailed_status' do