diff --git a/lib/gitlab/ci/config/entry/attributable.rb b/lib/gitlab/ci/config/entry/attributable.rb index 1c8b55ee4c4..3e87a09704e 100644 --- a/lib/gitlab/ci/config/entry/attributable.rb +++ b/lib/gitlab/ci/config/entry/attributable.rb @@ -8,6 +8,10 @@ module Gitlab class_methods do def attributes(*attributes) attributes.flatten.each do |attribute| + if method_defined?(attribute) + raise ArgumentError, 'Method already defined!' + end + define_method(attribute) do return unless config.is_a?(Hash) diff --git a/lib/gitlab/ci/config/entry/configurable.rb b/lib/gitlab/ci/config/entry/configurable.rb index e05aca9881b..68b6742385a 100644 --- a/lib/gitlab/ci/config/entry/configurable.rb +++ b/lib/gitlab/ci/config/entry/configurable.rb @@ -15,9 +15,10 @@ module Gitlab # module Configurable extend ActiveSupport::Concern - include Validatable included do + include Validatable + validations do validates :config, type: Hash end diff --git a/lib/gitlab/ci/config/entry/job.rb b/lib/gitlab/ci/config/entry/job.rb index 32f5c6ab142..91aac6df4b1 100644 --- a/lib/gitlab/ci/config/entry/job.rb +++ b/lib/gitlab/ci/config/entry/job.rb @@ -59,10 +59,10 @@ module Gitlab entry :services, Entry::Services, description: 'Services that will be used to execute this job.' - entry :only, Entry::Trigger, + entry :only, Entry::Policy, description: 'Refs policy this job will be executed for.' - entry :except, Entry::Trigger, + entry :except, Entry::Policy, description: 'Refs policy this job will be executed for.' entry :variables, Entry::Variables, diff --git a/lib/gitlab/ci/config/entry/node.rb b/lib/gitlab/ci/config/entry/node.rb index a6a914d79c1..c868943c42e 100644 --- a/lib/gitlab/ci/config/entry/node.rb +++ b/lib/gitlab/ci/config/entry/node.rb @@ -16,8 +16,9 @@ module Gitlab @metadata = metadata @entries = {} - @validator = self.class.validator.new(self) - @validator.validate(:new) + self.class.aspects.to_a.each do |aspect| + instance_exec(&aspect) + end end def [](key) @@ -47,7 +48,7 @@ module Gitlab end def errors - @validator.messages + descendants.flat_map(&:errors) + [] end def value @@ -70,6 +71,13 @@ module Gitlab true end + def location + name = @key.presence || self.class.name.to_s.demodulize + .underscore.humanize.downcase + + ancestors.map(&:key).append(name).compact.join(':') + end + def inspect val = leaf? ? config : descendants unspecified = specified? ? '' : '(unspecified) ' @@ -79,8 +87,8 @@ module Gitlab def self.default end - def self.validator - Validator + def self.aspects + @aspects ||= [] end end end diff --git a/lib/gitlab/ci/config/entry/policy.rb b/lib/gitlab/ci/config/entry/policy.rb new file mode 100644 index 00000000000..3cdae1cee4f --- /dev/null +++ b/lib/gitlab/ci/config/entry/policy.rb @@ -0,0 +1,31 @@ +module Gitlab + module Ci + class Config + module Entry + ## + # Entry that represents an only/except trigger policy for the job. + # + class Policy < Simplifiable + strategy :RefsPolicy, if: -> (config) { config.is_a?(Array) } + + class RefsPolicy < Entry::Node + include Entry::Validatable + + validations do + validates :config, array_of_strings_or_regexps: true + end + end + + class UnknownStrategy < Entry::Node + def errors + ["#{location} has to be either an array of conditions or a hash"] + end + end + + def self.default + end + end + end + end + end +end diff --git a/lib/gitlab/ci/config/entry/simplifiable.rb b/lib/gitlab/ci/config/entry/simplifiable.rb new file mode 100644 index 00000000000..12764629686 --- /dev/null +++ b/lib/gitlab/ci/config/entry/simplifiable.rb @@ -0,0 +1,43 @@ +module Gitlab + module Ci + class Config + module Entry + class Simplifiable < SimpleDelegator + EntryStrategy = Struct.new(:name, :condition) + + def initialize(config, **metadata) + unless self.class.const_defined?(:UnknownStrategy) + raise ArgumentError, 'UndefinedStrategy not available!' + end + + strategy = self.class.strategies.find do |variant| + variant.condition.call(config) + end + + entry = self.class.entry_class(strategy) + + super(entry.new(config, metadata)) + end + + def self.strategy(name, **opts) + EntryStrategy.new(name, opts.fetch(:if)).tap do |strategy| + strategies.append(strategy) + end + end + + def self.strategies + @strategies ||= [] + end + + def self.entry_class(strategy) + if strategy.present? + self.const_get(strategy.name) + else + self::UnknownStrategy + end + end + end + end + end + end +end diff --git a/lib/gitlab/ci/config/entry/trigger.rb b/lib/gitlab/ci/config/entry/trigger.rb deleted file mode 100644 index 16b234e6c59..00000000000 --- a/lib/gitlab/ci/config/entry/trigger.rb +++ /dev/null @@ -1,18 +0,0 @@ -module Gitlab - module Ci - class Config - module Entry - ## - # Entry that represents a trigger policy for the job. - # - class Trigger < Node - include Validatable - - validations do - validates :config, array_of_strings_or_regexps: true - end - end - end - end - end -end diff --git a/lib/gitlab/ci/config/entry/validatable.rb b/lib/gitlab/ci/config/entry/validatable.rb index f7f1b111571..5ced778d311 100644 --- a/lib/gitlab/ci/config/entry/validatable.rb +++ b/lib/gitlab/ci/config/entry/validatable.rb @@ -5,6 +5,17 @@ module Gitlab module Validatable extend ActiveSupport::Concern + def self.included(node) + node.aspects.append -> do + @validator = self.class.validator.new(self) + @validator.validate(:new) + end + end + + def errors + @validator.messages + descendants.flat_map(&:errors) + end + class_methods do def validator @validator ||= Class.new(Entry::Validator).tap do |validator| diff --git a/lib/gitlab/ci/config/entry/validator.rb b/lib/gitlab/ci/config/entry/validator.rb index 55343005fe3..2df23a3edcd 100644 --- a/lib/gitlab/ci/config/entry/validator.rb +++ b/lib/gitlab/ci/config/entry/validator.rb @@ -8,7 +8,6 @@ module Gitlab def initialize(entry) super(entry) - @entry = entry end def messages @@ -20,21 +19,6 @@ module Gitlab def self.name 'Validator' end - - private - - def location - predecessors = ancestors.map(&:key).compact - predecessors.append(key_name).join(':') - end - - def key_name - if key.blank? - @entry.class.name.demodulize.underscore.humanize - else - key - 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 ee28387cd48..c70a4cb55fe 100644 --- a/spec/lib/ci/gitlab_ci_yaml_processor_spec.rb +++ b/spec/lib/ci/gitlab_ci_yaml_processor_spec.rb @@ -344,28 +344,31 @@ module Ci let(:config) { { rspec: { script: "rspec", type: "test", only: only } } } let(:processor) { GitlabCiYamlProcessor.new(YAML.dump(config)) } - shared_examples 'raises an error' do - it do - expect { processor }.to raise_error(GitlabCiYamlProcessor::ValidationError, 'jobs:rspec:only config should be an array of strings or regexps') - end - end - context 'when it is integer' do let(:only) { 1 } - it_behaves_like 'raises an error' + it do + expect { processor }.to raise_error(GitlabCiYamlProcessor::ValidationError, + 'jobs:rspec:only has to be either an array of conditions or a hash') + end end context 'when it is an array of integers' do let(:only) { [1, 1] } - it_behaves_like 'raises an error' + it do + expect { processor }.to raise_error(GitlabCiYamlProcessor::ValidationError, + 'jobs:rspec:only config should be an array of strings or regexps') + end end context 'when it is invalid regex' do let(:only) { ["/*invalid/"] } - it_behaves_like 'raises an error' + it do + expect { processor }.to raise_error(GitlabCiYamlProcessor::ValidationError, + 'jobs:rspec:only config should be an array of strings or regexps') + end end end end @@ -518,28 +521,31 @@ module Ci let(:config) { { rspec: { script: "rspec", except: except } } } let(:processor) { GitlabCiYamlProcessor.new(YAML.dump(config)) } - shared_examples 'raises an error' do - it do - expect { processor }.to raise_error(GitlabCiYamlProcessor::ValidationError, 'jobs:rspec:except config should be an array of strings or regexps') - end - end - context 'when it is integer' do let(:except) { 1 } - it_behaves_like 'raises an error' + it do + expect { processor }.to raise_error(GitlabCiYamlProcessor::ValidationError, + 'jobs:rspec:except has to be either an array of conditions or a hash') + end end context 'when it is an array of integers' do let(:except) { [1, 1] } - it_behaves_like 'raises an error' + it do + expect { processor }.to raise_error(GitlabCiYamlProcessor::ValidationError, + 'jobs:rspec:except config should be an array of strings or regexps') + end end context 'when it is invalid regex' do let(:except) { ["/*invalid/"] } - it_behaves_like 'raises an error' + it do + expect { processor }.to raise_error(GitlabCiYamlProcessor::ValidationError, + 'jobs:rspec:except config should be an array of strings or regexps') + end end end end diff --git a/spec/lib/gitlab/ci/config/entry/attributable_spec.rb b/spec/lib/gitlab/ci/config/entry/attributable_spec.rb index fde03c51e2c..b028b771375 100644 --- a/spec/lib/gitlab/ci/config/entry/attributable_spec.rb +++ b/spec/lib/gitlab/ci/config/entry/attributable_spec.rb @@ -1,18 +1,21 @@ require 'spec_helper' describe Gitlab::Ci::Config::Entry::Attributable do - let(:node) { Class.new } + let(:node) do + Class.new do + include Gitlab::Ci::Config::Entry::Attributable + end + end + let(:instance) { node.new } before do - node.include(described_class) - node.class_eval do attributes :name, :test end end - context 'config is a hash' do + context 'when config is a hash' do before do allow(instance) .to receive(:config) @@ -29,7 +32,7 @@ describe Gitlab::Ci::Config::Entry::Attributable do end end - context 'config is not a hash' do + context 'when config is not a hash' do before do allow(instance) .to receive(:config) @@ -40,4 +43,18 @@ describe Gitlab::Ci::Config::Entry::Attributable do expect(instance.test).to be_nil end end + + context 'when method is already defined in a superclass' do + it 'raises an error' do + expectation = expect do + Class.new(String) do + include Gitlab::Ci::Config::Entry::Attributable + + attributes :length + end + end + + expectation.to raise_error(ArgumentError, 'Method already defined!') + end + end end diff --git a/spec/lib/gitlab/ci/config/entry/configurable_spec.rb b/spec/lib/gitlab/ci/config/entry/configurable_spec.rb index ae7e628b5b5..088d4b472da 100644 --- a/spec/lib/gitlab/ci/config/entry/configurable_spec.rb +++ b/spec/lib/gitlab/ci/config/entry/configurable_spec.rb @@ -1,40 +1,26 @@ require 'spec_helper' describe Gitlab::Ci::Config::Entry::Configurable do - let(:entry) { Class.new } - - before do - entry.include(described_class) + let(:entry) do + Class.new(Gitlab::Ci::Config::Entry::Node) do + include Gitlab::Ci::Config::Entry::Configurable + end end describe 'validations' do - let(:validator) { entry.validator.new(instance) } - - before do - entry.class_eval do - attr_reader :config - - def initialize(config) - @config = config - end - end - - validator.validate - end - - context 'when entry validator is invalid' do - let(:instance) { entry.new('ls') } - - it 'returns invalid validator' do - expect(validator).to be_invalid - end - end - - context 'when entry instance is valid' do + context 'when entry is a hash' do let(:instance) { entry.new(key: 'value') } - it 'returns valid validator' do - expect(validator).to be_valid + it 'correctly validates an instance' do + expect(instance).to be_valid + end + end + + context 'when entry is not a hash' do + let(:instance) { entry.new('ls') } + + it 'invalidates the instance' do + expect(instance).not_to be_valid end end end diff --git a/spec/lib/gitlab/ci/config/entry/policy_spec.rb b/spec/lib/gitlab/ci/config/entry/policy_spec.rb new file mode 100644 index 00000000000..36a84da4a52 --- /dev/null +++ b/spec/lib/gitlab/ci/config/entry/policy_spec.rb @@ -0,0 +1,73 @@ +require 'spec_helper' + +describe Gitlab::Ci::Config::Entry::Policy do + let(:entry) { described_class.new(config) } + + context 'when using simplified policy' do + describe 'validations' do + context 'when entry config value is valid' do + context 'when config is a branch or tag name' do + let(:config) { %w[master feature/branch] } + + describe '#valid?' do + it 'is valid' do + expect(entry).to be_valid + end + end + + describe '#value' do + it 'returns key value' do + expect(entry.value).to eq config + end + end + end + + context 'when config is a regexp' do + let(:config) { ['/^issue-.*$/'] } + + describe '#valid?' do + it 'is valid' do + expect(entry).to be_valid + end + end + end + + context 'when config is a special keyword' do + let(:config) { %w[tags triggers branches] } + + describe '#valid?' do + it 'is valid' do + expect(entry).to be_valid + end + end + end + end + + context 'when entry value is not valid' do + let(:config) { [1] } + + describe '#errors' do + it 'saves errors' do + expect(entry.errors) + .to include /policy config should be an array of strings or regexps/ + end + end + end + end + end + + context 'when policy strategy does not match' do + let(:config) { 'string strategy' } + + it 'returns information about errors' do + expect(entry.errors) + .to include /has to be either an array of conditions or a hash/ + end + end + + describe '.default' do + it 'does not have a default value' do + expect(described_class.default).to be_nil + end + end +end diff --git a/spec/lib/gitlab/ci/config/entry/simplifiable_spec.rb b/spec/lib/gitlab/ci/config/entry/simplifiable_spec.rb new file mode 100644 index 00000000000..395062207a3 --- /dev/null +++ b/spec/lib/gitlab/ci/config/entry/simplifiable_spec.rb @@ -0,0 +1,88 @@ +require 'spec_helper' + +describe Gitlab::Ci::Config::Entry::Simplifiable do + describe '.strategy' do + let(:entry) do + Class.new(described_class) do + strategy :Something, if: -> { 'condition' } + strategy :DifferentOne, if: -> { 'condition' } + end + end + + it 'defines entry strategies' do + expect(entry.strategies.size).to eq 2 + expect(entry.strategies.map(&:name)) + .to eq %i[Something DifferentOne] + end + end + + describe 'setting strategy by a condition' do + let(:first) { double('first strategy') } + let(:second) { double('second strategy') } + let(:unknown) { double('unknown strategy') } + + before do + stub_const("#{described_class.name}::Something", first) + stub_const("#{described_class.name}::DifferentOne", second) + stub_const("#{described_class.name}::UnknownStrategy", unknown) + end + + context 'when first strategy should be used' do + let(:entry) do + Class.new(described_class) do + strategy :Something, if: -> (arg) { arg == 'something' } + strategy :DifferentOne, if: -> (*) { false } + end + end + + it 'attemps to load a first strategy' do + expect(first).to receive(:new).with('something', anything) + + entry.new('something') + end + end + + context 'when second strategy should be used' do + let(:entry) do + Class.new(described_class) do + strategy :Something, if: -> (arg) { arg == 'something' } + strategy :DifferentOne, if: -> (arg) { arg == 'test' } + end + end + + it 'attemps to load a second strategy' do + expect(second).to receive(:new).with('test', anything) + + entry.new('test') + end + end + + context 'when neither one is a valid strategy' do + let(:entry) do + Class.new(described_class) do + strategy :Something, if: -> (*) { false } + strategy :DifferentOne, if: -> (*) { false } + end + end + + it 'instantiates an unknown strategy' do + expect(unknown).to receive(:new).with('test', anything) + + entry.new('test') + end + end + end + + context 'when a unknown strategy class is not defined' do + let(:entry) do + Class.new(described_class) do + strategy :String, if: -> (*) { true } + end + end + + it 'raises an error when being initialized' do + expect { entry.new('something') } + .to raise_error ArgumentError, /UndefinedStrategy not available!/ + end + end +end diff --git a/spec/lib/gitlab/ci/config/entry/trigger_spec.rb b/spec/lib/gitlab/ci/config/entry/trigger_spec.rb deleted file mode 100644 index e4ee44f1274..00000000000 --- a/spec/lib/gitlab/ci/config/entry/trigger_spec.rb +++ /dev/null @@ -1,56 +0,0 @@ -require 'spec_helper' - -describe Gitlab::Ci::Config::Entry::Trigger do - let(:entry) { described_class.new(config) } - - describe 'validations' do - context 'when entry config value is valid' do - context 'when config is a branch or tag name' do - let(:config) { %w[master feature/branch] } - - describe '#valid?' do - it 'is valid' do - expect(entry).to be_valid - end - end - - describe '#value' do - it 'returns key value' do - expect(entry.value).to eq config - end - end - end - - context 'when config is a regexp' do - let(:config) { ['/^issue-.*$/'] } - - describe '#valid?' do - it 'is valid' do - expect(entry).to be_valid - end - end - end - - context 'when config is a special keyword' do - let(:config) { %w[tags triggers branches] } - - describe '#valid?' do - it 'is valid' do - expect(entry).to be_valid - end - end - end - end - - context 'when entry value is not valid' do - let(:config) { [1] } - - describe '#errors' do - it 'saves errors' do - expect(entry.errors) - .to include 'trigger config should be an array of strings or regexps' - end - end - end - end -end diff --git a/spec/lib/gitlab/ci/config/entry/validatable_spec.rb b/spec/lib/gitlab/ci/config/entry/validatable_spec.rb index d1856801827..ae2a7a51ba6 100644 --- a/spec/lib/gitlab/ci/config/entry/validatable_spec.rb +++ b/spec/lib/gitlab/ci/config/entry/validatable_spec.rb @@ -1,10 +1,10 @@ require 'spec_helper' describe Gitlab::Ci::Config::Entry::Validatable do - let(:entry) { Class.new } - - before do - entry.include(described_class) + let(:entry) do + Class.new(Gitlab::Ci::Config::Entry::Node) do + include Gitlab::Ci::Config::Entry::Validatable + end end describe '.validator' do @@ -28,7 +28,7 @@ describe Gitlab::Ci::Config::Entry::Validatable do end context 'when validating entry instance' do - let(:entry_instance) { entry.new } + let(:entry_instance) { entry.new('something') } context 'when attribute is valid' do before do diff --git a/spec/lib/gitlab/ci/config/entry/validator_spec.rb b/spec/lib/gitlab/ci/config/entry/validator_spec.rb index ad7e6f07d3c..172b6b47a4f 100644 --- a/spec/lib/gitlab/ci/config/entry/validator_spec.rb +++ b/spec/lib/gitlab/ci/config/entry/validator_spec.rb @@ -48,7 +48,7 @@ describe Gitlab::Ci::Config::Entry::Validator do validator_instance.validate expect(validator_instance.messages) - .to include "node test attribute can't be blank" + .to include /test attribute can't be blank/ end end end