From 2240807c1aaa7d7df313dde9775e3ec99f7ad1b3 Mon Sep 17 00:00:00 2001
From: Grzegorz Bizon <grzesiek.bizon@gmail.com>
Date: Thu, 23 Jun 2016 10:07:42 +0200
Subject: [PATCH] Assume that unspecified CI config is undefined
We assume that when someone adds a key for the configuration entry, but
does not provide a valid value, which causes entry to be `nil`, then
entry should be considered as the undefined one. We also assume this is
semantically correct, this is also backwards compatible with legacy CI
config processor.
See issue #18775 for more details.
---
lib/gitlab/ci/config/node/configurable.rb | 1 -
lib/gitlab/ci/config/node/factory.rb | 18 ++-
lib/gitlab/ci/config/node/variables.rb | 6 +-
spec/lib/ci/gitlab_ci_yaml_processor_spec.rb | 8 +-
.../lib/gitlab/ci/config/node/factory_spec.rb | 5 +-
spec/lib/gitlab/ci/config/node/global_spec.rb | 145 +++++++++++-------
.../gitlab/ci/config/node/services_spec.rb | 4 +-
.../gitlab/ci/config/node/variables_spec.rb | 19 ---
8 files changed, 112 insertions(+), 94 deletions(-)
diff --git a/lib/gitlab/ci/config/node/configurable.rb b/lib/gitlab/ci/config/node/configurable.rb
index 25b5c2c9e21..0fb9092dafa 100644
--- a/lib/gitlab/ci/config/node/configurable.rb
+++ b/lib/gitlab/ci/config/node/configurable.rb
@@ -27,7 +27,6 @@ module Gitlab
def create_node(key, factory)
factory.with(value: @config[key], key: key)
- factory.undefine! unless @config.has_key?(key)
factory.create!
end
diff --git a/lib/gitlab/ci/config/node/factory.rb b/lib/gitlab/ci/config/node/factory.rb
index 2271c386df6..647b0c82a79 100644
--- a/lib/gitlab/ci/config/node/factory.rb
+++ b/lib/gitlab/ci/config/node/factory.rb
@@ -18,16 +18,20 @@ module Gitlab
self
end
- def undefine!
- @attributes[:value] = @node.dup
- @node = Node::Undefined
- self
- end
-
def create!
raise InvalidFactory unless @attributes.has_key?(:value)
- @node.new(@attributes[:value]).tap do |entry|
+ ##
+ # We assume unspecified entry is undefined.
+ # See issue #18775.
+ #
+ if @attributes[:value].nil?
+ node, value = Node::Undefined, @node
+ else
+ node, value = @node, @attributes[:value]
+ end
+
+ node.new(value).tap do |entry|
entry.description = @attributes[:description]
entry.key = @attributes[:key]
end
diff --git a/lib/gitlab/ci/config/node/variables.rb b/lib/gitlab/ci/config/node/variables.rb
index fd3ce8715a9..5f813f81f55 100644
--- a/lib/gitlab/ci/config/node/variables.rb
+++ b/lib/gitlab/ci/config/node/variables.rb
@@ -9,11 +9,7 @@ module Gitlab
include Validatable
validations do
- validates :value, variables: true
- end
-
- def value
- @config || self.class.default
+ validates :config, variables: true
end
def self.default
diff --git a/spec/lib/ci/gitlab_ci_yaml_processor_spec.rb b/spec/lib/ci/gitlab_ci_yaml_processor_spec.rb
index 97a08758c04..eb20f5f4c06 100644
--- a/spec/lib/ci/gitlab_ci_yaml_processor_spec.rb
+++ b/spec/lib/ci/gitlab_ci_yaml_processor_spec.rb
@@ -551,8 +551,8 @@ module Ci
config_processor = GitlabCiYamlProcessor.new(config, path)
##
- # TODO, in next version of CI configuration processor this
- # should be invalid configuration, see #18775 and #15060
+ # When variables config is empty, we asumme this is a correct,
+ # see issue #18775
#
expect(config_processor.job_variables(:rspec))
.to be_an_instance_of(Array).and be_empty
@@ -1098,14 +1098,14 @@ EOT
config = YAML.dump({ variables: "test", rspec: { script: "test" } })
expect do
GitlabCiYamlProcessor.new(config, path)
- end.to raise_error(GitlabCiYamlProcessor::ValidationError, "Variables value should be a hash of key value pairs")
+ end.to raise_error(GitlabCiYamlProcessor::ValidationError, "Variables config should be a hash of key value pairs")
end
it "returns errors if variables is not a map of key-value strings" do
config = YAML.dump({ variables: { test: false }, rspec: { script: "test" } })
expect do
GitlabCiYamlProcessor.new(config, path)
- end.to raise_error(GitlabCiYamlProcessor::ValidationError, "Variables value should be a hash of key value pairs")
+ end.to raise_error(GitlabCiYamlProcessor::ValidationError, "Variables config should be a hash of key value pairs")
end
it "returns errors if job when is not on_success, on_failure or always" do
diff --git a/spec/lib/gitlab/ci/config/node/factory_spec.rb b/spec/lib/gitlab/ci/config/node/factory_spec.rb
index 8e13e243d4d..dd5f6e62b3e 100644
--- a/spec/lib/gitlab/ci/config/node/factory_spec.rb
+++ b/spec/lib/gitlab/ci/config/node/factory_spec.rb
@@ -45,11 +45,10 @@ describe Gitlab::Ci::Config::Node::Factory do
end
end
- context 'when creating undefined entry' do
- it 'creates a null entry' do
+ context 'when creating entry with nil value' do
+ it 'creates an undefined entry' do
entry = factory
.with(value: nil)
- .undefine!
.create!
expect(entry).to be_an_instance_of Gitlab::Ci::Config::Node::Undefined
diff --git a/spec/lib/gitlab/ci/config/node/global_spec.rb b/spec/lib/gitlab/ci/config/node/global_spec.rb
index ef4b669c403..36a5b8041f0 100644
--- a/spec/lib/gitlab/ci/config/node/global_spec.rb
+++ b/spec/lib/gitlab/ci/config/node/global_spec.rb
@@ -20,84 +20,125 @@ describe Gitlab::Ci::Config::Node::Global do
end
context 'when hash is valid' do
- let(:hash) do
- { before_script: ['ls', 'pwd'],
- image: 'ruby:2.2',
- services: ['postgres:9.1', 'mysql:5.5'],
- variables: { VAR: 'value' },
- after_script: ['make clean'] }
- end
+ context 'when all entries defined' do
+ let(:hash) do
+ { before_script: ['ls', 'pwd'],
+ image: 'ruby:2.2',
+ services: ['postgres:9.1', 'mysql:5.5'],
+ variables: { VAR: 'value' },
+ after_script: ['make clean'] }
+ end
- describe '#process!' do
- before { global.process! }
+ describe '#process!' do
+ before { global.process! }
- it 'creates nodes hash' do
- expect(global.nodes).to be_an Array
- end
+ it 'creates nodes hash' do
+ expect(global.nodes).to be_an Array
+ end
- it 'creates node object for each entry' do
- expect(global.nodes.count).to eq 5
- end
+ it 'creates node object for each entry' do
+ expect(global.nodes.count).to eq 5
+ end
- it 'creates node object using valid class' do
- expect(global.nodes.first)
- .to be_an_instance_of Gitlab::Ci::Config::Node::Script
- expect(global.nodes.second)
- .to be_an_instance_of Gitlab::Ci::Config::Node::Image
+ it 'creates node object using valid class' do
+ expect(global.nodes.first)
+ .to be_an_instance_of Gitlab::Ci::Config::Node::Script
+ expect(global.nodes.second)
+ .to be_an_instance_of Gitlab::Ci::Config::Node::Image
+ end
+
+ it 'sets correct description for nodes' do
+ expect(global.nodes.first.description)
+ .to eq 'Script that will be executed before each job.'
+ expect(global.nodes.second.description)
+ .to eq 'Docker image that will be used to execute jobs.'
+ end
end
- it 'sets correct description for nodes' do
- expect(global.nodes.first.description)
- .to eq 'Script that will be executed before each job.'
- expect(global.nodes.second.description)
- .to eq 'Docker image that will be used to execute jobs.'
+ describe '#leaf?' do
+ it 'is not leaf' do
+ expect(global).not_to be_leaf
+ end
end
- end
- describe '#leaf?' do
- it 'is not leaf' do
- expect(global).not_to be_leaf
+ context 'when not processed' do
+ describe '#before_script' do
+ it 'returns nil' do
+ expect(global.before_script).to be nil
+ end
+ end
end
- end
- context 'when not processed' do
- describe '#before_script' do
- it 'returns nil' do
- expect(global.before_script).to be nil
+ context 'when processed' do
+ before { global.process! }
+
+ describe '#before_script' do
+ it 'returns correct script' do
+ expect(global.before_script).to eq ['ls', 'pwd']
+ end
+ end
+
+ describe '#image' do
+ it 'returns valid image' do
+ expect(global.image).to eq 'ruby:2.2'
+ end
+ end
+
+ describe '#services' do
+ it 'returns array of services' do
+ expect(global.services).to eq ['postgres:9.1', 'mysql:5.5']
+ end
+ end
+
+ describe '#after_script' do
+ it 'returns after script' do
+ expect(global.after_script).to eq ['make clean']
+ end
+ end
+
+ describe '#variables' do
+ it 'returns variables' do
+ expect(global.variables).to eq(VAR: 'value')
+ end
end
end
end
- context 'when processed' do
+ context 'when most of entires not defined' do
+ let(:hash) { { rspec: {} } }
before { global.process! }
- describe '#before_script' do
- it 'returns correct script' do
- expect(global.before_script).to eq ['ls', 'pwd']
+ describe '#nodes' do
+ it 'instantizes all nodes' do
+ expect(global.nodes.count).to eq 5
end
- end
- describe '#image' do
- it 'returns valid image' do
- expect(global.image).to eq 'ruby:2.2'
+ it 'contains undefined nodes' do
+ expect(global.nodes.last)
+ .to be_an_instance_of Gitlab::Ci::Config::Node::Undefined
end
end
- describe '#services' do
- it 'returns array of services' do
- expect(global.services).to eq ['postgres:9.1', 'mysql:5.5']
+ describe '#variables' do
+ it 'returns default value for variables' do
+ expect(global.variables).to eq({})
end
end
+ end
- describe '#after_script' do
- it 'returns after script' do
- expect(global.after_script).to eq ['make clean']
- end
- end
+ ##
+ # When nodes are specified but not defined, we assume that
+ # configuration is valid, and we asume that entry is simply undefined,
+ # despite the fact, that key is present. See issue #18775 for more
+ # details.
+ #
+ context 'when entires specified but not defined' do
+ let(:hash) { { variables: nil } }
+ before { global.process! }
describe '#variables' do
- it 'returns variables' do
- expect(global.variables).to eq(VAR: 'value')
+ it 'undefined entry returns a default value' do
+ expect(global.variables).to eq({})
end
end
end
diff --git a/spec/lib/gitlab/ci/config/node/services_spec.rb b/spec/lib/gitlab/ci/config/node/services_spec.rb
index bda4b976cb9..e38f6f6923f 100644
--- a/spec/lib/gitlab/ci/config/node/services_spec.rb
+++ b/spec/lib/gitlab/ci/config/node/services_spec.rb
@@ -3,9 +3,7 @@ require 'spec_helper'
describe Gitlab::Ci::Config::Node::Services do
let(:entry) { described_class.new(config) }
- describe '#process!' do
- before { entry.process! }
-
+ describe 'validations' do
context 'when entry config value is correct' do
let(:config) { ['postgres:9.1', 'mysql:5.5'] }
diff --git a/spec/lib/gitlab/ci/config/node/variables_spec.rb b/spec/lib/gitlab/ci/config/node/variables_spec.rb
index 67df70992b4..4b6d971ec71 100644
--- a/spec/lib/gitlab/ci/config/node/variables_spec.rb
+++ b/spec/lib/gitlab/ci/config/node/variables_spec.rb
@@ -44,24 +44,5 @@ describe Gitlab::Ci::Config::Node::Variables do
end
end
end
-
- ##
- # See #18775
- #
- context 'when entry value is not defined' do
- let(:config) { nil }
-
- describe '#valid?' do
- it 'is valid' do
- expect(entry).to be_valid
- end
- end
-
- describe '#values' do
- it 'returns an empty hash' do
- expect(entry.value).to eq({})
- end
- end
- end
end
end
--
GitLab