Skip to content

Optimize policy bot comment by being enqueued less often

Martin Čavoj requested to merge 433403-optimizations into master

What does this MR do and why?

Optimize policy bot comment by being enqueued less often.

With this change, the policy bot comment will be enqueued only after all violation_data for merge request are populated, resulting in less re-renders.

The bot message is created/updated by GeneratePolicyViolationCommentWorker which can get triggered from multiple other workers in parallel (depending on active policies):

  • SyncAnyMergeRequestApprovalRulesWorker
  • SyncMergeRequestApprovalsWorker
  • SyncPreexistingStatesApprovalRulesWorker
  • SyncReportsToApprovalRulesService
  • SyncPolicyViolationCommentWorker
  • UnenforceablePolicyRulesNotificationWorker

Previously we didn't have the violations persisted, so we had to pass violated_policy as parameter for the worker and we had to do it for each report_type. This causes the message to be re-rendered multiple times if there are multiple report_types evaluated.

Now we have the violation_data, so we can defer the message creation to when we have all data populated for a given merge request.

One potential downside could be that there will be no bot message created if we crash in the last evaluation, but I think that's acceptable.

MR acceptance checklist

Please evaluate this MR against the MR acceptance checklist. It helps you analyze changes to reduce risks in quality, performance, reliability, security, and maintainability.

Screenshots or screen recordings

Without CI setup (pipeline configuration errors):

CleanShot_2024-04-10_at_10.55.55

Partial errors (one artifact type missing) and resolving them:

CleanShot_2024-04-10_at_11.14.28

Without initial violations and adding violation later:

CleanShot_2024-04-10_at_11.18.19

All kinds of violations get created all at once, the message doesn't get updated:

CleanShot_2024-04-10_at_11.23.59

With feature flag disabled, the bot message gets updated multiple times:

CleanShot_2024-04-10_at_11.28.55

When targeting unprotected branch:

CleanShot_2024-04-10_at_11.58.48

How to set up and validate locally

  1. Create a project
  2. In rails console enable the feature flag 
    Feature.enable(:save_policy_violation_data, Project.last)
  3. Create a .gitlab-ci.yml: 
    include:
  - template: Jobs/Dependency-Scanning.gitlab-ci.yml
  - template: Jobs/Secret-Detection.gitlab-ci.yml

build-job:
  script:
    - echo "Compiling the code..."
    - echo "Compile complete."
  4. Create multiple policies:
    1. New scans
    type: approval_policy
name: New scans
description: ''
enabled: true
rules:
  - type: scan_finding
    scanners: []
    vulnerabilities_allowed: 0
    severity_levels: []
    vulnerability_states: []
    branch_type: protected
actions:
  - type: require_approval
    approvals_required: 1
    role_approvers:
      - developer
approval_settings:
  block_branch_modification: false
  prevent_pushing_and_force_pushing: false
    1. Previously existing:
    type: approval_policy
name: Previously existing
description: ''
enabled: true
rules:
  - type: scan_finding
    scanners: []
    vulnerabilities_allowed: 0
    severity_levels: []
    vulnerability_states:
      - detected
      - confirmed
    branch_type: protected
actions:
  - type: require_approval
    approvals_required: 1
    role_approvers:
      - developer
approval_settings:
  block_branch_modification: false
  prevent_pushing_and_force_pushing: false
    1. Any MR:
    type: approval_policy
name: Any MR
description: ''
enabled: true
rules:
  - type: any_merge_request
    branch_type: protected
    commits: unsigned
actions:
  - type: require_approval
    approvals_required: 1
    role_approvers:
      - developer
approval_settings:
  block_branch_modification: false
  prevent_pushing_and_force_pushing: false
  prevent_approval_by_author: true
  prevent_approval_by_commit_author: true
  remove_approvals_with_new_commit: true
  require_password_to_approve: true
    1. Licenses
    type: approval_policy
name: Licenses
description: Only allowed licenses can be added to the project.
enabled: true
rules:
  - type: license_finding
    match_on_inclusion: true
    license_types:
      - MIT License
    license_states:
      - newly_detected
    branch_type: protected
actions:
  - type: require_approval
    approvals_required: 1
    role_approvers:
      - developer
approval_settings:
  block_branch_modification: false
  prevent_pushing_and_force_pushing: false
  5. Create MR updating README. Verify that comment appears after pipeline finishes and is not re-rendered (there's no "Edited by..." footer)
  6. Test various cases documented in the screen recordings (no CI setup, missing security jobs, creating MR initially without violations, resolving violations, targeting a non-protected branch)

Database

merge_request.scan_result_policy_violations.without_violation_data.exists?:

SELECT 1 AS one FROM "scan_result_policy_violations" WHERE "scan_result_policy_violations"."merge_request_id" = 291233972 AND "scan_result_policy_violations"."violation_data" IS NULL LIMIT 1

Plan: https://console.postgres.ai/gitlab/gitlab-production-main/sessions/27457/commands/85489

Related to #433403 (closed)

Edited by Martin Čavoj

Merge request reports