Add backend support for Coverage-Check rule

Adds new reporter approval rule for code coverage.
Also renames the worker for syncing approval rules to a
more generic one because it is not security only anymore.

Changelog: added

app/models/ci/build.rb

@@ -205,6 +205,8 @@ module Ci
     end
 
     scope :with_coverage, -> { where.not(coverage: nil) }
+    scope :without_coverage, -> { where(coverage: nil) }
+    scope :with_coverage_regex, -> { where.not(coverage_regex: nil) }
 
     scope :for_project, -> (project_id) { where(project_id: project_id) }
 

app/models/ci/pipeline.rb

@@ -644,6 +644,10 @@ module Ci
       end
     end
 
+    def update_builds_coverage
+      builds.with_coverage_regex.without_coverage.each(&:update_coverage)
+    end
+
     def batch_lookup_report_artifact_for_file_type(file_type)
       latest_report_artifacts
         .values_at(*::Ci::JobArtifact.associated_file_types_for(file_type.to_s))

db/migrate/20210526155257_rename_sync_security_report_approval_rules_sidekiq_queue.rb

+# frozen_string_literal: true
+
+class RenameSyncSecurityReportApprovalRulesSidekiqQueue < ActiveRecord::Migration[6.0]
+  include Gitlab::Database::MigrationHelpers
+
+  DOWNTIME = false
+
+  def up
+    sidekiq_queue_migrate 'sync_security_reports_to_report_approval_rules', to: 'ci_sync_reports_to_report_approval_rules'
+  end
+
+  def down
+    sidekiq_queue_migrate 'ci_sync_reports_to_report_approval_rules', to: 'sync_security_reports_to_report_approval_rules'
+  end
+end

db/schema_migrations/20210526155257

+ec4cd687062118b30e516ed7c36677dda056f25c4d96c6ee0b503e457b5a18d4
\ No newline at end of file

ee/app/models/ee/ci/pipeline.rb

@@ -65,7 +65,12 @@ module EE
             pipeline.run_after_commit do
               StoreSecurityReportsWorker.perform_async(pipeline.id) if pipeline.default_branch?
               ::Security::StoreScansWorker.perform_async(pipeline.id)
-              SyncSecurityReportsToReportApprovalRulesWorker.perform_async(pipeline.id)
+            end
+          end
+
+          after_transition any => ::Ci::Pipeline.completed_statuses do |pipeline|
+            pipeline.run_after_commit do
+              ::Ci::SyncReportsToReportApprovalRulesWorker.perform_async(pipeline.id)
             end
           end
 

ee/app/services/security/sync_reports_to_approval_rules_service.rb → ee/app/services/ci/sync_reports_to_approval_rules_service.rb

 # frozen_string_literal: true
 
-module Security
-  # Service for syncing security reports results to report_approver approval rules
-  #
+module Ci
   class SyncReportsToApprovalRulesService < ::BaseService
     def initialize(pipeline)
       @pipeline = pipeline
@@ -11,6 +9,7 @@ module Security
     def execute
       sync_license_scanning_rules
       sync_vulnerability_rules
+      sync_coverage_rules
       success
     rescue StandardError => error
       log_error(
@@ -42,14 +41,35 @@ module Security
       # If we don't have reports, then we should wait until pipeline stops.
       return if reports.empty? && !pipeline.complete?
 
-      remove_required_approvals_for(ApprovalMergeRequestRule.vulnerability_report, sync_required_merge_requests)
+      remove_required_approvals_for(ApprovalMergeRequestRule.vulnerability_report, merge_requests_approved_security_reports)
+    end
+
+    def sync_coverage_rules
+      return unless pipeline.complete?
+
+      pipeline.update_builds_coverage
+      remove_required_approvals_for(ApprovalMergeRequestRule.code_coverage, merge_requests_approved_coverage)
     end
 
     def reports
       @reports ||= pipeline.security_reports
     end
 
-    def sync_required_merge_requests
+    def merge_requests_approved_coverage
+      pipeline.merge_requests_as_head_pipeline.reject do |merge_request|
+        base_pipeline = merge_request.base_pipeline
+
+        if base_pipeline.present?
+          pipeline.coverage < base_pipeline.coverage
+        else
+          # base pipeline is missing so we can't make an assumption
+          # if the coverage is better or not. We default to require approval.
+          true
+        end
+      end
+    end
+
+    def merge_requests_approved_security_reports
       pipeline.merge_requests_as_head_pipeline.reject do |merge_request|
         reports.violates_default_policy_against?(merge_request.base_pipeline&.security_reports)
       end

ee/app/services/ee/merge_requests/after_create_service.rb

@@ -15,7 +15,7 @@ module EE
       private
 
       def schedule_sync_for(pipeline_id)
-        ::SyncSecurityReportsToReportApprovalRulesWorker.perform_async(pipeline_id) if pipeline_id
+        ::Ci::SyncReportsToReportApprovalRulesWorker.perform_async(pipeline_id) if pipeline_id
       end
     end
   end

ee/app/workers/all_queues.yml

@@ -773,6 +773,15 @@
   :weight: 1
   :idempotent:
   :tags: []
+- :name: pipeline_background:ci_sync_reports_to_report_approval_rules
+  :worker_name: Ci::SyncReportsToReportApprovalRulesWorker
+  :feature_category: :continuous_integration
+  :has_external_dependencies:
+  :urgency: :high
+  :resource_boundary: :cpu
+  :weight: 1
+  :idempotent:
+  :tags: []
 - :name: pipeline_default:ci_trigger_downstream_subscriptions
   :worker_name: Ci::TriggerDownstreamSubscriptionsWorker
   :feature_category: :continuous_integration
@@ -819,15 +828,6 @@
   :weight: 2
   :idempotent:
   :tags: []
-- :name: security_scans:sync_security_reports_to_report_approval_rules
-  :worker_name: SyncSecurityReportsToReportApprovalRulesWorker
-  :feature_category: :vulnerability_management
-  :has_external_dependencies:
-  :urgency: :high
-  :resource_boundary: :cpu
-  :weight: 2
-  :idempotent:
-  :tags: []
 - :name: todos_destroyer:todos_destroyer_confidential_epic
   :worker_name: TodosDestroyer::ConfidentialEpicWorker
   :feature_category: :epics

ee/app/workers/ci/sync_reports_to_report_approval_rules_worker.rb

+# frozen_string_literal: true
+
+# Worker for syncing report_type approval_rules approvals_required
+module Ci
+  class SyncReportsToReportApprovalRulesWorker # rubocop:disable Scalability/IdempotentWorker
+    include ApplicationWorker
+
+    sidekiq_options retry: 3
+    include PipelineBackgroundQueue
+
+    urgency :high
+    worker_resource_boundary :cpu
+
+    def perform(pipeline_id)
+      pipeline = Ci::Pipeline.find_by_id(pipeline_id)
+      return unless pipeline
+
+      ::Ci::SyncReportsToApprovalRulesService.new(pipeline).execute
+    end
+  end
+end

ee/app/workers/sync_security_reports_to_report_approval_rules_worker.rb

-# frozen_string_literal: true
-
-# Worker for syncing report_type approval_rules approvals_required
-#
-class SyncSecurityReportsToReportApprovalRulesWorker # rubocop:disable Scalability/IdempotentWorker
-  include ApplicationWorker
-
-  sidekiq_options retry: 3
-  include SecurityScansQueue
-
-  urgency :high
-  worker_resource_boundary :cpu
-
-  def perform(pipeline_id)
-    pipeline = Ci::Pipeline.find_by_id(pipeline_id)
-    return unless pipeline
-
-    ::Security::SyncReportsToApprovalRulesService.new(pipeline).execute
-  end
-end

ee/spec/models/approval_merge_request_rule_spec.rb

@@ -115,6 +115,8 @@ RSpec.describe ApprovalMergeRequestRule, factory_default: :keep do
     let!(:css_rule) { create(:code_owner_rule, name: '*.css') }
     let!(:approval_rule) { create(:approval_merge_request_rule) }
     let!(:report_approver_rule) { create(:report_approver_rule) }
+    let!(:coverage_rule) { create(:report_approver_rule, :code_coverage) }
+    let!(:license_rule) { create(:report_approver_rule, :license_scanning) }
 
     describe '.not_matching_pattern' do
       it 'returns the correct rules' do
@@ -143,6 +145,20 @@ RSpec.describe ApprovalMergeRequestRule, factory_default: :keep do
           .to contain_exactly(report_approver_rule)
       end
     end
+
+    describe '.license_compliance' do
+      it 'returns the correct rules' do
+        expect(described_class.license_compliance)
+          .to contain_exactly(license_rule)
+      end
+    end
+
+    describe '.coverage' do
+      it 'returns the correct rules' do
+        expect(described_class.coverage)
+          .to contain_exactly(coverage_rule)
+      end
+    end
   end
 
   describe '.find_or_create_code_owner_rule' do

ee/spec/models/ci/pipeline_spec.rb

@@ -353,6 +353,22 @@ RSpec.describe Ci::Pipeline do
   end
 
   describe 'state machine transitions' do
+    context 'on pipeline complete' do
+      let(:pipeline) { create(:ci_empty_pipeline, status: from_status) }
+
+      Ci::HasStatus::ACTIVE_STATUSES.each do |status|
+        context "from #{status}" do
+          let(:from_status) { status }
+
+          it 'schedules Ci::SyncReportsToReportApprovalRulesWorker' do
+            expect(Ci::SyncReportsToReportApprovalRulesWorker).to receive(:perform_async).with(pipeline.id)
+
+            pipeline.succeed
+          end
+        end
+      end
+    end
+
     context 'when pipeline has downstream bridges' do
       before do
         pipeline.downstream_bridges << create(:ci_bridge)

ee/spec/services/ee/merge_requests/after_create_service_spec.rb

@@ -11,7 +11,7 @@ RSpec.describe MergeRequests::AfterCreateService do
     subject(:execute) { service_object.execute(merge_request) }
 
     before do
-      allow(SyncSecurityReportsToReportApprovalRulesWorker).to receive(:perform_async)
+      allow(Ci::SyncReportsToReportApprovalRulesWorker).to receive(:perform_async)
     end
 
     context 'when the merge request has actual_head_pipeline' do
@@ -26,7 +26,7 @@ RSpec.describe MergeRequests::AfterCreateService do
         execute
 
         expect(merge_request).to have_received(:update_head_pipeline).ordered
-        expect(SyncSecurityReportsToReportApprovalRulesWorker).to have_received(:perform_async).ordered.with(pipeline_id)
+        expect(Ci::SyncReportsToReportApprovalRulesWorker).to have_received(:perform_async).ordered.with(pipeline_id)
       end
     end
 
@@ -34,7 +34,7 @@ RSpec.describe MergeRequests::AfterCreateService do
       it 'does not schedule a background job to sync security reports to approval rules' do
         execute
 
-        expect(SyncSecurityReportsToReportApprovalRulesWorker).not_to have_received(:perform_async)
+        expect(Ci::SyncReportsToReportApprovalRulesWorker).not_to have_received(:perform_async)
       end
     end
   end

ee/spec/services/security/sync_reports_to_approval_rules_service_spec.rb

-# frozen_string_literal: true
-
-require 'spec_helper'
-
-RSpec.describe Security::SyncReportsToApprovalRulesService, '#execute' do
-  let(:merge_request) { create(:merge_request) }
-  let(:project) { merge_request.project }
-  let(:pipeline) { create(:ee_ci_pipeline, :success, project: project, merge_requests_as_head_pipeline: [merge_request]) }
-  let(:report_approver_rule) { create(:report_approver_rule, merge_request: merge_request, approvals_required: 2) }
-  let(:base_pipeline) { create(:ee_ci_pipeline, :success, project: project, ref: merge_request.target_branch, sha: merge_request.diff_base_sha) }
-
-  subject { described_class.new(pipeline).execute }
-
-  before do
-    allow(Ci::Pipeline).to receive(:find).with(pipeline.id) { pipeline }
-
-    stub_licensed_features(dependency_scanning: true, dast: true, license_scanning: true)
-  end
-
-  context 'when there are reports' do
-    context 'when pipeline passes' do
-      context 'when high-severity vulnerabilities are present' do
-        before do
-          create(:ee_ci_build, :success, :dependency_scanning, name: 'ds_job', pipeline: pipeline, project: project)
-        end
-
-        context 'when high-severity vulnerabilities already present in target branch pipeline' do
-          before do
-            create(:ee_ci_build, :success, :dependency_scanning, name: 'ds_job', pipeline: base_pipeline, project: project)
-          end
-
-          it 'lowers approvals_required count to zero' do
-            expect { subject }
-              .to change { report_approver_rule.reload.approvals_required }.from(2).to(0)
-          end
-        end
-
-        context 'when high-severity vulnerabilities do not present in target branch pipeline' do
-          it "won't change approvals_required count" do
-            expect { subject }
-              .not_to change { report_approver_rule.reload.approvals_required }
-          end
-        end
-      end
-
-      context 'when only low-severity vulnerabilities are present' do
-        before do
-          create(:ee_ci_build, :success, :low_severity_dast_report, name: 'dast_job', pipeline: pipeline, project: project)
-        end
-
-        it 'lowers approvals_required count to zero' do
-          expect { subject }
-            .to change { report_approver_rule.reload.approvals_required }.from(2).to(0)
-        end
-      end
-
-      context 'when merge_requests are merged' do
-        let!(:merge_request) { create(:merge_request, :merged) }
-
-        before do
-          create(:ee_ci_build, :success, :dast, name: 'dast_job', pipeline: pipeline, project: project)
-        end
-
-        it "won't change approvals_required count" do
-          expect { subject }
-            .not_to change { report_approver_rule.reload.approvals_required }
-        end
-      end
-
-      context "license compliance policy" do
-        let!(:license_compliance_rule) { create(:report_approver_rule, :license_scanning, merge_request: merge_request, approvals_required: 1) }
-
-        before do
-          stub_feature_flags(drop_license_management_artifact: false)
-        end
-
-        context "when a license violates the license compliance policy" do
-          let!(:software_license_policy) { create(:software_license_policy, :denied, project: project, software_license: denied_license) }
-          let(:denied_license) { create(:software_license, name: license_name) }
-          let(:license_name) { ci_build.pipeline.license_scanning_report.license_names[0] }
-
-          context 'with a new report' do
-            let!(:ci_build) { create(:ee_ci_build, :success, :license_scanning, pipeline: pipeline, project: project) }
-
-            specify { expect { subject }.not_to change { license_compliance_rule.reload.approvals_required } }
-            specify { expect(subject[:status]).to be(:success) }
-          end
-
-          context 'with an old report' do
-            let!(:ci_build) { create(:ee_ci_build, :success, :license_management, pipeline: pipeline, project: project) }
-
-            specify { expect { subject }.not_to change { license_compliance_rule.reload.approvals_required } }
-            specify { expect(subject[:status]).to be(:success) }
-          end
-        end
-
-        context "when no licenses violate the license compliance policy" do
-          context 'with a new report' do
-            let!(:ci_build) { create(:ee_ci_build, :success, :license_scanning, pipeline: pipeline, project: project) }
-
-            specify { expect { subject }.to change { license_compliance_rule.reload.approvals_required }.from(1).to(0) }
-            specify { expect(subject[:status]).to be(:success) }
-          end
-
-          context 'with an old report' do
-            let!(:ci_build) { create(:ee_ci_build, :success, :license_management, pipeline: pipeline, project: project) }
-
-            before do
-              stub_feature_flags(drop_license_management_artifact: false)
-            end
-
-            specify { expect { subject }.to change { license_compliance_rule.reload.approvals_required }.from(1).to(0) }
-            specify { expect(subject[:status]).to be(:success) }
-          end
-        end
-
-        context "when an unexpected error occurs" do
-          before do
-            allow_next_instance_of(Gitlab::Ci::Reports::LicenseScanning::Report) do |instance|
-              allow(instance).to receive(:violates?).and_raise('heck')
-            end
-          end
-
-          specify { expect(subject[:status]).to be(:error) }
-          specify { expect(subject[:message]).to eql("Failed to update approval rules") }
-        end
-      end
-    end
-
-    context 'when pipeline fails' do
-      before do
-        pipeline.update!(status: :failed)
-      end
-
-      context 'when high-severity vulnerabilities are present' do
-        before do
-          create(:ee_ci_build, :success, :dependency_scanning, name: 'ds_job', pipeline: pipeline, project: project)
-        end
-
-        context 'when high-severity vulnerabilities already present in target branch pipeline' do
-          before do
-            create(:ee_ci_build, :success, :dependency_scanning, name: 'ds_job', pipeline: base_pipeline, project: project)
-          end
-
-          it 'lowers approvals_required count to zero' do
-            expect { subject }
-              .to change { report_approver_rule.reload.approvals_required }.from(2).to(0)
-          end
-        end
-
-        context 'when high-severity vulnerabilities do not present in target branch pipeline' do
-          it "won't change approvals_required count" do
-            expect { subject }
-              .not_to change { report_approver_rule.reload.approvals_required }
-          end
-        end
-      end
-
-      context 'when only low-severity vulnerabilities are present' do
-        before do
-          create(:ee_ci_build, :success, :low_severity_dast_report, name: 'dast_job', pipeline: pipeline, project: project)
-        end
-
-        it 'lowers approvals_required count to zero' do
-          expect { subject }
-            .to change { report_approver_rule.reload.approvals_required }.from(2).to(0)
-        end
-      end
-    end
-  end
-
-  context 'without reports' do
-    let(:pipeline) { create(:ci_pipeline, :running, project: project, merge_requests_as_head_pipeline: [merge_request]) }
-
-    it "won't change approvals_required count" do
-      expect { subject }
-        .not_to change { report_approver_rule.reload.approvals_required }
-    end
-
-    context "license compliance policy" do
-      let!(:software_license_policy) { create(:software_license_policy, :denied, project: project, software_license: denied_license) }
-      let!(:license_compliance_rule) { create(:report_approver_rule, :license_scanning, merge_request: merge_request, approvals_required: 1) }
-      let!(:denied_license) { create(:software_license) }
-
-      specify { expect { subject }.not_to change { license_compliance_rule.reload.approvals_required } }
-      specify { expect(subject[:status]).to be(:success) }
-    end
-  end
-end

ee/spec/workers/sync_security_reports_to_report_approval_rules_worker_spec.rb → ee/spec/workers/ci/sync_reports_to_report_approval_rules_worker_spec.rb

@@ -2,7 +2,7 @@
 
 require 'spec_helper'
 
-RSpec.describe SyncSecurityReportsToReportApprovalRulesWorker do
+RSpec.describe Ci::SyncReportsToReportApprovalRulesWorker do
   describe '#perform' do
     let(:pipeline) { double(:pipeline, id: 42) }
     let(:sync_service) { double(:service, execute: true) }
@@ -13,7 +13,7 @@ RSpec.describe SyncSecurityReportsToReportApprovalRulesWorker do
       end
 
       it "executes SyncReportsToApprovalRulesService for given pipeline" do
-        expect(Security::SyncReportsToApprovalRulesService).to receive(:new)
+        expect(Ci::SyncReportsToApprovalRulesService).to receive(:new)
           .with(pipeline).once.and_return(sync_service)
 
         described_class.new.perform(pipeline.id)
@@ -22,7 +22,7 @@ RSpec.describe SyncSecurityReportsToReportApprovalRulesWorker do
 
     context 'when pipeline is missing' do
       it 'does not execute SyncReportsToApprovalRulesService' do
-        expect(Security::SyncReportsToApprovalRulesService).not_to receive(:new)
+        expect(Ci::SyncReportsToApprovalRulesService).not_to receive(:new)
 
         described_class.new.perform(pipeline.id)
       end

spec/factories/ci/builds.rb

@@ -238,6 +238,20 @@ FactoryBot.define do
       coverage_regex { '/(d+)/' }
     end
 
+    trait :trace_with_coverage do
+      coverage { nil }
+      coverage_regex { '(\d+\.\d+)%' }
+
+      transient do
+        trace_coverage { 60.0 }
+      end
+
+      after(:create) do |build, evaluator|
+        build.trace.set("Coverage #{evaluator.trace_coverage}%")
+        build.trace.archive! if build.complete?
+      end
+    end
+
     trait :trace_live do
       after(:create) do |build, evaluator|
         build.trace.set('BUILD TRACE')

spec/models/ci/build_spec.rb

@@ -5239,4 +5239,20 @@ RSpec.describe Ci::Build do
       end
     end
   end
+
+  describe '.without_coverage' do
+    let!(:build_with_coverage) { create(:ci_build, pipeline: pipeline, coverage: 100.0) }
+
+    it 'returns builds without coverage values' do
+      expect(described_class.without_coverage).to eq([build])
+    end
+  end
+
+  describe '.with_coverage_regex' do
+    let!(:build_with_coverage_regex) { create(:ci_build, pipeline: pipeline, coverage_regex: '\d') }
+
+    it 'returns builds with coverage regex values' do
+      expect(described_class.with_coverage_regex).to eq([build_with_coverage_regex])
+    end
+  end
 end

spec/models/ci/pipeline_spec.rb

@@ -744,6 +744,42 @@ RSpec.describe Ci::Pipeline, :mailer, factory_default: :keep do
     end
   end
 
+  describe '#update_builds_coverage' do
+    let_it_be(:pipeline) { create(:ci_empty_pipeline) }
+
+    context 'builds with coverage_regex defined' do
+      let!(:build_1) { create(:ci_build, :success, :trace_with_coverage, trace_coverage: 60.0, pipeline: pipeline) }
+      let!(:build_2) { create(:ci_build, :success, :trace_with_coverage, trace_coverage: 80.0, pipeline: pipeline) }
+
+      it 'updates the coverage value of each build from the trace' do
+        pipeline.update_builds_coverage
+
+        expect(build_1.reload.coverage).to eq(60.0)
+        expect(build_2.reload.coverage).to eq(80.0)
+      end
+    end
+
+    context 'builds without coverage_regex defined' do
+      let!(:build) { create(:ci_build, :success, :trace_with_coverage, coverage_regex: nil, trace_coverage: 60.0, pipeline: pipeline) }
+
+      it 'does not update the coverage value of each build from the trace' do
+        pipeline.update_builds_coverage
+
+        expect(build.reload.coverage).to eq(nil)
+      end
+    end
+
+    context 'builds with coverage values already present' do
+      let!(:build) { create(:ci_build, :success, :trace_with_coverage, trace_coverage: 60.0, coverage: 10.0, pipeline: pipeline) }
+
+      it 'does not update the coverage value of each build from the trace' do
+        pipeline.update_builds_coverage
+
+        expect(build.reload.coverage).to eq(10.0)
+      end
+    end
+  end
+
   describe '#retryable?' do
     subject { pipeline.retryable? }
 

spec/workers/every_sidekiq_worker_spec.rb

@@ -165,6 +165,7 @@ RSpec.describe 'Every Sidekiq worker' do
         'Ci::ResourceGroups::AssignResourceFromResourceGroupWorker' => 3,
         'Ci::TestFailureHistoryWorker' => 3,
         'Ci::TriggerDownstreamSubscriptionsWorker' => 3,
+        'Ci::SyncReportsToReportApprovalRulesWorker' => 3,
         'CleanupContainerRepositoryWorker' => 3,
         'ClusterConfigureIstioWorker' => 3,
         'ClusterInstallAppWorker' => 3,
@@ -428,7 +429,6 @@ RSpec.describe 'Every Sidekiq worker' do
         'StoreSecurityScansWorker' => 3,
         'SyncSeatLinkRequestWorker' => 20,
         'SyncSeatLinkWorker' => 12,
-        'SyncSecurityReportsToReportApprovalRulesWorker' => 3,
         'SystemHookPushWorker' => 3,
         'TodosDestroyer::ConfidentialEpicWorker' => 3,
         'TodosDestroyer::ConfidentialIssueWorker' => 3,