Consider only violations with data when creating a bot comment
What does this MR do and why?
This change fixes a case where with certain policy combinations, a policy bot comment can be posted before the violations are evaluated.
If there are two separate policies or rules for "Newly detected" and "Previously existing" scan findings, a bot comment with violations could be created prematurely before evaluating "newly detected" policies. After "newly detected" policies are evaluated, this comment is updated to "policies resolved", but it shouldn't be created in the first place.
The reason is that "previously existing" statuses are evaluated in SyncPreexistingStatesApprovalRulesWorker
which happens after merge request is created, whereas "newly detected" need a pipeline to finish first and are evaluated in SyncMergeRequestApprovalsWorker
.
Depends on !147561 (merged).
Database
merge_request.scan_result_policy_violations.for_approval_rules(approval_rules).with_violation_data
SELECT 1 AS one FROM "scan_result_policy_violations" WHERE "scan_result_policy_violations"."merge_request_id" = 263349793 AND "scan_result_policy_violations"."scan_result_policy_id" IN (945, 946) AND "scan_result_policy_violations"."violation_data" IS NOT NULL;
Plan: https://console.postgres.ai/gitlab/gitlab-production-tunnel-pg12/sessions/27079/commands/84174 (there's no rows returned, because violation_data
column is not being populated yet, as it's behind a feature flag)
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.
How to set up and validate locally
- Enable feature flag in rails console:
Feature.enable(:save_policy_violation_data)
- Create a new project
- Create
.gitlab-ci.yml
:include: - template: Jobs/Secret-Detection.gitlab-ci.yml test: script: - echo "Compiling the code..." - echo "Compile complete."
- Create a policy. Example YAML:
type: approval_policy name: Scan findings description: '' enabled: true rules: - type: scan_finding scanners: [] vulnerabilities_allowed: 0 severity_levels: [] vulnerability_states: [] branch_type: protected - type: scan_finding scanners: [] vulnerabilities_allowed: 0 severity_levels: [] vulnerability_states: - detected - confirmed - dismissed - resolved branch_type: protected actions: - type: require_approval approvals_required: 1 role_approvers: - developer
- Update README in a new merge request
- Verify that no bot comment is created
- Open another MR, adding a leaked secret. Example:
diff --git a/.env b/.env new file mode 100644 index 0000000000000000000000000000000000000000..ee4bf74ac3b632173dafc09e74ecd68c298bdfa1 --- /dev/null +++ b/.env @@ -0,0 +1 @@ +AWS_TOKEN=AKIAZYONPI3G4JNCCWGQ \ No newline at end of file
- Verify that a bot comment is created
Related to #433403 (closed)