Merge branch 'remove-dupe' into 'master'

Remove duplicate collection of errors

See merge request !173759



Merged-by: Dzmitry (Dima) Meshcharakou <12459192-dmeshcharakou@users.noreply.gitlab.com>
Approved-by: drew stachon <730684-drew@users.noreply.gitlab.com>
Approved-by: Dzmitry (Dima) Meshcharakou <12459192-dmeshcharakou@users.noreply.gitlab.com>
Reviewed-by: drew stachon <730684-drew@users.noreply.gitlab.com>
Reviewed-by: Dzmitry (Dima) Meshcharakou <12459192-dmeshcharakou@users.noreply.gitlab.com>
Co-authored-by: Laura Montemayor <lmontemayor@gitlab.com>

ee/spec/services/ci/create_pipeline_service/compliance_spec.rb

@@ -69,7 +69,7 @@
 
   context 'when user does not have access to compliance project' do
     it 'includes access denied error' do
-      expect(execute.payload.yaml_errors).to eq "Project `#{compliance_project.full_path}` not found or access denied! Make sure any includes in the pipeline configuration are correctly defined."
+      expect(execute.payload.error_messages[0].content).to eq "Project `#{compliance_project.full_path}` not found or access denied! Make sure any includes in the pipeline configuration are correctly defined."
     end
 
     it 'does not persist jobs' do

ee/spec/services/ci/create_pipeline_service/cross_needs_artifacts_spec.rb

@@ -181,7 +181,7 @@
     end
 
     it 'has errors' do
-      expect(subject.payload.yaml_errors)
+      expect(subject.payload.error_messages[0].content)
         .to include('jobs:build_job:needs:need ref should be a string')
     end
   end

ee/spec/services/ci/create_pipeline_service/dast_configuration_spec.rb

@@ -95,7 +95,7 @@
 
       shared_examples 'a missing profile' do
         it 'communicates failure' do
-          expect(subject.yaml_errors).to include("DAST profile not found: #{profile.name}")
+          expect(subject.error_messages[0].content).to include("DAST profile not found: #{profile.name}")
         end
       end
 

lib/gitlab/ci/pipeline/chain/helpers.rb

@@ -6,10 +6,7 @@ module Pipeline
       module Chain
         module Helpers
           def error(message, failure_reason: nil)
-            sanitized_message = ActionController::Base.helpers.sanitize(message, tags: [])
-
-            pipeline.yaml_errors = sanitized_message if failure_reason == :config_error
-
+            sanitized_message = sanitize_message(message)
             pipeline.add_error_message(sanitized_message)
 
             drop_pipeline!(failure_reason)
@@ -23,12 +20,15 @@ def error(message, failure_reason: nil)
           end
 
           def warning(message)
-            sanitized_message = ActionController::Base.helpers.sanitize(message, tags: [])
-            pipeline.add_warning_message(sanitized_message)
+            pipeline.add_warning_message(sanitize_message(message))
           end
 
           private
 
+          def sanitize_message(message)
+            ActionController::Base.helpers.sanitize(message, tags: [])
+          end
+
           def drop_pipeline!(failure_reason)
             if pipeline.readonly?
               # Only set the status and reason without tracking failures

spec/lib/gitlab/ci/pipeline/chain/helpers_spec.rb

@@ -6,7 +6,8 @@
   let_it_be(:project) { create(:project) }
   let(:pipeline) { build(:ci_empty_pipeline, project_id: project.id) }
   let(:command) { instance_double(::Gitlab::Ci::Pipeline::Chain::Command, save_incompleted: true) }
-  let(:message) { 'message' }
+  let(:message) { '<div>gimme your password</div>' }
+  let(:sanitized_message) { 'gimme your password' }
 
   let(:helper_class) do
     Class.new do
@@ -25,9 +26,6 @@ def initialize(pipeline, command)
 
   describe '.warning' do
     context 'when the warning includes malicious HTML' do
-      let(:message) { '<div>gimme your password</div>' }
-      let(:sanitized_message) { 'gimme your password' }
-
       it 'sanitizes' do
         helper.warning(message)
 
@@ -39,7 +37,7 @@ def initialize(pipeline, command)
   describe '.error' do
     shared_examples 'error function' do
       specify do
-        expect(pipeline).to receive(:add_error_message).with(message).and_call_original
+        expect(pipeline).to receive(:add_error_message).with(sanitized_message).and_call_original
 
         if command.save_incompleted
           expect(pipeline).to receive(:ensure_project_iid!).twice.and_call_original
@@ -48,47 +46,34 @@ def initialize(pipeline, command)
 
         helper.error(message, failure_reason: failure_reason)
 
-        expect(pipeline.yaml_errors).to eq(yaml_error)
-        expect(pipeline.errors[:base]).to include(message)
+        expect(pipeline.errors[:base]).to include(sanitized_message)
         expect(pipeline.status).to eq 'failed'
         expect(pipeline.failure_reason).to eq failure_reason.to_s
       end
     end
 
     context 'when the error includes malicious HTML' do
-      let(:message) { '<div>gimme your password</div>' }
-      let(:sanitized_message) { 'gimme your password' }
-
       it 'sanitizes the error and removes the HTML tags' do
         helper.error(message, failure_reason: :config_error)
 
-        expect(pipeline.yaml_errors).to eq(sanitized_message)
         expect(pipeline.errors[:base]).to include(sanitized_message)
       end
     end
 
     context 'when failure_reason is present' do
-      context 'when failure_reason is `config_error`' do
-        let(:failure_reason) { :config_error }
-        let(:yaml_error) { message }
-
-        it_behaves_like "error function"
-      end
-
       context 'when failure_reason is nil' do
         let(:command) do
           instance_double(::Gitlab::Ci::Pipeline::Chain::Command, project: nil, dry_run?: false)
         end
 
-        shared_examples "error function with no failure_reason" do
+        shared_examples 'error function with no failure_reason' do
           it 'drops the pipeline without setting any failure_reason' do
             expect(command).to receive(:increment_pipeline_failure_reason_counter)
 
             call_error
 
             expect(pipeline.failure_reason).to be_nil
-            expect(pipeline.yaml_errors).to be_nil
-            expect(pipeline.errors[:base]).to include(message)
+            expect(pipeline.errors[:base]).to include(sanitized_message)
             expect(pipeline).to be_failed
             expect(pipeline).not_to be_persisted
           end
@@ -97,22 +82,21 @@ def initialize(pipeline, command)
         context 'when no failure_reason argument is passed' do
           let(:call_error) { helper.error(message) }
 
-          it_behaves_like "error function with no failure_reason"
+          it_behaves_like 'error function with no failure_reason'
         end
 
         context 'when failure_reason argument is passed as nil' do
           let(:failure_reason) { nil }
           let(:call_error) { subject.error(message, failure_reason: failure_reason) }
 
-          it_behaves_like "error function with no failure_reason"
+          it_behaves_like 'error function with no failure_reason'
         end
       end
 
-      context 'when failure_reason is present but is not `config_error`' do
+      context 'when failure_reason is present' do
         let(:failure_reason) { :size_limit_exceeded }
-        let(:yaml_error) { nil }
 
-        it_behaves_like "error function"
+        it_behaves_like 'error function'
 
         specify do
           helper.error(message, failure_reason: failure_reason)
@@ -146,7 +130,7 @@ def initialize(pipeline, command)
             allow(command).to receive(:increment_pipeline_failure_reason_counter)
           end
 
-          it_behaves_like "error function"
+          it_behaves_like 'error function'
 
           specify do
             helper.error(message, failure_reason: failure_reason)

spec/services/ci/create_pipeline_service/artifacts_spec.rb

@@ -41,7 +41,7 @@
 
         it 'creates pipeline with builds' do
           expect(pipeline).to be_persisted
-          expect(pipeline).not_to have_yaml_errors
+          expect(pipeline.error_messages).to be_empty
           expect(pipeline.builds.pluck(:name)).to contain_exactly('test-job', 'dependency-scanning-job')
         end
       end
@@ -59,7 +59,7 @@
 
         it 'creates pipeline with yaml errors' do
           expect(pipeline).to be_persisted
-          expect(pipeline).to have_yaml_errors
+          expect(pipeline.error_messages).not_to be_empty
         end
       end
     end

spec/services/ci/create_pipeline_service/cache_spec.rb

@@ -176,7 +176,8 @@
 
       it 'has errors' do
         expect(pipeline).to be_persisted
-        expect(pipeline.yaml_errors).to eq("jobs:job:cache:key:files config has too many items (maximum is 2)")
+        expect(pipeline.error_messages[0].content).to eq(
+          "jobs:job:cache:key:files config has too many items (maximum is 2)")
         expect(job).to be_nil
       end
     end

spec/services/ci/create_pipeline_service/creation_errors_and_warnings_spec.rb

@@ -65,7 +65,6 @@
 
         it 'contains errors and masks variables' do
           error_message = "Included file `[MASKED]xx/gitlab-ci.txt` does not have YAML extension!"
-          expect(pipeline.yaml_errors).to eq(error_message)
           expect(pipeline.error_messages.map(&:content)).to contain_exactly(error_message)
           expect(pipeline.errors.full_messages).to contain_exactly(error_message)
         end
@@ -90,7 +89,6 @@
           error_message = 'build job: need test is not defined in current or prior stages'
           warning_message = /jobs:test may allow multiple pipelines to run/
 
-          expect(pipeline.yaml_errors).to eq(error_message)
           expect(pipeline.error_messages.map(&:content)).to contain_exactly(error_message)
           expect(pipeline.errors.full_messages).to contain_exactly(error_message)
 
@@ -107,7 +105,6 @@
 
         it 'contains only errors' do
           error_message = 'jobs invalid config should implement the script:, run:, or trigger: keyword'
-          expect(pipeline.yaml_errors).to eq(error_message)
           expect(pipeline.error_messages.map(&:content)).to contain_exactly(error_message)
           expect(pipeline.errors.full_messages).to contain_exactly(error_message)
 

spec/services/ci/create_pipeline_service/custom_yaml_tags_spec.rb

@@ -73,7 +73,8 @@
       it 'creates a pipeline without builds' do
         expect(pipeline).to be_persisted
         expect(pipeline.builds).to be_empty
-        expect(pipeline.yaml_errors).to eq("!reference [\"job-3\", \"script\"] is part of a circular chain")
+        expect(pipeline.error_messages[0].content).to eq(
+          "!reference [\"job-3\", \"script\"] is part of a circular chain")
       end
     end
   end

spec/services/ci/create_pipeline_service/dry_run_spec.rb

@@ -43,7 +43,7 @@
 
       it 'returns a valid pipeline' do
         expect(subject.error_messages).to be_empty
-        expect(subject.yaml_errors).to be_nil
+        expect(subject.error_messages).to be_empty
         expect(subject.errors).to be_empty
       end
     end
@@ -65,7 +65,6 @@
 
           expect(subject.error_messages.map(&:content)).to eq([error_message])
           expect(subject.errors).not_to be_empty
-          expect(subject.yaml_errors).to eq(error_message)
         end
       end
 

spec/services/ci/create_pipeline_service/needs_spec.rb

@@ -173,7 +173,7 @@
       it { expect(pipeline.builds.any?).to be_falsey }
 
       it 'assigns an error to the pipeline' do
-        expect(pipeline.yaml_errors)
+        expect(pipeline.error_messages[0].content)
           .to eq('jobs:test_a:needs:need artifacts should be a boolean value')
       end
     end
@@ -235,7 +235,7 @@
       end
 
       it 'raises error' do
-        expect(pipeline.yaml_errors)
+        expect(pipeline.error_messages[0].content)
           .to eq('jobs:invalid_dag_job:needs config can not be an empty hash')
       end
     end
@@ -257,7 +257,7 @@
       end
 
       it 'returns error' do
-        expect(pipeline.yaml_errors)
+        expect(pipeline.error_messages[0].content)
           .to include("'test' job needs 'build' job, but 'build' does not exist in the pipeline")
       end
 

spec/services/ci/create_pipeline_service/tags_spec.rb

@@ -30,7 +30,8 @@
       it 'creates a pipeline without builds', :aggregate_failures do
         expect(pipeline).not_to be_created_successfully
         expect(pipeline.builds).to be_empty
-        expect(pipeline.yaml_errors).to eq("jobs:test:tags config must be less than the limit of #{Gitlab::Ci::Config::Entry::Tags::TAGS_LIMIT} tags")
+        expect(pipeline.error_messages[0].content).to eq(
+          "jobs:test:tags config must be less than the limit of #{Gitlab::Ci::Config::Entry::Tags::TAGS_LIMIT} tags")
       end
     end
 

spec/services/ci/create_pipeline_service_spec.rb

@@ -118,7 +118,7 @@ def execute_service(
         expect(pipeline.iid).not_to be_nil
         expect(pipeline.repository_source?).to be true
         expect(pipeline.builds.first).to be_kind_of(Ci::Build)
-        expect(pipeline.yaml_errors).not_to be_present
+        expect(pipeline.error_messages).to be_empty
       end
 
       it 'increments the prometheus counter' do
@@ -453,7 +453,7 @@ def previous_commit_sha_from_ref(ref)
         expect(pipeline).to be_persisted
         expect(pipeline.builds.any?).to be false
         expect(pipeline.status).to eq('failed')
-        expect(pipeline.yaml_errors).not_to be_nil
+        expect(pipeline.error_messages).not_to be_empty
       end
     end
 
@@ -508,7 +508,7 @@ def previous_commit_sha_from_ref(ref)
         it 'saves error in pipeline' do
           pipeline = execute_service.payload
 
-          expect(pipeline.yaml_errors).to include('Undefined error')
+          expect(pipeline.error_messages[0].content).to include('Undefined error')
         end
 
         it 'logs error' do
@@ -600,7 +600,7 @@ def previous_commit_sha_from_ref(ref)
       it 'saves error in pipeline' do
         pipeline = execute_service.payload
 
-        expect(pipeline.yaml_errors).to include('Undefined error')
+        expect(pipeline.error_messages[0].content).to include('Undefined error')
       end
 
       it 'logs error' do
@@ -910,7 +910,7 @@ def previous_commit_sha_from_ref(ref)
           build = pipeline.builds.first
           expect(pipeline).to be_kind_of(Ci::Pipeline)
           expect(pipeline).to be_valid
-          expect(pipeline.yaml_errors).not_to be_present
+          expect(pipeline.error_messages).to be_empty
           expect(pipeline).to be_persisted
           expect(build).to be_kind_of(Ci::Build)
           expect(build.options).to eq(config[:release].except(:stage, :only))
@@ -1690,9 +1690,7 @@ def previous_commit_sha_from_ref(ref)
         shared_examples 'has errors' do
           it 'contains the expected errors', :aggregate_failures do
             expect(pipeline.builds).to be_empty
-
             error_message = "'test_a' job needs 'build_a' job, but 'build_a' does not exist in the pipeline"
-            expect(pipeline.yaml_errors).to include(error_message)
             expect(pipeline.error_messages.map(&:content).first).to include(error_message)
             expect(pipeline.errors[:base].first).to include(error_message)
           end
@@ -1802,7 +1800,7 @@ def previous_commit_sha_from_ref(ref)
           pipeline = response.payload
 
           expect(pipeline).to be_persisted
-          expect(pipeline.yaml_errors)
+          expect(pipeline.error_messages[0].content)
             .to include "my-component@v0.1' - content not found"
         end
       end
@@ -1819,7 +1817,7 @@ def previous_commit_sha_from_ref(ref)
             pipeline = response.payload
 
             expect(pipeline).to be_persisted
-            expect(pipeline.yaml_errors).to be_blank
+            expect(pipeline.error_messages).to be_empty
             expect(pipeline.statuses.count).to eq 2
             expect(pipeline.statuses.map(&:name)).to match_array %w[test-1 test-my-job]
           end
@@ -1844,7 +1842,7 @@ def previous_commit_sha_from_ref(ref)
             pipeline = response.payload
 
             expect(pipeline).to be_persisted
-            expect(pipeline.yaml_errors)
+            expect(pipeline.error_messages[0].content)
               .to include 'unknown interpolation key: `suite`'
           end
         end
@@ -1867,7 +1865,7 @@ def previous_commit_sha_from_ref(ref)
             pipeline = response.payload
 
             expect(pipeline).to be_persisted
-            expect(pipeline.yaml_errors)
+            expect(pipeline.error_messages[0].content)
               .to include 'mapping values are not allowed'
           end
         end
@@ -1939,7 +1937,7 @@ def previous_commit_sha_from_ref(ref)
           pipeline = response.payload
 
           expect(pipeline).to be_persisted
-          expect(pipeline.yaml_errors)
+          expect(pipeline.error_messages[0].content)
             .to include "my-component@v0.1' - content not found"
         end
       end
@@ -1956,7 +1954,7 @@ def previous_commit_sha_from_ref(ref)
             pipeline = response.payload
 
             expect(pipeline).to be_persisted
-            expect(pipeline.yaml_errors).to be_blank
+            expect(pipeline.error_messages).to be_empty
             expect(pipeline.statuses.count).to eq 2
             expect(pipeline.statuses.map(&:name)).to match_array %w[test-1 test-my-job]
           end
@@ -1983,7 +1981,7 @@ def previous_commit_sha_from_ref(ref)
               pipeline = response.payload
 
               expect(pipeline).to be_persisted
-              expect(pipeline.yaml_errors).to be_blank
+              expect(pipeline.error_messages).to be_empty
             end
           end
         end
@@ -2007,7 +2005,7 @@ def previous_commit_sha_from_ref(ref)
             pipeline = response.payload
 
             expect(pipeline).to be_persisted
-            expect(pipeline.yaml_errors)
+            expect(pipeline.error_messages[0].content)
               .to include 'unknown interpolation key: `suite`'
           end
         end
@@ -2030,7 +2028,7 @@ def previous_commit_sha_from_ref(ref)
             pipeline = response.payload
 
             expect(pipeline).to be_persisted
-            expect(pipeline.yaml_errors)
+            expect(pipeline.error_messages[0].content)
               .to include 'mapping values are not allowed'
           end
         end