Merge branch...

Merge branch '353125-feature-flag-enable-display-of-security-report-validation-warnings' into 'master'

Remove show_report_validation_warnings feature flag

See merge request gitlab-org/gitlab!82405

config/feature_flags/development/show_report_validation_warnings.yml

----
-name: show_report_validation_warnings
-introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/80930
-rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/353125
-milestone: '14.9'
-type: development
-group: group::threat insights
-default_enabled: true

lib/gitlab/ci/parsers/security/common.rb

@@ -43,26 +43,25 @@ module Gitlab
           attr_reader :json_data, :report, :validate
 
           def valid?
-            if Feature.enabled?(:show_report_validation_warnings, default_enabled: :yaml)
-              # We want validation to happen regardless of VALIDATE_SCHEMA CI variable
-              schema_validation_passed = schema_validator.valid?
-
-              if validate
-                schema_validator.errors.each { |error| report.add_error('Schema', error) } unless schema_validation_passed
-
-                schema_validation_passed
-              else
-                # We treat all schema validation errors as warnings
-                schema_validator.errors.each { |error| report.add_warning('Schema', error) }
-
-                true
-              end
+            # We want validation to happen regardless of VALIDATE_SCHEMA
+            # CI variable.
+            #
+            # Previously it controlled BOTH validation and enforcement of
+            # schema validation result.
+            #
+            # After 15.0 we will enforce schema validation by default
+            # See: https://gitlab.com/groups/gitlab-org/-/epics/6968
+            schema_validation_passed = schema_validator.valid?
+
+            if validate
+              schema_validator.errors.each { |error| report.add_error('Schema', error) } unless schema_validation_passed
+
+              schema_validation_passed
             else
-              return true if !validate || schema_validator.valid?
-
-              schema_validator.errors.each { |error| report.add_error('Schema', error) }
+              # We treat all schema validation errors as warnings
+              schema_validator.errors.each { |error| report.add_warning('Schema', error) }
 
-              false
+              true
             end
           end
 

spec/lib/gitlab/ci/parsers/security/common_spec.rb

@@ -38,172 +38,102 @@ RSpec.describe Gitlab::Ci::Parsers::Security::Common do
           allow(validator_class).to receive(:new).and_call_original
         end
 
-        context 'when show_report_validation_warnings is enabled' do
-          before do
-            stub_feature_flags(show_report_validation_warnings: true)
-          end
-
-          context 'when the validate flag is set to `false`' do
-            let(:validate) { false }
-            let(:valid?) { false }
-            let(:errors) { ['foo'] }
-
-            before do
-              allow_next_instance_of(validator_class) do |instance|
-                allow(instance).to receive(:valid?).and_return(valid?)
-                allow(instance).to receive(:errors).and_return(errors)
-              end
-
-              allow(parser).to receive_messages(create_scanner: true, create_scan: true)
-            end
-
-            it 'instantiates the validator with correct params' do
-              parse_report
-
-              expect(validator_class).to have_received(:new).with(report.type, {}, report.version)
-            end
-
-            context 'when the report data is not valid according to the schema' do
-              it 'adds warnings to the report' do
-                expect { parse_report }.to change { report.warnings }.from([]).to([{ message: 'foo', type: 'Schema' }])
-              end
+        context 'when the validate flag is set to `false`' do
+          let(:validate) { false }
+          let(:valid?) { false }
+          let(:errors) { ['foo'] }
 
-              it 'keeps the execution flow as normal' do
-                parse_report
-
-                expect(parser).to have_received(:create_scanner)
-                expect(parser).to have_received(:create_scan)
-              end
+          before do
+            allow_next_instance_of(validator_class) do |instance|
+              allow(instance).to receive(:valid?).and_return(valid?)
+              allow(instance).to receive(:errors).and_return(errors)
             end
 
-            context 'when the report data is valid according to the schema' do
-              let(:valid?) { true }
-              let(:errors) { [] }
-
-              it 'does not add warnings to the report' do
-                expect { parse_report }.not_to change { report.errors }
-              end
-
-              it 'keeps the execution flow as normal' do
-                parse_report
-
-                expect(parser).to have_received(:create_scanner)
-                expect(parser).to have_received(:create_scan)
-              end
-            end
+            allow(parser).to receive_messages(create_scanner: true, create_scan: true)
           end
 
-          context 'when the validate flag is set to `true`' do
-            let(:validate) { true }
-            let(:valid?) { false }
-            let(:errors) { ['foo'] }
+          it 'instantiates the validator with correct params' do
+            parse_report
 
-            before do
-              allow_next_instance_of(validator_class) do |instance|
-                allow(instance).to receive(:valid?).and_return(valid?)
-                allow(instance).to receive(:errors).and_return(errors)
-              end
+            expect(validator_class).to have_received(:new).with(report.type, {}, report.version)
+          end
 
-              allow(parser).to receive_messages(create_scanner: true, create_scan: true)
+          context 'when the report data is not valid according to the schema' do
+            it 'adds warnings to the report' do
+              expect { parse_report }.to change { report.warnings }.from([]).to([{ message: 'foo', type: 'Schema' }])
             end
 
-            it 'instantiates the validator with correct params' do
+            it 'keeps the execution flow as normal' do
               parse_report
 
-              expect(validator_class).to have_received(:new).with(report.type, {}, report.version)
+              expect(parser).to have_received(:create_scanner)
+              expect(parser).to have_received(:create_scan)
             end
+          end
 
-            context 'when the report data is not valid according to the schema' do
-              it 'adds errors to the report' do
-                expect { parse_report }.to change { report.errors }.from([]).to([{ message: 'foo', type: 'Schema' }])
-              end
-
-              it 'does not try to create report entities' do
-                parse_report
+          context 'when the report data is valid according to the schema' do
+            let(:valid?) { true }
+            let(:errors) { [] }
 
-                expect(parser).not_to have_received(:create_scanner)
-                expect(parser).not_to have_received(:create_scan)
-              end
+            it 'does not add warnings to the report' do
+              expect { parse_report }.not_to change { report.errors }
             end
 
-            context 'when the report data is valid according to the schema' do
-              let(:valid?) { true }
-              let(:errors) { [] }
-
-              it 'does not add errors to the report' do
-                expect { parse_report }.not_to change { report.errors }.from([])
-              end
-
-              it 'keeps the execution flow as normal' do
-                parse_report
+            it 'keeps the execution flow as normal' do
+              parse_report
 
-                expect(parser).to have_received(:create_scanner)
-                expect(parser).to have_received(:create_scan)
-              end
+              expect(parser).to have_received(:create_scanner)
+              expect(parser).to have_received(:create_scan)
             end
           end
         end
 
-        context 'when show_report_validation_warnings is disabled' do
-          before do
-            stub_feature_flags(show_report_validation_warnings: false)
-          end
-
-          context 'when the validate flag is set as `false`' do
-            let(:validate) { false }
+        context 'when the validate flag is set to `true`' do
+          let(:validate) { true }
+          let(:valid?) { false }
+          let(:errors) { ['foo'] }
 
-            it 'does not run the validation logic' do
-              parse_report
-
-              expect(validator_class).not_to have_received(:new)
+          before do
+            allow_next_instance_of(validator_class) do |instance|
+              allow(instance).to receive(:valid?).and_return(valid?)
+              allow(instance).to receive(:errors).and_return(errors)
             end
+
+            allow(parser).to receive_messages(create_scanner: true, create_scan: true)
           end
 
-          context 'when the validate flag is set as `true`' do
-            let(:validate) { true }
-            let(:valid?) { false }
+          it 'instantiates the validator with correct params' do
+            parse_report
 
-            before do
-              allow_next_instance_of(validator_class) do |instance|
-                allow(instance).to receive(:valid?).and_return(valid?)
-                allow(instance).to receive(:errors).and_return(['foo'])
-              end
+            expect(validator_class).to have_received(:new).with(report.type, {}, report.version)
+          end
 
-              allow(parser).to receive_messages(create_scanner: true, create_scan: true)
+          context 'when the report data is not valid according to the schema' do
+            it 'adds errors to the report' do
+              expect { parse_report }.to change { report.errors }.from([]).to([{ message: 'foo', type: 'Schema' }])
             end
 
-            it 'instantiates the validator with correct params' do
+            it 'does not try to create report entities' do
               parse_report
 
-              expect(validator_class).to have_received(:new).with(report.type, {}, report.version)
+              expect(parser).not_to have_received(:create_scanner)
+              expect(parser).not_to have_received(:create_scan)
             end
+          end
 
-            context 'when the report data is not valid according to the schema' do
-              it 'adds errors to the report' do
-                expect { parse_report }.to change { report.errors }.from([]).to([{ message: 'foo', type: 'Schema' }])
-              end
-
-              it 'does not try to create report entities' do
-                parse_report
+          context 'when the report data is valid according to the schema' do
+            let(:valid?) { true }
+            let(:errors) { [] }
 
-                expect(parser).not_to have_received(:create_scanner)
-                expect(parser).not_to have_received(:create_scan)
-              end
+            it 'does not add errors to the report' do
+              expect { parse_report }.not_to change { report.errors }.from([])
             end
 
-            context 'when the report data is valid according to the schema' do
-              let(:valid?) { true }
-
-              it 'does not add errors to the report' do
-                expect { parse_report }.not_to change { report.errors }.from([])
-              end
-
-              it 'keeps the execution flow as normal' do
-                parse_report
+            it 'keeps the execution flow as normal' do
+              parse_report
 
-                expect(parser).to have_received(:create_scanner)
-                expect(parser).to have_received(:create_scan)
-              end
+              expect(parser).to have_received(:create_scanner)
+              expect(parser).to have_received(:create_scan)
             end
           end
         end