Skip to content

G
gitlab-ce
Commit 6f31b77a authored by Tan Le's avatar Tan Le
Browse files
Options

Drop feature flag on compliance MR settings scope 

Remove code and test related to scoping MR approval settings to
compliance-labeled projects feature. This is ready for GA.
parent 1b1aa81a
Hide whitespace changes
Inline Side-by-side
Showing with 98 additions and 291 deletions
doc/user/admin_area/merge_requests_approvals.md
View file @ 6f31b77a
...@@ -37,12 +37,12 @@ Merge request approval rules that can be set at an instance level are: ...@@ -37,12 +37,12 @@ Merge request approval rules that can be set at an instance level are:
## Scope rules to compliance-labeled projects ## 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. 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 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) [project-level settings](../project/merge_requests/merge_request_approvals.md#adding--editing-a-default-approval-rule)
is locked for modification. is locked for modification.
...@@ -53,18 +53,3 @@ Maintainer role and above can modify these. ...@@ -53,18 +53,3 @@ Maintainer role and above can modify these.
| Instance-level | Project-level | | 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) | | ![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 @ 6f31b77a
...@@ -3,8 +3,7 @@ ...@@ -3,8 +3,7 @@
module Admin module Admin
module MergeRequestApprovalSettingsHelper module MergeRequestApprovalSettingsHelper
def show_compliance_merge_request_approval_settings? 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 end
end end
ee/app/models/ee/project.rb
View file @ 6f31b77a
...@@ -635,7 +635,6 @@ module EE ...@@ -635,7 +635,6 @@ module EE
def disable_overriding_approvers_per_merge_request def disable_overriding_approvers_per_merge_request
return super unless License.feature_available?(:admin_merge_request_approvers_rules) 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? return super unless has_regulated_settings?
::Gitlab::CurrentSettings.disable_overriding_approvers_per_merge_request? ::Gitlab::CurrentSettings.disable_overriding_approvers_per_merge_request?
...@@ -644,8 +643,6 @@ module EE ...@@ -644,8 +643,6 @@ module EE
def merge_requests_author_approval def merge_requests_author_approval
return super unless License.feature_available?(:admin_merge_request_approvers_rules) 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? return super unless has_regulated_settings?
!::Gitlab::CurrentSettings.prevent_merge_requests_author_approval? !::Gitlab::CurrentSettings.prevent_merge_requests_author_approval?
...@@ -654,17 +651,12 @@ module EE ...@@ -654,17 +651,12 @@ module EE
def merge_requests_disable_committers_approval def merge_requests_disable_committers_approval
return super unless License.feature_available?(:admin_merge_request_approvers_rules) 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? return super unless has_regulated_settings?
::Gitlab::CurrentSettings.prevent_merge_requests_committers_approval? ::Gitlab::CurrentSettings.prevent_merge_requests_committers_approval?
end end
alias_method :merge_requests_disable_committers_approval?, :merge_requests_disable_committers_approval 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)) def license_compliance(pipeline = latest_pipeline_with_reports(::Ci::JobArtifact.license_scanning_reports))
SCA::LicenseCompliance.new(self, pipeline) SCA::LicenseCompliance.new(self, pipeline)
end end
......
ee/app/policies/ee/project_policy.rb
View file @ 6f31b77a
...@@ -76,27 +76,15 @@ module EE ...@@ -76,27 +76,15 @@ module EE
end end
condition(:cannot_modify_approvers_rules) do condition(:cannot_modify_approvers_rules) do
if @subject.project_compliance_mr_approval_settings? regulated_merge_request_approval_settings?
regulated_merge_request_approval_settings?
else
owner_cannot_modify_approvers_rules? && !admin?
end
end end
condition(:cannot_modify_merge_request_author_setting) do condition(:cannot_modify_merge_request_author_setting) do
if @subject.project_compliance_mr_approval_settings? regulated_merge_request_approval_settings?
regulated_merge_request_approval_settings?
else
owner_cannot_modify_merge_request_author_setting? && !admin?
end
end end
condition(:cannot_modify_merge_request_committer_setting) do condition(:cannot_modify_merge_request_committer_setting) do
if @subject.project_compliance_mr_approval_settings? regulated_merge_request_approval_settings?
regulated_merge_request_approval_settings?
else
owner_cannot_modify_merge_request_committer_setting? && !admin?
end
end end
with_scope :subject with_scope :subject
......
ee/changelogs/unreleased/219359-scope-mr-settings-to-compliance.yml 0 → 100644
View file @ 6f31b77a
---
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 @ 6f31b77a
...@@ -155,6 +155,7 @@ RSpec.describe Admin::ApplicationSettingsController do ...@@ -155,6 +155,7 @@ RSpec.describe Admin::ApplicationSettingsController do
prevent_merge_requests_committers_approval: true prevent_merge_requests_committers_approval: true
} }
end end
let(:feature) { :admin_merge_request_approvers_rules } let(:feature) { :admin_merge_request_approvers_rules }
it_behaves_like 'settings for licensed features' it_behaves_like 'settings for licensed features'
...@@ -164,27 +165,7 @@ RSpec.describe Admin::ApplicationSettingsController do ...@@ -164,27 +165,7 @@ RSpec.describe Admin::ApplicationSettingsController do
let(:settings) { { compliance_frameworks: [1, 2, 3, 4, 5] } } let(:settings) { { compliance_frameworks: [1, 2, 3, 4, 5] } }
let(:feature) { :admin_merge_request_approvers_rules } let(:feature) { :admin_merge_request_approvers_rules }
context 'when feature flag is enabled' do it_behaves_like 'settings for licensed features'
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
end end
context 'required instance ci template' do context 'required instance ci template' do
......
ee/spec/helpers/admin/merge_request_approval_settings_helper_spec.rb
View file @ 6f31b77a
...@@ -8,16 +8,13 @@ RSpec.describe Admin::MergeRequestApprovalSettingsHelper do ...@@ -8,16 +8,13 @@ RSpec.describe Admin::MergeRequestApprovalSettingsHelper do
subject { helper.show_compliance_merge_request_approval_settings? } subject { helper.show_compliance_merge_request_approval_settings? }
where(:feature_flag, :licensed, :result) do where(:licensed, :result) do
true | true | true true | true
true | false | false false | false
false | true | false
false | false | false
end end
with_them do with_them do
before do before do
stub_feature_flags(admin_compliance_merge_request_approval_settings: feature_flag)
stub_licensed_features(admin_merge_request_approvers_rules: licensed) stub_licensed_features(admin_merge_request_approvers_rules: licensed)
end end
......
ee/spec/models/project_spec.rb
View file @ 6f31b77a
...@@ -488,68 +488,31 @@ RSpec.describe Project do ...@@ -488,68 +488,31 @@ RSpec.describe Project do
context 'merge requests related settings' do context 'merge requests related settings' do
shared_examples 'setting modified by application setting' do shared_examples 'setting modified by application setting' do
context 'when compliance merge request approval settings feature flag is enabled' do where(:app_setting, :project_setting, :regulated_settings, :final_setting) do
using RSpec::Parameterized::TableSyntax true | true | true | true
false | true | true | false
where(:app_setting, :project_setting, :regulated_settings, :final_setting) do true | false | true | true
true | true | true | true false | false | true | false
false | true | true | false true | true | false | true
true | false | true | true false | true | false | true
false | false | true | false true | false | false | false
true | true | false | true false | false | false | false
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
end end
context 'when compliance merge request approval settings feature flag is disabled' do with_them do
using RSpec::Parameterized::TableSyntax let(:project) { create(:project) }
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 before do
let(:project) { create(:project) } stub_licensed_features(admin_merge_request_approvers_rules: true)
before do
stub_feature_flags(project_compliance_merge_request_approval_settings: false)
stub_licensed_features(feature => feature_enabled) allow(project).to receive(:has_regulated_settings?).and_return(regulated_settings)
stub_application_setting(application_setting => app_setting) stub_application_setting(application_setting => app_setting)
project.update(setting => project_setting) project.update(setting => project_setting)
end end
it 'shows proper setting' do it 'shows proper setting' do
expect(project.send(setting)).to eq(final_setting) expect(project.send(setting)).to eq(final_setting)
expect(project.send("#{setting}?")).to eq(final_setting) expect(project.send("#{setting}?")).to eq(final_setting)
end
end end
end end
end end
...@@ -576,66 +539,31 @@ RSpec.describe Project do ...@@ -576,66 +539,31 @@ RSpec.describe Project do
let(:setting) { :merge_requests_author_approval } let(:setting) { :merge_requests_author_approval }
let(:application_setting) { :prevent_merge_requests_author_approval } let(:application_setting) { :prevent_merge_requests_author_approval }
context 'when compliance merge request approval settings feature flag is enabled' do where(:app_setting, :project_setting, :regulated_settings, :final_setting) do
using RSpec::Parameterized::TableSyntax true | true | true | false
false | true | true | true
where(:app_setting, :project_setting, :regulated_settings, :final_setting) do true | false | true | false
true | true | true | false false | false | true | true
false | true | true | true true | true | false | true
true | false | true | false false | true | false | true
false | false | true | true true | false | false | false
true | true | false | true false | false | false | false
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
end end
context 'when compliance merge request approval settings feature flag is disabled' do with_them do
using RSpec::Parameterized::TableSyntax let(:project) { create(:project) }
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 before do
before do stub_licensed_features(admin_merge_request_approvers_rules: true)
stub_feature_flags(project_compliance_merge_request_approval_settings: false)
stub_licensed_features(feature => feature_enabled) allow(project).to receive(:has_regulated_settings?).and_return(regulated_settings)
stub_application_setting(application_setting => app_setting) stub_application_setting(application_setting => app_setting)
project.update(setting => project_setting) project.update(setting => project_setting)
end end
it 'shows proper setting' do it 'shows proper setting' do
expect(project.send(setting)).to eq(final_setting) expect(project.send(setting)).to eq(final_setting)
expect(project.send("#{setting}?")).to eq(final_setting) expect(project.send("#{setting}?")).to eq(final_setting)
end
end end
end end
end end
......
ee/spec/policies/project_policy_spec.rb
View file @ 6f31b77a
...@@ -1201,129 +1201,61 @@ RSpec.describe ProjectPolicy do ...@@ -1201,129 +1201,61 @@ RSpec.describe ProjectPolicy do
shared_examples 'merge request rules' do shared_examples 'merge request rules' do
let(:project) { create(:project, namespace: owner.namespace) } let(:project) { create(:project, namespace: owner.namespace) }
context 'when compliance merge request approval settings feature flag is enabled' do using RSpec::Parameterized::TableSyntax
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
it { is_expected.to(allowed ? be_allowed(policy) : be_disallowed(policy)) } context 'with merge request approvers rules available in license' do
end 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 end
context 'with merge request approvers not available in license' do with_them do
where(:role, :regulated_setting, :admin_mode, :allowed) do let(:current_user) { public_send(role) }
: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
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 end
it { is_expected.to(allowed ? be_allowed(policy) : be_disallowed(policy)) }
end end
end end
context 'when compliance merge request approval settings feature flag is disabled' do context 'with merge request approvers not available in license' do
before do where(:role, :regulated_setting, :admin_mode, :allowed) do
stub_feature_flags(project_compliance_merge_request_approval_settings: false) :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 end
using RSpec::Parameterized::TableSyntax with_them do
context 'with merge request approvers rules available in license' do let(:current_user) { public_send(role) }
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
context 'with merge request approvers not available in license' do before do
where(:role, :setting, :admin_mode, :allowed) do stub_licensed_features(admin_merge_request_approvers_rules: false)
:guest | true | nil | false allow(project).to receive(:has_regulated_settings?).and_return(regulated_setting)
:reporter | true | nil | false enable_admin_mode!(current_user) if admin_mode
: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 end
with_them do it { is_expected.to(allowed ? be_allowed(policy) : be_disallowed(policy)) }
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
end end
end 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