From 76aea978c6b2d8c69881836408c4947d816d2db2 Mon Sep 17 00:00:00 2001 From: Grzegorz Bizon Date: Thu, 9 Jun 2016 14:48:40 +0200 Subject: [PATCH] Add class that encapsulates error in new Ci config --- lib/gitlab/ci/config.rb | 10 +++++-- lib/gitlab/ci/config/node/configurable.rb | 4 +-- lib/gitlab/ci/config/node/entry.rb | 9 +++++++ lib/gitlab/ci/config/node/error.rb | 26 +++++++++++++++++++ lib/gitlab/ci/config/node/factory.rb | 1 + lib/gitlab/ci/config/node/script.rb | 2 +- spec/lib/ci/gitlab_ci_yaml_processor_spec.rb | 2 +- spec/lib/gitlab/ci/config/node/error_spec.rb | 23 ++++++++++++++++ .../lib/gitlab/ci/config/node/factory_spec.rb | 10 +++++++ spec/lib/gitlab/ci/config/node/global_spec.rb | 8 +++++- spec/lib/gitlab/ci/config/node/script_spec.rb | 2 +- spec/lib/gitlab/ci/config_spec.rb | 6 +++++ 12 files changed, 95 insertions(+), 8 deletions(-) create mode 100644 lib/gitlab/ci/config/node/error.rb create 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 b48d3592f16..26028547fa0 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? + errors.none? + end + + def errors + @global.errors.map(&:to_s) + 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..2f4abbf9ffe 100644 --- a/lib/gitlab/ci/config/node/configurable.rb +++ b/lib/gitlab/ci/config/node/configurable.rb @@ -24,12 +24,12 @@ module Gitlab def prevalidate! unless @value.is_a?(Hash) - @errors << 'should be a configuration entry with hash value' + add_error('should be a configuration entry with hash value') end end def create_node(key, factory) - factory.with(value: @value[key]) + factory.with(value: @value[key], key: key) factory.nullify! unless @value.has_key?(key) factory.create! end diff --git a/lib/gitlab/ci/config/node/entry.rb b/lib/gitlab/ci/config/node/entry.rb index 52758a962f3..f41e191d611 100644 --- a/lib/gitlab/ci/config/node/entry.rb +++ b/lib/gitlab/ci/config/node/entry.rb @@ -8,6 +8,7 @@ module Gitlab class Entry class InvalidError < StandardError; end + attr_writer :key attr_accessor :description def initialize(value) @@ -40,10 +41,18 @@ module Gitlab allowed_nodes.none? end + def key + @key || self.class.name.demodulize.underscore + end + def errors @errors + nodes.map(&:errors).flatten end + def add_error(message) + @errors << Error.new(message, self) + end + def allowed_nodes {} end diff --git a/lib/gitlab/ci/config/node/error.rb b/lib/gitlab/ci/config/node/error.rb new file mode 100644 index 00000000000..5b8d0288e4e --- /dev/null +++ b/lib/gitlab/ci/config/node/error.rb @@ -0,0 +1,26 @@ +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/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/script.rb b/lib/gitlab/ci/config/node/script.rb index 5072bf0db7d..314877a035c 100644 --- a/lib/gitlab/ci/config/node/script.rb +++ b/lib/gitlab/ci/config/node/script.rb @@ -19,7 +19,7 @@ module Gitlab def validate! unless validate_array_of_strings(@value) - @errors << 'before_script should be an array of strings' + add_error('should be an array of strings') 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 5e1d2b8e4f5..40ee1eb64ca 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: 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/error_spec.rb b/spec/lib/gitlab/ci/config/node/error_spec.rb new file mode 100644 index 00000000000..764fdfdb821 --- /dev/null +++ b/spec/lib/gitlab/ci/config/node/error_spec.rb @@ -0,0 +1,23 @@ +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/factory_spec.rb b/spec/lib/gitlab/ci/config/node/factory_spec.rb index d681aa32456..01a707a6bd4 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 b1972172435..89a7183b655 100644 --- a/spec/lib/gitlab/ci/config/node/global_spec.rb +++ b/spec/lib/gitlab/ci/config/node/global_spec.rb @@ -13,6 +13,12 @@ describe Gitlab::Ci::Config::Node::Global do end end + describe '#key' do + it 'returns underscored class name' do + expect(global.key).to eq 'global' + end + end + context 'when hash is valid' do let(:hash) do { before_script: ['ls', 'pwd'] } @@ -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: 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 e4d6481f8a5..ff7016046cc 100644 --- a/spec/lib/gitlab/ci/config/node/script_spec.rb +++ b/spec/lib/gitlab/ci/config/node/script_spec.rb @@ -34,7 +34,7 @@ describe Gitlab::Ci::Config::Node::Script do describe '#errors' do it 'saves errors' do expect(entry.errors) - .to include /should be an array of strings/ + .to include 'script: should be an array of strings' end end diff --git a/spec/lib/gitlab/ci/config_spec.rb b/spec/lib/gitlab/ci/config_spec.rb index 3871d939feb..2a5d132db7b 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