Improve MR approval policy handling of pipeline failure errors
Description
Today, if a pipeline failure occurs, it appears like this, with a bot comment that feels irrelevant/unuseful:
| MR view | Pipeline view |
|---|---|
![]() |
![]() |
The bot comment should likely either not appear or provide some more meaningful feedback regarding the policy error. Else it may be spam and distracting.
It's true that the policy would be a violation in this state since the pipeline didn't run at all, but we may want a more generic or useful message here.
Implementation plan
diff --git a/ee/app/models/security/scan_result_policy_violation.rb b/ee/app/models/security/scan_result_policy_violation.rb
index 8fcfb0e9ca24..051efcc2e3f0 100644
--- a/ee/app/models/security/scan_result_policy_violation.rb
+++ b/ee/app/models/security/scan_result_policy_violation.rb
@@ -38,7 +38,8 @@ class ScanResultPolicyViolation < ApplicationRecord
scan_removed: 'SCAN_REMOVED',
target_pipeline_missing: 'TARGET_PIPELINE_MISSING',
artifacts_missing: 'ARTIFACTS_MISSING',
- evaluation_skipped: 'EVALUATION_SKIPPED'
+ evaluation_skipped: 'EVALUATION_SKIPPED',
+ pipeline_failed: 'PIPELINE_FAILED'
}.freeze
MAX_VIOLATIONS = 10
diff --git a/ee/app/services/security/unenforceable_policy_rules_notification_service.rb b/ee/app/services/security/unenforceable_policy_rules_notification_service.rb
index 5028c0d10c25..a82b4e3461ed 100644
--- a/ee/app/services/security/unenforceable_policy_rules_notification_service.rb
+++ b/ee/app/services/security/unenforceable_policy_rules_notification_service.rb
@@ -44,7 +44,7 @@ def update_for_report_type(merge_request, report_type, approval_rules)
policy_evaluation = Security::SecurityOrchestrationPolicies::PolicyRuleEvaluationService.new(merge_request)
applicable_rules.each do |rule|
- policy_evaluation.error!(rule, :artifacts_missing, context: validation_context(report_type))
+ policy_evaluation.error!(rule, pipeline_error, context: validation_context(report_type))
end
policy_evaluation.save
@@ -70,6 +70,10 @@ def unblock_fail_open_rules(report_type)
.execute
end
+ def pipeline_error
+ pipeline.failed? ? :pipeline_failed : :artifacts_missing
+ end
+
def validation_context(report_type)
return if pipeline.nil?
diff --git a/ee/lib/security/scan_result_policies/policy_violation_details.rb b/ee/lib/security/scan_result_policies/policy_violation_details.rb
index f35808187078..0dd1155e6913 100644
--- a/ee/lib/security/scan_result_policies/policy_violation_details.rb
+++ b/ee/lib/security/scan_result_policies/policy_violation_details.rb
@@ -32,7 +32,9 @@ class PolicyViolationDetails
},
'EVALUATION_SKIPPED' => 'Policy `%{policy}` could not be evaluated within the specified timeframe ' \
'and, as a result, approvals are required for the policy. ' \
- 'Ensure that scanners are present in the latest pipeline.'
+ 'Ensure that scanners are present in the latest pipeline.',
+ 'PIPELINE_FAILED' => 'Policy `%{policy}` could not be evaluated because the latest pipeline failed. ' \
+ 'Ensure that the pipeline is configured properly and the scanners are present.'
}.freeze
# These messages correspond to the possible errors above and are shown when a policy is configured to fail open
Verification steps
- Create a project
- Configure a merge request approval policy
- Add an invalid
.gitlab-ci.ymlinto the project so that the pipeline fails to runinvalid - Create MR (update
README.md) - Verify the message
- Update
.gitlab-ci.ymlso that pipeline starts, runs the scanners but fails afterwardsinclude: - template: Jobs/Secret-Detection.gitlab-ci.yml project-job: stage: test script: sleep 10 deploy-job: stage: deploy script: exit 1 - Create another MR
- Verify that no comment is created and there are no violations
Edited by Martin Cavoj

