Merge branch 'make-yaml-processor-return-results-2' into 'master'

Refactor YamlProcessor to always return a result object

See merge request gitlab-org/gitlab!40013

ee/app/graphql/ee/types/project_type.rb

@@ -89,8 +89,6 @@ module EE
 
         def self.sast_ci_configuration(project)
           ::Security::CiConfiguration::SastParserService.new(project).configuration
-        rescue ::Gitlab::Ci::YamlProcessor::ValidationError => ex
-          raise ::GraphQL::ExecutionError, ex.message
         end
       end
     end

ee/app/services/security/ci_configuration/sast_parser_service.rb

@@ -66,7 +66,10 @@ module Security
 
       def build_sast_attributes(content)
         options = { project: @project, user: current_user, sha: @project.repository.commit.sha }
-        sast_attributes = Gitlab::Ci::YamlProcessor.new(content, options).build_attributes(:sast)
+        yaml_result = Gitlab::Ci::YamlProcessor.new(content, options).execute
+        return {} unless yaml_result.valid?
+
+        sast_attributes = yaml_result.build_attributes(:sast)
         extract_required_attributes(sast_attributes)
       end
 

ee/spec/graphql/types/project_type_spec.rb

@@ -25,7 +25,6 @@ RSpec.describe GitlabSchema.types['Project'] do
 
   describe 'sast_ci_configuration' do
     include_context 'read ci configuration for sast enabled project'
-    let(:error_message) { "This is an error for YamlProcessor." }
 
     let_it_be(:query) do
       %(
@@ -110,14 +109,6 @@ RSpec.describe GitlabSchema.types['Project'] do
       expect(analyzer['label']).to eq('Brakeman')
       expect(analyzer['enabled']).to eq(true)
     end
-
-    it 'returns an error if there is an exception in YamlProcessor' do
-      allow_next_instance_of(::Security::CiConfiguration::SastParserService) do |service|
-        allow(service).to receive(:configuration).and_raise(::Gitlab::Ci::YamlProcessor::ValidationError.new(error_message))
-      end
-
-      expect(subject["errors"].first["message"]).to eql(error_message)
-    end
   end
 
   describe 'security_scanners' do

ee/spec/lib/ee/gitlab/ci/templates/templates_spec.rb

@@ -5,7 +5,7 @@ require 'spec_helper'
 RSpec.describe "CI YML Templates" do
   using RSpec::Parameterized::TableSyntax
 
-  subject { Gitlab::Ci::YamlProcessor.new(content) }
+  subject { Gitlab::Ci::YamlProcessor.new(content).execute }
 
   where(:template_name) do
     Gitlab::Template::GitlabCiYmlTemplate.all.map(&:full_name)
@@ -24,7 +24,7 @@ RSpec.describe "CI YML Templates" do
     end
 
     it 'is valid' do
-      expect { subject }.not_to raise_error
+      expect(subject).to be_valid
     end
 
     it 'require default stages to be included' do

ee/spec/lib/gitlab/ci/yaml_processor_spec.rb

@@ -11,7 +11,7 @@ RSpec.describe Gitlab::Ci::YamlProcessor do
       }
     end
 
-    subject { described_class.new(YAML.dump(config)) }
+    subject { described_class.new(YAML.dump(config)).execute }
 
     context 'needs upstream pipeline' do
       let(:needs) { { pipeline: 'some/project' } }
@@ -174,9 +174,8 @@ RSpec.describe Gitlab::Ci::YamlProcessor do
       end
 
       it 'returns errors' do
-        expect { subject }
-          .to raise_error(Gitlab::Ci::YamlProcessor::ValidationError,
-                          'jobs:bridge config should contain either a trigger or a needs:pipeline')
+        expect(subject.errors).to include(
+          'jobs:bridge config should contain either a trigger or a needs:pipeline')
       end
     end
 
@@ -198,9 +197,8 @@ RSpec.describe Gitlab::Ci::YamlProcessor do
       end
 
       it 'returns errors' do
-        expect { subject }
-          .to raise_error(Gitlab::Ci::YamlProcessor::ValidationError,
-                          'jobs:test:needs:need ref should be a string')
+        expect(subject.errors).to contain_exactly(
+          'jobs:test:needs:need ref should be a string')
       end
     end
   end
@@ -216,10 +214,10 @@ RSpec.describe Gitlab::Ci::YamlProcessor do
 
     let(:config) { { deploy_to_production: { stage: 'deploy', script: ['echo'], secrets: secrets } } }
 
-    subject(:processor) { described_class.new(YAML.dump(config)) }
+    subject(:result) { described_class.new(YAML.dump(config)).execute }
 
     it "returns secrets info" do
-      secrets = processor.stage_builds_attributes('deploy').first.fetch(:secrets)
+      secrets = result.stage_builds_attributes('deploy').first.fetch(:secrets)
 
       expect(secrets).to eq({
         DATABASE_PASSWORD: {

lib/gitlab/ci/config.rb

@@ -62,6 +62,10 @@ module Gitlab
         root.jobs_value
       end
 
+      def normalized_jobs
+        @normalized_jobs ||= Ci::Config::Normalizer.new(jobs).normalize_jobs
+      end
+
       private
 
       def expand_config(config)

lib/gitlab/ci/config/normalizer.rb

@@ -11,6 +11,7 @@ module Gitlab
         end
 
         def normalize_jobs
+          return {} unless @jobs_config
           return @jobs_config if parallelized_jobs.empty?
 
           expand_parallelize_jobs do |job_name, config|

lib/gitlab/ci/lint.rb

@@ -45,14 +45,15 @@ module Gitlab
       end
 
       def static_validation(content)
-        result = Gitlab::Ci::YamlProcessor.new_with_validation_errors(
+        result = Gitlab::Ci::YamlProcessor.new(
           content,
           project: @project,
           user: @current_user,
-          sha: @project.repository.commit.sha)
+          sha: @project.repository.commit.sha
+        ).execute
 
         Result.new(
-          jobs: static_validation_convert_to_jobs(result.config&.stages, result.config&.builds),
+          jobs: static_validation_convert_to_jobs(result),
           errors: result.errors,
           warnings: result.warnings
         )
@@ -76,12 +77,12 @@ module Gitlab
         end
       end
 
-      def static_validation_convert_to_jobs(stages, all_jobs)
+      def static_validation_convert_to_jobs(result)
         jobs = []
-        return jobs unless stages || all_jobs
+        return jobs unless result.valid?
 
-        stages.each do |stage_name|
-          all_jobs.each do |job|
+        result.stages.each do |stage_name|
+          result.builds.each do |job|
             next unless job[:stage] == stage_name
 
             jobs << {

lib/gitlab/ci/pipeline/chain/config/process.rb

@@ -11,20 +11,23 @@ module Gitlab
             def perform!
               raise ArgumentError, 'missing config content' unless @command.config_content
 
-              @command.config_processor = ::Gitlab::Ci::YamlProcessor.new(
+              result = ::Gitlab::Ci::YamlProcessor.new(
                 @command.config_content, {
                   project: project,
                   sha: @pipeline.sha,
                   user: current_user,
                   parent_pipeline: parent_pipeline
                 }
-              )
+              ).execute
+
+              add_warnings_to_pipeline(result.warnings)
 
-              add_warnings_to_pipeline(@command.config_processor.warnings)
-            rescue Gitlab::Ci::YamlProcessor::ValidationError => ex
-              add_warnings_to_pipeline(ex.warnings)
+              if result.valid?
+                @command.config_processor = result
+              else
+                error(result.errors.first, config_error: true)
+              end
 
-              error(ex.message, config_error: true)
             rescue => ex
               Gitlab::ErrorTracking.track_exception(ex,
                 project_id: project.id,

lib/gitlab/ci/yaml_processor.rb

 # frozen_string_literal: true
 
+# This is the CI Linter component that runs the syntax validations
+# while parsing the YAML config into a data structure that is
+# then presented to the caller as result object.
+# After syntax validations (done by Ci::Config), this component also
+# runs logical validation on the built data structure.
 module Gitlab
   module Ci
     class YamlProcessor
-      # ValidationError is treated like a result object in the form of an exception.
-      # We can return any warnings, raised during the config validation, along with
-      # the error object until we support multiple messages to be returned.
-      class ValidationError < StandardError
-        attr_reader :warnings
-
-        def initialize(message, warnings: [])
-          @warnings = warnings
-          super(message)
-        end
-      end
-
-      include Gitlab::Config::Entry::LegacyValidationHelpers
+      ValidationError = Class.new(StandardError)
 
-      attr_reader :stages, :jobs
+      def self.validation_message(content, opts = {})
+        result = new(content, opts).execute
 
-      class Result
-        attr_reader :config, :errors, :warnings
+        result.errors.first
+      end
 
-        def initialize(config: nil, errors: [], warnings: [])
-          @config = config
-          @errors = errors
-          @warnings = warnings
-        end
+      def initialize(config_content, opts = {})
+        @config_content = config_content
+        @opts = opts
+      end
 
-        def valid?
-          config.present? && errors.empty?
+      def execute
+        if @config_content.blank?
+          return Result.new(errors: ['Please provide content of .gitlab-ci.yml'])
         end
-      end
 
-      def initialize(config, opts = {})
-        @ci_config = Gitlab::Ci::Config.new(config, **opts)
-        @config = @ci_config.to_hash
+        @ci_config = Gitlab::Ci::Config.new(@config_content, **@opts)
 
         unless @ci_config.valid?
-          error!(@ci_config.errors.first)
+          return Result.new(ci_config: @ci_config, errors: @ci_config.errors, warnings: @ci_config.warnings)
         end
 
-        initial_parsing
-      rescue Gitlab::Ci::Config::ConfigError => e
-        error!(e.message)
-      end
-
-      def self.new_with_validation_errors(content, opts = {})
-        return Result.new(errors: ['Please provide content of .gitlab-ci.yml']) if content.blank?
+        run_logical_validations!
 
-        config = Gitlab::Ci::Config.new(content, **opts)
-        return Result.new(errors: config.errors, warnings: config.warnings) unless config.valid?
-
-        config = Gitlab::Ci::YamlProcessor.new(content, opts)
-        Result.new(config: config, warnings: config.warnings)
-
-      rescue ValidationError => e
-        Result.new(errors: [e.message], warnings: e.warnings)
+        Result.new(ci_config: @ci_config, warnings: @ci_config&.warnings)
 
       rescue Gitlab::Ci::Config::ConfigError => e
-        Result.new(errors: [e.message])
-      end
-
-      def warnings
-        @ci_config&.warnings || []
-      end
-
-      def builds
-        @jobs.map do |name, _|
-          build_attributes(name)
-        end
-      end
-
-      def build_attributes(name)
-        job = @jobs.fetch(name.to_sym, {})
-
-        { stage_idx: @stages.index(job[:stage]),
-          stage: job[:stage],
-          tag_list: job[:tags],
-          name: job[:name].to_s,
-          allow_failure: job[:ignore],
-          when: job[:when] || 'on_success',
-          environment: job[:environment_name],
-          coverage_regex: job[:coverage],
-          yaml_variables: transform_to_yaml_variables(job[:variables]),
-          needs_attributes: job.dig(:needs, :job),
-          interruptible: job[:interruptible],
-          only: job[:only],
-          except: job[:except],
-          rules: job[:rules],
-          cache: job[:cache],
-          resource_group_key: job[:resource_group],
-          scheduling_type: job[:scheduling_type],
-          secrets: job[:secrets],
-          options: {
-            image: job[:image],
-            services: job[:services],
-            artifacts: job[:artifacts],
-            dependencies: job[:dependencies],
-            cross_dependencies: job.dig(:needs, :cross_dependency),
-            job_timeout: job[:timeout],
-            before_script: job[:before_script],
-            script: job[:script],
-            after_script: job[:after_script],
-            environment: job[:environment],
-            retry: job[:retry],
-            parallel: job[:parallel],
-            instance: job[:instance],
-            start_in: job[:start_in],
-            trigger: job[:trigger],
-            bridge_needs: job.dig(:needs, :bridge)&.first,
-            release: release(job)
-          }.compact }.compact
-      end
+        Result.new(ci_config: @ci_config, errors: [e.message], warnings: @ci_config&.warnings)
 
-      def release(job)
-        job[:release]
-      end
-
-      def stage_builds_attributes(stage)
-        @jobs.values
-          .select { |job| job[:stage] == stage }
-          .map { |job| build_attributes(job[:name]) }
-      end
-
-      def stages_attributes
-        @stages.uniq.map do |stage|
-          seeds = stage_builds_attributes(stage)
-
-          { name: stage, index: @stages.index(stage), builds: seeds }
-        end
-      end
-
-      def workflow_attributes
-        {
-          rules: @config.dig(:workflow, :rules),
-          yaml_variables: transform_to_yaml_variables(@variables)
-        }
-      end
-
-      def self.validation_message(content, opts = {})
-        return 'Please provide content of .gitlab-ci.yml' if content.blank?
-
-        begin
-          Gitlab::Ci::YamlProcessor.new(content, opts)
-          nil
-        rescue ValidationError => e
-          e.message
-        end
+      rescue ValidationError => e
+        Result.new(ci_config: @ci_config, errors: [e.message], warnings: @ci_config&.warnings)
       end
 
       private
 
-      def initial_parsing
-        ##
-        # Global config
-        #
-        @variables = @ci_config.variables
+      def run_logical_validations!
         @stages = @ci_config.stages
-
-        ##
-        # Jobs
-        #
-        @jobs = Ci::Config::Normalizer.new(@ci_config.jobs).normalize_jobs
+        @jobs = @ci_config.normalized_jobs
 
         @jobs.each do |name, job|
-          # logical validation for job
-          validate_job_stage!(name, job)
-          validate_job_dependencies!(name, job)
-          validate_job_needs!(name, job)
-          validate_dynamic_child_pipeline_dependencies!(name, job)
-          validate_job_environment!(name, job)
+          validate_job!(name, job)
         end
       end
 
-      def transform_to_yaml_variables(variables)
-        variables.to_h.map do |key, value|
-          { key: key.to_s, value: value, public: true }
-        end
+      def validate_job!(name, job)
+        validate_job_stage!(name, job)
+        validate_job_dependencies!(name, job)
+        validate_job_needs!(name, job)
+        validate_dynamic_child_pipeline_dependencies!(name, job)
+        validate_job_environment!(name, job)
       end
 
       def validate_job_stage!(name, job)
@@ -188,10 +70,6 @@ module Gitlab
         end
       end
 
-      def error!(message)
-        raise ValidationError.new(message, warnings: warnings)
-      end
-
       def validate_job_dependencies!(name, job)
         return unless job[:dependencies]
 
@@ -267,6 +145,10 @@ module Gitlab
           error!("#{name} job: on_stop job #{on_stop} needs to have action stop defined")
         end
       end
+
+      def error!(message)
+        raise ValidationError.new(message)
+      end
     end
   end
 end

lib/gitlab/ci/yaml_processor/result.rb

+# frozen_string_literal: true
+
+# A data object that wraps `Ci::Config` and any messages
+# (errors, warnings) generated by the YamlProcessor.
+module Gitlab
+  module Ci
+    class YamlProcessor
+      class Result
+        attr_reader :errors, :warnings
+
+        def initialize(ci_config: nil, errors: [], warnings: [])
+          @ci_config = ci_config
+          @errors = errors || []
+          @warnings = warnings || []
+        end
+
+        def valid?
+          errors.empty?
+        end
+
+        def stages_attributes
+          stages.uniq.map do |stage|
+            seeds = stage_builds_attributes(stage)
+
+            { name: stage, index: stages.index(stage), builds: seeds }
+          end
+        end
+
+        def builds
+          jobs.map do |name, _|
+            build_attributes(name)
+          end
+        end
+
+        def stage_builds_attributes(stage)
+          jobs.values
+            .select { |job| job[:stage] == stage }
+            .map { |job| build_attributes(job[:name]) }
+        end
+
+        def workflow_attributes
+          {
+            rules: hash_config.dig(:workflow, :rules),
+            yaml_variables: transform_to_yaml_variables(variables)
+          }
+        end
+
+        def jobs
+          @jobs ||= @ci_config.normalized_jobs
+        end
+
+        def stages
+          @stages ||= @ci_config.stages
+        end
+
+        def build_attributes(name)
+          job = jobs.fetch(name.to_sym, {})
+
+          { stage_idx: stages.index(job[:stage]),
+            stage: job[:stage],
+            tag_list: job[:tags],
+            name: job[:name].to_s,
+            allow_failure: job[:ignore],
+            when: job[:when] || 'on_success',
+            environment: job[:environment_name],
+            coverage_regex: job[:coverage],
+            yaml_variables: transform_to_yaml_variables(job[:variables]),
+            needs_attributes: job.dig(:needs, :job),
+            interruptible: job[:interruptible],
+            only: job[:only],
+            except: job[:except],
+            rules: job[:rules],
+            cache: job[:cache],
+            resource_group_key: job[:resource_group],
+            scheduling_type: job[:scheduling_type],
+            secrets: job[:secrets],
+            options: {
+              image: job[:image],
+              services: job[:services],
+              artifacts: job[:artifacts],
+              dependencies: job[:dependencies],
+              cross_dependencies: job.dig(:needs, :cross_dependency),
+              job_timeout: job[:timeout],
+              before_script: job[:before_script],
+              script: job[:script],
+              after_script: job[:after_script],
+              environment: job[:environment],
+              retry: job[:retry],
+              parallel: job[:parallel],
+              instance: job[:instance],
+              start_in: job[:start_in],
+              trigger: job[:trigger],
+              bridge_needs: job.dig(:needs, :bridge)&.first,
+              release: release(job)
+            }.compact }.compact
+        end
+
+        private
+
+        def variables
+          @variables ||= @ci_config.variables
+        end
+
+        def hash_config
+          @hash_config ||= @ci_config.to_hash
+        end
+
+        def release(job)
+          job[:release]
+        end
+
+        def transform_to_yaml_variables(variables)
+          variables.to_h.map do |key, value|
+            { key: key.to_s, value: value, public: true }
+          end
+        end
+      end
+    end
+  end
+end

spec/controllers/projects/ci/lints_controller_spec.rb

@@ -98,7 +98,7 @@ RSpec.describe Projects::Ci::LintsController do
         it_behaves_like 'returns a successful validation'
 
         it 'runs validations through YamlProcessor' do
-          expect(Gitlab::Ci::YamlProcessor).to receive(:new_with_validation_errors).and_call_original
+          expect(Gitlab::Ci::YamlProcessor).to receive(:new).and_call_original
 
           subject
         end
@@ -126,7 +126,7 @@ RSpec.describe Projects::Ci::LintsController do
           it_behaves_like 'returns a successful validation'
 
           it 'runs validations through YamlProcessor' do
-            expect(Gitlab::Ci::YamlProcessor).to receive(:new_with_validation_errors).and_call_original
+            expect(Gitlab::Ci::YamlProcessor).to receive(:new).and_call_original
 
             subject
           end

spec/lib/gitlab/ci/config/normalizer_spec.rb

@@ -264,5 +264,11 @@ RSpec.describe Gitlab::Ci::Config::Normalizer do
         is_expected.to match(config)
       end
     end
+
+    context 'when jobs config is nil' do
+      let(:config) { nil }
+
+      it { is_expected.to eq({}) }
+    end
   end
 end

spec/lib/gitlab/ci/config_spec.rb

@@ -312,7 +312,7 @@ RSpec.describe Gitlab::Ci::Config do
         HEREDOC
       end
 
-      it 'raises error YamlProcessor validationError' do
+      it 'raises ConfigError' do
         expect { config }.to raise_error(
           described_class::ConfigError,
           "Included file `invalid` does not have YAML extension!"
@@ -329,7 +329,7 @@ RSpec.describe Gitlab::Ci::Config do
         HEREDOC
       end
 
-      it 'raises error YamlProcessor validationError' do
+      it 'raises ConfigError' do
         expect { config }.to raise_error(
           described_class::ConfigError,
           'Include `{"remote":"http://url","local":"/local/file.yml"}` needs to match exactly one accessor!'

spec/lib/gitlab/ci/lint_spec.rb

@@ -157,7 +157,7 @@ RSpec.describe Gitlab::Ci::Lint do
 
         it 'uses YamlProcessor' do
           expect(Gitlab::Ci::YamlProcessor)
-            .to receive(:new_with_validation_errors)
+            .to receive(:new)
             .and_call_original
 
           subject

spec/lib/gitlab/ci/pipeline/chain/validate/external_spec.rb

@@ -31,20 +31,20 @@ RSpec.describe Gitlab::Ci::Pipeline::Chain::Validate::External do
     CI_YAML
   end
 
-  let(:yaml_processor) do
+  let(:yaml_processor_result) do
     ::Gitlab::Ci::YamlProcessor.new(
       ci_yaml, {
         project: project,
         sha: pipeline.sha,
         user: user
       }
-    )
+    ).execute
   end
 
   let(:save_incompleted) { true }
   let(:command) do
     Gitlab::Ci::Pipeline::Chain::Command.new(
-      project: project, current_user: user, config_processor: yaml_processor, save_incompleted: save_incompleted
+      project: project, current_user: user, config_processor: yaml_processor_result, save_incompleted: save_incompleted
     )
   end
 

spec/lib/gitlab/ci/templates/templates_spec.rb

@@ -3,7 +3,7 @@
 require 'spec_helper'
 
 RSpec.describe 'CI YML Templates' do
-  subject { Gitlab::Ci::YamlProcessor.new(content) }
+  subject { Gitlab::Ci::YamlProcessor.new(content).execute }
 
   let(:all_templates) { Gitlab::Template::GitlabCiYmlTemplate.all.map(&:full_name) }
 
@@ -33,7 +33,7 @@ RSpec.describe 'CI YML Templates' do
       end
 
       it 'is valid' do
-        expect { subject }.not_to raise_error
+        expect(subject).to be_valid
       end
 
       it 'require default stages to be included' do

spec/lib/gitlab/ci/yaml_processor_spec.rb

@@ -7,16 +7,16 @@ module Gitlab
     RSpec.describe YamlProcessor do
       include StubRequests
 
-      subject { described_class.new(config, user: nil) }
+      subject { described_class.new(config, user: nil).execute }
 
       shared_examples 'returns errors' do |error_message|
-        it 'raises exception when error encountered' do
-          expect { subject }.to raise_error(described_class::ValidationError, error_message)
+        it 'adds a message when an error is encountered' do
+          expect(subject.errors).to include(error_message)
         end
       end
 
       describe '#build_attributes' do
-        subject { described_class.new(config, user: nil).build_attributes(:rspec) }
+        subject { described_class.new(config, user: nil).execute.build_attributes(:rspec) }
 
         describe 'attributes list' do
           let(:config) do
@@ -98,7 +98,7 @@ module Gitlab
             config = YAML.dump({ default: { tags: %w[A B] },
                                  rspec: { script: "rspec" } })
 
-            config_processor = Gitlab::Ci::YamlProcessor.new(config)
+            config_processor = Gitlab::Ci::YamlProcessor.new(config).execute
 
             expect(config_processor.stage_builds_attributes("test").size).to eq(1)
             expect(config_processor.stage_builds_attributes("test").first).to eq({
@@ -145,7 +145,7 @@ module Gitlab
             config = YAML.dump({ default: { interruptible: true },
                                  rspec: { script: "rspec" } })
 
-            config_processor = Gitlab::Ci::YamlProcessor.new(config)
+            config_processor = Gitlab::Ci::YamlProcessor.new(config).execute
 
             expect(config_processor.stage_builds_attributes("test").size).to eq(1)
             expect(config_processor.stage_builds_attributes("test").first).to eq({
@@ -474,9 +474,8 @@ module Gitlab
           end
 
           it 'is propagated all the way up into the raised exception' do
-            expect { subject }.to raise_error do |error|
-              expect(error.warnings).to contain_exactly(/jobs:rspec may allow multiple pipelines to run/)
-            end
+            expect(subject).not_to be_valid
+            expect(subject.warnings).to contain_exactly(/jobs:rspec may allow multiple pipelines to run/)
           end
 
           it_behaves_like 'returns errors', 'jobs:invalid:artifacts config should be a hash'
@@ -493,10 +492,8 @@ module Gitlab
             EOYML
           end
 
-          it 'raises an exception with empty warnings array' do
-            expect { subject }.to raise_error do |error|
-              expect(error.warnings).to be_empty
-            end
+          it 'has empty warnings' do
+            expect(subject.warnings).to be_empty
           end
 
           it_behaves_like 'returns errors', 'Local file `unknown/file.yml` does not have project!'
@@ -504,12 +501,9 @@ module Gitlab
 
         context 'when error is raised after composing the config with warnings' do
           shared_examples 'has warnings and expected error' do |error_message|
-            it 'raises an exception including warnings' do
-              expect { subject }.to raise_error do |error|
-                expect(error).to be_a(described_class::ValidationError)
-                expect(error.message).to match(error_message)
-                expect(error.warnings).to be_present
-              end
+            it 'returns errors and warnings', :aggregate_failures do
+              expect(subject.errors).to include(error_message)
+              expect(subject.warnings).to be_present
             end
           end
 
@@ -590,7 +584,7 @@ module Gitlab
         context 'when `only` has an invalid value' do
           let(:config) { { rspec: { script: "rspec", type: "test", only: only } } }
 
-          subject { Gitlab::Ci::YamlProcessor.new(YAML.dump(config)) }
+          subject { Gitlab::Ci::YamlProcessor.new(YAML.dump(config)).execute }
 
           context 'when it is integer' do
             let(:only) { 1 }
@@ -614,7 +608,7 @@ module Gitlab
         context 'when `except` has an invalid value' do
           let(:config) { { rspec: { script: "rspec", except: except } } }
 
-          subject { Gitlab::Ci::YamlProcessor.new(YAML.dump(config)) }
+          subject { Gitlab::Ci::YamlProcessor.new(YAML.dump(config)).execute }
 
           context 'when it is integer' do
             let(:except) { 1 }
@@ -638,7 +632,7 @@ module Gitlab
 
       describe "Scripts handling" do
         let(:config_data) { YAML.dump(config) }
-        let(:config_processor) { Gitlab::Ci::YamlProcessor.new(config_data) }
+        let(:config_processor) { Gitlab::Ci::YamlProcessor.new(config_data).execute }
 
         subject { config_processor.stage_builds_attributes('test').first }
 
@@ -807,7 +801,7 @@ module Gitlab
                                  before_script: ["pwd"],
                                  rspec: { script: "rspec" } })
 
-            config_processor = Gitlab::Ci::YamlProcessor.new(config)
+            config_processor = Gitlab::Ci::YamlProcessor.new(config).execute
 
             expect(config_processor.stage_builds_attributes("test").size).to eq(1)
             expect(config_processor.stage_builds_attributes("test").first).to eq({
@@ -840,7 +834,7 @@ module Gitlab
                                                        command: ["/usr/local/bin/init", "run"] }, "docker:dind"],
                                           script: "rspec" } })
 
-            config_processor = Gitlab::Ci::YamlProcessor.new(config)
+            config_processor = Gitlab::Ci::YamlProcessor.new(config).execute
 
             expect(config_processor.stage_builds_attributes("test").size).to eq(1)
             expect(config_processor.stage_builds_attributes("test").first).to eq({
@@ -871,7 +865,7 @@ module Gitlab
                                  before_script: ["pwd"],
                                  rspec: { script: "rspec" } })
 
-            config_processor = Gitlab::Ci::YamlProcessor.new(config)
+            config_processor = Gitlab::Ci::YamlProcessor.new(config).execute
 
             expect(config_processor.stage_builds_attributes("test").size).to eq(1)
             expect(config_processor.stage_builds_attributes("test").first).to eq({
@@ -898,7 +892,7 @@ module Gitlab
                                  before_script: ["pwd"],
                                  rspec: { image: "ruby:2.5", services: ["postgresql", "docker:dind"], script: "rspec" } })
 
-            config_processor = Gitlab::Ci::YamlProcessor.new(config)
+            config_processor = Gitlab::Ci::YamlProcessor.new(config).execute
 
             expect(config_processor.stage_builds_attributes("test").size).to eq(1)
             expect(config_processor.stage_builds_attributes("test").first).to eq({
@@ -922,9 +916,9 @@ module Gitlab
       end
 
       describe 'Variables' do
-        let(:config_processor) { Gitlab::Ci::YamlProcessor.new(YAML.dump(config)) }
+        subject { Gitlab::Ci::YamlProcessor.new(YAML.dump(config)).execute }
 
-        subject { config_processor.builds.first[:yaml_variables] }
+        let(:build_variables) { subject.builds.first[:yaml_variables] }
 
         context 'when global variables are defined' do
           let(:variables) do
@@ -940,7 +934,7 @@ module Gitlab
           end
 
           it 'returns global variables' do
-            expect(subject).to contain_exactly(
+            expect(build_variables).to contain_exactly(
               { key: 'VAR1', value: 'value1', public: true },
               { key: 'VAR2', value: 'value2', public: true }
             )
@@ -968,7 +962,7 @@ module Gitlab
             let(:inherit) { }
 
             it 'returns all unique variables' do
-              expect(subject).to contain_exactly(
+              expect(build_variables).to contain_exactly(
                 { key: 'VAR4', value: 'global4', public: true },
                 { key: 'VAR3', value: 'global3', public: true },
                 { key: 'VAR1', value: 'value1', public: true },
@@ -981,7 +975,7 @@ module Gitlab
             let(:inherit) { { variables: false } }
 
             it 'does not inherit variables' do
-              expect(subject).to contain_exactly(
+              expect(build_variables).to contain_exactly(
                 { key: 'VAR1', value: 'value1', public: true },
                 { key: 'VAR2', value: 'value2', public: true }
               )
@@ -992,7 +986,7 @@ module Gitlab
             let(:inherit) { { variables: %w[VAR1 VAR4] } }
 
             it 'returns all unique variables and inherits only specified variables' do
-              expect(subject).to contain_exactly(
+              expect(build_variables).to contain_exactly(
                 { key: 'VAR4', value: 'global4', public: true },
                 { key: 'VAR1', value: 'value1', public: true },
                 { key: 'VAR2', value: 'value2', public: true }
@@ -1015,7 +1009,7 @@ module Gitlab
             end
 
             it 'returns job variables' do
-              expect(subject).to contain_exactly(
+              expect(build_variables).to contain_exactly(
                 { key: 'VAR1', value: 'value1', public: true },
                 { key: 'VAR2', value: 'value2', public: true }
               )
@@ -1041,8 +1035,8 @@ module Gitlab
                 # When variables config is empty, we assume this is a valid
                 # configuration, see issue #18775
                 #
-                expect(subject).to be_an_instance_of(Array)
-                expect(subject).to be_empty
+                expect(build_variables).to be_an_instance_of(Array)
+                expect(build_variables).to be_empty
               end
             end
           end
@@ -1057,14 +1051,14 @@ module Gitlab
           end
 
           it 'returns empty array' do
-            expect(subject).to be_an_instance_of(Array)
-            expect(subject).to be_empty
+            expect(build_variables).to be_an_instance_of(Array)
+            expect(build_variables).to be_empty
           end
         end
       end
 
       context 'when using `extends`' do
-        let(:config_processor) { Gitlab::Ci::YamlProcessor.new(config) }
+        let(:config_processor) { Gitlab::Ci::YamlProcessor.new(config).execute }
 
         subject { config_processor.builds.first }
 
@@ -1126,15 +1120,13 @@ module Gitlab
           }
         end
 
-        subject { Gitlab::Ci::YamlProcessor.new(YAML.dump(config), opts) }
+        subject { Gitlab::Ci::YamlProcessor.new(YAML.dump(config), opts).execute }
 
         context "when validating a ci config file with no project context" do
           context "when a single string is provided" do
             let(:include_content) { "/local.gitlab-ci.yml" }
 
-            it "returns a validation error" do
-              expect { subject }.to raise_error /does not have project/
-            end
+            it_behaves_like 'returns errors', /does not have project/
           end
 
           context "when an array is provided" do
@@ -1165,9 +1157,7 @@ module Gitlab
                   body: 'prepare: { script: ls -al }')
             end
 
-            it "does not return any error" do
-              expect { subject }.not_to raise_error
-            end
+            it { is_expected.to be_valid }
           end
 
           context "when the include type is incorrect" do
@@ -1188,9 +1178,7 @@ module Gitlab
                 .and_return(YAML.dump({ job1: { script: 'hello' } }))
             end
 
-            it "does not return an error" do
-              expect { subject }.not_to raise_error
-            end
+            it { is_expected.to be_valid }
           end
 
           context "when the included internal file is not present" do
@@ -1206,7 +1194,7 @@ module Gitlab
               rspec: { script: 'rspec', when: when_state }
             })
 
-            config_processor = Gitlab::Ci::YamlProcessor.new(config)
+            config_processor = Gitlab::Ci::YamlProcessor.new(config).execute
             builds = config_processor.stage_builds_attributes("test")
 
             expect(builds.size).to eq(1)
@@ -1250,7 +1238,7 @@ module Gitlab
                              variables: { 'VAR1' => 1 } })
         end
 
-        let(:config_processor) { Gitlab::Ci::YamlProcessor.new(config) }
+        let(:config_processor) { Gitlab::Ci::YamlProcessor.new(config).execute }
         let(:builds) { config_processor.stage_builds_attributes('test') }
 
         context 'when job is parallelized' do
@@ -1366,7 +1354,7 @@ module Gitlab
                                }
                              })
 
-          config_processor = Gitlab::Ci::YamlProcessor.new(config)
+          config_processor = Gitlab::Ci::YamlProcessor.new(config).execute
 
           expect(config_processor.stage_builds_attributes("test").size).to eq(1)
           expect(config_processor.stage_builds_attributes("test").first[:cache]).to eq(
@@ -1388,7 +1376,7 @@ module Gitlab
               }
             })
 
-          config_processor = Gitlab::Ci::YamlProcessor.new(config)
+          config_processor = Gitlab::Ci::YamlProcessor.new(config).execute
 
           expect(config_processor.stage_builds_attributes("test").size).to eq(1)
           expect(config_processor.stage_builds_attributes("test").first[:cache]).to eq(
@@ -1407,7 +1395,7 @@ module Gitlab
                                }
                              })
 
-          config_processor = Gitlab::Ci::YamlProcessor.new(config)
+          config_processor = Gitlab::Ci::YamlProcessor.new(config).execute
 
           expect(config_processor.stage_builds_attributes('test').size).to eq(1)
           expect(config_processor.stage_builds_attributes('test').first[:cache]).to eq(
@@ -1430,7 +1418,7 @@ module Gitlab
             }
           )
 
-          config_processor = Gitlab::Ci::YamlProcessor.new(config)
+          config_processor = Gitlab::Ci::YamlProcessor.new(config).execute
 
           expect(config_processor.stage_builds_attributes('test').size).to eq(1)
           expect(config_processor.stage_builds_attributes('test').first[:cache]).to eq(
@@ -1453,7 +1441,7 @@ module Gitlab
             }
           )
 
-          config_processor = Gitlab::Ci::YamlProcessor.new(config)
+          config_processor = Gitlab::Ci::YamlProcessor.new(config).execute
 
           expect(config_processor.stage_builds_attributes('test').size).to eq(1)
           expect(config_processor.stage_builds_attributes('test').first[:cache]).to eq(
@@ -1473,7 +1461,7 @@ module Gitlab
                                }
                              })
 
-          config_processor = Gitlab::Ci::YamlProcessor.new(config)
+          config_processor = Gitlab::Ci::YamlProcessor.new(config).execute
 
           expect(config_processor.stage_builds_attributes("test").size).to eq(1)
           expect(config_processor.stage_builds_attributes("test").first[:cache]).to eq(
@@ -1503,7 +1491,7 @@ module Gitlab
                                }
                              })
 
-          config_processor = Gitlab::Ci::YamlProcessor.new(config)
+          config_processor = Gitlab::Ci::YamlProcessor.new(config).execute
 
           expect(config_processor.stage_builds_attributes("test").size).to eq(1)
           expect(config_processor.stage_builds_attributes("test").first).to eq({
@@ -1539,7 +1527,7 @@ module Gitlab
                                 }
                               })
 
-          config_processor = Gitlab::Ci::YamlProcessor.new(config)
+          config_processor = Gitlab::Ci::YamlProcessor.new(config).execute
           builds = config_processor.stage_builds_attributes("test")
 
           expect(builds.size).to eq(1)
@@ -1555,7 +1543,7 @@ module Gitlab
                                  }
                                })
 
-            config_processor = Gitlab::Ci::YamlProcessor.new(config)
+            config_processor = Gitlab::Ci::YamlProcessor.new(config).execute
             builds = config_processor.stage_builds_attributes("test")
 
             expect(builds.size).to eq(1)
@@ -1591,14 +1579,14 @@ module Gitlab
                   - my/test/something
           YAML
 
-          attributes = Gitlab::Ci::YamlProcessor.new(config).build_attributes('test')
+          attributes = Gitlab::Ci::YamlProcessor.new(config).execute.build_attributes('test')
 
           expect(attributes.dig(*%i[options artifacts exclude])).to eq(%w[my/test/something])
         end
       end
 
       describe "release" do
-        let(:processor) { Gitlab::Ci::YamlProcessor.new(YAML.dump(config)) }
+        let(:processor) { Gitlab::Ci::YamlProcessor.new(YAML.dump(config)).execute }
         let(:config) do
           {
             stages: %w[build test release],
@@ -1643,7 +1631,7 @@ module Gitlab
           }
         end
 
-        subject { Gitlab::Ci::YamlProcessor.new(YAML.dump(config)) }
+        subject { Gitlab::Ci::YamlProcessor.new(YAML.dump(config)).execute }
 
         let(:builds) { subject.stage_builds_attributes('deploy') }
 
@@ -1753,7 +1741,7 @@ module Gitlab
           }
         end
 
-        subject { Gitlab::Ci::YamlProcessor.new(YAML.dump(config)) }
+        subject { Gitlab::Ci::YamlProcessor.new(YAML.dump(config)).execute }
 
         let(:builds) { subject.stage_builds_attributes('deploy') }
 
@@ -1795,24 +1783,24 @@ module Gitlab
           }
         end
 
-        subject { Gitlab::Ci::YamlProcessor.new(YAML.dump(config)) }
+        subject { Gitlab::Ci::YamlProcessor.new(YAML.dump(config)).execute }
 
         context 'no dependencies' do
           let(:dependencies) { }
 
-          it { expect { subject }.not_to raise_error }
+          it { is_expected.to be_valid }
         end
 
         context 'dependencies to builds' do
           let(:dependencies) { %w(build1 build2) }
 
-          it { expect { subject }.not_to raise_error }
+          it { is_expected.to be_valid }
         end
 
         context 'dependencies to builds defined as symbols' do
           let(:dependencies) { [:build1, :build2] }
 
-          it { expect { subject }.not_to raise_error }
+          it { is_expected.to be_valid }
         end
 
         context 'undefined dependency' do
@@ -1868,10 +1856,10 @@ module Gitlab
           }
         end
 
-        subject { Gitlab::Ci::YamlProcessor.new(YAML.dump(config)) }
+        subject { Gitlab::Ci::YamlProcessor.new(YAML.dump(config)).execute }
 
         context 'no needs' do
-          it { expect { subject }.not_to raise_error }
+          it { is_expected.to be_valid }
         end
 
         context 'needs two builds' do
@@ -2063,7 +2051,7 @@ module Gitlab
       end
 
       context 'with when/rules conflict' do
-        subject { Gitlab::Ci::YamlProcessor.new(YAML.dump(config)) }
+        subject { Gitlab::Ci::YamlProcessor.new(YAML.dump(config)).execute }
 
         let(:config) do
           {
@@ -2079,9 +2067,7 @@ module Gitlab
           }
         end
 
-        it 'raises no exceptions' do
-          expect { subject }.not_to raise_error
-        end
+        it { is_expected.to be_valid }
 
         it 'returns all jobs regardless of their inclusion' do
           expect(subject.builds.count).to eq(config.keys.count)
@@ -2120,7 +2106,7 @@ module Gitlab
       end
 
       describe "Hidden jobs" do
-        let(:config_processor) { Gitlab::Ci::YamlProcessor.new(config) }
+        let(:config_processor) { Gitlab::Ci::YamlProcessor.new(config).execute }
 
         subject { config_processor.stage_builds_attributes("test") }
 
@@ -2167,7 +2153,7 @@ module Gitlab
       end
 
       describe "YAML Alias/Anchor" do
-        let(:config_processor) { Gitlab::Ci::YamlProcessor.new(config) }
+        let(:config_processor) { Gitlab::Ci::YamlProcessor.new(config).execute }
 
         subject { config_processor.stage_builds_attributes("build") }
 
@@ -2264,7 +2250,7 @@ module Gitlab
             })
           end
 
-          it { expect { subject }.not_to raise_error }
+          it { is_expected.to be_valid }
         end
 
         context 'when job is not specified specified while artifact is' do
@@ -2277,11 +2263,7 @@ module Gitlab
             })
           end
 
-          it do
-            expect { subject }.to raise_error(
-              described_class::ValidationError,
-              /include config must specify the job where to fetch the artifact from/)
-          end
+          it_behaves_like 'returns errors', /include config must specify the job where to fetch the artifact from/
         end
 
         context 'when include is a string' do
@@ -2297,12 +2279,12 @@ module Gitlab
             })
           end
 
-          it { expect { subject }.not_to raise_error }
+          it { is_expected.to be_valid }
         end
       end
 
       describe "Error handling" do
-        subject { described_class.new(config) }
+        subject { described_class.new(config).execute }
 
         context 'when YAML syntax is invalid' do
           let(:config) { 'invalid: yaml: test' }
@@ -2651,8 +2633,8 @@ module Gitlab
         end
       end
 
-      describe '.new_with_validation_errors' do
-        subject { Gitlab::Ci::YamlProcessor.new_with_validation_errors(content) }
+      describe '#execute' do
+        subject { Gitlab::Ci::YamlProcessor.new(content).execute }
 
         context 'when the YAML could not be parsed' do
           let(:content) { YAML.dump('invalid: yaml: test') }
@@ -2660,7 +2642,6 @@ module Gitlab
           it 'returns errors and empty configuration' do
             expect(subject.valid?).to eq(false)
             expect(subject.errors).to eq(['Invalid configuration format'])
-            expect(subject.config).to be_blank
           end
         end
 
@@ -2670,7 +2651,6 @@ module Gitlab
           it 'returns errors and empty configuration' do
             expect(subject.valid?).to eq(false)
             expect(subject.errors).to eq(['jobs:rspec:tags config should be an array of strings'])
-            expect(subject.config).to be_blank
           end
         end
 
@@ -2682,7 +2662,6 @@ module Gitlab
             expect(subject.errors).to contain_exactly(
               'jobs:rspec config contains unknown keys: bad_tags',
               'jobs:rspec rules should be an array of hashes')
-            expect(subject.config).to be_blank
           end
         end
 
@@ -2692,7 +2671,6 @@ module Gitlab
           it 'returns errors and empty configuration' do
             expect(subject.valid?).to eq(false)
             expect(subject.errors).to eq(['Please provide content of .gitlab-ci.yml'])
-            expect(subject.config).to be_blank
           end
         end
 
@@ -2702,7 +2680,6 @@ module Gitlab
           it 'returns errors and empty configuration' do
             expect(subject.valid?).to eq(false)
             expect(subject.errors).to eq(['Unknown alias: bad_alias'])
-            expect(subject.config).to be_blank
           end
         end
 
@@ -2712,7 +2689,7 @@ module Gitlab
           it 'returns errors and empty configuration' do
             expect(subject.valid?).to eq(true)
             expect(subject.errors).to be_empty
-            expect(subject.config).to be_present
+            expect(subject.builds).to be_present
           end
         end
       end

spec/lib/gitlab/import_export/fast_hash_serializer_spec.rb

@@ -133,12 +133,6 @@ RSpec.describe Gitlab::ImportExport::FastHashSerializer do
     expect(builds_count).to eq(1)
   end
 
-  it 'has no when YML attributes but only the DB column' do
-    expect_any_instance_of(Gitlab::Ci::YamlProcessor).not_to receive(:build_attributes)
-
-    subject
-  end
-
   it 'has pipeline commits' do
     expect(subject['ci_pipelines']).not_to be_empty
   end

spec/lib/gitlab/import_export/project/tree_saver_spec.rb

@@ -381,12 +381,6 @@ RSpec.describe Gitlab::ImportExport::Project::TreeSaver do
 
         expect(project_tree_saver.save).to be true
       end
-
-      it 'has no when YML attributes but only the DB column' do
-        expect_any_instance_of(Gitlab::Ci::YamlProcessor).not_to receive(:build_attributes)
-
-        project_tree_saver.save
-      end
     end
   end