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] 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 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
--
GitLab