From 3149b5cfea4d8f14d69bb2520974f588454a0762 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Matija=20=C4=8Cupi=C4=87?= <matteeyah@gmail.com>
Date: Fri, 7 Sep 2018 22:03:05 +0200
Subject: [PATCH] Improve external architecture

CE mirror of 4f17c7b2c30188302e6a73421acbf5a09fb2c823
---
 app/models/blob_viewer/gitlab_ci_yml.rb       |  8 +-
 app/models/ci/pipeline.rb                     |  6 +-
 .../blob/viewers/_gitlab_ci_yml.html.haml     |  4 +-
 doc/ci/yaml/README.md                         | 16 ++--
 lib/gitlab/ci/config.rb                       | 16 ++--
 lib/gitlab/ci/external/file/local.rb          | 35 +++++++
 lib/gitlab/ci/external/file/remote.rb         | 25 +++++
 lib/gitlab/ci/external/mapper.rb              | 31 ++++++
 lib/gitlab/ci/external/processor.rb           | 50 ++++++++++
 lib/gitlab/ci/external_files/external_file.rb | 42 ---------
 lib/gitlab/ci/external_files/mapper.rb        | 28 ------
 lib/gitlab/ci/external_files/processor.rb     | 45 ---------
 lib/gitlab/ci/yaml_processor.rb               |  8 +-
 spec/lib/gitlab/ci/config_spec.rb             | 21 +++--
 .../lib/gitlab/ci/external/file/local_spec.rb | 54 +++++++++++
 .../gitlab/ci/external/file/remote_spec.rb    | 68 ++++++++++++++
 spec/lib/gitlab/ci/external/mapper_spec.rb    | 92 ++++++++++++++++++
 .../processor_spec.rb                         | 52 +++++++---
 .../ci/external_files/external_file_spec.rb   | 94 -------------------
 .../gitlab/ci/external_files/mapper_spec.rb   | 57 -----------
 spec/models/blob_viewer/gitlab_ci_yml_spec.rb |  6 +-
 spec/models/ci/pipeline_spec.rb               |  2 +-
 22 files changed, 445 insertions(+), 315 deletions(-)
 create mode 100644 lib/gitlab/ci/external/file/local.rb
 create mode 100644 lib/gitlab/ci/external/file/remote.rb
 create mode 100644 lib/gitlab/ci/external/mapper.rb
 create mode 100644 lib/gitlab/ci/external/processor.rb
 delete mode 100644 lib/gitlab/ci/external_files/external_file.rb
 delete mode 100644 lib/gitlab/ci/external_files/mapper.rb
 delete mode 100644 lib/gitlab/ci/external_files/processor.rb
 create mode 100644 spec/lib/gitlab/ci/external/file/local_spec.rb
 create mode 100644 spec/lib/gitlab/ci/external/file/remote_spec.rb
 create mode 100644 spec/lib/gitlab/ci/external/mapper_spec.rb
 rename spec/lib/gitlab/ci/{external_files => external}/processor_spec.rb (69%)
 delete mode 100644 spec/lib/gitlab/ci/external_files/external_file_spec.rb
 delete mode 100644 spec/lib/gitlab/ci/external_files/mapper_spec.rb

diff --git a/app/models/blob_viewer/gitlab_ci_yml.rb b/app/models/blob_viewer/gitlab_ci_yml.rb
index f5e4367fc09..54462d01b66 100644
--- a/app/models/blob_viewer/gitlab_ci_yml.rb
+++ b/app/models/blob_viewer/gitlab_ci_yml.rb
@@ -10,16 +10,16 @@ module BlobViewer
     self.file_types = %i(gitlab_ci)
     self.binary = false
 
-    def validation_message
+    def validation_message(project, ref)
       return @validation_message if defined?(@validation_message)
 
       prepare!
 
-      @validation_message = Gitlab::Ci::YamlProcessor.validation_message(blob.data, project)
+      @validation_message = Gitlab::Ci::YamlProcessor.validation_message(blob.data, { project: project, branch_name: ref })
     end
 
-    def valid?
-      validation_message.blank?
+    def valid?(project, ref)
+      validation_message(project, ref).blank?
     end
   end
 end
diff --git a/app/models/ci/pipeline.rb b/app/models/ci/pipeline.rb
index f3a40204305..27e0ade2722 100644
--- a/app/models/ci/pipeline.rb
+++ b/app/models/ci/pipeline.rb
@@ -464,7 +464,7 @@ module Ci
       return @config_processor if defined?(@config_processor)
 
       @config_processor ||= begin
-        Gitlab::Ci::YamlProcessor.new(ci_yaml_file, project)
+        initialize_yaml_processor
       rescue Gitlab::Ci::YamlProcessor::ValidationError => e
         self.yaml_errors = e.message
         nil
@@ -474,6 +474,10 @@ module Ci
       end
     end
 
+    def initialize_yaml_processor
+      Gitlab::Ci::YamlProcessor.new(ci_yaml_file, { project: project, branch_name: ref })
+    end
+
     def ci_yaml_file_path
       if project.ci_config_path.blank?
         '.gitlab-ci.yml'
diff --git a/app/views/projects/blob/viewers/_gitlab_ci_yml.html.haml b/app/views/projects/blob/viewers/_gitlab_ci_yml.html.haml
index 28c5be6ebf3..6ef09278ee8 100644
--- a/app/views/projects/blob/viewers/_gitlab_ci_yml.html.haml
+++ b/app/views/projects/blob/viewers/_gitlab_ci_yml.html.haml
@@ -1,9 +1,9 @@
-- if viewer.valid?
+- if viewer.valid?(@project, @ref)
   = icon('check fw')
   This GitLab CI configuration is valid.
 - else
   = icon('warning fw')
   This GitLab CI configuration is invalid:
-  = viewer.validation_message
+  = viewer.validation_message(@project, @ref)
 
 = link_to 'Learn more', help_page_path('ci/yaml/README')
diff --git a/doc/ci/yaml/README.md b/doc/ci/yaml/README.md
index 418d5b88375..3ade7201177 100644
--- a/doc/ci/yaml/README.md
+++ b/doc/ci/yaml/README.md
@@ -981,6 +981,7 @@ before_script:
 ```
 
 ```yaml
+# Content of gitlab-ci.yml
 include: 'https://gitlab.com/awesome-project/raw/master/.gitlab-ci-before-script-template.yml'
 
 rspec:
@@ -992,13 +993,13 @@ rubocop:
     - bundle exec rubocop
 ```
 
-In the above example `.gitlab-ci-before-script-template.yml` content will be automatically fetched and evaluated as it were a single local file.
+In the above example `.gitlab-ci-before-script-template.yml` content will be automatically fetched and evaluated along with the content of `gitlab-ci.yml`.
 
 `include` supports two types of files:
   - **local** to the same repository, referenced using the relative path, or 
-  - **remote** in a different location, accessed using HTTP(S) protocol, referenced using the full URL
+  - **remote** in a different location, accessed using HTTP protocol, referenced using the full URL
 
-Also, `include` supports a single string or an array of different values, so
+Also, `include` supports a single string or an array composed by different values, so
 
 ```yaml
 include: '/templates/.gitlab-ci-templates.yml'
@@ -1016,11 +1017,12 @@ are both valid use cases.
 
 #### Restrictions
 
-- We can only use files that are currently tracked by Git on the default repository branch, so when using a **local file** make sure it's on the latest commit of your default branch, otherwise feel free to use a remote location.
-- Since external files defined on `include` are evaluated first, the configuration on this files will take precedence over the content of `.gitlab-ci.yml`, for example:
+- We can only use files that are currently tracked by Git on the same branch and commit your configuration file is. In other words, when using a **local file** make sure it's on the latest commit of your branch.
+- Since external files defined on `include` are evaluated first, the content on `gitlab-ci.yml` **will take precedence over the content of the external files**, for example:
 
 ```yaml
 # Content of http://company.com/default-gitlab-ci.yml
+
 image: php:5-fpm-alpine
 
 job2:
@@ -1029,6 +1031,8 @@ job2:
 
 
 ```yaml
+# Content of gitlab-ci.yml
+
 include:
   - http://company.com/default-gitlab-ci.yml
 
@@ -1038,7 +1042,7 @@ job1:
   script: ruby -v
 ```
 
-In this case both, `job1` and `job2` will be executed with `php:5-fpm-alpine`
+In this case both, `job1` and `job2` will be executed with `ruby:2.1`
 
 
 
diff --git a/lib/gitlab/ci/config.rb b/lib/gitlab/ci/config.rb
index 4a0d67720a9..1779fbb2a39 100644
--- a/lib/gitlab/ci/config.rb
+++ b/lib/gitlab/ci/config.rb
@@ -6,7 +6,7 @@ module Gitlab
     class Config
       ConfigError = Class.new(StandardError)
 
-      def initialize(config, project = nil, opts = {})
+      def initialize(config, opts = {})
         @config = Config::Extendable
           .new(build_config(config, opts))
           .to_hash
@@ -64,17 +64,21 @@ module Gitlab
         @global.jobs_value
       end
 
-      # 'opts' argument is used in EE see /ee/lib/ee/gitlab/ci/config.rb
-      def build_config(config, project,  opts = {})
+      def build_config(config, opts = {})
         initial_config = Loader.new(config).load!
+        project = opts.fetch(:project, nil)
 
-        if project.present?
-          processor = ::Gitlab::Ci::ExternalFiles::Processor.new(initial_config, project)
-          processor.perform
+        if project
+          process_external_files(initial_config, project, opts)
         else
           initial_config
         end
       end
+
+      def process_external_files(config, project, opts)
+        branch_name = opts.fetch(:branch_name, project.default_branch)
+        ::Gitlab::Ci::External::Processor.new(config, project, branch_name).perform
+      end
     end
   end
 end
diff --git a/lib/gitlab/ci/external/file/local.rb b/lib/gitlab/ci/external/file/local.rb
new file mode 100644
index 00000000000..dc1ba923411
--- /dev/null
+++ b/lib/gitlab/ci/external/file/local.rb
@@ -0,0 +1,35 @@
+module Gitlab
+  module Ci
+    module External
+      module File
+        class Local
+          attr_reader :value, :project, :branch_name
+
+          def initialize(value, project, branch_name)
+            @value = value
+            @project = project
+            @branch_name = branch_name
+          end
+
+          def valid?
+            commit && local_file_content
+          end
+
+          def content
+            local_file_content
+          end
+
+          private
+
+          def commit
+            @commit ||= project.repository.commit(branch_name)
+          end
+
+          def local_file_content
+            @local_file_content ||= project.repository.blob_data_at(commit.sha, value)
+          end
+        end
+      end
+    end
+  end
+end
diff --git a/lib/gitlab/ci/external/file/remote.rb b/lib/gitlab/ci/external/file/remote.rb
new file mode 100644
index 00000000000..7d8cd1957ad
--- /dev/null
+++ b/lib/gitlab/ci/external/file/remote.rb
@@ -0,0 +1,25 @@
+module Gitlab
+  module Ci
+    module External
+      module File
+        class Remote
+          attr_reader :value
+
+          def initialize(value)
+            @value = value
+          end
+
+          def valid?
+            ::Gitlab::UrlSanitizer.valid?(value) && content
+          end
+
+          def content
+            HTTParty.get(value)
+          rescue HTTParty::Error, Timeout::Error
+            false
+          end
+        end
+      end
+    end
+  end
+end
diff --git a/lib/gitlab/ci/external/mapper.rb b/lib/gitlab/ci/external/mapper.rb
new file mode 100644
index 00000000000..f2e5ec972df
--- /dev/null
+++ b/lib/gitlab/ci/external/mapper.rb
@@ -0,0 +1,31 @@
+module Gitlab
+  module Ci
+    module External
+      class Mapper
+        def initialize(values, project, branch_name)
+          @paths = Array(values.fetch(:include, []))
+          @project = project
+          @branch_name = branch_name
+        end
+
+        def process
+          paths.map { |path| build_external_file(path) }
+        end
+
+        private
+
+        attr_reader :paths, :project, :branch_name
+
+        def build_external_file(path)
+          remote_file = Gitlab::Ci::External::File::Remote.new(path)
+
+          if remote_file.valid?
+            remote_file
+          else
+            ::Gitlab::Ci::External::File::Local.new(path, project, branch_name)
+          end
+        end
+      end
+    end
+  end
+end
diff --git a/lib/gitlab/ci/external/processor.rb b/lib/gitlab/ci/external/processor.rb
new file mode 100644
index 00000000000..5cae0166346
--- /dev/null
+++ b/lib/gitlab/ci/external/processor.rb
@@ -0,0 +1,50 @@
+module Gitlab
+  module Ci
+    module External
+      class Processor
+        FileError = Class.new(StandardError)
+
+        def initialize(values, project, branch_name)
+          @values = values
+          @external_files = ::Gitlab::Ci::External::Mapper.new(values, project, branch_name).process
+          @content = {}
+        end
+
+        def perform
+          return values if external_files.empty?
+
+          external_files.each do |external_file|
+            validate_external_file(external_file)
+            @content.merge!(content_of(external_file))
+          end
+
+          append_external_content
+          remove_include_keyword
+        end
+
+        private
+
+        attr_reader :values, :external_files, :content
+
+        def validate_external_file(external_file)
+          unless external_file.valid?
+            raise FileError, "External file: '#{external_file.value}' should be a valid local or remote file"
+          end
+        end
+
+        def content_of(external_file)
+          ::Gitlab::Ci::Config::Loader.new(external_file.content).load!
+        end
+
+        def append_external_content
+          @content.merge!(@values)
+        end
+
+        def remove_include_keyword
+          content.delete(:include)
+          content
+        end
+      end
+    end
+  end
+end
diff --git a/lib/gitlab/ci/external_files/external_file.rb b/lib/gitlab/ci/external_files/external_file.rb
deleted file mode 100644
index 4fffdc5f716..00000000000
--- a/lib/gitlab/ci/external_files/external_file.rb
+++ /dev/null
@@ -1,42 +0,0 @@
-module Gitlab
-  module Ci
-    module ExternalFiles
-      class ExternalFile
-        attr_reader :value, :project
-
-        def initialize(value, project)
-          @value = value
-          @project = project
-        end
-
-        def content
-          if remote_url?
-            HTTParty.get(value)
-          else
-            local_file_content
-          end
-        rescue HTTParty::Error, Timeout::Error
-          nil
-        end
-
-        def valid?
-          remote_url? || local_file_content
-        end
-
-        private
-
-        def remote_url?
-          ::Gitlab::UrlSanitizer.valid?(value)
-        end
-
-        def local_file_content
-          project.repository.blob_data_at(sha, value)
-        end
-
-        def sha
-          @sha ||= project.repository.commit.sha
-        end
-      end
-    end
-  end
-end
diff --git a/lib/gitlab/ci/external_files/mapper.rb b/lib/gitlab/ci/external_files/mapper.rb
deleted file mode 100644
index fa252cc3b9e..00000000000
--- a/lib/gitlab/ci/external_files/mapper.rb
+++ /dev/null
@@ -1,28 +0,0 @@
-module Gitlab
-  module Ci
-    module ExternalFiles
-      class Mapper
-        def initialize(values, project)
-          @paths = values.fetch(:include, [])
-          @project = project
-        end
-
-        def process
-          if paths.is_a?(String)
-            [build_external_file(paths)]
-          else
-            paths.map { |path| build_external_file(path) }
-          end
-        end
-
-        private
-
-        attr_reader :paths, :project
-
-        def build_external_file(path)
-          ::Gitlab::Ci::ExternalFiles::ExternalFile.new(path, project)
-        end
-      end
-    end
-  end
-end
diff --git a/lib/gitlab/ci/external_files/processor.rb b/lib/gitlab/ci/external_files/processor.rb
deleted file mode 100644
index 5657f025084..00000000000
--- a/lib/gitlab/ci/external_files/processor.rb
+++ /dev/null
@@ -1,45 +0,0 @@
-module Gitlab
-  module Ci
-    module ExternalFiles
-      class Processor
-        ExternalFileError = Class.new(StandardError)
-
-        def initialize(values, project)
-          @values = values
-          @external_files = ::Gitlab::Ci::ExternalFiles::Mapper.new(values, project).process
-        end
-
-        def perform
-          return values if external_files.empty?
-
-          external_files.each do |external_file|
-            validate_external_file(external_file)
-            append_external_content(external_file)
-          end
-
-          remove_include_keyword
-        end
-
-        private
-
-        attr_reader :values, :external_files
-
-        def validate_external_file(external_file)
-          unless external_file.valid?
-            raise ExternalFileError, "External file: '#{external_file.value}' should be a valid local or remote file"
-          end
-        end
-
-        def append_external_content(external_file)
-          external_values = ::Gitlab::Ci::Config::Loader.new(external_file.content).load!
-          @values.merge!(external_values)
-        end
-
-        def remove_include_keyword
-          values.delete(:include)
-          values
-        end
-      end
-    end
-  end
-end
diff --git a/lib/gitlab/ci/yaml_processor.rb b/lib/gitlab/ci/yaml_processor.rb
index 0f79fbede9f..67f5d2d6ae4 100644
--- a/lib/gitlab/ci/yaml_processor.rb
+++ b/lib/gitlab/ci/yaml_processor.rb
@@ -7,8 +7,8 @@ module Gitlab
 
       attr_reader :cache, :stages, :jobs
 
-      def initialize(config, project = nil, opts = {})
-        @ci_config = Gitlab::Ci::Config.new(config, project, opts)
+      def initialize(config, opts = {})
+        @ci_config = Gitlab::Ci::Config.new(config, opts)
         @config = @ci_config.to_hash
 
         unless @ci_config.valid?
@@ -73,11 +73,11 @@ module Gitlab
         end
       end
 
-      def self.validation_message(content, project = nil, opts = {})
+      def self.validation_message(content, opts = {})
         return 'Please provide content of .gitlab-ci.yml' if content.blank?
 
         begin
-          Gitlab::Ci::YamlProcessor.new(content, project, opts)
+          Gitlab::Ci::YamlProcessor.new(content, opts)
           nil
         rescue ValidationError, ::Gitlab::Ci::ExternalFiles::Processor::ExternalFileError => e
           e.message
diff --git a/spec/lib/gitlab/ci/config_spec.rb b/spec/lib/gitlab/ci/config_spec.rb
index fe9d3b3a570..1ac832e2b16 100644
--- a/spec/lib/gitlab/ci/config_spec.rb
+++ b/spec/lib/gitlab/ci/config_spec.rb
@@ -5,11 +5,11 @@ require_dependency 'active_model'
 describe Gitlab::Ci::Config do
   let(:project) { create(:project, :repository) }
   let(:config) do
-    described_class.new(yml, project)
+    described_class.new(gitlab_ci_yml, { project: project, branch_name: 'testing' })
   end
 
   context 'when config is valid' do
-    let(:yml) do
+    let(:gitlab_ci_yml) do
       <<-EOS
         image: ruby:2.2
 
@@ -126,7 +126,7 @@ describe Gitlab::Ci::Config do
     end
   end
 
-  context "when yml has valid 'include' defined" do
+  context "when gitlab_ci_yml has valid 'include' defined" do
     let(:http_file_content) do
       <<~HEREDOC
       variables:
@@ -149,7 +149,8 @@ describe Gitlab::Ci::Config do
     end
 
     before do
-      allow_any_instance_of(::Gitlab::Ci::ExternalFiles::ExternalFile).to receive(:local_file_content).and_return(local_file_content)
+      allow_any_instance_of(::Gitlab::Ci::External::File::Local).to receive(:commit).and_return('12345')
+      allow_any_instance_of(::Gitlab::Ci::External::File::Local).to receive(:local_file_content).and_return(local_file_content)
       allow(HTTParty).to receive(:get).and_return(http_file_content)
     end
 
@@ -178,8 +179,8 @@ describe Gitlab::Ci::Config do
     end
   end
 
-  context "when yml has invalid 'include' defined"  do
-    let(:yml) do
+  context "when gitlab_ci.yml has invalid 'include' defined"  do
+    let(:gitlab_ci_yml) do
       <<-EOS
       include: invalid
       EOS
@@ -193,11 +194,11 @@ describe Gitlab::Ci::Config do
     end
   end
 
-  context "when both external files and gitlab_ci defined the same key" do
-    let(:yml) do
+  context "when both external files and gitlab_ci.yml defined the same key" do
+    let(:gitlab_ci_yml) do
       <<~HEREDOC
         include:
-          - https://gitlab.com/gitlab-org/gitlab-ce/blob/1234/.gitlab-ci-1.yml
+          - https://gitlab.com/gitlab-org/gitlab-ce/blob/1234/.gitlab-ci-1.gitlab_ci_yml
 
         image: ruby:2.2
       HEREDOC
@@ -211,7 +212,7 @@ describe Gitlab::Ci::Config do
 
     it 'should take precedence' do
       allow(HTTParty).to receive(:get).and_return(http_file_content)
-      expect(config.to_hash).to eq({ image: 'php:5-fpm-alpine' })
+      expect(config.to_hash).to eq({ image: 'ruby:2.2' })
     end
   end
 end
diff --git a/spec/lib/gitlab/ci/external/file/local_spec.rb b/spec/lib/gitlab/ci/external/file/local_spec.rb
new file mode 100644
index 00000000000..9e1e49da7cb
--- /dev/null
+++ b/spec/lib/gitlab/ci/external/file/local_spec.rb
@@ -0,0 +1,54 @@
+require 'fast_spec_helper'
+
+describe Gitlab::Ci::External::File::Local do
+  let(:project) { create(:project, :repository) }
+  let(:local_file) { described_class.new(value, project, 'testing') }
+
+  describe "#valid?" do
+    context 'when is a valid local path' do
+      let(:value) { '/vendor/gitlab-ci-yml/existent-file.yml' }
+
+      before do
+        allow_any_instance_of(described_class).to receive(:commit).and_return('12345')
+        allow_any_instance_of(described_class).to receive(:local_file_content).and_return("image: 'ruby2:2'")
+      end
+
+      it 'should return true' do
+        expect(local_file.valid?).to be_truthy
+      end
+    end
+
+    context 'when is not a valid local path' do
+      let(:value) { '/vendor/gitlab-ci-yml/non-existent-file.yml' }
+
+      it 'should return false' do
+        expect(local_file.valid?).to be_falsy
+      end
+    end
+
+    describe "#content" do
+      let(:local_file_content) do
+        <<~HEREDOC
+      before_script:
+        - apt-get update -qq && apt-get install -y -qq sqlite3 libsqlite3-dev nodejs
+        - ruby -v
+        - which ruby
+        - gem install bundler --no-ri --no-rdoc
+        - bundle install --jobs $(nproc)  "${FLAGS[@]}"
+        HEREDOC
+      end
+
+      context 'with a local file' do
+        let(:value) { '/vendor/gitlab-ci-yml/non-existent-file.yml' }
+
+        before do
+          allow_any_instance_of(described_class).to receive(:local_file_content).and_return(local_file_content)
+        end
+
+        it 'should return the content of the file' do
+          expect(local_file.content).to eq(local_file_content)
+        end
+      end
+    end
+  end
+end
diff --git a/spec/lib/gitlab/ci/external/file/remote_spec.rb b/spec/lib/gitlab/ci/external/file/remote_spec.rb
new file mode 100644
index 00000000000..91b50dcf5fd
--- /dev/null
+++ b/spec/lib/gitlab/ci/external/file/remote_spec.rb
@@ -0,0 +1,68 @@
+require 'fast_spec_helper'
+
+describe Gitlab::Ci::External::File::Remote do
+  let(:remote_file) { described_class.new(value) }
+  let(:value) { 'https://gitlab.com/gitlab-org/gitlab-ce/blob/1234/.gitlab-ci-1.yml' }
+  let(:remote_file_content) do
+    <<~HEREDOC
+      before_script:
+        - apt-get update -qq && apt-get install -y -qq sqlite3 libsqlite3-dev nodejs
+        - ruby -v
+        - which ruby
+        - gem install bundler --no-ri --no-rdoc
+        - bundle install --jobs $(nproc)  "${FLAGS[@]}"
+    HEREDOC
+  end
+
+  describe "#valid?" do
+    context 'when is a valid remote url' do
+      before do
+        allow(HTTParty).to receive(:get).and_return(remote_file_content)
+      end
+
+      it 'should return true' do
+        expect(remote_file.valid?).to be_truthy
+      end
+    end
+
+    context 'when is not a valid remote url' do
+      let(:value) { 'not-valid://gitlab.com/gitlab-org/gitlab-ce/blob/1234/.gitlab-ci-1.yml' }
+
+      it 'should return false' do
+        expect(remote_file.valid?).to be_falsy
+      end
+    end
+
+    context 'with a timeout' do
+      before do
+        allow(HTTParty).to receive(:get).and_raise(Timeout::Error)
+      end
+
+      it 'should be falsy' do
+        expect(remote_file.valid?).to be_falsy
+      end
+    end
+  end
+
+  describe "#content" do
+    context 'with a valid remote file' do
+      before do
+        allow(HTTParty).to receive(:get).and_return(remote_file_content)
+      end
+
+      it 'should return the content of the file' do
+        expect(remote_file.content).to eq(remote_file_content)
+      end
+    end
+
+    context 'with a timeout' do
+      before do
+        allow(HTTParty).to receive(:get).and_raise(Timeout::Error)
+      end
+
+      it 'should be falsy' do
+        expect(remote_file.content).to be_falsy
+      end
+    end
+  end
+end
diff --git a/spec/lib/gitlab/ci/external/mapper_spec.rb b/spec/lib/gitlab/ci/external/mapper_spec.rb
new file mode 100644
index 00000000000..c752a23def6
--- /dev/null
+++ b/spec/lib/gitlab/ci/external/mapper_spec.rb
@@ -0,0 +1,92 @@
+require 'fast_spec_helper'
+
+describe Gitlab::Ci::External::Mapper do
+  let(:project) { create(:project, :repository) }
+  let(:file_content) do
+    <<~HEREDOC
+    image: 'ruby:2.2'
+    HEREDOC
+  end
+
+  describe '#process' do
+    subject { described_class.new(values, project, 'testing').process }
+
+    context "when 'include' keyword is defined as string" do
+      context 'when the string is a local file' do
+        let(:values) do
+          {
+            include: '/vendor/gitlab-ci-yml/non-existent-file.yml',
+            image: 'ruby:2.2'
+          }
+        end
+
+        it 'returns an array' do
+          expect(subject).to be_an(Array)
+        end
+
+        it 'returns File instances' do
+          expect(subject.first).to be_an_instance_of(::Gitlab::Ci::External::File::Local)
+        end
+      end
+
+      context 'when the string is a remote file' do
+        let(:values) do
+          {
+            include: 'https://gitlab.com/gitlab-org/gitlab-ce/blob/1234/.gitlab-ci-1.yml',
+            image: 'ruby:2.2'
+          }
+        end
+
+        before do
+          allow(HTTParty).to receive(:get).and_return(file_content)
+        end
+
+        it 'returns an array' do
+          expect(subject).to be_an(Array)
+        end
+
+        it 'returns File instances' do
+          expect(subject.first).to be_an_instance_of(::Gitlab::Ci::External::File::Remote)
+        end
+      end
+    end
+
+    context "when 'include' is defined as an array" do
+      let(:values) do
+        {
+          include:
+          [
+            'https://gitlab.com/gitlab-org/gitlab-ce/blob/1234/.gitlab-ci-1.yml',
+            '/vendor/gitlab-ci-yml/template.yml'
+          ],
+          image: 'ruby:2.2'
+        }
+      end
+
+      before do
+        allow(HTTParty).to receive(:get).and_return(file_content)
+      end
+
+      it 'returns an array' do
+        expect(subject).to be_an(Array)
+      end
+
+      it 'returns Files instances' do
+        expect(subject).to all(respond_to(:valid?))
+        expect(subject).to all(respond_to(:content))
+      end
+    end
+
+    context "when 'include' is not defined" do
+      let(:values) do
+        {
+          image: 'ruby:2.2'
+        }
+      end
+
+      it 'returns an empty array' do
+        expect(subject).to be_empty
+      end
+    end
+  end
+end
diff --git a/spec/lib/gitlab/ci/external_files/processor_spec.rb b/spec/lib/gitlab/ci/external/processor_spec.rb
similarity index 69%
rename from spec/lib/gitlab/ci/external_files/processor_spec.rb
rename to spec/lib/gitlab/ci/external/processor_spec.rb
index 36eec0b90d3..ba9e106706e 100644
--- a/spec/lib/gitlab/ci/external_files/processor_spec.rb
+++ b/spec/lib/gitlab/ci/external/processor_spec.rb
@@ -1,8 +1,8 @@
 require 'fast_spec_helper'
 
-describe Gitlab::Ci::ExternalFiles::Processor do
+describe Gitlab::Ci::External::Processor do
   let(:project) { create(:project, :repository) }
-  let(:processor) { described_class.new(values, project) }
+  let(:processor) { described_class.new(values, project, 'testing') }
 
   describe "#perform" do
     context 'when no external files defined' do
@@ -18,7 +18,7 @@ describe Gitlab::Ci::ExternalFiles::Processor do
 
       it 'should raise an error' do
         expect { processor.perform }.to raise_error(
-          described_class::ExternalFileError,
+          described_class::FileError,
           "External file: '/vendor/gitlab-ci-yml/non-existent-file.yml' should be a valid local or remote file"
         )
       end
@@ -29,7 +29,7 @@ describe Gitlab::Ci::ExternalFiles::Processor do
 
       it 'should raise an error' do
         expect { processor.perform }.to raise_error(
-          described_class::ExternalFileError,
+          described_class::FileError,
           "External file: 'not-valid://gitlab.com/gitlab-org/gitlab-ce/blob/1234/.gitlab-ci-1.yml' should be a valid local or remote file"
         )
       end
@@ -72,7 +72,7 @@ describe Gitlab::Ci::ExternalFiles::Processor do
 
     context 'with a valid local external file is defined' do
       let(:values) { { include: '/vendor/gitlab-ci-yml/template.yml', image: 'ruby:2.2' } }
-      let(:external_file_content) do
+      let(:local_file_content) do
         <<-HEREDOC
         before_script:
           - apt-get update -qq && apt-get install -y -qq sqlite3 libsqlite3-dev nodejs
@@ -84,7 +84,8 @@ describe Gitlab::Ci::ExternalFiles::Processor do
       end
 
       before do
-        allow_any_instance_of(::Gitlab::Ci::ExternalFiles::ExternalFile).to receive(:local_file_content).and_return(external_file_content)
+        allow_any_instance_of(Gitlab::Ci::External::File::Local).to receive(:commit).and_return('12345')
+        allow_any_instance_of(Gitlab::Ci::External::File::Local).to receive(:local_file_content).and_return(local_file_content)
       end
 
       it 'should append the file to the values' do
@@ -93,7 +94,7 @@ describe Gitlab::Ci::ExternalFiles::Processor do
       end
 
       it "should remove the 'include' keyword" do
-        expect(processor.perform[:includes]).to be_nil
+        expect(processor.perform[:include]).to be_nil
       end
     end
 
@@ -104,7 +105,12 @@ describe Gitlab::Ci::ExternalFiles::Processor do
           'https://gitlab.com/gitlab-org/gitlab-ce/blob/1234/.gitlab-ci-1.yml'
         ]
       end
-      let(:values) { { include: external_files, image: 'ruby:2.2' } }
+      let(:values) do
+        {
+          include: external_files,
+          image: 'ruby:2.2'
+        }
+      end
 
       let(:remote_file_content) do
         <<-HEREDOC
@@ -116,8 +122,9 @@ describe Gitlab::Ci::ExternalFiles::Processor do
       end
 
       before do
-        file_content = File.read("#{Rails.root}/spec/ee/fixtures/gitlab/ci/external_files/.gitlab-ci-template-1.yml")
-        allow_any_instance_of(::Gitlab::Ci::ExternalFiles::ExternalFile).to receive(:local_file_content).and_return(file_content)
+        local_file_content = File.read("#{Rails.root}/spec/ee/fixtures/gitlab/ci/external_files/.gitlab-ci-template-1.yml")
+        allow_any_instance_of(Gitlab::Ci::External::File::Local).to receive(:commit).and_return('12345')
+        allow_any_instance_of(Gitlab::Ci::External::File::Local).to receive(:local_file_content).and_return(local_file_content)
         allow(HTTParty).to receive(:get).and_return(remote_file_content)
       end
 
@@ -133,15 +140,36 @@ describe Gitlab::Ci::ExternalFiles::Processor do
     context 'when external files are defined but not valid' do
       let(:values) { { include: '/vendor/gitlab-ci-yml/template.yml', image: 'ruby:2.2' } }
 
-      let(:external_file_content) { 'invalid content file ////' }
+      let(:local_file_content) { 'invalid content file ////' }
 
       before do
-        allow_any_instance_of(::Gitlab::Ci::ExternalFiles::ExternalFile).to receive(:local_file_content).and_return(external_file_content)
+        allow_any_instance_of(Gitlab::Ci::External::File::Local).to receive(:commit).and_return('12345')
+        allow_any_instance_of(Gitlab::Ci::External::File::Local).to receive(:local_file_content).and_return(local_file_content)
       end
 
       it 'should raise an error' do
         expect { processor.perform }.to raise_error(Gitlab::Ci::Config::Loader::FormatError)
       end
     end
+
+    context "when both external files and values defined the same key" do
+      let(:values) do
+        {
+          include: 'https://gitlab.com/gitlab-org/gitlab-ce/blob/1234/.gitlab-ci-1.yml',
+          image: 'ruby:2.2'
+        }
+      end
+
+      let(:remote_file_content) do
+        <<~HEREDOC
+        image: php:5-fpm-alpine
+        HEREDOC
+      end
+
+      it 'should take precedence' do
+        allow(HTTParty).to receive(:get).and_return(remote_file_content)
+        expect(processor.perform[:image]).to eq('ruby:2.2')
+      end
+    end
   end
 end
diff --git a/spec/lib/gitlab/ci/external_files/external_file_spec.rb b/spec/lib/gitlab/ci/external_files/external_file_spec.rb
deleted file mode 100644
index b149b4200d8..00000000000
--- a/spec/lib/gitlab/ci/external_files/external_file_spec.rb
+++ /dev/null
@@ -1,94 +0,0 @@
-require 'fast_spec_helper'
-
-describe Gitlab::Ci::ExternalFiles::ExternalFile do
-  let(:project) { create(:project, :repository) }
-  let(:external_file) { described_class.new(value, project) }
-
-  describe "#valid?" do
-    context 'when is a valid remote url' do
-      let(:value) { 'https://gitlab.com/gitlab-org/gitlab-ce/blob/1234/.gitlab-ci-1.yml' }
-
-      before do
-        allow_any_instance_of(described_class).to receive(:local_file_content).and_return("image: 'ruby2:2'")
-      end
-
-      it 'should return true' do
-        expect(external_file.valid?).to be_truthy
-      end
-    end
-
-    context 'when is not a valid remote url' do
-      let(:value) { 'not-valid://gitlab.com/gitlab-org/gitlab-ce/blob/1234/.gitlab-ci-1.yml' }
-
-      it 'should return false' do
-        expect(external_file.valid?).to be_falsy
-      end
-    end
-
-    context 'when is a valid local path' do
-      let(:value) { '/vendor/gitlab-ci-yml/existent-file.yml' }
-
-      it 'should return true' do
-        allow(File).to receive(:exists?).and_return(true)
-        expect(external_file.valid?).to be_truthy
-      end
-    end
-
-    context 'when is not a valid local path' do
-      let(:value) { '/vendor/gitlab-ci-yml/non-existent-file.yml' }
-
-      it 'should return false' do
-        expect(external_file.valid?).to be_falsy
-      end
-    end
-  end
-
-  describe "#content" do
-    let(:external_file_content) do
-      <<-HEREDOC
-        before_script:
-          - apt-get update -qq && apt-get install -y -qq sqlite3 libsqlite3-dev nodejs
-          - ruby -v
-          - which ruby
-          - gem install bundler --no-ri --no-rdoc
-          - bundle install --jobs $(nproc)  "${FLAGS[@]}"
-      HEREDOC
-    end
-
-    context 'with a local file' do
-      let(:value) { '/vendor/gitlab-ci-yml/non-existent-file.yml' }
-
-      before do
-        allow_any_instance_of(described_class).to receive(:local_file_content).and_return(external_file_content)
-      end
-
-      it 'should return the content of the file' do
-        expect(external_file.content).to eq(external_file_content)
-      end
-    end
-
-    context 'with a valid remote file' do
-      let(:value) { 'https://gitlab.com/gitlab-org/gitlab-ce/blob/1234/.gitlab-ci-1.yml' }
-
-      before do
-        allow(HTTParty).to receive(:get).and_return(external_file_content)
-      end
-
-      it 'should return the content of the file' do
-        expect(external_file.content).to eq(external_file_content)
-      end
-    end
-
-    context 'with a timeout' do
-      let(:value) { 'https://gitlab.com/gitlab-org/gitlab-ce/blob/1234/.gitlab-ci-1.yml' }
-
-      before do
-        allow(HTTParty).to receive(:get).and_raise(Timeout::Error)
-      end
-
-      it 'should return nil' do
-        expect(external_file.content).to be_nil
-      end
-    end
-  end
-end
diff --git a/spec/lib/gitlab/ci/external_files/mapper_spec.rb b/spec/lib/gitlab/ci/external_files/mapper_spec.rb
deleted file mode 100644
index 84957c1f6b4..00000000000
--- a/spec/lib/gitlab/ci/external_files/mapper_spec.rb
+++ /dev/null
@@ -1,57 +0,0 @@
-require 'fast_spec_helper'
-
-describe Gitlab::Ci::ExternalFiles::Mapper do
-  let(:project) { create(:project, :repository) }
-
-  describe '#process' do
-    subject { described_class.new(values, project).process }
-
-    context "when 'include' keyword is defined as string" do
-      let(:values) do
-        {
-          include: '/vendor/gitlab-ci-yml/non-existent-file.yml',
-          image: 'ruby:2.2'
-        }
-      end
-
-      it 'returns an array' do
-        expect(subject).to be_an(Array)
-      end
-
-      it 'returns ExternalFile instances' do
-        expect(subject.first).to be_an_instance_of(::Gitlab::Ci::ExternalFiles::ExternalFile)
-      end
-    end
-
-    context "when 'include' is defined as an array" do
-      let(:values) do
-        {
-          include:
-          [
-            'https://gitlab.com/gitlab-org/gitlab-ce/blob/1234/.gitlab-ci-1.yml',
-            '/vendor/gitlab-ci-yml/template.yml'
-          ],
-          image: 'ruby:2.2'
-        }
-      end
-
-      it 'returns an array' do
-        expect(subject).to be_an(Array)
-      end
-
-      it 'returns ExternalFile instances' do
-        expect(subject).to all(be_an_instance_of(::Gitlab::Ci::ExternalFiles::ExternalFile))
-      end
-    end
-
-    context "when 'include' is not defined" do
-      let(:values) do
-        { image: 'ruby:2.2' }
-      end
-
-      it 'returns an empty array' do
-        expect(subject).to be_empty
-      end
-    end
-  end
-end
diff --git a/spec/models/blob_viewer/gitlab_ci_yml_spec.rb b/spec/models/blob_viewer/gitlab_ci_yml_spec.rb
index bed364a8c14..4c8a5aae83a 100644
--- a/spec/models/blob_viewer/gitlab_ci_yml_spec.rb
+++ b/spec/models/blob_viewer/gitlab_ci_yml_spec.rb
@@ -12,12 +12,12 @@ describe BlobViewer::GitlabCiYml do
     it 'calls prepare! on the viewer' do
       expect(subject).to receive(:prepare!)
 
-      subject.validation_message
+      subject.validation_message(project, project.default_branch)
     end
 
     context 'when the configuration is valid' do
       it 'returns nil' do
-        expect(subject.validation_message).to be_nil
+        expect(subject.validation_message(project, project.default_branch)).to be_nil
       end
     end
 
@@ -25,7 +25,7 @@ describe BlobViewer::GitlabCiYml do
       let(:data) { 'oof' }
 
       it 'returns the error message' do
-        expect(subject.validation_message).to eq('Invalid configuration format')
+        expect(subject.validation_message(project, project.default_branch)).to eq('Invalid configuration format')
       end
     end
   end
diff --git a/spec/models/ci/pipeline_spec.rb b/spec/models/ci/pipeline_spec.rb
index 14ccc2960bb..2216705c032 100644
--- a/spec/models/ci/pipeline_spec.rb
+++ b/spec/models/ci/pipeline_spec.rb
@@ -1743,7 +1743,7 @@ describe Ci::Pipeline, :mailer do
         create(:ci_pipeline, config: { rspec: { script: 'rake test' } })
       end
 
-      it 'does not containyaml errors' do
+      it 'does not contain yaml errors' do
         expect(pipeline).not_to have_yaml_errors
       end
     end