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.

12 files changed, 108 insertions, 101 deletions

diff --git a/lib/gitlab/ci/config.rb b/lib/gitlab/ci/config.rb
index 26028547fa0..adfd097736e 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 2f4abbf9ffe..23d3f9f3277 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 f41e191d611..823c8bb5d13 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 5b8d0288e4e..00000000000
--- 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 314877a035c..18592acdac6 100644
--- a/lib/gitlab/ci/config/node/script.rb
+++ b/lib/gitlab/ci/config/node/script.rb
@@ -11,17 +11,21 @@ module Gitlab
         # implementation in Runner.
         #
         class Script < Entry
-          include ValidationHelpers
+          validations do
+            include ValidationHelpers
 
-          def value
-            @value.join("\n")
-          end
+            validate :array_of_strings
 
-          def validate!
-            unless validate_array_of_strings(@value)
-              add_error('should be an 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 value
+            @config.join("\n")
+          end
         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 00000000000..a0617d21ad8
--- /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 00000000000..891b19c9743
--- /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 40ee1eb64ca..42cbb555694 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 47c68f96dc8..8ec7919f4d4 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 764fdfdb821..00000000000
--- 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 89a7183b655..8bb76cf920a 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 ff7016046cc..6af6aa15eef 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