Integrate instance-level with group-level MR approval settings
Problem to solve
At the moment, instance-level MR approval settings do not have any effect on the group-level counterparts.
Proposal
- When instance-level settings restrict permission, the corresponding settings on the group level are enforced and locked.
- When instance-level settings do not restrict permission, the corresponding settings on the group level are unlocked and revert to group default or previous value.
- All new top-level group MR approval settings inherit from the corresponding instance-level ones
Instance Value | Group Value |
---|---|
Restrict |
Disabled (and locked (Instance level is SSOT) |
Do not restrict | Revert to group default or previous value (Group level is SSOT) |
Implementation
Current instance and group setting map:
Group Setting | Instance Setting |
---|---|
allow_overrides_to_approver_list_per_merge_request | disable_overriding_approvers_per_merge_request |
allow_author_approval | prevent_merge_requests_author_approval |
allow_committer_approval | prevent_merge_requests_committers_approval |
retain_approvals_on_push | N/A |
require_password_to_approve | N/A |
Option 1
- Create custom methods for every settings on
group_merge_request_approval_setting
model that take into account the instance-level MR approval settings. Note: watch out for flipping logic 🤯.
class GroupMergeRequestApprovalSetting
def allow_overrides_to_approver_list_per_merge_request
strong_memoize(:allow_overrides_to_approver_list_per_merge_request) do
next false if ::Gitlab::CurrentSettings.disable_overriding_approvers_per_merge_request?
super
end
end
end
- Ensure the group settings value are locked, both in UI and API (ie. using
GroupPolicy
)
# GroupPolicy
with_scope :global
condition(:locked_approvers_rules) do
@subject.feature_available?(:group_merge_request_approval_settings) &&
::Gitlab::CurrentSettings.disable_overriding_approvers_per_merge_request
end
Option 2
- Create a class that encapsulates the compliance rules (ie.
ComplianceManagement::MergeRequestApprovalSettings
)
# Resolve settings using compliance rules
class ComplianceManagement::MergeRequestApprovalSettings::Resolver
def initialize(group)
@group = group
end
def execute
[
allow_overrides_to_approver_list_per_merge_request,
allow_author_approval,
allow_committer_approval
]
end
private
def allow_overrides_to_approver_list_per_merge_request
instance_value = ::Gitlab::CurrentSettings.disable_overriding_approvers_per_merge_request?
group_value = group.merge_request_approval_setting.allow_overrides_to_approver_list_per_merge_request?
ComplianceManagement::MergeRequestApprovalSettings::Setting.new(
name: :allow_overrides_to_approver_list_per_merge_request,
value: instance_value ? false : group_value,
locked: instance_value,
inherited_from: instance_value ? :instance : nil
)
end
end
# Value object
class ComplianceManagement::MergeRequestApprovalSettings::Setting
attr_reader :name, :value, :locked, :inherited_from
def initialize(name:, value:, locked:, inherited_from:)
@name = name
@value = value
@locked = locked
@inherited_from = inherited_from
end
end
# Usage in API
class API::GroupMergeRequestApprovalSettings
get do
setting = ComplianceManagement::MergeRequestApprovalSetting::Resolver.new(group).execute
end
end
Edited by Tan Le