From 76aea978c6b2d8c69881836408c4947d816d2db2 Mon Sep 17 00:00:00 2001
From: Grzegorz Bizon <grzesiek.bizon@gmail.com>
Date: Thu, 9 Jun 2016 14:48:40 +0200
Subject: [PATCH 1/6] Add class that encapsulates error in new Ci config

---
 lib/gitlab/ci/config.rb                       | 10 +++++--
 lib/gitlab/ci/config/node/configurable.rb     |  4 +--
 lib/gitlab/ci/config/node/entry.rb            |  9 +++++++
 lib/gitlab/ci/config/node/error.rb            | 26 +++++++++++++++++++
 lib/gitlab/ci/config/node/factory.rb          |  1 +
 lib/gitlab/ci/config/node/script.rb           |  2 +-
 spec/lib/ci/gitlab_ci_yaml_processor_spec.rb  |  2 +-
 spec/lib/gitlab/ci/config/node/error_spec.rb  | 23 ++++++++++++++++
 .../lib/gitlab/ci/config/node/factory_spec.rb | 10 +++++++
 spec/lib/gitlab/ci/config/node/global_spec.rb |  8 +++++-
 spec/lib/gitlab/ci/config/node/script_spec.rb |  2 +-
 spec/lib/gitlab/ci/config_spec.rb             |  6 +++++
 12 files changed, 95 insertions(+), 8 deletions(-)
 create mode 100644 lib/gitlab/ci/config/node/error.rb
 create mode 100644 spec/lib/gitlab/ci/config/node/error_spec.rb

diff --git a/lib/gitlab/ci/config.rb b/lib/gitlab/ci/config.rb
index b48d3592f1..26028547fa 100644
--- a/lib/gitlab/ci/config.rb
+++ b/lib/gitlab/ci/config.rb
@@ -4,8 +4,6 @@ module Gitlab
     # Base GitLab CI Configuration facade
     #
     class Config
-      delegate :valid?, :errors, to: :@global
-
       ##
       # Temporary delegations that should be removed after refactoring
       #
@@ -18,6 +16,14 @@ module Gitlab
         @global.process!
       end
 
+      def valid?
+        errors.none?
+      end
+
+      def errors
+        @global.errors.map(&:to_s)
+      end
+
       def to_hash
         @config
       end
diff --git a/lib/gitlab/ci/config/node/configurable.rb b/lib/gitlab/ci/config/node/configurable.rb
index d60f87f3f9..2f4abbf9ff 100644
--- a/lib/gitlab/ci/config/node/configurable.rb
+++ b/lib/gitlab/ci/config/node/configurable.rb
@@ -24,12 +24,12 @@ module Gitlab
 
           def prevalidate!
             unless @value.is_a?(Hash)
-              @errors << 'should be a configuration entry with hash value'
+              add_error('should be a configuration entry with hash value')
             end
           end
 
           def create_node(key, factory)
-            factory.with(value: @value[key])
+            factory.with(value: @value[key], key: key)
             factory.nullify! unless @value.has_key?(key)
             factory.create!
           end
diff --git a/lib/gitlab/ci/config/node/entry.rb b/lib/gitlab/ci/config/node/entry.rb
index 52758a962f..f41e191d61 100644
--- a/lib/gitlab/ci/config/node/entry.rb
+++ b/lib/gitlab/ci/config/node/entry.rb
@@ -8,6 +8,7 @@ module Gitlab
         class Entry
           class InvalidError < StandardError; end
 
+          attr_writer :key
           attr_accessor :description
 
           def initialize(value)
@@ -40,10 +41,18 @@ module Gitlab
             allowed_nodes.none?
           end
 
+          def key
+            @key || self.class.name.demodulize.underscore
+          end
+
           def errors
             @errors + nodes.map(&:errors).flatten
           end
 
+          def add_error(message)
+            @errors << Error.new(message, self)
+          end
+
           def allowed_nodes
             {}
           end
diff --git a/lib/gitlab/ci/config/node/error.rb b/lib/gitlab/ci/config/node/error.rb
new file mode 100644
index 0000000000..5b8d0288e4
--- /dev/null
+++ b/lib/gitlab/ci/config/node/error.rb
@@ -0,0 +1,26 @@
+module Gitlab
+  module Ci
+    class Config
+      module Node
+        class Error
+          def initialize(message, parent)
+            @message = message
+            @parent = parent
+          end
+
+          def key
+            @parent.key
+          end
+
+          def to_s
+            "#{key}: #{@message}"
+          end
+
+          def ==(other)
+            other.to_s == to_s
+          end
+        end
+      end
+    end
+  end
+end
diff --git a/lib/gitlab/ci/config/node/factory.rb b/lib/gitlab/ci/config/node/factory.rb
index 787ca006f5..025ae40ef9 100644
--- a/lib/gitlab/ci/config/node/factory.rb
+++ b/lib/gitlab/ci/config/node/factory.rb
@@ -30,6 +30,7 @@ module Gitlab
 
             @entry_class.new(@attributes[:value]).tap do |entry|
               entry.description = @attributes[:description]
+              entry.key = @attributes[:key]
             end
           end
         end
diff --git a/lib/gitlab/ci/config/node/script.rb b/lib/gitlab/ci/config/node/script.rb
index 5072bf0db7..314877a035 100644
--- a/lib/gitlab/ci/config/node/script.rb
+++ b/lib/gitlab/ci/config/node/script.rb
@@ -19,7 +19,7 @@ module Gitlab
 
           def validate!
             unless validate_array_of_strings(@value)
-              @errors << 'before_script should be an array of strings'
+              add_error('should be an array of strings')
             end
           end
         end
diff --git a/spec/lib/ci/gitlab_ci_yaml_processor_spec.rb b/spec/lib/ci/gitlab_ci_yaml_processor_spec.rb
index 5e1d2b8e4f..40ee1eb64c 100644
--- a/spec/lib/ci/gitlab_ci_yaml_processor_spec.rb
+++ b/spec/lib/ci/gitlab_ci_yaml_processor_spec.rb
@@ -820,7 +820,7 @@ EOT
         config = YAML.dump({ before_script: "bundle update", rspec: { script: "test" } })
         expect do
           GitlabCiYamlProcessor.new(config, path)
-        end.to raise_error(GitlabCiYamlProcessor::ValidationError, "before_script should be an array of strings")
+        end.to raise_error(GitlabCiYamlProcessor::ValidationError, "before_script: should be an array of strings")
       end
 
       it "returns errors if job before_script parameter is not an array of strings" do
diff --git a/spec/lib/gitlab/ci/config/node/error_spec.rb b/spec/lib/gitlab/ci/config/node/error_spec.rb
new file mode 100644
index 0000000000..764fdfdb82
--- /dev/null
+++ b/spec/lib/gitlab/ci/config/node/error_spec.rb
@@ -0,0 +1,23 @@
+require 'spec_helper'
+
+describe Gitlab::Ci::Config::Node::Error do
+  let(:error) { described_class.new(message, parent) }
+  let(:message) { 'some error' }
+  let(:parent) { spy('parent') }
+
+  before do
+    allow(parent).to receive(:key).and_return('parent_key')
+  end
+
+  describe '#key' do
+    it 'returns underscored class name' do
+      expect(error.key).to eq 'parent_key'
+    end
+  end
+
+  describe '#to_s' do
+    it 'returns valid error message' do
+      expect(error.to_s).to eq 'parent_key: some error'
+    end
+  end
+end
diff --git a/spec/lib/gitlab/ci/config/node/factory_spec.rb b/spec/lib/gitlab/ci/config/node/factory_spec.rb
index d681aa3245..01a707a6bd 100644
--- a/spec/lib/gitlab/ci/config/node/factory_spec.rb
+++ b/spec/lib/gitlab/ci/config/node/factory_spec.rb
@@ -25,6 +25,16 @@ describe Gitlab::Ci::Config::Node::Factory do
           expect(entry.description).to eq 'test description'
         end
       end
+
+      context 'when setting key' do
+        it 'creates entry with custom key' do
+          entry = factory
+            .with(value: ['ls', 'pwd'], key: 'test key')
+            .create!
+
+          expect(entry.key).to eq 'test key'
+        end
+      end
     end
 
     context 'when not setting value' do
diff --git a/spec/lib/gitlab/ci/config/node/global_spec.rb b/spec/lib/gitlab/ci/config/node/global_spec.rb
index b197217243..89a7183b65 100644
--- a/spec/lib/gitlab/ci/config/node/global_spec.rb
+++ b/spec/lib/gitlab/ci/config/node/global_spec.rb
@@ -13,6 +13,12 @@ describe Gitlab::Ci::Config::Node::Global do
     end
   end
 
+  describe '#key' do
+    it 'returns underscored class name' do
+      expect(global.key).to eq 'global'
+    end
+  end
+
   context 'when hash is valid' do
     let(:hash) do
       { before_script: ['ls', 'pwd'] }
@@ -79,7 +85,7 @@ describe Gitlab::Ci::Config::Node::Global do
     describe '#errors' do
       it 'reports errors from child nodes' do
         expect(global.errors)
-          .to include 'before_script should be an array of strings'
+          .to include 'before_script: should be an array of strings'
       end
     end
 
diff --git a/spec/lib/gitlab/ci/config/node/script_spec.rb b/spec/lib/gitlab/ci/config/node/script_spec.rb
index e4d6481f8a..ff7016046c 100644
--- a/spec/lib/gitlab/ci/config/node/script_spec.rb
+++ b/spec/lib/gitlab/ci/config/node/script_spec.rb
@@ -34,7 +34,7 @@ describe Gitlab::Ci::Config::Node::Script do
       describe '#errors' do
         it 'saves errors' do
           expect(entry.errors)
-            .to include /should be an array of strings/
+            .to include 'script: should be an array of strings'
         end
       end
 
diff --git a/spec/lib/gitlab/ci/config_spec.rb b/spec/lib/gitlab/ci/config_spec.rb
index 3871d939fe..2a5d132db7 100644
--- a/spec/lib/gitlab/ci/config_spec.rb
+++ b/spec/lib/gitlab/ci/config_spec.rb
@@ -67,6 +67,12 @@ describe Gitlab::Ci::Config do
             expect(config.errors).not_to be_empty
           end
         end
+
+        describe '#errors' do
+          it 'returns an array of strings' do
+            expect(config.errors).to all(be_an_instance_of(String))
+          end
+        end
       end
     end
   end

From 95520dfc72770955ae99c73f90e02037f3b1d4b5 Mon Sep 17 00:00:00 2001
From: Grzegorz Bizon <grzesiek.bizon@gmail.com>
Date: Thu, 16 Jun 2016 14:16:33 +0200
Subject: [PATCH 2/6] Add prototype of CI config node validator

This makes use of `ActiveModel::Validations` encapsulated in a separate
class that is accessible from a node object.
---
 lib/gitlab/ci/config.rb                       |  4 +-
 lib/gitlab/ci/config/node/configurable.rb     | 23 +++++++----
 lib/gitlab/ci/config/node/entry.rb            | 41 ++++++++-----------
 lib/gitlab/ci/config/node/error.rb            | 26 ------------
 lib/gitlab/ci/config/node/script.rb           | 18 ++++----
 lib/gitlab/ci/config/node/validatable.rb      | 30 ++++++++++++++
 lib/gitlab/ci/config/node/validator.rb        | 25 +++++++++++
 spec/lib/ci/gitlab_ci_yaml_processor_spec.rb  |  2 +-
 .../ci/config/node/configurable_spec.rb       |  1 +
 spec/lib/gitlab/ci/config/node/error_spec.rb  | 23 -----------
 spec/lib/gitlab/ci/config/node/global_spec.rb |  2 +-
 spec/lib/gitlab/ci/config/node/script_spec.rb | 14 +++----
 12 files changed, 108 insertions(+), 101 deletions(-)
 delete mode 100644 lib/gitlab/ci/config/node/error.rb
 create mode 100644 lib/gitlab/ci/config/node/validatable.rb
 create mode 100644 lib/gitlab/ci/config/node/validator.rb
 delete mode 100644 spec/lib/gitlab/ci/config/node/error_spec.rb

diff --git a/lib/gitlab/ci/config.rb b/lib/gitlab/ci/config.rb
index 26028547fa..adfd097736 100644
--- a/lib/gitlab/ci/config.rb
+++ b/lib/gitlab/ci/config.rb
@@ -17,11 +17,11 @@ module Gitlab
       end
 
       def valid?
-        errors.none?
+        @global.valid?
       end
 
       def errors
-        @global.errors.map(&:to_s)
+        @global.errors
       end
 
       def to_hash
diff --git a/lib/gitlab/ci/config/node/configurable.rb b/lib/gitlab/ci/config/node/configurable.rb
index 2f4abbf9ff..23d3f9f327 100644
--- a/lib/gitlab/ci/config/node/configurable.rb
+++ b/lib/gitlab/ci/config/node/configurable.rb
@@ -16,21 +16,27 @@ module Gitlab
         module Configurable
           extend ActiveSupport::Concern
 
+          included do
+            validations do
+              validate :hash_config_value
+
+              def hash_config_value
+                unless self.config.is_a?(Hash)
+                  errors.add(:config, 'should be a configuration entry hash')
+                end
+              end
+            end
+          end
+
           def allowed_nodes
             self.class.allowed_nodes || {}
           end
 
           private
 
-          def prevalidate!
-            unless @value.is_a?(Hash)
-              add_error('should be a configuration entry with hash value')
-            end
-          end
-
           def create_node(key, factory)
-            factory.with(value: @value[key], key: key)
-            factory.nullify! unless @value.has_key?(key)
+            factory.with(value: @config[key], key: key)
+            factory.nullify! unless @config.has_key?(key)
             factory.create!
           end
 
@@ -47,7 +53,6 @@ module Gitlab
 
               define_method(symbol) do
                 raise Entry::InvalidError unless valid?
-
                 @nodes[symbol].try(:value)
               end
 
diff --git a/lib/gitlab/ci/config/node/entry.rb b/lib/gitlab/ci/config/node/entry.rb
index f41e191d61..823c8bb5d1 100644
--- a/lib/gitlab/ci/config/node/entry.rb
+++ b/lib/gitlab/ci/config/node/entry.rb
@@ -7,16 +7,15 @@ module Gitlab
         #
         class Entry
           class InvalidError < StandardError; end
+          include Validatable
 
-          attr_writer :key
-          attr_accessor :description
+          attr_reader :config
+          attr_accessor :key, :description
 
-          def initialize(value)
-            @value = value
+          def initialize(config)
+            @config = config
             @nodes = {}
-            @errors = []
-
-            prevalidate!
+            @validator = self.class.validator.new(self)
           end
 
           def process!
@@ -24,19 +23,13 @@ module Gitlab
             return unless valid?
 
             compose!
-
-            nodes.each(&:process!)
-            nodes.each(&:validate!)
+            process_nodes!
           end
 
           def nodes
             @nodes.values
           end
 
-          def valid?
-            errors.none?
-          end
-
           def leaf?
             allowed_nodes.none?
           end
@@ -45,37 +38,35 @@ module Gitlab
             @key || self.class.name.demodulize.underscore
           end
 
-          def errors
-            @errors + nodes.map(&:errors).flatten
+          def valid?
+            errors.none?
           end
 
-          def add_error(message)
-            @errors << Error.new(message, self)
+          def errors
+            @validator.full_errors +
+              nodes.map(&:errors).flatten
           end
 
           def allowed_nodes
             {}
           end
 
-          def validate!
-            raise NotImplementedError
-          end
-
           def value
             raise NotImplementedError
           end
 
           private
 
-          def prevalidate!
-          end
-
           def compose!
             allowed_nodes.each do |key, essence|
               @nodes[key] = create_node(key, essence)
             end
           end
 
+          def process_nodes!
+            nodes.each(&:process!)
+          end
+
           def create_node(key, essence)
             raise NotImplementedError
           end
diff --git a/lib/gitlab/ci/config/node/error.rb b/lib/gitlab/ci/config/node/error.rb
deleted file mode 100644
index 5b8d0288e4..0000000000
--- a/lib/gitlab/ci/config/node/error.rb
+++ /dev/null
@@ -1,26 +0,0 @@
-module Gitlab
-  module Ci
-    class Config
-      module Node
-        class Error
-          def initialize(message, parent)
-            @message = message
-            @parent = parent
-          end
-
-          def key
-            @parent.key
-          end
-
-          def to_s
-            "#{key}: #{@message}"
-          end
-
-          def ==(other)
-            other.to_s == to_s
-          end
-        end
-      end
-    end
-  end
-end
diff --git a/lib/gitlab/ci/config/node/script.rb b/lib/gitlab/ci/config/node/script.rb
index 314877a035..18592acdac 100644
--- a/lib/gitlab/ci/config/node/script.rb
+++ b/lib/gitlab/ci/config/node/script.rb
@@ -11,16 +11,20 @@ module Gitlab
         # implementation in Runner.
         #
         class Script < Entry
-          include ValidationHelpers
+          validations do
+            include ValidationHelpers
 
-          def value
-            @value.join("\n")
+            validate :array_of_strings
+
+            def array_of_strings
+              unless validate_array_of_strings(self.config)
+                errors.add(:config, 'should be an array of strings')
+              end
+            end
           end
 
-          def validate!
-            unless validate_array_of_strings(@value)
-              add_error('should be an array of strings')
-            end
+          def value
+            @config.join("\n")
           end
         end
       end
diff --git a/lib/gitlab/ci/config/node/validatable.rb b/lib/gitlab/ci/config/node/validatable.rb
new file mode 100644
index 0000000000..a0617d21ad
--- /dev/null
+++ b/lib/gitlab/ci/config/node/validatable.rb
@@ -0,0 +1,30 @@
+module Gitlab
+  module Ci
+    class Config
+      module Node
+        module Validatable
+          extend ActiveSupport::Concern
+
+          class_methods do
+            def validator
+              validator = Class.new(Node::Validator)
+              validator.include(ActiveModel::Validations)
+
+              if defined?(@validations)
+                @validations.each { |rules| validator.class_eval(&rules) }
+              end
+
+              validator
+            end
+
+            private
+
+            def validations(&block)
+              (@validations ||= []).append(block)
+            end
+          end
+        end
+      end
+    end
+  end
+end
diff --git a/lib/gitlab/ci/config/node/validator.rb b/lib/gitlab/ci/config/node/validator.rb
new file mode 100644
index 0000000000..891b19c974
--- /dev/null
+++ b/lib/gitlab/ci/config/node/validator.rb
@@ -0,0 +1,25 @@
+module Gitlab
+  module Ci
+    class Config
+      module Node
+        class Validator < SimpleDelegator
+          def initialize(node)
+            @node = node
+            super(node)
+            validate
+          end
+
+          def full_errors
+            errors.full_messages.map do |error|
+              "#{@node.key} #{error}".humanize
+            end
+          end
+
+          def self.name
+            'Validator'
+          end
+        end
+      end
+    end
+  end
+end
diff --git a/spec/lib/ci/gitlab_ci_yaml_processor_spec.rb b/spec/lib/ci/gitlab_ci_yaml_processor_spec.rb
index 40ee1eb64c..42cbb55569 100644
--- a/spec/lib/ci/gitlab_ci_yaml_processor_spec.rb
+++ b/spec/lib/ci/gitlab_ci_yaml_processor_spec.rb
@@ -820,7 +820,7 @@ EOT
         config = YAML.dump({ before_script: "bundle update", rspec: { script: "test" } })
         expect do
           GitlabCiYamlProcessor.new(config, path)
-        end.to raise_error(GitlabCiYamlProcessor::ValidationError, "before_script: should be an array of strings")
+        end.to raise_error(GitlabCiYamlProcessor::ValidationError, "Before script config should be an array of strings")
       end
 
       it "returns errors if job before_script parameter is not an array of strings" do
diff --git a/spec/lib/gitlab/ci/config/node/configurable_spec.rb b/spec/lib/gitlab/ci/config/node/configurable_spec.rb
index 47c68f96dc..8ec7919f4d 100644
--- a/spec/lib/gitlab/ci/config/node/configurable_spec.rb
+++ b/spec/lib/gitlab/ci/config/node/configurable_spec.rb
@@ -4,6 +4,7 @@ describe Gitlab::Ci::Config::Node::Configurable do
   let(:node) { Class.new }
 
   before do
+    node.include(Gitlab::Ci::Config::Node::Validatable)
     node.include(described_class)
   end
 
diff --git a/spec/lib/gitlab/ci/config/node/error_spec.rb b/spec/lib/gitlab/ci/config/node/error_spec.rb
deleted file mode 100644
index 764fdfdb82..0000000000
--- a/spec/lib/gitlab/ci/config/node/error_spec.rb
+++ /dev/null
@@ -1,23 +0,0 @@
-require 'spec_helper'
-
-describe Gitlab::Ci::Config::Node::Error do
-  let(:error) { described_class.new(message, parent) }
-  let(:message) { 'some error' }
-  let(:parent) { spy('parent') }
-
-  before do
-    allow(parent).to receive(:key).and_return('parent_key')
-  end
-
-  describe '#key' do
-    it 'returns underscored class name' do
-      expect(error.key).to eq 'parent_key'
-    end
-  end
-
-  describe '#to_s' do
-    it 'returns valid error message' do
-      expect(error.to_s).to eq 'parent_key: some error'
-    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 89a7183b65..8bb76cf920 100644
--- a/spec/lib/gitlab/ci/config/node/global_spec.rb
+++ b/spec/lib/gitlab/ci/config/node/global_spec.rb
@@ -85,7 +85,7 @@ describe Gitlab::Ci::Config::Node::Global do
     describe '#errors' do
       it 'reports errors from child nodes' do
         expect(global.errors)
-          .to include 'before_script: should be an array of strings'
+          .to include 'Before script config should be an array of strings'
       end
     end
 
diff --git a/spec/lib/gitlab/ci/config/node/script_spec.rb b/spec/lib/gitlab/ci/config/node/script_spec.rb
index ff7016046c..6af6aa15ee 100644
--- a/spec/lib/gitlab/ci/config/node/script_spec.rb
+++ b/spec/lib/gitlab/ci/config/node/script_spec.rb
@@ -1,13 +1,13 @@
 require 'spec_helper'
 
 describe Gitlab::Ci::Config::Node::Script do
-  let(:entry) { described_class.new(value) }
+  let(:entry) { described_class.new(config) }
 
-  describe '#validate!' do
-    before { entry.validate! }
+  describe '#process!' do
+    before { entry.process! }
 
-    context 'when entry value is correct' do
-      let(:value) { ['ls', 'pwd'] }
+    context 'when entry config value is correct' do
+      let(:config) { ['ls', 'pwd'] }
 
       describe '#value' do
         it 'returns concatenated command' do
@@ -29,12 +29,12 @@ describe Gitlab::Ci::Config::Node::Script do
     end
 
     context 'when entry value is not correct' do
-      let(:value) { 'ls' }
+      let(:config) { 'ls' }
 
       describe '#errors' do
         it 'saves errors' do
           expect(entry.errors)
-            .to include 'script: should be an array of strings'
+            .to include 'Script config should be an array of strings'
         end
       end
 

From 002e6ed1f042852bef78c3a749e13567d7a0ee5a Mon Sep 17 00:00:00 2001
From: Grzegorz Bizon <grzesiek.bizon@gmail.com>
Date: Fri, 17 Jun 2016 09:33:32 +0200
Subject: [PATCH 3/6] Improve CI config entries validations prototype

---
 lib/gitlab/ci/config/node/configurable.rb     |  7 +-
 lib/gitlab/ci/config/node/entry.rb            | 16 +++--
 lib/gitlab/ci/config/node/script.rb           |  2 +
 lib/gitlab/ci/config/node/validatable.rb      |  1 -
 lib/gitlab/ci/config/node/validator.rb        |  5 +-
 .../ci/config/node/configurable_spec.rb       | 15 ++---
 spec/lib/gitlab/ci/config/node/global_spec.rb |  6 +-
 .../gitlab/ci/config/node/validatable_spec.rb | 50 ++++++++++++++
 .../gitlab/ci/config/node/validator_spec.rb   | 67 +++++++++++++++++++
 9 files changed, 144 insertions(+), 25 deletions(-)
 create mode 100644 spec/lib/gitlab/ci/config/node/validatable_spec.rb
 create mode 100644 spec/lib/gitlab/ci/config/node/validator_spec.rb

diff --git a/lib/gitlab/ci/config/node/configurable.rb b/lib/gitlab/ci/config/node/configurable.rb
index 23d3f9f327..7b428c1362 100644
--- a/lib/gitlab/ci/config/node/configurable.rb
+++ b/lib/gitlab/ci/config/node/configurable.rb
@@ -15,6 +15,7 @@ module Gitlab
         #
         module Configurable
           extend ActiveSupport::Concern
+          include Validatable
 
           included do
             validations do
@@ -28,10 +29,6 @@ module Gitlab
             end
           end
 
-          def allowed_nodes
-            self.class.allowed_nodes || {}
-          end
-
           private
 
           def create_node(key, factory)
@@ -41,7 +38,7 @@ module Gitlab
           end
 
           class_methods do
-            def allowed_nodes
+            def nodes
               Hash[@allowed_nodes.map { |key, factory| [key, factory.dup] }]
             end
 
diff --git a/lib/gitlab/ci/config/node/entry.rb b/lib/gitlab/ci/config/node/entry.rb
index 823c8bb5d1..f044ef965e 100644
--- a/lib/gitlab/ci/config/node/entry.rb
+++ b/lib/gitlab/ci/config/node/entry.rb
@@ -7,7 +7,6 @@ module Gitlab
         #
         class Entry
           class InvalidError < StandardError; end
-          include Validatable
 
           attr_reader :config
           attr_accessor :key, :description
@@ -16,6 +15,7 @@ module Gitlab
             @config = config
             @nodes = {}
             @validator = self.class.validator.new(self)
+            @validator.validate
           end
 
           def process!
@@ -31,7 +31,7 @@ module Gitlab
           end
 
           def leaf?
-            allowed_nodes.none?
+            self.class.nodes.none?
           end
 
           def key
@@ -47,18 +47,22 @@ module Gitlab
               nodes.map(&:errors).flatten
           end
 
-          def allowed_nodes
+          def value
+            raise NotImplementedError
+          end
+
+          def self.nodes
             {}
           end
 
-          def value
-            raise NotImplementedError
+          def self.validator
+            Validator
           end
 
           private
 
           def compose!
-            allowed_nodes.each do |key, essence|
+            self.class.nodes.each do |key, essence|
               @nodes[key] = create_node(key, essence)
             end
           end
diff --git a/lib/gitlab/ci/config/node/script.rb b/lib/gitlab/ci/config/node/script.rb
index 18592acdac..059650d8a5 100644
--- a/lib/gitlab/ci/config/node/script.rb
+++ b/lib/gitlab/ci/config/node/script.rb
@@ -11,6 +11,8 @@ module Gitlab
         # implementation in Runner.
         #
         class Script < Entry
+          include Validatable
+
           validations do
             include ValidationHelpers
 
diff --git a/lib/gitlab/ci/config/node/validatable.rb b/lib/gitlab/ci/config/node/validatable.rb
index a0617d21ad..f6e2896dfb 100644
--- a/lib/gitlab/ci/config/node/validatable.rb
+++ b/lib/gitlab/ci/config/node/validatable.rb
@@ -8,7 +8,6 @@ module Gitlab
           class_methods do
             def validator
               validator = Class.new(Node::Validator)
-              validator.include(ActiveModel::Validations)
 
               if defined?(@validations)
                 @validations.each { |rules| validator.class_eval(&rules) }
diff --git a/lib/gitlab/ci/config/node/validator.rb b/lib/gitlab/ci/config/node/validator.rb
index 891b19c974..2454cc6d95 100644
--- a/lib/gitlab/ci/config/node/validator.rb
+++ b/lib/gitlab/ci/config/node/validator.rb
@@ -3,10 +3,11 @@ module Gitlab
     class Config
       module Node
         class Validator < SimpleDelegator
+          include ActiveModel::Validations
+
           def initialize(node)
-            @node = node
             super(node)
-            validate
+            @node = node
           end
 
           def full_errors
diff --git a/spec/lib/gitlab/ci/config/node/configurable_spec.rb b/spec/lib/gitlab/ci/config/node/configurable_spec.rb
index 8ec7919f4d..9bbda6e739 100644
--- a/spec/lib/gitlab/ci/config/node/configurable_spec.rb
+++ b/spec/lib/gitlab/ci/config/node/configurable_spec.rb
@@ -4,30 +4,29 @@ describe Gitlab::Ci::Config::Node::Configurable do
   let(:node) { Class.new }
 
   before do
-    node.include(Gitlab::Ci::Config::Node::Validatable)
     node.include(described_class)
   end
 
-  describe 'allowed nodes' do
+  describe 'configured nodes' do
     before do
       node.class_eval do
         allow_node :object, Object, description: 'test object'
       end
     end
 
-    describe '#allowed_nodes' do
-      it 'has valid allowed nodes' do
-        expect(node.allowed_nodes).to include :object
+    describe '.nodes' do
+      it 'has valid nodes' do
+        expect(node.nodes).to include :object
       end
 
       it 'creates a node factory' do
-        expect(node.allowed_nodes[:object])
+        expect(node.nodes[:object])
           .to be_an_instance_of Gitlab::Ci::Config::Node::Factory
       end
 
       it 'returns a duplicated factory object' do
-        first_factory = node.allowed_nodes[:object]
-        second_factory = node.allowed_nodes[:object]
+        first_factory = node.nodes[:object]
+        second_factory = node.nodes[:object]
 
         expect(first_factory).not_to be_equal(second_factory)
       end
diff --git a/spec/lib/gitlab/ci/config/node/global_spec.rb b/spec/lib/gitlab/ci/config/node/global_spec.rb
index 8bb76cf920..fddd53a2b5 100644
--- a/spec/lib/gitlab/ci/config/node/global_spec.rb
+++ b/spec/lib/gitlab/ci/config/node/global_spec.rb
@@ -3,13 +3,13 @@ require 'spec_helper'
 describe Gitlab::Ci::Config::Node::Global do
   let(:global) { described_class.new(hash) }
 
-  describe '#allowed_nodes' do
+  describe '.nodes' do
     it 'can contain global config keys' do
-      expect(global.allowed_nodes).to include :before_script
+      expect(described_class.nodes).to include :before_script
     end
 
     it 'returns a hash' do
-      expect(global.allowed_nodes).to be_a Hash
+      expect(described_class.nodes).to be_a Hash
     end
   end
 
diff --git a/spec/lib/gitlab/ci/config/node/validatable_spec.rb b/spec/lib/gitlab/ci/config/node/validatable_spec.rb
new file mode 100644
index 0000000000..10cd01afcd
--- /dev/null
+++ b/spec/lib/gitlab/ci/config/node/validatable_spec.rb
@@ -0,0 +1,50 @@
+require 'spec_helper'
+
+describe Gitlab::Ci::Config::Node::Validatable do
+  let(:node) { Class.new }
+
+  before do
+    node.include(described_class)
+  end
+
+  describe '.validator' do
+    before do
+      node.class_eval do
+        attr_accessor :test_attribute
+
+        validations do
+          validates :test_attribute, presence: true
+        end
+      end
+    end
+
+    it 'returns validator' do
+      expect(node.validator.superclass)
+        .to be Gitlab::Ci::Config::Node::Validator
+    end
+
+    context 'when validating node instance' do
+      let(:node_instance) { node.new }
+
+      context 'when attribute is valid' do
+        before do
+          node_instance.test_attribute = 'valid'
+        end
+
+        it 'instance of validator is valid' do
+          expect(node.validator.new(node_instance)).to be_valid
+        end
+      end
+
+      context 'when attribute is not valid' do
+        before do
+          node_instance.test_attribute = nil
+        end
+
+        it 'instance of validator is invalid' do
+          expect(node.validator.new(node_instance)).to be_invalid
+        end
+      end
+    end
+  end
+end
diff --git a/spec/lib/gitlab/ci/config/node/validator_spec.rb b/spec/lib/gitlab/ci/config/node/validator_spec.rb
new file mode 100644
index 0000000000..ad875d5538
--- /dev/null
+++ b/spec/lib/gitlab/ci/config/node/validator_spec.rb
@@ -0,0 +1,67 @@
+require 'spec_helper'
+
+describe Gitlab::Ci::Config::Node::Validator do
+  let(:validator) { Class.new(described_class) }
+  let(:validator_instance) { validator.new(node) }
+  let(:node) { spy('node') }
+
+  shared_examples 'delegated validator' do
+    context 'when node is valid' do
+      before do
+        allow(node).to receive(:test_attribute).and_return('valid value')
+      end
+
+      it 'validates attribute in node' do
+        expect(node).to receive(:test_attribute)
+        expect(validator_instance).to be_valid
+      end
+
+      it 'returns no errors' do
+        validator_instance.validate
+
+        expect(validator_instance.full_errors).to be_empty
+      end
+    end
+
+    context 'when node is invalid' do
+      before do
+        allow(node).to receive(:test_attribute).and_return(nil)
+      end
+
+      it 'validates attribute in node' do
+        expect(node).to receive(:test_attribute)
+        expect(validator_instance).to be_invalid
+      end
+
+      it 'returns errors' do
+        validator_instance.validate
+
+        expect(validator_instance.full_errors).not_to be_empty
+      end
+    end
+  end
+
+  describe 'attributes validations' do
+    before do
+      validator.class_eval do
+        validates :test_attribute, presence: true
+      end
+    end
+
+    it_behaves_like 'delegated validator'
+  end
+
+  describe 'interface validations' do
+    before do
+      validator.class_eval do
+        validate do
+          unless @node.test_attribute == 'valid value'
+            errors.add(:test_attribute, 'invalid value')
+          end
+        end
+      end
+    end
+
+    it_behaves_like 'delegated validator'
+  end
+end

From a9bd16bd0a44332133d0f9fd859d9ffaba9e262f Mon Sep 17 00:00:00 2001
From: Grzegorz Bizon <grzesiek.bizon@gmail.com>
Date: Fri, 17 Jun 2016 11:55:55 +0200
Subject: [PATCH 4/6] Rename legacy validation helpers for new ci config

---
 lib/ci/gitlab_ci_yaml_processor.rb                              | 2 +-
 .../{validation_helpers.rb => legacy_validation_helpers.rb}     | 2 +-
 lib/gitlab/ci/config/node/script.rb                             | 2 +-
 3 files changed, 3 insertions(+), 3 deletions(-)
 rename lib/gitlab/ci/config/node/{validation_helpers.rb => legacy_validation_helpers.rb} (95%)

diff --git a/lib/ci/gitlab_ci_yaml_processor.rb b/lib/ci/gitlab_ci_yaml_processor.rb
index e0b89cead0..1a5be9dbc4 100644
--- a/lib/ci/gitlab_ci_yaml_processor.rb
+++ b/lib/ci/gitlab_ci_yaml_processor.rb
@@ -2,7 +2,7 @@ module Ci
   class GitlabCiYamlProcessor
     class ValidationError < StandardError; end
 
-    include Gitlab::Ci::Config::Node::ValidationHelpers
+    include Gitlab::Ci::Config::Node::LegacyValidationHelpers
 
     DEFAULT_STAGES = %w(build test deploy)
     DEFAULT_STAGE = 'test'
diff --git a/lib/gitlab/ci/config/node/validation_helpers.rb b/lib/gitlab/ci/config/node/legacy_validation_helpers.rb
similarity index 95%
rename from lib/gitlab/ci/config/node/validation_helpers.rb
rename to lib/gitlab/ci/config/node/legacy_validation_helpers.rb
index 42ef60244b..970763670c 100644
--- a/lib/gitlab/ci/config/node/validation_helpers.rb
+++ b/lib/gitlab/ci/config/node/legacy_validation_helpers.rb
@@ -2,7 +2,7 @@ module Gitlab
   module Ci
     class Config
       module Node
-        module ValidationHelpers
+        module LegacyValidationHelpers
           private
 
           def validate_duration(value)
diff --git a/lib/gitlab/ci/config/node/script.rb b/lib/gitlab/ci/config/node/script.rb
index 059650d8a5..44490096f2 100644
--- a/lib/gitlab/ci/config/node/script.rb
+++ b/lib/gitlab/ci/config/node/script.rb
@@ -14,7 +14,7 @@ module Gitlab
           include Validatable
 
           validations do
-            include ValidationHelpers
+            include LegacyValidationHelpers
 
             validate :array_of_strings
 

From d9ca84015c04d8836c09c3ebb70a8240262b60e8 Mon Sep 17 00:00:00 2001
From: Grzegorz Bizon <grzesiek.bizon@gmail.com>
Date: Fri, 17 Jun 2016 12:06:48 +0200
Subject: [PATCH 5/6] Add first custom validator for new ci config

This follows a standard `ActiveModel` pattern of creating a custom
validators. We use `ActiveModel::EachValidator` here that reuses methods
provided by `LegacyValidationHelpers`.

We will remove `LegacyValidationHelpers` on some point in the future, at
the later stages of CI configuration refactoring. It may be possible
to rewrite custom validators to use format like:

`validates :config, array_of: String`
---
 lib/gitlab/ci/config/node/script.rb           | 10 +---------
 lib/gitlab/ci/config/node/validator.rb        |  1 +
 lib/gitlab/ci/config/node/validators.rb       | 19 +++++++++++++++++++
 .../array_of_strings_validator_spec.rb        |  0
 4 files changed, 21 insertions(+), 9 deletions(-)
 create mode 100644 lib/gitlab/ci/config/node/validators.rb
 create mode 100644 spec/lib/gitlab/ci/config/node/validators/array_of_strings_validator_spec.rb

diff --git a/lib/gitlab/ci/config/node/script.rb b/lib/gitlab/ci/config/node/script.rb
index 44490096f2..c044f5c5e7 100644
--- a/lib/gitlab/ci/config/node/script.rb
+++ b/lib/gitlab/ci/config/node/script.rb
@@ -14,15 +14,7 @@ module Gitlab
           include Validatable
 
           validations do
-            include LegacyValidationHelpers
-
-            validate :array_of_strings
-
-            def array_of_strings
-              unless validate_array_of_strings(self.config)
-                errors.add(:config, 'should be an array of strings')
-              end
-            end
+            validates :config, array_of_strings: true
           end
 
           def value
diff --git a/lib/gitlab/ci/config/node/validator.rb b/lib/gitlab/ci/config/node/validator.rb
index 2454cc6d95..02edc9219c 100644
--- a/lib/gitlab/ci/config/node/validator.rb
+++ b/lib/gitlab/ci/config/node/validator.rb
@@ -4,6 +4,7 @@ module Gitlab
       module Node
         class Validator < SimpleDelegator
           include ActiveModel::Validations
+          include Node::Validators
 
           def initialize(node)
             super(node)
diff --git a/lib/gitlab/ci/config/node/validators.rb b/lib/gitlab/ci/config/node/validators.rb
new file mode 100644
index 0000000000..36d48394a8
--- /dev/null
+++ b/lib/gitlab/ci/config/node/validators.rb
@@ -0,0 +1,19 @@
+module Gitlab
+  module Ci
+    class Config
+      module Node
+        module Validators
+          class ArrayOfStringsValidator < ActiveModel::EachValidator
+            include LegacyValidationHelpers
+
+            def validate_each(record, attribute, value)
+              unless validate_array_of_strings(value)
+                record.errors.add(attribute, 'should be an array of strings')
+              end
+            end
+          end
+        end
+      end
+    end
+  end
+end
diff --git a/spec/lib/gitlab/ci/config/node/validators/array_of_strings_validator_spec.rb b/spec/lib/gitlab/ci/config/node/validators/array_of_strings_validator_spec.rb
new file mode 100644
index 0000000000..e69de29bb2

From 44b00a1ebbfeaa095343f55f6c12dcbc65b85924 Mon Sep 17 00:00:00 2001
From: Grzegorz Bizon <grzesiek.bizon@gmail.com>
Date: Fri, 17 Jun 2016 14:51:39 +0200
Subject: [PATCH 6/6] Extract CI entry config hash validation to validator

---
 lib/gitlab/ci/config/node/configurable.rb                 | 8 +-------
 lib/gitlab/ci/config/node/validators.rb                   | 8 ++++++++
 .../node/validators/array_of_strings_validator_spec.rb    | 0
 3 files changed, 9 insertions(+), 7 deletions(-)
 delete mode 100644 spec/lib/gitlab/ci/config/node/validators/array_of_strings_validator_spec.rb

diff --git a/lib/gitlab/ci/config/node/configurable.rb b/lib/gitlab/ci/config/node/configurable.rb
index 7b428c1362..374ff71d0f 100644
--- a/lib/gitlab/ci/config/node/configurable.rb
+++ b/lib/gitlab/ci/config/node/configurable.rb
@@ -19,13 +19,7 @@ module Gitlab
 
           included do
             validations do
-              validate :hash_config_value
-
-              def hash_config_value
-                unless self.config.is_a?(Hash)
-                  errors.add(:config, 'should be a configuration entry hash')
-                end
-              end
+              validates :config, hash: true
             end
           end
 
diff --git a/lib/gitlab/ci/config/node/validators.rb b/lib/gitlab/ci/config/node/validators.rb
index 36d48394a8..dc9cdb9a22 100644
--- a/lib/gitlab/ci/config/node/validators.rb
+++ b/lib/gitlab/ci/config/node/validators.rb
@@ -12,6 +12,14 @@ module Gitlab
               end
             end
           end
+
+          class HashValidator < ActiveModel::EachValidator
+            def validate_each(record, attribute, value)
+              unless value.is_a?(Hash)
+                record.errors.add(attribute, 'should be a configuration entry hash')
+              end
+            end
+          end
         end
       end
     end
diff --git a/spec/lib/gitlab/ci/config/node/validators/array_of_strings_validator_spec.rb b/spec/lib/gitlab/ci/config/node/validators/array_of_strings_validator_spec.rb
deleted file mode 100644
index e69de29bb2..0000000000