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

@@ -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

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

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

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

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

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
-    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
+    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
-      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
-      describe '#after_script' do
-        it 'returns after script' do
-          expect(global.after_script).to eq ['make clean']
-        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 '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

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'] }

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