From 1e3be45143c2dcb447dfcff1c8638034580e3264 Mon Sep 17 00:00:00 2001 From: Thomas Randolph Date: Tue, 22 Jul 2025 03:58:57 -0600 Subject: [PATCH] Remove policy_mergability_check feature flag Changelog: removed --- doc/api/merge_requests.md | 1 - ee/app/models/ee/merge_request.rb | 3 +- ...heck_security_policy_violations_service.rb | 4 +-- .../update_violations_service.rb | 2 -- ...pending_merge_request_violations_worker.rb | 1 - ...licy_rules_pipeline_notification_worker.rb | 6 ++-- .../beta/policy_mergability_check.yml | 9 ----- .../ee/types/merge_request_type_spec.rb | 2 -- .../ee/gitlab/data_builder/pipeline_spec.rb | 2 -- ee/spec/models/merge_request_spec.rb | 13 ------- ...security_policy_violations_service_spec.rb | 10 ------ ...ync_report_approver_approval_rules_spec.rb | 36 ------------------- .../update_violations_service_spec.rb | 20 ----------- ...ng_merge_request_violations_worker_spec.rb | 9 ----- ...rules_pipeline_notification_worker_spec.rb | 8 ----- 15 files changed, 4 insertions(+), 122 deletions(-) delete mode 100644 ee/config/feature_flags/beta/policy_mergability_check.yml diff --git a/doc/api/merge_requests.md b/doc/api/merge_requests.md index 657760c69b6149..3c453a0243e6d6 100644 --- a/doc/api/merge_requests.md +++ b/doc/api/merge_requests.md @@ -923,7 +923,6 @@ Use `detailed_merge_status` instead of `merge_status` to account for all potenti - `preparing`: Merge request diff is being created. - `requested_changes`: The merge request has reviewers who have requested changes. - `security_policy_violations`: All security policies must be satisfied. - Requires the `policy_mergability_check` feature flag to be enabled. - `status_checks_must_pass`: All status checks must pass before merge. - `unchecked`: Git has not yet tested if a valid merge is possible. - `locked_paths`: Paths locked by other users must be unlocked before merging to default branch. diff --git a/ee/app/models/ee/merge_request.rb b/ee/app/models/ee/merge_request.rb index 87f360969722b4..f3ae5fc5495d5a 100644 --- a/ee/app/models/ee/merge_request.rb +++ b/ee/app/models/ee/merge_request.rb @@ -450,11 +450,10 @@ def synchronize_approval_rules_from_target_project return if merged? project_rules = target_project.approval_rules.report_approver.includes(:users, :groups) - feature_enabled = ::Feature.enabled?(:policy_mergability_check, project) project_rules.find_each do |project_rule| project_rule.apply_report_approver_rules_to(self) do |rule_attributes| - rule_attributes[:approvals_required] = 0 if feature_enabled && project_rule.from_scan_result_policy? + rule_attributes[:approvals_required] = 0 if project_rule.from_scan_result_policy? end end end diff --git a/ee/app/services/merge_requests/mergeability/check_security_policy_violations_service.rb b/ee/app/services/merge_requests/mergeability/check_security_policy_violations_service.rb index fd55abf2044f7d..97cc3fcc38c7d3 100644 --- a/ee/app/services/merge_requests/mergeability/check_security_policy_violations_service.rb +++ b/ee/app/services/merge_requests/mergeability/check_security_policy_violations_service.rb @@ -7,9 +7,7 @@ class CheckSecurityPolicyViolationsService < CheckBaseService description 'Checks whether the security policies are satisfied' def execute - if ::Feature.disabled?(:policy_mergability_check, - merge_request.project) || - !merge_request.project.licensed_feature_available?(:security_orchestration_policies) || + if !merge_request.project.licensed_feature_available?(:security_orchestration_policies) || merge_request.scan_result_policy_reads_through_approval_rules.none? return inactive end diff --git a/ee/app/services/security/security_orchestration_policies/update_violations_service.rb b/ee/app/services/security/security_orchestration_policies/update_violations_service.rb index a9d4123e44f20a..6958335b2e9d41 100644 --- a/ee/app/services/security/security_orchestration_policies/update_violations_service.rb +++ b/ee/app/services/security/security_orchestration_policies/update_violations_service.rb @@ -113,8 +113,6 @@ def violation_status_for_policy(policy, default_status) end def publish_violations_updated_event - return unless ::Feature.enabled?(:policy_mergability_check, merge_request.project) - ::Gitlab::EventStore.publish( ::MergeRequests::ViolationsUpdatedEvent.new( data: { merge_request_id: merge_request.id } diff --git a/ee/app/workers/security/scan_result_policies/unblock_pending_merge_request_violations_worker.rb b/ee/app/workers/security/scan_result_policies/unblock_pending_merge_request_violations_worker.rb index 2d7dce1831b83e..a5bbb6067a11de 100644 --- a/ee/app/workers/security/scan_result_policies/unblock_pending_merge_request_violations_worker.rb +++ b/ee/app/workers/security/scan_result_policies/unblock_pending_merge_request_violations_worker.rb @@ -14,7 +14,6 @@ class UnblockPendingMergeRequestViolationsWorker def perform(pipeline_id) pipeline = ::Ci::Pipeline.find_by_id(pipeline_id) || return project = pipeline.project - return if ::Feature.disabled?(:policy_mergability_check, project) return unless project.licensed_feature_available?(:security_orchestration_policies) pipeline.opened_merge_requests_with_head_sha.each do |merge_request| diff --git a/ee/app/workers/security/unenforceable_policy_rules_pipeline_notification_worker.rb b/ee/app/workers/security/unenforceable_policy_rules_pipeline_notification_worker.rb index 4aa3088ad8a4a2..d56ab3aa9e4dad 100644 --- a/ee/app/workers/security/unenforceable_policy_rules_pipeline_notification_worker.rb +++ b/ee/app/workers/security/unenforceable_policy_rules_pipeline_notification_worker.rb @@ -29,10 +29,8 @@ def perform(pipeline_id) related_merge_requests = pipeline.opened_merge_requests_with_head_sha return if related_merge_requests.none? - if ::Feature.enabled?(:policy_mergability_check, project) - Security::ScanResultPolicies::UnblockPendingMergeRequestViolationsWorker - .perform_in(UNBLOCK_PENDING_VIOLATIONS_TIMEOUT, pipeline_id) - end + Security::ScanResultPolicies::UnblockPendingMergeRequestViolationsWorker + .perform_in(UNBLOCK_PENDING_VIOLATIONS_TIMEOUT, pipeline_id) related_merge_requests.each do |merge_request| Security::UnenforceablePolicyRulesNotificationService.new(merge_request).execute diff --git a/ee/config/feature_flags/beta/policy_mergability_check.yml b/ee/config/feature_flags/beta/policy_mergability_check.yml deleted file mode 100644 index 06c30cfe715381..00000000000000 --- a/ee/config/feature_flags/beta/policy_mergability_check.yml +++ /dev/null @@ -1,9 +0,0 @@ ---- -name: policy_mergability_check -feature_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/444459 -introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/159077 -rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/473704 -milestone: '17.3' -group: group::code review -type: beta -default_enabled: true diff --git a/ee/spec/graphql/ee/types/merge_request_type_spec.rb b/ee/spec/graphql/ee/types/merge_request_type_spec.rb index 06161f59ad0409..6a021f186aa159 100644 --- a/ee/spec/graphql/ee/types/merge_request_type_spec.rb +++ b/ee/spec/graphql/ee/types/merge_request_type_spec.rb @@ -363,8 +363,6 @@ def setup_approval_rules(merge_request) multiple_approval_rules: true, blocking_merge_requests: true ) - - stub_feature_flags(policy_mergability_check: true) end it_behaves_like 'avoids N+1 queries related to approval rules' diff --git a/ee/spec/lib/ee/gitlab/data_builder/pipeline_spec.rb b/ee/spec/lib/ee/gitlab/data_builder/pipeline_spec.rb index b6d5d32d950f99..ece0d0c205fd3b 100644 --- a/ee/spec/lib/ee/gitlab/data_builder/pipeline_spec.rb +++ b/ee/spec/lib/ee/gitlab/data_builder/pipeline_spec.rb @@ -196,8 +196,6 @@ def setup_approvals(merge_request, user, group_user) merge_request_approvers: true, multiple_approval_rules: true ) - - stub_feature_flags(policy_mergability_check: true) end it_behaves_like 'avoids N+1 queries related to approval rules' diff --git a/ee/spec/models/merge_request_spec.rb b/ee/spec/models/merge_request_spec.rb index d9bd7e4c630f24..7df1eace808547 100644 --- a/ee/spec/models/merge_request_spec.rb +++ b/ee/spec/models/merge_request_spec.rb @@ -2059,19 +2059,6 @@ def create_mr(metrics_data = {}) expect(merge_request.approval_rules.map { |rule| [rule.report_type, rule.approvals_required] }) .to match_array([['license_scanning', 0], ['code_coverage', 2], ['scan_finding', 0], ['any_merge_request', 0]]) end - - context 'with feature flag policy_mergability_check false' do - before do - stub_feature_flags(policy_mergability_check: false) - end - - it 'does not reset any approval rules' do - subject - - expect(merge_request.approval_rules.map { |rule| [rule.report_type, rule.approvals_required] }) - .to match_array([['license_scanning', 2], ['code_coverage', 2], ['scan_finding', 2], ['any_merge_request', 2]]) - end - end end describe '#sync_project_approval_rules_for_policy_configuration' do diff --git a/ee/spec/services/merge_requests/mergeability/check_security_policy_violations_service_spec.rb b/ee/spec/services/merge_requests/mergeability/check_security_policy_violations_service_spec.rb index d101bbf840c166..881c9709e9a8f0 100644 --- a/ee/spec/services/merge_requests/mergeability/check_security_policy_violations_service_spec.rb +++ b/ee/spec/services/merge_requests/mergeability/check_security_policy_violations_service_spec.rb @@ -34,16 +34,6 @@ scan_result_policy_read: policy, name: 'Policy 1') end - context 'when policy_mergability_check is false' do - before do - stub_feature_flags(policy_mergability_check: false) - end - - it 'returns a check result with inactive status' do - expect(result.status).to eq Gitlab::MergeRequests::Mergeability::CheckResult::INACTIVE_STATUS - end - end - context 'when security_orchestration_policies license is false' do before do stub_licensed_features(security_orchestration_policies: false) diff --git a/ee/spec/services/merge_requests/sync_report_approver_approval_rules_spec.rb b/ee/spec/services/merge_requests/sync_report_approver_approval_rules_spec.rb index 3f9a834b116c02..a5f15d938f8c4d 100644 --- a/ee/spec/services/merge_requests/sync_report_approver_approval_rules_spec.rb +++ b/ee/spec/services/merge_requests/sync_report_approver_approval_rules_spec.rb @@ -25,42 +25,6 @@ let!(:report_approval_project_rule) { create(:approval_project_rule, report_type, project: merge_request.target_project, approvals_required: 2, name: default_name) } let!(:regular_approval_project_rule) { create(:approval_project_rule, project: merge_request.target_project, approvals_required: 2) } - context 'when the policy_mergability_check feature flag is false' do - before do - stub_feature_flags(policy_mergability_check: false) - end - - context 'when report_approver_rules are enabled' do - it 'creates rule for report approvers' do - expect { service.execute } - .to change { merge_request.approval_rules.where(name: default_name).count }.from(0).to(1) - - rule = merge_request.approval_rules.find_by(name: default_name) - - expect(rule).to be_report_approver - expect(rule.report_type).to eq(report_type.to_s) - expect(rule.name).to eq(report_approval_project_rule.name) - expect(rule.approvals_required).to eq(report_approval_project_rule.approvals_required) - expect(rule.approval_project_rule).to eq(report_approval_project_rule) - end - - it 'updates previous report approval rule if defined' do - previous_rule = create(:report_approver_rule, report_type, merge_request: merge_request, approvals_required: 3) - create(:approval_merge_request_rule_source, approval_merge_request_rule: previous_rule, - approval_project_rule: report_approval_project_rule) - - expect { service.execute } - .not_to change { merge_request.approval_rules.count } - - expect(previous_rule.reload).to be_report_approver - expect(previous_rule.report_type).to eq(report_type.to_s) - expect(previous_rule.name).to eq(report_approval_project_rule.name) - expect(previous_rule.approvals_required).to eq(report_approval_project_rule.approvals_required) - expect(previous_rule.approval_project_rule).to eq(report_approval_project_rule) - end - end - end - context 'when report_approver_rules are enabled' do it 'creates rule for report approvers' do expect { service.execute } diff --git a/ee/spec/services/security/security_orchestration_policies/update_violations_service_spec.rb b/ee/spec/services/security/security_orchestration_policies/update_violations_service_spec.rb index 7736070fe7348c..3dc1e814834d8d 100644 --- a/ee/spec/services/security/security_orchestration_policies/update_violations_service_spec.rb +++ b/ee/spec/services/security/security_orchestration_policies/update_violations_service_spec.rb @@ -98,16 +98,6 @@ def last_violation .to publish_event(::MergeRequests::ViolationsUpdatedEvent) .with(merge_request_id: merge_request.id) end - - context 'when policy_mergability_check is off' do - before do - stub_feature_flags(policy_mergability_check: false) - end - - it 'does not publish MergeRequests::ViolationsUpdatedEvent' do - expect { service.execute }.not_to publish_event(MergeRequests::ViolationsUpdatedEvent) - end - end end context 'with pre-existing violations' do @@ -174,16 +164,6 @@ def last_violation .to publish_event(::MergeRequests::ViolationsUpdatedEvent) .with(merge_request_id: merge_request.id) end - - context 'when policy_mergability_check is off' do - before do - stub_feature_flags(policy_mergability_check: false) - end - - it 'does not publish MergeRequests::ViolationsUpdatedEvent' do - expect { service.execute }.not_to publish_event(MergeRequests::ViolationsUpdatedEvent) - end - end end context 'without violations' do diff --git a/ee/spec/workers/security/scan_result_policies/unblock_pending_merge_request_violations_worker_spec.rb b/ee/spec/workers/security/scan_result_policies/unblock_pending_merge_request_violations_worker_spec.rb index da24b4ec7261ab..f68bf4fea7059a 100644 --- a/ee/spec/workers/security/scan_result_policies/unblock_pending_merge_request_violations_worker_spec.rb +++ b/ee/spec/workers/security/scan_result_policies/unblock_pending_merge_request_violations_worker_spec.rb @@ -135,15 +135,6 @@ end end - context 'when feature flag "policy_mergability_check" is disabled' do - before do - stub_feature_flags(policy_mergability_check: false) - end - - it_behaves_like 'does not update merge request violations' - it_behaves_like 'does not update merge request report_approver approvals' - end - context 'when feature is not licensed' do let(:feature_licensed) { false } diff --git a/ee/spec/workers/security/unenforceable_policy_rules_pipeline_notification_worker_spec.rb b/ee/spec/workers/security/unenforceable_policy_rules_pipeline_notification_worker_spec.rb index 8804354a7193c4..58db3ef2dbe24b 100644 --- a/ee/spec/workers/security/unenforceable_policy_rules_pipeline_notification_worker_spec.rb +++ b/ee/spec/workers/security/unenforceable_policy_rules_pipeline_notification_worker_spec.rb @@ -68,14 +68,6 @@ it_behaves_like 'does not schedule UnblockPendingMergeRequestViolationsWorker' end - - context 'when feature flag "policy_mergability_check" is disabled' do - before do - stub_feature_flags(policy_mergability_check: false) - end - - it_behaves_like 'does not schedule UnblockPendingMergeRequestViolationsWorker' - end end context 'when pipeline is manual' do -- GitLab