Merge branch 'feature/gb/improve-include-config-errors-reporting' into 'master'

Improve validation errors for external CI/CD configuration

Closes #51369

See merge request gitlab-org/gitlab-ce!22394

6 files changed, 126 insertions, 23 deletions

diff --git a/spec/lib/gitlab/ci/config/external/file/base_spec.rb b/spec/lib/gitlab/ci/config/external/file/base_spec.rb
new file mode 100644
index 00000000000..2e92d5204d6
--- /dev/null
+++ b/spec/lib/gitlab/ci/config/external/file/base_spec.rb
@@ -0,0 +1,52 @@
+# frozen_string_literal: true
+
+require 'fast_spec_helper'
+
+describe Gitlab::Ci::Config::External::File::Base do
+  subject { described_class.new(location) }
+
+  before do
+    allow_any_instance_of(described_class)
+      .to receive(:content).and_return('key: value')
+  end
+
+  describe '#valid?' do
+    context 'when location is not a YAML file' do
+      let(:location) { 'some/file.txt' }
+
+      it { is_expected.not_to be_valid }
+    end
+
+    context 'when location has not a valid naming scheme' do
+      let(:location) { 'some/file/.yml' }
+
+      it { is_expected.not_to be_valid }
+    end
+
+    context 'when location is a valid .yml extension' do
+      let(:location) { 'some/file/config.yml' }
+
+      it { is_expected.to be_valid }
+    end
+
+    context 'when location is a valid .yaml extension' do
+      let(:location) { 'some/file/config.yaml' }
+
+      it { is_expected.to be_valid }
+    end
+
+    context 'when there are YAML syntax errors' do
+      let(:location) { 'some/file/config.yml' }
+
+      before do
+        allow_any_instance_of(described_class)
+          .to receive(:content).and_return('invalid_syntax')
+      end
+
+      it 'is not a valid file' do
+        expect(subject).not_to be_valid
+        expect(subject.error_message).to match /does not have valid YAML syntax/
+      end
+    end
+  end
+end
diff --git a/spec/lib/gitlab/ci/external/file/local_spec.rb b/spec/lib/gitlab/ci/config/external/file/local_spec.rb
index 73bb4ccf468..2708d8d5b6b 100644
--- a/spec/lib/gitlab/ci/external/file/local_spec.rb
+++ b/spec/lib/gitlab/ci/config/external/file/local_spec.rb
@@ -2,7 +2,7 @@
 
 require 'spec_helper'
 
-describe Gitlab::Ci::External::File::Local do
+describe Gitlab::Ci::Config::External::File::Local do
   let(:project) { create(:project, :repository) }
   let(:local_file) { described_class.new(location, { project: project, sha: '12345' }) }
 
@@ -72,7 +72,7 @@ describe Gitlab::Ci::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
diff --git a/spec/lib/gitlab/ci/external/file/remote_spec.rb b/spec/lib/gitlab/ci/config/external/file/remote_spec.rb
index b1819c8960b..7c1a1c38736 100644
--- a/spec/lib/gitlab/ci/external/file/remote_spec.rb
+++ b/spec/lib/gitlab/ci/config/external/file/remote_spec.rb
@@ -2,7 +2,7 @@
 
 require 'spec_helper'
 
-describe Gitlab::Ci::External::File::Remote do
+describe Gitlab::Ci::Config::External::File::Remote do
   let(:remote_file) { described_class.new(location) }
   let(:location) { 'https://gitlab.com/gitlab-org/gitlab-ce/blob/1234/.gitlab-ci-1.yml' }
   let(:remote_file_content) do
@@ -105,10 +105,53 @@ describe Gitlab::Ci::External::File::Remote do
   end
 
   describe "#error_message" do
-    let(:location) { 'not-valid://gitlab.com/gitlab-org/gitlab-ce/blob/1234/.gitlab-ci-1.yml' }
+    subject { remote_file.error_message }
 
-    it 'should return an error message' do
-      expect(remote_file.error_message).to eq("Remote file '#{location}' is not valid.")
+    context 'when remote file location is not valid' do
+      let(:location) { 'not-valid://gitlab.com/gitlab-org/gitlab-ce/blob/1234/.gitlab-ci-1.yml' }
+
+      it 'returns an error message describing invalid address' do
+        expect(subject).to match /does not have a valid address!/
+      end
+    end
+
+    context 'when timeout error has been raised' do
+      before do
+        WebMock.stub_request(:get, location).to_timeout
+      end
+
+      it 'should returns error message about a timeout' do
+        expect(subject).to match /could not be fetched because of a timeout error!/
+      end
+    end
+
+    context 'when HTTP error has been raised' do
+      before do
+        WebMock.stub_request(:get, location).to_raise(Gitlab::HTTP::Error)
+      end
+
+      it 'should returns error message about a HTTP error' do
+        expect(subject).to match /could not be fetched because of HTTP error!/
+      end
+    end
+
+    context 'when response has 404 status' do
+      before do
+        WebMock.stub_request(:get, location).to_return(body: remote_file_content, status: 404)
+      end
+
+      it 'should returns error message about a timeout' do
+        expect(subject).to match /could not be fetched because of HTTP code `404` error!/
+      end
+    end
+
+    context 'when the URL is blocked' do
+      let(:location) { 'http://127.0.0.1/some/path/to/config.yaml' }
+
+      it 'should include details about blocked URL' do
+        expect(subject).to eq "Remote file could not be fetched because URL '#{location}' " \
+                              'is blocked: Requests to localhost are not allowed!'
+      end
     end
   end
 end
diff --git a/spec/lib/gitlab/ci/external/mapper_spec.rb b/spec/lib/gitlab/ci/config/external/mapper_spec.rb
index d925d6af73d..5b236fe99f1 100644
--- a/spec/lib/gitlab/ci/external/mapper_spec.rb
+++ b/spec/lib/gitlab/ci/config/external/mapper_spec.rb
@@ -2,7 +2,7 @@
 
 require 'spec_helper'
 
-describe Gitlab::Ci::External::Mapper do
+describe Gitlab::Ci::Config::External::Mapper do
   let(:project) { create(:project, :repository) }
   let(:file_content) do
     <<~HEREDOC
@@ -27,7 +27,8 @@ describe Gitlab::Ci::External::Mapper do
         end
 
         it 'returns File instances' do
-          expect(subject.first).to be_an_instance_of(Gitlab::Ci::External::File::Local)
+          expect(subject.first)
+            .to be_an_instance_of(Gitlab::Ci::Config::External::File::Local)
         end
       end
 
@@ -49,7 +50,8 @@ describe Gitlab::Ci::External::Mapper do
         end
 
         it 'returns File instances' do
-          expect(subject.first).to be_an_instance_of(Gitlab::Ci::External::File::Remote)
+          expect(subject.first)
+            .to be_an_instance_of(Gitlab::Ci::Config::External::File::Remote)
         end
       end
     end
diff --git a/spec/lib/gitlab/ci/external/processor_spec.rb b/spec/lib/gitlab/ci/config/external/processor_spec.rb
index 3c7394f53d2..1a05f716247 100644
--- a/spec/lib/gitlab/ci/external/processor_spec.rb
+++ b/spec/lib/gitlab/ci/config/external/processor_spec.rb
@@ -2,7 +2,7 @@
 
 require 'spec_helper'
 
-describe Gitlab::Ci::External::Processor do
+describe Gitlab::Ci::Config::External::Processor do
   let(:project) { create(:project, :repository) }
   let(:processor) { described_class.new(values, project, '12345') }
 
@@ -20,8 +20,8 @@ describe Gitlab::Ci::External::Processor do
 
       it 'should raise an error' do
         expect { processor.perform }.to raise_error(
-          described_class::FileError,
-          "Local file '/lib/gitlab/ci/templates/non-existent-file.yml' is not valid."
+          described_class::IncludeError,
+          "Local file `/lib/gitlab/ci/templates/non-existent-file.yml` does not exist!"
         )
       end
     end
@@ -36,8 +36,8 @@ describe Gitlab::Ci::External::Processor do
 
       it 'should raise an error' do
         expect { processor.perform }.to raise_error(
-          described_class::FileError,
-          "Remote file '#{remote_file}' is not valid."
+          described_class::IncludeError,
+          "Remote file `#{remote_file}` could not be fetched because of a socket error!"
         )
       end
     end
@@ -92,7 +92,8 @@ describe Gitlab::Ci::External::Processor do
       end
 
       before do
-        allow_any_instance_of(Gitlab::Ci::External::File::Local).to receive(:fetch_local_content).and_return(local_file_content)
+        allow_any_instance_of(Gitlab::Ci::Config::External::File::Local)
+          .to receive(:fetch_local_content).and_return(local_file_content)
       end
 
       it 'should append the file to the values' do
@@ -131,7 +132,10 @@ describe Gitlab::Ci::External::Processor do
 
       before do
         local_file_content = File.read(Rails.root.join('spec/fixtures/gitlab/ci/external_files/.gitlab-ci-template-1.yml'))
-        allow_any_instance_of(Gitlab::Ci::External::File::Local).to receive(:fetch_local_content).and_return(local_file_content)
+
+        allow_any_instance_of(Gitlab::Ci::Config::External::File::Local)
+          .to receive(:fetch_local_content).and_return(local_file_content)
+
         WebMock.stub_request(:get, remote_file).to_return(body: remote_file_content)
       end
 
@@ -150,11 +154,15 @@ describe Gitlab::Ci::External::Processor do
       let(:local_file_content) { 'invalid content file ////' }
 
       before do
-        allow_any_instance_of(Gitlab::Ci::External::File::Local).to receive(:fetch_local_content).and_return(local_file_content)
+        allow_any_instance_of(Gitlab::Ci::Config::External::File::Local)
+          .to receive(:fetch_local_content).and_return(local_file_content)
       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
 
diff --git a/spec/lib/gitlab/ci/config_spec.rb b/spec/lib/gitlab/ci/config_spec.rb
index b43aca8a354..975e11e8cc1 100644
--- a/spec/lib/gitlab/ci/config_spec.rb
+++ b/spec/lib/gitlab/ci/config_spec.rb
@@ -1,6 +1,4 @@
-require 'fast_spec_helper'
-
-require_dependency 'active_model'
+require 'spec_helper'
 
 describe Gitlab::Ci::Config do
   let(:config) do
@@ -202,8 +200,8 @@ describe Gitlab::Ci::Config do
 
       it 'raises error YamlProcessor validationError' do
         expect { config }.to raise_error(
-          ::Gitlab::Ci::YamlProcessor::ValidationError,
-          "Local file 'invalid' is not valid."
+          described_class::ConfigError,
+          "Included file `invalid` does not have YAML extension!"
         )
       end
     end