Add class that encapsulates error in new Ci config

12 files changed, 95 insertions, 8 deletions

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