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

10 files changed, 134 insertions, 46 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