From 5065612a0a1a5dd68c075e54f5f5f89c5c025a6b Mon Sep 17 00:00:00 2001
From: Grzegorz Bizon <grzesiek.bizon@gmail.com>
Date: Wed, 8 Jun 2016 13:01:44 +0200
Subject: [PATCH] Add minor improvements in new Ci config design

---
 lib/gitlab/ci/config/node/entry.rb            | 40 +++++++++++++------
 lib/gitlab/ci/config/node/null.rb             |  1 +
 .../ci/config/node/before_script_spec.rb      | 12 ++++++
 spec/lib/gitlab/ci/config/node/global_spec.rb | 12 ++++++
 4 files changed, 52 insertions(+), 13 deletions(-)

diff --git a/lib/gitlab/ci/config/node/entry.rb b/lib/gitlab/ci/config/node/entry.rb
index 0767fadcb9a..302cded664f 100644
--- a/lib/gitlab/ci/config/node/entry.rb
+++ b/lib/gitlab/ci/config/node/entry.rb
@@ -12,22 +12,16 @@ module Gitlab
             @nodes = {}
             @errors = []
 
-            unless leaf? || value.is_a?(Hash)
+            unless leaf? || has_config?
               @errors << 'should be a configuration entry with hash value'
             end
           end
 
           def process!
-            return if leaf? || !valid?
+            return if leaf? || invalid?
 
             keys.each do |key, entry_class|
-              if @value.has_key?(key)
-                entry = entry_class.new(@value[key], @root, self)
-              else
-                entry = Node::Null.new(nil, @root, self)
-              end
-
-              @nodes[key] = entry
+              add_node(key, entry_class)
             end
 
             nodes.each(&:process!)
@@ -38,22 +32,30 @@ module Gitlab
             @nodes.values
           end
 
-          def errors
-            @errors + nodes.map(&:errors).flatten
-          end
-
           def valid?
             errors.none?
           end
 
+          def invalid?
+            !valid?
+          end
+
           def leaf?
             keys.none?
           end
 
+          def has_config?
+            @value.is_a?(Hash)
+          end
+
           def keys
             self.class.nodes || {}
           end
 
+          def errors
+            @errors + nodes.map(&:errors).flatten
+          end
+
           def method_missing(name, *args)
             super unless keys.has_key?(name)
             raise InvalidError unless valid?
@@ -73,6 +75,18 @@ module Gitlab
             raise NotImplementedError
           end
 
+          private
+
+          def add_node(key, entry_class)
+            if @value.has_key?(key)
+              entry = entry_class.new(@value[key], @root, self)
+            else
+              entry = Node::Null.new(nil, @root, self)
+            end
+
+            @nodes[key] = entry
+          end
+
           class << self
             attr_reader :nodes
 
diff --git a/lib/gitlab/ci/config/node/null.rb b/lib/gitlab/ci/config/node/null.rb
index db3fa05c328..bf8bc62dc91 100644
--- a/lib/gitlab/ci/config/node/null.rb
+++ b/lib/gitlab/ci/config/node/null.rb
@@ -8,6 +8,7 @@ module Gitlab
           end
 
           def validate!
+            nil
           end
 
           def method_missing(*)
diff --git a/spec/lib/gitlab/ci/config/node/before_script_spec.rb b/spec/lib/gitlab/ci/config/node/before_script_spec.rb
index bc34b9c9b56..b506b9743c6 100644
--- a/spec/lib/gitlab/ci/config/node/before_script_spec.rb
+++ b/spec/lib/gitlab/ci/config/node/before_script_spec.rb
@@ -18,6 +18,12 @@ describe Gitlab::Ci::Config::Node::BeforeScript do
         expect(entry.errors).to be_empty
       end
     end
+
+    describe '#has_config?' do
+      it 'does not have config' do
+        expect(entry).not_to have_config
+      end
+    end
   end
 
   context 'when entry value is not correct' do
@@ -29,5 +35,11 @@ describe Gitlab::Ci::Config::Node::BeforeScript do
           .to include /should be an array of strings/
       end
     end
+
+    describe '#invalid?' do
+      it 'is not valid' do
+        expect(entry).to be_invalid
+      end
+    end
   end
 end
diff --git a/spec/lib/gitlab/ci/config/node/global_spec.rb b/spec/lib/gitlab/ci/config/node/global_spec.rb
index 66d40be6e6e..74a64c6df98 100644
--- a/spec/lib/gitlab/ci/config/node/global_spec.rb
+++ b/spec/lib/gitlab/ci/config/node/global_spec.rb
@@ -35,6 +35,12 @@ describe Gitlab::Ci::Config::Node::Global do
       end
     end
 
+    describe '#has_config?' do
+      it 'has config' do
+        expect(global).to have_config
+      end
+    end
+
     describe '#leaf?' do
       it 'is not leaf' do
         expect(global).not_to be_leaf
@@ -59,6 +65,12 @@ describe Gitlab::Ci::Config::Node::Global do
       end
     end
 
+    describe '#invalid?' do
+      it 'is not valid' do
+        expect(global).to be_invalid
+      end
+    end
+
     describe '#errors' do
       it 'reports errors from child nodes' do
         expect(global.errors)
-- 
GitLab