Refactor `include` code and improve error reporting

lib/gitlab/ci/config/external/file/base.rb

@@ -6,22 +6,62 @@ module Gitlab
       module External
         module File
           class Base
+            include Gitlab::Utils::StrongMemoize
+
+            attr_reader :location, :opts, :errors
+
             YAML_WHITELIST_EXTENSION = /(yml|yaml)$/i.freeze
 
             def initialize(location, opts = {})
               @location = location
+              @opts = opts
+              @errors = []
+
+              validate_location!
+              validate_content!
+              validate_hash!
+            end
+
+            def invalid_extension?
+              !location.match(YAML_WHITELIST_EXTENSION)
             end
 
             def valid?
-              location.match(YAML_WHITELIST_EXTENSION) && content
+              errors.none?
+            end
+
+            def error_message
+              errors.first
             end
 
             def content
-              raise NotImplementedError, 'content must be implemented and return a string or nil'
+              raise NotImplementedError, 'subclass must implement fetching raw content'
             end
 
-            def error_message
-              raise NotImplementedError, 'error_message must be implemented and return a string'
+            def to_hash
+              @hash ||= Ci::Config::Loader.new(content).load!
+            rescue Ci::Config::Loader::FormatError
+              nil
+            end
+
+            protected
+
+            def validate_location!
+              if invalid_extension?
+                errors.push("Included file `#{location}` does not have YAML extension!")
+              end
+            end
+
+            def validate_content!
+              if errors.none? && content.blank?
+                errors.push("Included file `#{location}` is empty or does not exist!")
+              end
+            end
+
+            def validate_hash!
+              if errors.none? && to_hash.blank?
+                errors.push("Included file `#{location}` does not have valid YAML syntax!")
+              end
             end
           end
         end

lib/gitlab/ci/config/external/file/local.rb

@@ -6,25 +6,33 @@ module Gitlab
       module External
         module File
           class Local < Base
-            attr_reader :location, :project, :sha
+            include Gitlab::Utils::StrongMemoize
 
-            def initialize(location, opts = {})
-              super
+            attr_reader :project, :sha
 
+            def initialize(location, opts = {})
               @project = opts.fetch(:project)
               @sha = opts.fetch(:sha)
-            end
 
-            def content
-              @content ||= fetch_local_content
+              super
             end
 
-            def error_message
-              "Local file '#{location}' is not valid."
+            def content
+              strong_memoize(:content) { fetch_local_content }
             end
 
             private
 
+            def validate_content!
+              return if errors.any?
+
+              if content.nil?
+                errors.push("Local file `#{location}` does not exist!")
+              elsif content.blank?
+                errors.push("Local file `#{location}` is empty!")
+              end
+            end
+
             def fetch_local_content
               project.repository.blob_data_at(sha, location)
             end

lib/gitlab/ci/config/external/file/remote.rb

@@ -7,21 +7,34 @@ module Gitlab
         module File
           class Remote < Base
             include Gitlab::Utils::StrongMemoize
-            attr_reader :location
 
             def content
               strong_memoize(:content) { fetch_remote_content }
             end
 
-            def error_message
-              "Remote file '#{location}' is not valid."
-            end
-
             private
 
+            def validate_location!
+              super
+
+              unless ::Gitlab::UrlSanitizer.valid?(location)
+                errors.push("Remote file `#{location}` does not have a valid address!")
+              end
+            end
+
             def fetch_remote_content
               Gitlab::HTTP.get(location)
-            rescue Gitlab::HTTP::Error, Timeout::Error, SocketError, Gitlab::HTTP::BlockedUrlError
+            rescue SocketError
+              errors.push("Remote file `#{location}` could not be fetched because of a socket error!")
+              nil
+            rescue Timeout::Error
+              errors.push("Remote file `#{location}` could not be fetched because of a timeout error!")
+              nil
+            rescue Gitlab::HTTP::Error
+              errors.push("Remote file `#{location}` could not be fetched because of a HTTP error!")
+              nil
+            rescue Gitlab::HTTP::BlockedUrlError
+              errors.push("Remote file `#{location}` could not be fetched because the URL is blocked!")
               nil
             end
           end

lib/gitlab/ci/config/external/processor.rb

@@ -14,38 +14,34 @@ module Gitlab
           end
 
           def perform
-            return values if external_files.empty?
+            return @values if @external_files.empty?
 
-            external_files.each do |external_file|
-              validate_external_file(external_file)
-              @content.deep_merge!(content_of(external_file))
-            end
-
-            append_inline_content
-            remove_include_keyword
+            validate_external_files!
+            merge_external_files!
+            append_inline_content!
+            remove_include_keyword!
           end
 
           private
 
-          attr_reader :values, :external_files, :content
-
-          def validate_external_file(external_file)
-            unless external_file.valid?
-              raise IncludeError, external_file.error_message
+          def validate_external_files!
+            @external_files.each do |file|
+              raise IncludeError, file.error_message unless file.valid?
             end
           end
 
-          def content_of(external_file)
-            Config::Loader.new(external_file.content).load!
+          def merge_external_files!
+            @external_files.each do |file|
+              @content.deep_merge!(file.to_hash)
+            end
           end
 
-          def append_inline_content
+          def append_inline_content!
             @content.deep_merge!(@values)
           end
 
-          def remove_include_keyword
-            content.delete(:include)
-            content
+          def remove_include_keyword!
+            @content.tap { @content.delete(:include) }
           end
         end
       end

spec/lib/gitlab/ci/config/external/file/local_spec.rb

@@ -72,7 +72,7 @@ describe Gitlab::Ci::Config::External::File::Local do
     let(:location) { '/lib/gitlab/ci/templates/non-existent-file.yml' }
 
     it 'should return an error message' do
-      expect(local_file.error_message).to eq("Local file '#{location}' is not valid.")
+      expect(local_file.error_message).to eq("Local file `#{location}` does not exist!")
     end
   end
 end

spec/lib/gitlab/ci/config/external/file/remote_spec.rb

@@ -108,7 +108,7 @@ describe Gitlab::Ci::Config::External::File::Remote do
     let(:location) { 'not-valid://gitlab.com/gitlab-org/gitlab-ce/blob/1234/.gitlab-ci-1.yml' }
 
     it 'should return an error message' do
-      expect(remote_file.error_message).to eq("Remote file '#{location}' is not valid.")
+      expect(remote_file.error_message).to eq("Remote file `#{location}` does not have a valid address!")
     end
   end
 end

spec/lib/gitlab/ci/config/external/processor_spec.rb

@@ -21,7 +21,7 @@ describe Gitlab::Ci::Config::External::Processor do
       it 'should raise an error' do
         expect { processor.perform }.to raise_error(
           described_class::IncludeError,
-          "Local file '/lib/gitlab/ci/templates/non-existent-file.yml' is not valid."
+          "Local file `/lib/gitlab/ci/templates/non-existent-file.yml` does not exist!"
         )
       end
     end
@@ -37,7 +37,7 @@ describe Gitlab::Ci::Config::External::Processor do
       it 'should raise an error' do
         expect { processor.perform }.to raise_error(
           described_class::IncludeError,
-          "Remote file '#{remote_file}' is not valid."
+          "Remote file `#{remote_file}` could not be fetched because of a socket error!"
         )
       end
     end
@@ -159,7 +159,10 @@ describe Gitlab::Ci::Config::External::Processor do
       end
 
       it 'should raise an error' do
-        expect { processor.perform }.to raise_error(Gitlab::Ci::Config::Loader::FormatError)
+        expect { processor.perform }.to raise_error(
+          described_class::IncludeError,
+          "Included file `/lib/gitlab/ci/templates/template.yml` does not have valid YAML syntax!"
+        )
       end
     end
 

spec/lib/gitlab/ci/config_spec.rb

@@ -201,7 +201,7 @@ describe Gitlab::Ci::Config do
       it 'raises error YamlProcessor validationError' do
         expect { config }.to raise_error(
           described_class::ConfigError,
-          "Local file 'invalid' is not valid."
+          "Included file `invalid` does not have YAML extension!"
         )
       end
     end