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'] } + 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! } + + 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 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 + + describe '#leaf?' do + it 'is not leaf' do + expect(global).not_to be_leaf + end + end + + context 'when not processed' do + describe '#before_script' do + it 'returns nil' do + expect(global.before_script).to be nil + end + end + end + + 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 - describe '#process!' do + context 'when most of entires not defined' do + let(:hash) { { rspec: {} } } before { global.process! } - 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 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 - - describe '#leaf?' do - it 'is not leaf' do - expect(global).not_to be_leaf - end - end - - context 'when not processed' do - describe '#before_script' do - it 'returns nil' do - expect(global.before_script).to be nil + describe '#nodes' do + it 'instantizes all nodes' do + expect(global.nodes.count).to eq 5 end - end - end - 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'] + it 'contains undefined nodes' do + expect(global.nodes.last) + .to be_an_instance_of Gitlab::Ci::Config::Node::Undefined end end describe '#variables' do - it 'returns variables' do - expect(global.variables).to eq(VAR: 'value') + it 'returns default value for variables' do + expect(global.variables).to eq({}) + end + 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 '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