Address MR comments

CE mirror of 1269dc47b7f8d1a9913de326c9bd356d3e603663

8 files changed, 67 insertions, 48 deletions

diff --git a/lib/gitlab/ci/config.rb b/lib/gitlab/ci/config.rb
index caa3a7c3c86..4a0d67720a9 100644
--- a/lib/gitlab/ci/config.rb
+++ b/lib/gitlab/ci/config.rb
@@ -7,17 +7,10 @@ module Gitlab
       ConfigError = Class.new(StandardError)
 
       def initialize(config, project = nil, opts = {})
-        initial_config = Config::Extendable
+        @config = Config::Extendable
           .new(build_config(config, opts))
           .to_hash
 
-        if project.present?
-          processor = ::Gitlab::Ci::ExternalFiles::Processor.new(initial_config, project)
-          @config = processor.perform
-        else
-          @config = initial_config
-        end
-
         @global = Entry::Global.new(@config)
         @global.compose!
       rescue Loader::FormatError, Extendable::ExtensionError => e
@@ -72,8 +65,15 @@ module Gitlab
       end
 
       # 'opts' argument is used in EE see /ee/lib/ee/gitlab/ci/config.rb
-      def build_config(config, opts = {})
-        Loader.new(config).load!
+      def build_config(config, project,  opts = {})
+        initial_config = Loader.new(config).load!
+
+        if project.present?
+          processor = ::Gitlab::Ci::ExternalFiles::Processor.new(initial_config, project)
+          processor.perform
+        else
+          initial_config
+        end
       end
     end
   end
diff --git a/lib/gitlab/ci/external_files/external_file.rb b/lib/gitlab/ci/external_files/external_file.rb
index 45c6ca3fbfe..f481e9b0a39 100644
--- a/lib/gitlab/ci/external_files/external_file.rb
+++ b/lib/gitlab/ci/external_files/external_file.rb
@@ -4,6 +4,8 @@ module Gitlab
   module Ci
     module ExternalFiles
       class ExternalFile
+        attr_reader :value, :project
+
         def initialize(value, project)
           @value = value
           @project = project
@@ -11,10 +13,12 @@ module Gitlab
 
         def content
           if remote_url?
-            open(value).read
+            HTTParty.get(value)
           else
             local_file_content
           end
+        rescue HTTParty::Error, Timeout::Error
+          nil
         end
 
         def valid?
@@ -23,8 +27,6 @@ module Gitlab
 
         private
 
-        attr_reader :value, :project
-
         def remote_url?
           ::Gitlab::UrlSanitizer.valid?(value)
         end
diff --git a/lib/gitlab/ci/external_files/mapper.rb b/lib/gitlab/ci/external_files/mapper.rb
index 2d40e296546..fa252cc3b9e 100644
--- a/lib/gitlab/ci/external_files/mapper.rb
+++ b/lib/gitlab/ci/external_files/mapper.rb
@@ -3,7 +3,7 @@ module Gitlab
     module ExternalFiles
       class Mapper
         def initialize(values, project)
-          @paths = values.fetch(:includes, [])
+          @paths = values.fetch(:include, [])
           @project = project
         end
 
@@ -17,7 +17,7 @@ module Gitlab
 
         private
 
-        attr_reaer :paths, :project
+        attr_reader :paths, :project
 
         def build_external_file(path)
           ::Gitlab::Ci::ExternalFiles::ExternalFile.new(path, project)
diff --git a/lib/gitlab/ci/external_files/processor.rb b/lib/gitlab/ci/external_files/processor.rb
index ac51db05d8e..5657f025084 100644
--- a/lib/gitlab/ci/external_files/processor.rb
+++ b/lib/gitlab/ci/external_files/processor.rb
@@ -6,7 +6,7 @@ module Gitlab
 
         def initialize(values, project)
           @values = values
-          @external_files = ::Gitlab::Ci::ExternalFiles::Mapper.fetch_paths(values, project).process
+          @external_files = ::Gitlab::Ci::ExternalFiles::Mapper.new(values, project).process
         end
 
         def perform
@@ -26,7 +26,7 @@ module Gitlab
 
         def validate_external_file(external_file)
           unless external_file.valid?
-            raise ExternalFileError, 'External files should be a valid local or remote file'
+            raise ExternalFileError, "External file: '#{external_file.value}' should be a valid local or remote file"
           end
         end
 
@@ -36,7 +36,7 @@ module Gitlab
         end
 
         def remove_include_keyword
-          values.delete(:includes)
+          values.delete(:include)
           values
         end
       end
diff --git a/spec/lib/gitlab/ci/config_spec.rb b/spec/lib/gitlab/ci/config_spec.rb
index b1c801ff052..6e1edf76c19 100644
--- a/spec/lib/gitlab/ci/config_spec.rb
+++ b/spec/lib/gitlab/ci/config_spec.rb
@@ -126,7 +126,7 @@ describe Gitlab::Ci::Config do
     end
   end
 
-  context "when yml has valid 'includes' defined" do
+  context "when yml has valid 'include' defined" do
     let(:http_file_content) do
       <<~HEREDOC
       variables:
@@ -140,9 +140,8 @@ describe Gitlab::Ci::Config do
     let(:local_file_content) {  File.read("#{Rails.root}/spec/ee/fixtures/gitlab/ci/external_files/.gitlab-ci-template-1.yml") }
     let(:yml) do
       <<-EOS
-        includes:
+        include:
           - /spec/fixtures/gitlab/ci/external_files/.gitlab-ci-template-1.yml
-          - /spec/fixtures/gitlab/ci/external_files/.gitlab-ci-template-2.yml
           - https://gitlab.com/gitlab-org/gitlab-ce/blob/1234/.gitlab-ci-1.yml
 
         image: ruby:2.2
@@ -151,7 +150,7 @@ describe Gitlab::Ci::Config do
 
     before do
       allow_any_instance_of(::Gitlab::Ci::ExternalFiles::ExternalFile).to receive(:local_file_content).and_return(local_file_content)
-      allow_any_instance_of(Kernel).to receive_message_chain(:open, :read).and_return(http_file_content)
+      allow(HTTParty).to receive(:get).and_return(http_file_content)
     end
 
     it 'should return a composed hash' do
@@ -179,17 +178,17 @@ describe Gitlab::Ci::Config do
     end
   end
 
-  context "when yml has invalid 'includes' defined"  do
+  context "when yml has invalid 'include' defined"  do
     let(:yml) do
       <<-EOS
-      includes: invalid
+      include: invalid
       EOS
     end
 
-    it 'raises error' do
+    it 'raises error YamlProcessor ValidationError' do
       expect { config }.to raise_error(
-        ::Gitlab::Ci::ExternalFiles::Processor::ExternalFileError,
-        /External files should be a valid local or remote file/
+        ::Gitlab::Ci::YamlProcessor::ValidationError,
+        "External file: 'invalid' should be a valid local or remote file"
       )
     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
index b2aeb0ac67a..822ed2970bf 100644
--- a/spec/lib/gitlab/ci/external_files/external_file_spec.rb
+++ b/spec/lib/gitlab/ci/external_files/external_file_spec.rb
@@ -71,12 +71,24 @@ describe Gitlab::Ci::ExternalFiles::ExternalFile do
       let(:value) { 'https://gitlab.com/gitlab-org/gitlab-ce/blob/1234/.gitlab-ci-1.yml' }
 
       before do
-        allow_any_instance_of(Kernel).to receive_message_chain(:open, :read).and_return(external_file_content)
+        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
index 4bdb6c200c4..ab1bc242c19 100644
--- a/spec/lib/gitlab/ci/external_files/mapper_spec.rb
+++ b/spec/lib/gitlab/ci/external_files/mapper_spec.rb
@@ -6,10 +6,10 @@ describe Gitlab::Ci::ExternalFiles::Mapper do
   describe '#process' do
     subject { described_class.new(values, project).process }
 
-    context 'when includes keyword is defined as string' do
+    context "when 'include' keyword is defined as string" do
       let(:values) do
         {
-          includes: '/vendor/gitlab-ci-yml/non-existent-file.yml',
+          include: '/vendor/gitlab-ci-yml/non-existent-file.yml',
           image: 'ruby:2.2'
         }
       end
@@ -23,10 +23,10 @@ describe Gitlab::Ci::ExternalFiles::Mapper do
       end
     end
 
-    context 'when includes is defined as an array' do
+    context "when 'include' is defined as an array" do
       let(:values) do
         {
-          includes:
+          include:
           [
             'https://gitlab.com/gitlab-org/gitlab-ce/blob/1234/.gitlab-ci-1.yml',
             '/vendor/gitlab-ci-yml/template.yml'
@@ -44,7 +44,7 @@ describe Gitlab::Ci::ExternalFiles::Mapper do
       end
     end
 
-    context 'when includes is not defined' do
+    context "when 'include' is not defined" do
       let(:values) do
         { image: 'ruby:2.2' }
       end
diff --git a/spec/lib/gitlab/ci/external_files/processor_spec.rb b/spec/lib/gitlab/ci/external_files/processor_spec.rb
index f009ece8821..a3db566f428 100644
--- a/spec/lib/gitlab/ci/external_files/processor_spec.rb
+++ b/spec/lib/gitlab/ci/external_files/processor_spec.rb
@@ -14,23 +14,29 @@ describe Gitlab::Ci::ExternalFiles::Processor do
     end
 
     context 'when an invalid local file is defined' do
-      let(:values) { { includes: '/vendor/gitlab-ci-yml/non-existent-file.yml', image: 'ruby:2.2' } }
+      let(:values) { { include: '/vendor/gitlab-ci-yml/non-existent-file.yml', image: 'ruby:2.2' } }
 
       it 'should raise an error' do
-        expect { processor.perform }.to raise_error(described_class::ExternalFileError)
+        expect { processor.perform }.to raise_error(
+          described_class::ExternalFileError,
+          "External file: '/vendor/gitlab-ci-yml/non-existent-file.yml' should be a valid local or remote file"
+        )
       end
     end
 
     context 'when an invalid remote file is defined' do
-      let(:values) { { includes: 'not-valid://gitlab.com/gitlab-org/gitlab-ce/blob/1234/.gitlab-ci-1.yml', image: 'ruby:2.2' } }
+      let(:values) { { include: 'not-valid://gitlab.com/gitlab-org/gitlab-ce/blob/1234/.gitlab-ci-1.yml', image: 'ruby:2.2' } }
 
       it 'should raise an error' do
-        expect { processor.perform }.to raise_error(described_class::ExternalFileError)
+        expect { processor.perform }.to raise_error(
+          described_class::ExternalFileError,
+          "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
     end
 
     context 'with a valid remote external file is defined' do
-      let(:values) { { includes: 'https://gitlab.com/gitlab-org/gitlab-ce/blob/1234/.gitlab-ci-1.yml', image: 'ruby:2.2' } }
+      let(:values) { { include: 'https://gitlab.com/gitlab-org/gitlab-ce/blob/1234/.gitlab-ci-1.yml', image: 'ruby:2.2' } }
       let(:external_file_content) do
         <<-HEREDOC
         before_script:
@@ -51,7 +57,7 @@ describe Gitlab::Ci::ExternalFiles::Processor do
       end
 
       before do
-        allow_any_instance_of(Kernel).to receive_message_chain(:open, :read).and_return(external_file_content)
+        allow(HTTParty).to receive(:get).and_return(external_file_content)
       end
 
       it 'should append the file to the values' do
@@ -59,13 +65,13 @@ describe Gitlab::Ci::ExternalFiles::Processor do
         expect(output.keys).to match_array([:image, :before_script, :rspec, :rubocop])
       end
 
-      it "should remove the 'includes' keyword" do
-        expect(processor.perform[:includes]).to be_nil
+      it "should remove the 'include' keyword" do
+        expect(processor.perform[:include]).to be_nil
       end
     end
 
     context 'with a valid local external file is defined' do
-      let(:values) { { includes: '/vendor/gitlab-ci-yml/template.yml', image: 'ruby:2.2' } }
+      let(:values) { { include: '/vendor/gitlab-ci-yml/template.yml', image: 'ruby:2.2' } }
       let(:external_file_content) do
         <<-HEREDOC
         before_script:
@@ -86,7 +92,7 @@ describe Gitlab::Ci::ExternalFiles::Processor do
         expect(output.keys).to match_array([:image, :before_script])
       end
 
-      it "should remove the 'includes' keyword" do
+      it "should remove the 'include' keyword" do
         expect(processor.perform[:includes]).to be_nil
       end
     end
@@ -98,7 +104,7 @@ describe Gitlab::Ci::ExternalFiles::Processor do
           'https://gitlab.com/gitlab-org/gitlab-ce/blob/1234/.gitlab-ci-1.yml'
         ]
       end
-      let(:values) { { includes: external_files, image: 'ruby:2.2' } }
+      let(:values) { { include: external_files, image: 'ruby:2.2' } }
 
       let(:remote_file_content) do
         <<-HEREDOC
@@ -112,20 +118,20 @@ describe Gitlab::Ci::ExternalFiles::Processor do
       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)
-        allow_any_instance_of(Kernel).to receive_message_chain(:open, :read).and_return(remote_file_content)
+        allow(HTTParty).to receive(:get).and_return(remote_file_content)
       end
 
       it 'should append the files to the values' do
         expect(processor.perform.keys).to match_array([:image, :stages, :before_script, :rspec])
       end
 
-      it "should remove the 'includes' keyword" do
-        expect(processor.perform[:includes]).to be_nil
+      it "should remove the 'include' keyword" do
+        expect(processor.perform[:include]).to be_nil
       end
     end
 
     context 'when external files are defined but not valid' do
-      let(:values) { { includes: '/vendor/gitlab-ci-yml/template.yml', image: 'ruby:2.2' } }
+      let(:values) { { include: '/vendor/gitlab-ci-yml/template.yml', image: 'ruby:2.2' } }
 
       let(:external_file_content) { 'invalid content file ////' }