diff options
author | Rémy Coutable <remy@rymai.me> | 2016-06-27 11:24:17 +0300 |
---|---|---|
committer | Rémy Coutable <remy@rymai.me> | 2016-06-27 11:24:17 +0300 |
commit | a9dbd394a60de76ffd9f5773560c8e9126751d91 (patch) | |
tree | 9b65e62ca6b46f0bce1d23d5c405d07c3868094b /lib | |
parent | 030db5c33e5433e4f27e1ea8c9e32729e79e4e95 (diff) | |
parent | 9510d31b4dd5955af2941a20a09d2dff6a55249a (diff) |
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
Diffstat (limited to 'lib')
-rw-r--r-- | lib/ci/gitlab_ci_yaml_processor.rb | 2 | ||||
-rw-r--r-- | lib/gitlab/ci/config.rb | 10 | ||||
-rw-r--r-- | lib/gitlab/ci/config/node/configurable.rb | 20 | ||||
-rw-r--r-- | lib/gitlab/ci/config/node/entry.rb | 50 | ||||
-rw-r--r-- | lib/gitlab/ci/config/node/factory.rb | 1 | ||||
-rw-r--r-- | lib/gitlab/ci/config/node/legacy_validation_helpers.rb (renamed from lib/gitlab/ci/config/node/validation_helpers.rb) | 2 | ||||
-rw-r--r-- | lib/gitlab/ci/config/node/script.rb | 12 | ||||
-rw-r--r-- | lib/gitlab/ci/config/node/validatable.rb | 29 | ||||
-rw-r--r-- | lib/gitlab/ci/config/node/validator.rb | 27 | ||||
-rw-r--r-- | lib/gitlab/ci/config/node/validators.rb | 27 |
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 |