diff --git a/lib/ci/gitlab_ci_yaml_processor.rb b/lib/ci/gitlab_ci_yaml_processor.rb
index ed86de819eb7c5c8add2031cfc6a06facdf597c3..c52d4d6338209d5c1cb05f6ad247b929649e7072 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 b48d3592f1699c6473700703b299b3ccd3e6968c..adfd097736e3e0347dd6dd889ed13b4aa38b658e 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 d60f87f3f94d5e127932c410600aa2183c89a3bc..374ff71d0f5544e40bc0ac9eeb04a164d000da57 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 52758a962f3c79989cd36582be5dc86114049853..f044ef965e9c1a19b100a2f9bd9d55caf96a724a 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 787ca006f5abb8291ee4a034a56348db121aa06f..025ae40ef944ebb5a8ea4ccbe5d8a7949f11142a 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
similarity index 97%
rename from lib/gitlab/ci/config/node/validation_helpers.rb
rename to lib/gitlab/ci/config/node/legacy_validation_helpers.rb
index 72f648975dcf1c3ee2f6249ef31499ad535309ae..4d9a508796abff70c3731d0a2248998e7d5a83ab 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 5072bf0db7d56ef4016629cfb229ffa8a6b0fb5c..c044f5c5e717b38edb77c22b8fcee2f9978065c0 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 0000000000000000000000000000000000000000..f6e2896dfb23ce2760a62aa46f50a1dbc5ba15e6
--- /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 0000000000000000000000000000000000000000..02edc9219c395330ee2fc6eac9bfbceb6373858c
--- /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 0000000000000000000000000000000000000000..dc9cdb9a2205038fc928b8a8f06662e55f649476
--- /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 d562d8b25ea4f8ed00a13e9ce866cab2afaae142..200ca6aeeea32c059ad3082dc90a117ba02dc689 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 47c68f96dc8ab603743f67ee38aa0e0b8dff4742..9bbda6e73967ba358f44808a5cf1bca7784210f5 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 d681aa32456fbf43a1f8ebb84876525260a8cc1d..01a707a6bd49846bd865ada732d6b02c294cd33f 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 b1972172435a29f0950d665f63138860438a2b7b..fddd53a2b57508c713f14072eb21611f555bc96e 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 e4d6481f8a5711bacfe6705aa082d50d7b369c1d..6af6aa15eef2dbe618bc20bdda64b9cfb3531d1b 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 0000000000000000000000000000000000000000..10cd01afcd1da4e16314af3ab8a0ca7f34a493ff
--- /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 0000000000000000000000000000000000000000..ad875d553840bf6b0b0797e8d2c645fd8ae35edd
--- /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 3871d939feb62c16ebfaef7d32c06aef82524a2a..2a5d132db7bb6f218df734d21f3227e1d7e84afd 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