Merge branch 'extract-validate-from-ci-external-file-base-initialize' into 'master'

Move CI external file validation from the initialize method

See merge request gitlab-org/gitlab!83949

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

@@ -16,8 +16,6 @@ module Gitlab
               @params = params
               @context = context
               @errors = []
-
-              validate!
             end
 
             def matching?
@@ -48,6 +46,15 @@ module Gitlab
               expanded_content_hash
             end
 
+            def validate!
+              context.logger.instrument(:config_file_validation) do
+                validate_execution_time!
+                validate_location!
+                validate_content! if errors.none?
+                validate_hash! if errors.none?
+              end
+            end
+
             protected
 
             def expanded_content_hash
@@ -66,13 +73,6 @@ module Gitlab
               nil
             end
 
-            def validate!
-              validate_execution_time!
-              validate_location!
-              validate_content! if errors.none?
-              validate_hash! if errors.none?
-            end
-
             def validate_execution_time!
               context.check_execution_time!
             end

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

@@ -50,6 +50,7 @@ module Gitlab
               .map(&method(:expand_variables))
               .each(&method(:verify_duplicates!))
               .map(&method(:select_first_matching))
+              .each(&method(:verify!))
           end
 
           def normalize_location(location)
@@ -147,6 +148,10 @@ module Gitlab
             matching.first
           end
 
+          def verify!(location_object)
+            location_object.validate!
+          end
+
           def expand_variables(data)
             logger.instrument(:config_mapper_variables) do
               expand_variables_without_instrumentation(data)

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

@@ -29,14 +29,15 @@ RSpec.describe Gitlab::Ci::Config::External::File::Artifact do
   end
 
   describe '#valid?' do
-    shared_examples 'is invalid' do
-      it 'is not valid' do
-        expect(external_file).not_to be_valid
-      end
+    subject(:valid?) do
+      external_file.validate!
+      external_file.valid?
+    end
 
+    shared_examples 'is invalid' do
       it 'sets the expected error' do
-        expect(external_file.errors)
-          .to contain_exactly(expected_error)
+        expect(valid?).to be_falsy
+        expect(external_file.errors).to contain_exactly(expected_error)
       end
     end
 
@@ -142,7 +143,7 @@ RSpec.describe Gitlab::Ci::Config::External::File::Artifact do
 
                 context 'when file is not empty' do
                   it 'is valid' do
-                    expect(external_file).to be_valid
+                    expect(valid?).to be_truthy
                     expect(external_file.content).to be_present
                   end
 
@@ -154,6 +155,7 @@ RSpec.describe Gitlab::Ci::Config::External::File::Artifact do
                       user: anything
                     }
                     expect(context).to receive(:mutate).with(expected_attrs).and_call_original
+                    external_file.validate!
                     external_file.content
                   end
                 end

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

@@ -16,7 +16,7 @@ RSpec.describe Gitlab::Ci::Config::External::File::Base do
     end
   end
 
-  subject { test_class.new(location, context) }
+  subject(:file) { test_class.new(location, context) }
 
   before do
     allow_any_instance_of(test_class)
@@ -31,7 +31,7 @@ RSpec.describe Gitlab::Ci::Config::External::File::Base do
       let(:location) { 'some-location' }
 
       it 'returns true' do
-        expect(subject).to be_matching
+        expect(file).to be_matching
       end
     end
 
@@ -39,40 +39,45 @@ RSpec.describe Gitlab::Ci::Config::External::File::Base do
       let(:location) { nil }
 
       it 'returns false' do
-        expect(subject).not_to be_matching
+        expect(file).not_to be_matching
       end
     end
   end
 
   describe '#valid?' do
+    subject(:valid?) do
+      file.validate!
+      file.valid?
+    end
+
     context 'when location is not a string' do
       let(:location) { %w(some/file.txt other/file.txt) }
 
-      it { is_expected.not_to be_valid }
+      it { is_expected.to be_falsy }
     end
 
     context 'when location is not a YAML file' do
       let(:location) { 'some/file.txt' }
 
-      it { is_expected.not_to be_valid }
+      it { is_expected.to be_falsy }
     end
 
     context 'when location has not a valid naming scheme' do
       let(:location) { 'some/file/.yml' }
 
-      it { is_expected.not_to be_valid }
+      it { is_expected.to be_falsy }
     end
 
     context 'when location is a valid .yml extension' do
       let(:location) { 'some/file/config.yml' }
 
-      it { is_expected.to be_valid }
+      it { is_expected.to be_truthy }
     end
 
     context 'when location is a valid .yaml extension' do
       let(:location) { 'some/file/config.yaml' }
 
-      it { is_expected.to be_valid }
+      it { is_expected.to be_truthy }
     end
 
     context 'when there are YAML syntax errors' do
@@ -84,8 +89,8 @@ RSpec.describe Gitlab::Ci::Config::External::File::Base do
       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/
+        expect(valid?).to be_falsy
+        expect(file.error_message).to match /does not have valid YAML syntax/
       end
     end
   end
@@ -101,7 +106,7 @@ RSpec.describe Gitlab::Ci::Config::External::File::Base do
       end
 
       it 'does expand hash to include the template' do
-        expect(subject.to_hash).to include(:before_script)
+        expect(file.to_hash).to include(:before_script)
       end
     end
   end

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

@@ -54,6 +54,11 @@ RSpec.describe Gitlab::Ci::Config::External::File::Local do
   end
 
   describe '#valid?' do
+    subject(:valid?) do
+      local_file.validate!
+      local_file.valid?
+    end
+
     context 'when is a valid local path' do
       let(:location) { '/lib/gitlab/ci/templates/existent-file.yml' }
 
@@ -61,25 +66,19 @@ RSpec.describe Gitlab::Ci::Config::External::File::Local do
         allow_any_instance_of(described_class).to receive(:fetch_local_content).and_return("image: 'ruby2:2'")
       end
 
-      it 'returns true' do
-        expect(local_file.valid?).to be_truthy
-      end
+      it { is_expected.to be_truthy }
     end
 
     context 'when is not a valid local path' do
       let(:location) { '/lib/gitlab/ci/templates/non-existent-file.yml' }
 
-      it 'returns false' do
-        expect(local_file.valid?).to be_falsy
-      end
+      it { is_expected.to be_falsy }
     end
 
     context 'when is not a yaml file' do
       let(:location) { '/config/application.rb' }
 
-      it 'returns false' do
-        expect(local_file.valid?).to be_falsy
-      end
+      it { is_expected.to be_falsy }
     end
 
     context 'when the given sha is not valid' do
@@ -87,7 +86,7 @@ RSpec.describe Gitlab::Ci::Config::External::File::Local do
       let(:sha) { ':' }
 
       it 'returns false and adds an error message stating that included file does not exist' do
-        expect(local_file).not_to be_valid
+        expect(valid?).to be_falsy
         expect(local_file.errors).to include("Sha #{sha} is not valid!")
       end
     end
@@ -128,6 +127,10 @@ RSpec.describe Gitlab::Ci::Config::External::File::Local do
   describe '#error_message' do
     let(:location) { '/lib/gitlab/ci/templates/non-existent-file.yml' }
 
+    before do
+      local_file.validate!
+    end
+
     it 'returns an error message' do
       expect(local_file.error_message).to eq("Local file `#{location}` does not exist!")
     end
@@ -162,6 +165,8 @@ RSpec.describe Gitlab::Ci::Config::External::File::Local do
 
         allow(project.repository).to receive(:blob_data_at).with(sha, another_location)
           .and_return(another_content)
+
+        local_file.validate!
       end
 
       it 'does expand hash to include the template' do

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

@@ -65,6 +65,11 @@ RSpec.describe Gitlab::Ci::Config::External::File::Project do
   end
 
   describe '#valid?' do
+    subject(:valid?) do
+      project_file.validate!
+      project_file.valid?
+    end
+
     context 'when a valid path is used' do
       let(:params) do
         { project: project.full_path, file: '/file.yml' }
@@ -76,15 +81,13 @@ RSpec.describe Gitlab::Ci::Config::External::File::Project do
         stub_project_blob(root_ref_sha, '/file.yml') { 'image: ruby:2.7' }
       end
 
-      it 'returns true' do
-        expect(project_file).to be_valid
-      end
+      it { is_expected.to be_truthy }
 
       context 'when user does not have permission to access file' do
         let(:context_user) { create(:user) }
 
         it 'returns false' do
-          expect(project_file).not_to be_valid
+          expect(valid?).to be_falsy
           expect(project_file.error_message).to include("Project `#{project.full_path}` not found or access denied!")
         end
       end
@@ -101,9 +104,7 @@ RSpec.describe Gitlab::Ci::Config::External::File::Project do
         stub_project_blob(ref_sha, '/file.yml') { 'image: ruby:2.7' }
       end
 
-      it 'returns true' do
-        expect(project_file).to be_valid
-      end
+      it { is_expected.to be_truthy }
     end
 
     context 'when an empty file is used' do
@@ -118,7 +119,7 @@ RSpec.describe Gitlab::Ci::Config::External::File::Project do
       end
 
       it 'returns false' do
-        expect(project_file).not_to be_valid
+        expect(valid?).to be_falsy
         expect(project_file.error_message).to include("Project `#{project.full_path}` file `/file.yml` is empty!")
       end
     end
@@ -129,7 +130,7 @@ RSpec.describe Gitlab::Ci::Config::External::File::Project do
       end
 
       it 'returns false' do
-        expect(project_file).not_to be_valid
+        expect(valid?).to be_falsy
         expect(project_file.error_message).to include("Project `#{project.full_path}` reference `I-Do-Not-Exist` does not exist!")
       end
     end
@@ -140,7 +141,7 @@ RSpec.describe Gitlab::Ci::Config::External::File::Project do
       end
 
       it 'returns false' do
-        expect(project_file).not_to be_valid
+        expect(valid?).to be_falsy
         expect(project_file.error_message).to include("Project `#{project.full_path}` file `/invalid-file.yml` does not exist!")
       end
     end
@@ -151,7 +152,7 @@ RSpec.describe Gitlab::Ci::Config::External::File::Project do
       end
 
       it 'returns false' do
-        expect(project_file).not_to be_valid
+        expect(valid?).to be_falsy
         expect(project_file.error_message).to include('Included file `/invalid-file` does not have YAML extension!')
       end
     end

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

@@ -53,22 +53,23 @@ RSpec.describe Gitlab::Ci::Config::External::File::Remote do
   end
 
   describe "#valid?" do
+    subject(:valid?) do
+      remote_file.validate!
+      remote_file.valid?
+    end
+
     context 'when is a valid remote url' do
       before do
         stub_full_request(location).to_return(body: remote_file_content)
       end
 
-      it 'returns true' do
-        expect(remote_file.valid?).to be_truthy
-      end
+      it { is_expected.to be_truthy }
     end
 
     context 'with an irregular url' do
       let(:location) { 'not-valid://gitlab.com/gitlab-org/gitlab-foss/blob/1234/.gitlab-ci-1.yml' }
 
-      it 'returns false' do
-        expect(remote_file.valid?).to be_falsy
-      end
+      it { is_expected.to be_falsy }
     end
 
     context 'with a timeout' do
@@ -76,25 +77,19 @@ RSpec.describe Gitlab::Ci::Config::External::File::Remote do
         allow(Gitlab::HTTP).to receive(:get).and_raise(Timeout::Error)
       end
 
-      it 'is falsy' do
-        expect(remote_file.valid?).to be_falsy
-      end
+      it { is_expected.to be_falsy }
     end
 
     context 'when is not a yaml file' do
       let(:location) { 'https://asdasdasdaj48ggerexample.com' }
 
-      it 'is falsy' do
-        expect(remote_file.valid?).to be_falsy
-      end
+      it { is_expected.to be_falsy }
     end
 
     context 'with an internal url' do
       let(:location) { 'http://localhost:8080' }
 
-      it 'is falsy' do
-        expect(remote_file.valid?).to be_falsy
-      end
+      it { is_expected.to be_falsy }
     end
   end
 
@@ -141,7 +136,10 @@ RSpec.describe Gitlab::Ci::Config::External::File::Remote do
   end
 
   describe "#error_message" do
-    subject { remote_file.error_message }
+    subject(:error_message) do
+      remote_file.validate!
+      remote_file.error_message
+    end
 
     context 'when remote file location is not valid' do
       let(:location) { 'not-valid://gitlab.com/gitlab-org/gitlab-foss/blob/1234/.gitlab-ci-1.yml' }

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

@@ -45,19 +45,22 @@ RSpec.describe Gitlab::Ci::Config::External::File::Template do
   end
 
   describe "#valid?" do
+    subject(:valid?) do
+      template_file.validate!
+      template_file.valid?
+    end
+
     context 'when is a valid template name' do
       let(:template) { 'Auto-DevOps.gitlab-ci.yml' }
 
-      it 'returns true' do
-        expect(template_file).to be_valid
-      end
+      it { is_expected.to be_truthy }
     end
 
     context 'with invalid template name' do
       let(:template) { 'Template.yml' }
 
       it 'returns false' do
-        expect(template_file).not_to be_valid
+        expect(valid?).to be_falsy
         expect(template_file.error_message).to include('Template file `Template.yml` is not a valid location!')
       end
     end
@@ -66,7 +69,7 @@ RSpec.describe Gitlab::Ci::Config::External::File::Template do
       let(:template) { 'I-Do-Not-Have-This-Template.gitlab-ci.yml' }
 
       it 'returns false' do
-        expect(template_file).not_to be_valid
+        expect(valid?).to be_falsy
         expect(template_file.error_message).to include('Included file `I-Do-Not-Have-This-Template.gitlab-ci.yml` is empty or does not exist!')
       end
     end