diff --git a/app/models/ci/build.rb b/app/models/ci/build.rb
index 591aba6bdc993fc8e6f603b42d04cd41facd1358..cb76cdf5981421e7ec9072bd89191e7283d1590d 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 0000000000000000000000000000000000000000..3abff1e3b085b67091fc360a506330125b337f07
--- /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 fef652cb975319a7324f2b2a0c797c96e5fda7f6..7463bd719d5bb14398d4f516d648ebde9b95593c 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 0000000000000000000000000000000000000000..3a9443bfcd981e7fae080836c79a69f20b6a5f3e
--- /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 62466c061941a4df5a06a32fb57c567656b7d2a1..0397d5d400185349606de1693b2687d5f2ecae3f 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 ff5dcc06ab30781f9d6e7cc3b51e83b0323a9429..62d68721574bc588af8bdcb3ac4fa3b8cd9c5cbe 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 0000000000000000000000000000000000000000..7ea74da5252efec6b713024b29b42e0ffa279e3c
--- /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 6f1c2ae0fd84da0a9545b9193fc80cdd5131f2ba..cd3b6d51545803be41a355811706fef380ff2fd5 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