Merge branch 'refactor/ci-config-add-entry-error' into 'master'

Improve validations and error handling in new CI config entries

## What does this MR do?

This MR improves validation in new CI config.

## Why was this MR needed?

With that it will be easier to handle errors during validation and post-processing.

## What are the relevant issue numbers?

This is a continuation of #15060

See merge request !4560

18 files changed, 292 insertions, 65 deletions

diff --git a/lib/ci/gitlab_ci_yaml_processor.rb b/lib/ci/gitlab_ci_yaml_processor.rb
index ed86de819eb..c52d4d63382 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.rb b/lib/gitlab/ci/config.rb
index b48d3592f16..adfd097736e 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?
+        @global.valid?
+      end
+
+      def errors
+        @global.errors
+      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 d60f87f3f94..374ff71d0f5 100644
--- a/lib/gitlab/ci/config/node/configurable.rb
+++ b/lib/gitlab/ci/config/node/configurable.rb
@@ -15,27 +15,24 @@ module Gitlab
         #
         module Configurable
           extend ActiveSupport::Concern
+          include Validatable
 
-          def allowed_nodes
-            self.class.allowed_nodes || {}
+          included do
+            validations do
+              validates :config, hash: true
+            end
           end
 
           private
 
-          def prevalidate!
-            unless @value.is_a?(Hash)
-              @errors << 'should be a configuration entry with hash value'
-            end
-          end
-
           def create_node(key, factory)
-            factory.with(value: @value[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
 
           class_methods do
-            def allowed_nodes
+            def nodes
               Hash[@allowed_nodes.map { |key, factory| [key, factory.dup] }]
             end
 
@@ -47,7 +44,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 52758a962f3..f044ef965e9 100644
--- a/lib/gitlab/ci/config/node/entry.rb
+++ b/lib/gitlab/ci/config/node/entry.rb
@@ -8,14 +8,14 @@ module Gitlab
         class Entry
           class InvalidError < StandardError; end
 
-          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)
+            @validator.validate
           end
 
           def process!
@@ -23,50 +23,54 @@ 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?
+            self.class.nodes.none?
           end
 
-          def errors
-            @errors + nodes.map(&:errors).flatten
+          def key
+            @key || self.class.name.demodulize.underscore
           end
 
-          def allowed_nodes
-            {}
+          def valid?
+            errors.none?
           end
 
-          def validate!
-            raise NotImplementedError
+          def errors
+            @validator.full_errors +
+              nodes.map(&:errors).flatten
           end
 
           def value
             raise NotImplementedError
           end
 
-          private
+          def self.nodes
+            {}
+          end
 
-          def prevalidate!
+          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
 
+          def process_nodes!
+            nodes.each(&:process!)
+          end
+
           def create_node(key, essence)
             raise NotImplementedError
           end
diff --git a/lib/gitlab/ci/config/node/factory.rb b/lib/gitlab/ci/config/node/factory.rb
index 787ca006f5a..025ae40ef94 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/validation_helpers.rb b/lib/gitlab/ci/config/node/legacy_validation_helpers.rb
index 72f648975dc..4d9a508796a 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 5072bf0db7d..c044f5c5e71 100644
--- a/lib/gitlab/ci/config/node/script.rb
+++ b/lib/gitlab/ci/config/node/script.rb
@@ -11,16 +11,14 @@ module Gitlab
         # implementation in Runner.
         #
         class Script < Entry
-          include ValidationHelpers
+          include Validatable
 
-          def value
-            @value.join("\n")
+          validations do
+            validates :config, array_of_strings: true
           end
 
-          def validate!
-            unless validate_array_of_strings(@value)
-              @errors << 'before_script 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 00000000000..f6e2896dfb2
--- /dev/null
+++ b/lib/gitlab/ci/config/node/validatable.rb
@@ -0,0 +1,29 @@
+module Gitlab
+  module Ci
+    class Config
+      module Node
+        module Validatable
+          extend ActiveSupport::Concern
+
+          class_methods do
+            def validator
+              validator = Class.new(Node::Validator)
+
+              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..02edc9219c3
--- /dev/null
+++ b/lib/gitlab/ci/config/node/validator.rb
@@ -0,0 +1,27 @@
+module Gitlab
+  module Ci
+    class Config
+      module Node
+        class Validator < SimpleDelegator
+          include ActiveModel::Validations
+          include Node::Validators
+
+          def initialize(node)
+            super(node)
+            @node = node
+          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/lib/gitlab/ci/config/node/validators.rb b/lib/gitlab/ci/config/node/validators.rb
new file mode 100644
index 00000000000..dc9cdb9a220
--- /dev/null
+++ b/lib/gitlab/ci/config/node/validators.rb
@@ -0,0 +1,27 @@
+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
+
+          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
+  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 d562d8b25ea..200ca6aeeea 100644
--- a/spec/lib/ci/gitlab_ci_yaml_processor_spec.rb
+++ b/spec/lib/ci/gitlab_ci_yaml_processor_spec.rb
@@ -951,7 +951,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..9bbda6e7396 100644
--- a/spec/lib/gitlab/ci/config/node/configurable_spec.rb
+++ b/spec/lib/gitlab/ci/config/node/configurable_spec.rb
@@ -7,26 +7,26 @@ describe Gitlab::Ci::Config::Node::Configurable do
     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/factory_spec.rb b/spec/lib/gitlab/ci/config/node/factory_spec.rb
index d681aa32456..01a707a6bd4 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 b1972172435..fddd53a2b57 100644
--- a/spec/lib/gitlab/ci/config/node/global_spec.rb
+++ b/spec/lib/gitlab/ci/config/node/global_spec.rb
@@ -3,13 +3,19 @@ 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
+
+  describe '#key' do
+    it 'returns underscored class name' do
+      expect(global.key).to eq 'global'
     end
   end
 
@@ -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 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 e4d6481f8a5..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 /should be an array of strings/
+            .to include 'Script config should be an array of strings'
         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 00000000000..10cd01afcd1
--- /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 00000000000..ad875d55384
--- /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
diff --git a/spec/lib/gitlab/ci/config_spec.rb b/spec/lib/gitlab/ci/config_spec.rb
index 3871d939feb..2a5d132db7b 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