Skip to content

G
gitlab-ce
Commit e6477942 authored by Dmytro Zaporozhets's avatar Dmytro Zaporozhets
Browse files
Options

Merge branch '219359-remove-mr-scope-feature-flags' into 'master' 

Remove feature flag on compliance MR settings scope

See merge request gitlab-org/gitlab!36685
parents 5433dadb 6f31b77a
Hide whitespace changes
Inline Side-by-side
Showing with 98 additions and 291 deletions
doc/user/admin_area/merge_requests_approvals.md
View file @ e6477942
......@@ -37,12 +37,12 @@ Merge request approval rules that can be set at an instance level are:
## Scope rules to compliance-labeled projects
> Introduced in [GitLab Premium](https://gitlab.com/groups/gitlab-org/-/epics/3432) 13.1.
> Introduced in [GitLab Premium](https://gitlab.com/groups/gitlab-org/-/epics/3432) 13.2.
Merge request approval rules can be further scoped to specific compliance frameworks.
When the compliance framework label is selected and the project is assigned the compliance
label, the instance-level MR approval settings will take effect and
label, the instance-level MR approval settings will take effect and the
[project-level settings](../project/merge_requests/merge_request_approvals.md#adding--editing-a-default-approval-rule)
is locked for modification.
......@@ -53,18 +53,3 @@ Maintainer role and above can modify these.
| Instance-level | Project-level |
| -------------- | ------------- |
| ![Scope MR approval settings to compliance frameworks](img/scope_mr_approval_settings_v13_1.png) | ![MR approval settings on compliance projects](img/mr_approval_settings_compliance_project_v13_1.png) |
### Enabling the feature
This feature comes with two feature flags which are disabled by default.
- The configuration in Admin area is controlled via `admin_compliance_merge_request_approval_settings`.
- The application of these rules is controlled via `project_compliance_merge_request_approval_settings`.
These feature flags can be managed by feature flag [API endpoint](../../api/features.md#set-or-create-a-feature) or
by [GitLab administrators with access to the GitLab Rails console](../../administration/feature_flags.md) with the following commands:
```ruby
Feature.enable(:admin_compliance_merge_request_approval_settings)
Feature.enable(:project_compliance_merge_request_approval_settings)
```
ee/app/helpers/admin/merge_request_approval_settings_helper.rb
View file @ e6477942
......@@ -3,8 +3,7 @@
module Admin
module MergeRequestApprovalSettingsHelper
def show_compliance_merge_request_approval_settings?
Feature.enabled?(:admin_compliance_merge_request_approval_settings) &&
License.feature_available?(:admin_merge_request_approvers_rules)
License.feature_available?(:admin_merge_request_approvers_rules)
end
end
end
ee/app/models/ee/project.rb
View file @ e6477942
......@@ -640,7 +640,6 @@ module EE
def disable_overriding_approvers_per_merge_request
return super unless License.feature_available?(:admin_merge_request_approvers_rules)
return (::Gitlab::CurrentSettings.disable_overriding_approvers_per_merge_request? || super) unless project_compliance_mr_approval_settings?
return super unless has_regulated_settings?
::Gitlab::CurrentSettings.disable_overriding_approvers_per_merge_request?
......@@ -649,8 +648,6 @@ module EE
def merge_requests_author_approval
return super unless License.feature_available?(:admin_merge_request_approvers_rules)
return false if !project_compliance_mr_approval_settings? && ::Gitlab::CurrentSettings.prevent_merge_requests_author_approval?
return super if !project_compliance_mr_approval_settings? && !::Gitlab::CurrentSettings.prevent_merge_requests_author_approval?
return super unless has_regulated_settings?
!::Gitlab::CurrentSettings.prevent_merge_requests_author_approval?
......@@ -659,17 +656,12 @@ module EE
def merge_requests_disable_committers_approval
return super unless License.feature_available?(:admin_merge_request_approvers_rules)
return (::Gitlab::CurrentSettings.prevent_merge_requests_committers_approval? || super) unless project_compliance_mr_approval_settings?
return super unless has_regulated_settings?
::Gitlab::CurrentSettings.prevent_merge_requests_committers_approval?
end
alias_method :merge_requests_disable_committers_approval?, :merge_requests_disable_committers_approval
def project_compliance_mr_approval_settings?
::Feature.enabled?(:project_compliance_merge_request_approval_settings, self)
end
def license_compliance(pipeline = latest_pipeline_with_reports(::Ci::JobArtifact.license_scanning_reports))
SCA::LicenseCompliance.new(self, pipeline)
end
......
ee/app/policies/ee/project_policy.rb
View file @ e6477942
......@@ -76,27 +76,15 @@ module EE
end
condition(:cannot_modify_approvers_rules) do
if @subject.project_compliance_mr_approval_settings?
regulated_merge_request_approval_settings?
else
owner_cannot_modify_approvers_rules? && !admin?
end
regulated_merge_request_approval_settings?
end
condition(:cannot_modify_merge_request_author_setting) do
if @subject.project_compliance_mr_approval_settings?
regulated_merge_request_approval_settings?
else
owner_cannot_modify_merge_request_author_setting? && !admin?
end
regulated_merge_request_approval_settings?
end
condition(:cannot_modify_merge_request_committer_setting) do
if @subject.project_compliance_mr_approval_settings?
regulated_merge_request_approval_settings?
else
owner_cannot_modify_merge_request_committer_setting? && !admin?
end
regulated_merge_request_approval_settings?
end
with_scope :subject
......
ee/changelogs/unreleased/219359-scope-mr-settings-to-compliance.yml 0 → 100644
View file @ e6477942
---
title: Scope instance-level MR settings to compliance-labeled projects
merge_request: 36685
author:
type: added
ee/spec/controllers/admin/application_settings_controller_spec.rb
View file @ e6477942
......@@ -155,6 +155,7 @@ RSpec.describe Admin::ApplicationSettingsController do
prevent_merge_requests_committers_approval: true
}
end
let(:feature) { :admin_merge_request_approvers_rules }
it_behaves_like 'settings for licensed features'
......@@ -164,27 +165,7 @@ RSpec.describe Admin::ApplicationSettingsController do
let(:settings) { { compliance_frameworks: [1, 2, 3, 4, 5] } }
let(:feature) { :admin_merge_request_approvers_rules }
context 'when feature flag is enabled' do
before do
stub_feature_flags(admin_compliance_merge_request_approval_settings: true)
end
it_behaves_like 'settings for licensed features'
end
context 'when feature flag is disabled' do
before do
stub_licensed_features(feature => true)
stub_feature_flags(admin_compliance_merge_request_approval_settings: false)
end
it 'does not update settings' do
attribute_names = settings.keys.map(&:to_s)
expect { put :update, params: { application_setting: settings } }
.not_to change { ApplicationSetting.current.reload.attributes.slice(*attribute_names) }
end
end
it_behaves_like 'settings for licensed features'
end
context 'required instance ci template' do
......
ee/spec/helpers/admin/merge_request_approval_settings_helper_spec.rb
View file @ e6477942
......@@ -8,16 +8,13 @@ RSpec.describe Admin::MergeRequestApprovalSettingsHelper do
subject { helper.show_compliance_merge_request_approval_settings? }
where(:feature_flag, :licensed, :result) do
true | true | true
true | false | false
false | true | false
false | false | false
where(:licensed, :result) do
true | true
false | false
end
with_them do
before do
stub_feature_flags(admin_compliance_merge_request_approval_settings: feature_flag)
stub_licensed_features(admin_merge_request_approvers_rules: licensed)
end
......
ee/spec/models/project_spec.rb
View file @ e6477942
......@@ -488,68 +488,31 @@ RSpec.describe Project do
context 'merge requests related settings' do
shared_examples 'setting modified by application setting' do
context 'when compliance merge request approval settings feature flag is enabled' do
using RSpec::Parameterized::TableSyntax
where(:app_setting, :project_setting, :regulated_settings, :final_setting) do
true | true | true | true
false | true | true | false
true | false | true | true
false | false | true | false
true | true | false | true
false | true | false | true
true | false | false | false
false | false | false | false
end
with_them do
let(:project) { create(:project) }
before do
stub_feature_flags(project_compliance_merge_request_approval_settings: true)
stub_licensed_features(admin_merge_request_approvers_rules: true)
allow(project).to receive(:has_regulated_settings?).and_return(regulated_settings)
stub_application_setting(application_setting => app_setting)
project.update(setting => project_setting)
end
it 'shows proper setting' do
expect(project.send(setting)).to eq(final_setting)
expect(project.send("#{setting}?")).to eq(final_setting)
end
end
where(:app_setting, :project_setting, :regulated_settings, :final_setting) do
true | true | true | true
false | true | true | false
true | false | true | true
false | false | true | false
true | true | false | true
false | true | false | true
true | false | false | false
false | false | false | false
end
context 'when compliance merge request approval settings feature flag is disabled' do
using RSpec::Parameterized::TableSyntax
where(:app_setting, :project_setting, :feature_enabled, :final_setting) do
true | true | true | true
false | true | true | true
true | false | true | true
false | false | true | false
true | true | false | true
false | true | false | true
true | false | false | false
false | false | false | false
end
with_them do
let(:project) { create(:project) }
with_them do
let(:project) { create(:project) }
before do
stub_feature_flags(project_compliance_merge_request_approval_settings: false)
before do
stub_licensed_features(admin_merge_request_approvers_rules: true)
stub_licensed_features(feature => feature_enabled)
stub_application_setting(application_setting => app_setting)
project.update(setting => project_setting)
end
allow(project).to receive(:has_regulated_settings?).and_return(regulated_settings)
stub_application_setting(application_setting => app_setting)
project.update(setting => project_setting)
end
it 'shows proper setting' do
expect(project.send(setting)).to eq(final_setting)
expect(project.send("#{setting}?")).to eq(final_setting)
end
it 'shows proper setting' do
expect(project.send(setting)).to eq(final_setting)
expect(project.send("#{setting}?")).to eq(final_setting)
end
end
end
......@@ -576,66 +539,31 @@ RSpec.describe Project do
let(:setting) { :merge_requests_author_approval }
let(:application_setting) { :prevent_merge_requests_author_approval }
context 'when compliance merge request approval settings feature flag is enabled' do
using RSpec::Parameterized::TableSyntax
where(:app_setting, :project_setting, :regulated_settings, :final_setting) do
true | true | true | false
false | true | true | true
true | false | true | false
false | false | true | true
true | true | false | true
false | true | false | true
true | false | false | false
false | false | false | false
end
with_them do
let(:project) { create(:project) }
before do
stub_feature_flags(project_compliance_merge_request_approval_settings: true)
stub_licensed_features(admin_merge_request_approvers_rules: true)
allow(project).to receive(:has_regulated_settings?).and_return(regulated_settings)
stub_application_setting(application_setting => app_setting)
project.update(setting => project_setting)
end
it 'shows proper setting' do
expect(project.send(setting)).to eq(final_setting)
expect(project.send("#{setting}?")).to eq(final_setting)
end
end
where(:app_setting, :project_setting, :regulated_settings, :final_setting) do
true | true | true | false
false | true | true | true
true | false | true | false
false | false | true | true
true | true | false | true
false | true | false | true
true | false | false | false
false | false | false | false
end
context 'when compliance merge request approval settings feature flag is disabled' do
using RSpec::Parameterized::TableSyntax
where(:app_setting, :project_setting, :feature_enabled, :final_setting) do
true | true | true | false
false | true | true | true
true | false | true | false
false | false | true | false
true | true | false | true
false | true | false | true
true | false | false | false
false | false | false | false
end
with_them do
let(:project) { create(:project) }
with_them do
before do
stub_feature_flags(project_compliance_merge_request_approval_settings: false)
before do
stub_licensed_features(admin_merge_request_approvers_rules: true)
stub_licensed_features(feature => feature_enabled)
stub_application_setting(application_setting => app_setting)
project.update(setting => project_setting)
end
allow(project).to receive(:has_regulated_settings?).and_return(regulated_settings)
stub_application_setting(application_setting => app_setting)
project.update(setting => project_setting)
end
it 'shows proper setting' do
expect(project.send(setting)).to eq(final_setting)
expect(project.send("#{setting}?")).to eq(final_setting)
end
it 'shows proper setting' do
expect(project.send(setting)).to eq(final_setting)
expect(project.send("#{setting}?")).to eq(final_setting)
end
end
end
......
ee/spec/policies/project_policy_spec.rb
View file @ e6477942
......@@ -1201,129 +1201,61 @@ RSpec.describe ProjectPolicy do
shared_examples 'merge request rules' do
let(:project) { create(:project, namespace: owner.namespace) }
context 'when compliance merge request approval settings feature flag is enabled' do
before do
stub_feature_flags(project_compliance_merge_request_approval_settings: true)
end
using RSpec::Parameterized::TableSyntax
context 'with merge request approvers rules available in license' do
where(:role, :regulated_setting, :admin_mode, :allowed) do
:guest | true | nil | false
:reporter | true | nil | false
:developer | true | nil | false
:maintainer | false | nil | true
:maintainer | true | nil | false
:owner | false | nil | true
:owner | true | nil | false
:admin | false | false | false
:admin | false | true | true
:admin | true | false | false
:admin | true | true | false
end
with_them do
let(:current_user) { public_send(role) }
before do
stub_licensed_features(admin_merge_request_approvers_rules: true)
allow(project).to receive(:has_regulated_settings?).and_return(regulated_setting)
enable_admin_mode!(current_user) if admin_mode
end
using RSpec::Parameterized::TableSyntax
it { is_expected.to(allowed ? be_allowed(policy) : be_disallowed(policy)) }
end
context 'with merge request approvers rules available in license' do
where(:role, :regulated_setting, :admin_mode, :allowed) do
:guest | true | nil | false
:reporter | true | nil | false
:developer | true | nil | false
:maintainer | false | nil | true
:maintainer | true | nil | false
:owner | false | nil | true
:owner | true | nil | false
:admin | false | false | false
:admin | false | true | true
:admin | true | false | false
:admin | true | true | false
end
context 'with merge request approvers not available in license' do
where(:role, :regulated_setting, :admin_mode, :allowed) do
:guest | true | nil | false
:reporter | true | nil | false
:developer | true | nil | false
:maintainer | false | nil | true
:maintainer | true | nil | true
:owner | false | nil | true
:owner | true | nil | true
:admin | false | false | false
:admin | false | true | true
:admin | true | false | false
:admin | true | true | true
end
with_them do
let(:current_user) { public_send(role) }
before do
stub_licensed_features(admin_merge_request_approvers_rules: false)
allow(project).to receive(:has_regulated_settings?).and_return(regulated_setting)
enable_admin_mode!(current_user) if admin_mode
end
with_them do
let(:current_user) { public_send(role) }
it { is_expected.to(allowed ? be_allowed(policy) : be_disallowed(policy)) }
before do
stub_licensed_features(admin_merge_request_approvers_rules: true)
allow(project).to receive(:has_regulated_settings?).and_return(regulated_setting)
enable_admin_mode!(current_user) if admin_mode
end
it { is_expected.to(allowed ? be_allowed(policy) : be_disallowed(policy)) }
end
end
context 'when compliance merge request approval settings feature flag is disabled' do
before do
stub_feature_flags(project_compliance_merge_request_approval_settings: false)
context 'with merge request approvers not available in license' do
where(:role, :regulated_setting, :admin_mode, :allowed) do
:guest | true | nil | false
:reporter | true | nil | false
:developer | true | nil | false
:maintainer | false | nil | true
:maintainer | true | nil | true
:owner | false | nil | true
:owner | true | nil | true
:admin | false | false | false
:admin | false | true | true
:admin | true | false | false
:admin | true | true | true
end
using RSpec::Parameterized::TableSyntax
context 'with merge request approvers rules available in license' do
where(:role, :setting, :admin_mode, :allowed) do
:guest | true | nil | false
:reporter | true | nil | false
:developer | true | nil | false
:maintainer | false | nil | true
:maintainer | true | nil | false
:owner | false | nil | true
:owner | true | nil | false
:admin | false | false | false
:admin | false | true | true
:admin | true | false | false
:admin | true | true | true
end
with_them do
let(:current_user) { public_send(role) }
before do
stub_licensed_features(admin_merge_request_approvers_rules: true)
stub_application_setting(setting_name => setting)
enable_admin_mode!(current_user) if admin_mode
end
it { is_expected.to(allowed ? be_allowed(policy) : be_disallowed(policy)) }
end
end
with_them do
let(:current_user) { public_send(role) }
context 'with merge request approvers not available in license' do
where(:role, :setting, :admin_mode, :allowed) do
:guest | true | nil | false
:reporter | true | nil | false
:developer | true | nil | false
:maintainer | false | nil | true
:maintainer | true | nil | true
:owner | false | nil | true
:owner | true | nil | true
:admin | false | false | false
:admin | false | true | true
:admin | true | false | false
:admin | true | true | true
before do
stub_licensed_features(admin_merge_request_approvers_rules: false)
allow(project).to receive(:has_regulated_settings?).and_return(regulated_setting)
enable_admin_mode!(current_user) if admin_mode
end
with_them do
let(:current_user) { public_send(role) }
before do
stub_licensed_features(admin_merge_request_approvers_rules: false)
stub_application_setting(setting_name => setting)
enable_admin_mode!(current_user) if admin_mode
end
it { is_expected.to(allowed ? be_allowed(policy) : be_disallowed(policy)) }
end
it { is_expected.to(allowed ? be_allowed(policy) : be_disallowed(policy)) }
end
end
end
......
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment
GitLab Nexedi Edition | About GitLab | About Nexedi | 沪ICP备2021021310号-2 | 沪ICP备2021021310号-7