Refactor `include` code and improve error reporting

4 files changed, 94 insertions, 37 deletions

diff --git a/lib/gitlab/ci/config/external/file/base.rb b/lib/gitlab/ci/config/external/file/base.rb
index dc96c97b129..390db997c0b 100644
--- a/lib/gitlab/ci/config/external/file/base.rb
+++ b/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
diff --git a/lib/gitlab/ci/config/external/file/local.rb b/lib/gitlab/ci/config/external/file/local.rb
index 8c0ffd36449..acf4201a672 100644
--- a/lib/gitlab/ci/config/external/file/local.rb
+++ b/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
diff --git a/lib/gitlab/ci/config/external/file/remote.rb b/lib/gitlab/ci/config/external/file/remote.rb
index 291fe87849c..b1ba37a3959 100644
--- a/lib/gitlab/ci/config/external/file/remote.rb
+++ b/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
diff --git a/lib/gitlab/ci/config/external/processor.rb b/lib/gitlab/ci/config/external/processor.rb
index 4f07494925a..eae0bdeb644 100644
--- a/lib/gitlab/ci/config/external/processor.rb
+++ b/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