Some organizations require all changes to be reviewed and approved by specific groups of people before a change can be merged (e.g. every change must be approved by at least: 1 staff engineer, 1 product manager, 1 production engineer). GitLab only supports selecting users (and groups) who can approve the change and a minimum number of approvals required. In this structure it is impossible to enforce the sign off structure in the example above.
This feature is supported by Phabricator by being able to mark a reviewer as "blocking" and is also supported by BitBucket. GitLab should support blocking approver groups so that organizations can be sure the right people review each change.
Original description
We currently use phabricator for our code reviews, and one of the more useful features it has is the ability to mark a reviewer as "blocking", meaning the change is not ready to be released until that specific person approves it.
This is extremely important and useful to us, because our release workflow for one of our products requires changes to be signed off on by at least one person from two or more groups.
Gitlab supports approval groups now, but no way to add multiple groups to a review; nor does it have the ability to mark a reviewer as blocking.
The proposal to add blocking comments kinda would work for this, but honestly, it's not an ideal solution.
Proposal
Extend the approval model to support multiple rules, where a project may have
n
rules, where a rule is:
a list of users and groups –
Ra
is the set of explicit approvers for rule
a
a minimum number of approvals —
ma
is the number of required approvers for rule
a
Each eligible approver who approves the merge request approves the merge request in it's entirety. That is, approvers do not check off each rule one by one, instead they approve the merge request as a whole. This means: when a person who is an eligible approver for multiple approval rules, approves the merge request, their approval will be counted towards each approval rule for which they are an eligible approver. Removing their approval of the merge request will remove their approval from every approval rule.
This will allow the following new behaviors:
individual approvals to be required (Approval from @user required) by creating an approval rule with one user and required approval count of one.
group approvals to be required (At least n approvals from @group members required) by creating an approval rule with one group and an approval count of
n
Configurations:
zero approval rules (Starter, Premium)
one approval rule (Starter, Premium)
multiple approval rules (Premium)
Interactions with existing features:
Override approvers and approvals required per merge request
If enabled the list of approvers and approvals required in an approval rule may be modified.
Removing approval rules per merge request is not allowed. Project approval rules can be nullified by reducing the approvals required to zero. Required approval counts can only be increased, but not decreased.
Adding approval rules per merge request is out of scope for this iteration.
Enable self approval of merge requests
Self approval should continue to function as is.
If the project approval rules include the merge request author, they will automatically be an eligible approver for one or more rules.
If the project approval rules do not include the merge request author, they will not be eligible unless added to a merge request approval rule subject to the override approvers feature being enabled.
Additional approvals once approvals required have been met
This should be reworded 'Additional approvals once all approvals required have been met'
All approvals related project settings have been moved to a separate section under Project Settings → General → Merge request approvals.
Add/Modify approvers
Add
Modify
Undo removal
Mobile
Merge request create/edit form
Desktop
Mobile
Only the number of approvals required is editable on project-level approval groups. If "Override approvers and approvals required per merge request" is enabled, the MR author can add additional approval groups from the MR create/edit form.
MR widget
Ideal
Mobile
Expanded
Mobile
Only one (without name)
User cannot approve
Approve additionally
Revoke approval
In all the MR widget cases, a maximum of 5 approver avatars is displayed in the collapsed state of the widget. These 5 approvers are considered the best approvers meaning they are in more than one approver group and so there is a greater chance of getting the MR fully approved by them.
Approver groups (in the table) and approver (in the avatar rosters) are listed in alphabetical order.
What does success look like, and how can we measure that?
This feature has been prioritized on that basis that customers require greater granularity for meeting their approval requirements, and to enable granular code ownership approvals.
This can be validated by measuring the number of projects with multiple required approval rules, multiple_approval_rules
@markglenfletcher Neither of those solutions is a clean implementation of what I'm talking about, unfortunately.
Assigning to a specific approver doesn't work, because I don't necessarily want a specific person; I want at least one person from two or more groups approving.
Likewise, the unresolved discussions approach would work, in that I can block the MR by opening a discussion and then waiting for someone to sign off before resolving it, but again, it seems clunky, and not intuitive at all. It's not a workflow I can really sell to my coworkers over what they're already using.
My goal here is to have MRs be feature-complete compared to other code review tools on the market, so that I can sell them as a viable approach at my company. We currently use phabricator, and there's a few features that it has that MRs are lacking, due to which we can't migrate to using MRs as our preferred workflow.
@smcgivern : What do you think about a generic design to achieve the feature request here by having multiple "approval buckets" that need to be AND-ed together to unblock a merge request.
On the project level (and override-able at the mr level), you can configure any number m of approval buckets.
In each bucket, you specify the required number of approvals, individual approvers and group approvers as usual.
All buckets need to be satisfied for the entire merge request to be considered passed for approval.
With this scheme, when m=1, we are back to our existing approval scheme. So it's easy to understand from a product / design perspective (and possibly makes it easier to implement?)
Use-case wise, e.g. you would have one bucket for the QA approvers, another bucket for the BE approvers, another for the FE approvers, another for the Database approvers, etc.
@sarrahvesselov : Would you be able to comment on above or have somebody on UX team consider this? I know @dimitrieh and @hazelyang have been deep in approvals.
Would you be able to comment on above or have somebody on UX team consider this? I know @dimitrieh and @hazelyang have been deep in approvals.
UX is overloaded @victorwu. We are stretched very thin between the hiring, OKR, deliverable, and vision initiatives we are involved in. As a result, I have asked the team to be more selective with their time so we can ensure we are focused on the right things. I can't guarantee a UXer will have the capacity to look at this right now.
With this scheme, when m=1, we are back to our existing approval scheme. So it's easy to understand from a product / design perspective (and possibly makes it easier to implement?)
I think the problem is actually our existing approval scheme. (Use named approvers, ignoring the author, unless there aren't enough to meet the number of approvers required. Then, if there aren't enough of those to meet the number of approvers required, reduce the count.) Maybe we'd want to reconsider that as part of this?
@smcgivern : I looked at GitHub and Bitbucket and some of the plugins. I haven't seen any cohesive system that is easy to set up and understand. I propose we make our scheme fairly "basic" in that provides the building blocks to users and they use those building blocks to devise their specific workflows. Specifically, I propose we continue / further separate approver eligibility (who can approve) and approvals required (how many needed). So this is my proposed scheme:
General concept
Continue to have same overriding concept from project settings -> mr settings.
You configure approvals required first.
You configure eligible approvers secondly. GitLab does this for you automatically.
Approvals required
There are three types of conditions. The user specifies these conditions. All conditions must be satisfied in order for the merge request to be unblocked for merging (approvals-wise).
Condition type
Number of allowable conditions for this condition type
Condition format
Example
Overall
1
At least m approvals overall required
At least 3 approvals overall required
Individual
0, 1, 2, ...
Approval from @user required
Approval from @smcgivern required
Group
0, 1, 2, ...
At least n approvals from @group members required
At least 2 approvals from @gl-product members required
In the following example, the user has configured: 1 overall-type condition, 2 individual-type conditions, and 2 group-type conditions:
At least 2 approvals overall required
Approval from @smcgivern required
Approval from @victorwu required
At least 3 approvals from @gl-product members required
At least 3 approvals from @gl-frontend members required
This example is what a user may specify at the project level or merge request level.
GitLab doesn't do anything smart with checking if your conditions can even be fulfilled. For example, it doesn't care if @gl-frontend only has 2 people. (Future iterations may address this.) One approval can satisfy multiple conditions. For example, if @victorwu is in the @gl-product group and in the gl-frontend group, then @victorwu's approval contributes to the first, third, fourth, and fifth conditions. The condition formats are designed so that they cannot contradict each other. (Because they are in "+" format, expect for the individual-type condition.)
Eligible approvers
You configure eligible individual approvers.
GitLab is smart so that it either enforces (or strongly suggests) that the users specified in the
individual-type conditions above are eligible individual approvers.
So in the above example, @smcgivern and @victorwu are eligible individual approvers. But you can specify other individual approvers on top of these.
@smcgivern and @victorwu become eligible individual approvers. You cannot specify other individual approvers.
You configure eligible group approvers.
GitLab is smart so that it either enforces (or strongly suggests) that the groups specified in the group-type individual conditions above are eligible group approvers.
So in the above example, gl-product and gl-frontend are eligible group approvers. But you can specify other group approvers on top of these.
gl-product and gl-frontend become eligible group approvers. You cannot specify other individual approvers.
You configure Developer+ project members to be eligible or ineligible approvers. (This is by default configured on, but can be turned off manually. This departs from https://gitlab.com/gitlab-org/gitlab-ee/issues/4134, but I think makes things simpler.)
Merge request author continues to be always ineligible as an approver.
The project by default has only one Approvals required condition, and that is At least 0 approvals overall required.
The project by default only has an entry for Eligible approvers, and that is Developer+ project members.
Users don't really "use" the approvals feature even though it is "always on". Since the condition is 0+, there is no friction in the workflow, but they are exposed to the feature nonetheless.
Users are exposed to the feature organically in the product and decide to explicitly use it.
At the project level, a project admin decides to change the Approvers required conditions to be 5+ overall and specifies 3+ FE group and 2+ BE group. The project admin then goes to the Eligible approvers section and sees FE group and BE group have already been suggested/enforced by GitLab. They like that. They see that Developer+ project members are eligible too. They don't like that. So they remove that as eligible approvers.
When you first create a new project, or when you're not using approvals explicitly, you'd see something like this in the config:
And then later on, once you start using approvals, you'd see this:
I think the design needs to be improved so that you don't need to have the "Eligible approvers" section in the settings, because it's implied. Not sure.
There is a use case that isn't possible with the suggested solutions:
If you need specific types of approval and one person is allowed to approve multiple types, but doesn't want to check all types all the time. For example if someone is in the code review and the functional review group and only wants to approve the code, but didn't check the functionality. With the suggestions above he could either apprive both or none.
@saithis : this is currently intended for only code review cases. but the logic indeed doesn't actually care about the code itself. are you suggesting a more generic design where a QA person or a business user could test the feature in a review app and then approve?
@victorwu Yes, exactly. Currently we use gerrit, where 2 developer and 1 QA person have to approve. But we also have 2 special developers which can give the QA approval. They are told to only do this when the code change is limited to build or deployment script changes. Additionally our manager (who is also a developer) can give QA and even double code review approval. So he alone could fully approve a PR/MR and merge it. This is needed in case we need an emergency hotfix and not enough people are there to approve it (could for example happen in the 2 weeks around christmas and new year).
The same as our existing approvals, it's much more straightforward to calculate for an individual MR than it is for a selection of MRs (https://gitlab.com/gitlab-org/gitlab-ee/issues/4738). This makes that harder, not easier
I think this is just an oversight, but shouldn't you be able to edit the 'Approval from @victorwu required' section in this mockup?
I think this is just an oversight, but shouldn't you be able to edit the 'Approval from @victorwu required' section in this mockup?
Sure. That's just a UI detail to me. You can achieve the same thing by deleting that and adding someone else back per my UI design. But I expect the UX team to come up with something better overall.
The same as our existing approvals, it's much more straightforward to calculate for an individual MR than it is for a selection of MRs (https://gitlab.com/gitlab-org/gitlab-ee/issues/4738). This makes that harder, not easier
This one is related to the needs of the list view designs. Those designs from @hazelyang were from a while back. Let me ping you and Hazel back on those issues and see which ones make sense now and see how we can make them more reasonable, technically-wise. For example, store the minimal amount of approvals info per mr and use that to generate the list views for each list page load. For more detailed information, you have to either click into the mr itself, or hover on a list item and the system calculates the detailed information on demand.
For example if someone is in the code review and the functional review group and only wants to approve the code, but didn't check the functionality.
@victorwu Perhaps we should make it possible to name approvals in the interface? We could have buttons for Approve Frontend, Approve Backend, and Approve QA with each handled in isolation. A user eligible to approve two categories could choose to click both buttons or just one. Requirements for who is eligible and the number of approvals required per category could be as above.
The existing scheme could be left with a generic "Approve" button, with approval counts moved from the MR to the individual approval categories. MRs list views could then display the number of approval categories left to fulfill in https://gitlab.com/gitlab-org/gitlab-ee/issues/1317.
For our own use it would simplify the separation between Approve Backend (first pass) and Approve Backend (maintainer). The second option could be more restricted, but a maintainer could choose to click both approvals.
@jamedjo : How would we generalize that so that it's not specific to BE/QA/FE, etc.? Different teams have different groups of users right? So we wouldn't want to specify approval types right?
Okay. But why name an approval bucket when we can just name the approval group to be FE/BE, etc.?
Because members and @ groups might belong to multiple buckets, and because it makes the UI clearer.
So for gitlab-ce an individual might be a member of the @gitlab_ce_reviewers group and the @gitlab_ce_maintainers, but only want to approve the Approve Backend (first pass) bucket. Or you might want the @gitlab/developers group to be able to approve both a Code style and Functionality approval bucket.
Then having named it we can have clear buttons in the UI for Approve X and Approve Y which are independent from the eligibility rules which have given you access to both approval buckets.
Can you talk about what problem you are trying to solve with named approval buckets? Which particular scenario would that help with?
@victorwu When a user is a member of multiple groups, but wishes to choose which approval bucket their approval counts towards.
Say team-member-1 is a member of the Database group as well as the Backend reviewers group: it should be possible to approve the MR from a database perspective having only looked at the migrations, without having that mark the MR as approved for Backend. Unless there are separate buttons for Approve Database and Approve Backend this would be hard to do.
Named approvals might not be needed by the majority of users, so it would be interesting to find out, but it felt like a viable solution for some of what was being asked in a handful of the comments:
There is a use case that isn't possible with the suggested solutions:
If you need specific types of approval and one person is allowed to approve multiple types, but doesn't want to check all types all the time. For example if someone is in the code review and the functional review group and only wants to approve the code, but didn't check the functionality. With the suggestions above he could either approve both or none.
They want a method to allow multiple groups to approve certain things. For example, the manager of a group, QA and then perhaps legal or security.
I think your proposal is simpler for users who can guarantee groups don't overlap, e.g. Legal vs Frontend, but that named buckets might be needed when groups can overlap.
Even when groups don't overlap I think having clear buttons for Approve QA, Approve Legal and Approve Security with non-eligible buttons disabled could make it clearer for users when they go to approve an MR. On the other hand it makes it more complicated to set up, so there is a trade off.
Both solutions would allow us to move away from this for UX/Frontend/Backend since the groups don't overlap. Having named approvals would allow us to also have an Approve Database and Approve Performance button.
With backend review we also could split those buttons into Approve Backend (draft review) and Approve Backend (maintainer). A workaround in proposal from your mockup might be to set @gitlab_reviewers -> 2 Approvals needed, @gitlab_maintainers -> 1 Approval needed so that when a maintainer reviews it doesn't mark both groups as approved. It would also be possible to ensure that everyone on the maintainers group was excluded from the reviewers group, but if someone was added to both then they might accidentally count as approving both groups. Of course, having maintainers automatically approve both might be the preferred workflow but workflows vary from team to team.
Having named approval groups might also make it easier to skip approval groups in a later iteration. When creating/editing the MR we could allow approval groups for Performance,Database and UX to be removed, while requiring stronger permissions/group-memeberships to have the MR skip Backend maintainer review.
This would be a huge benefit for our team. We have requirements that changes must be approved by the dev team, QA, and PO/PM. Currently our team is running EE-Starter, but we will absolutely upgrade if this is released in Premium.
@jramsay in this MR, could we assume that all code owners belongs to one rule for now? I feel making distinct rule can be quite complex and also I don't think it's something being requested by customers yet. Thanks.
I've moved merge request approval settings to its own section since the MR settings section was getting out of hand. Approvers can be added/modified through a modal dialog.
Adding/Editing approvers
Add
Edit
Undo remove
MR create/edit form
I've moved code owners outside the table.
MR Widget
Case
Design
Ideal case
Expanded
Only one approval rule (without name)
Revoke approval
Additional approval
Cannot approve
In all the cases, a maximum of 5 approver avatars is displayed in the collapsed state of the widget. These 5 approvers are considered the best approvers meaning they are in more than one approver group and so there is a greater chance of getting the MR fully approved by them.
Approver groups (in the table) and approver (in the avatar rosters) are listed in alphabetical order.
I'll create mockups for mobile screens once this is good to go.
@jkarthik small note regarding the following. It seems there is some ambiguity for what those avatars represent at first glance. Are they all approvers or potential approvers?
@dimitrieh I made a copy fix. View all approvers to View all eligible approvers. I checked the docs and we use the term "approvers" to denote eligible and implicit members who can approve the MR.
@jkarthik, quick question: When a group is included in a rule, will we show the group's avatar in the table or are we expanding the group so that we only show user avatars? The designs only have people faces, so I wanted to clarify this. Thanks for your help!
When a group is included in a rule, will we show the group's avatar in the table or are we expanding the group so that we only show user avatars?
@pslaughter We'll only show users. Groups are to be expanded to show the members. This applies everywhere - project settings, MR create form and MR widget.
@jkarthik from the screenshots I thought it was intended to only expand users at readonly section (MR#show), but in editable sections (MR#new and MR#widget) it displays group name. It makes more sense this way and could you double check?
@lulalala For consistency, we need to stick to a single solution everywhere which is why I decided to always expand groups to its individual members. The user will see the same table UI everywhere. So it will be confusing if the list of avatars were to change.
@jkarthik Would it be possible to allow users that are in multiple approver groups to only give approval for one of their groups? We currently use gerrit and have some users (me included), that are code reviewers and functional reviewers. But most of the time I only code review and someone else does the functional review.
@jkarthik Would it be possible to allow users that are in multiple approver groups to only give approval for one of their groups? We currently use gerrit and have some users (me included), that are code reviewers and functional reviewers. But most of the time I only code review and someone else does the functional review.
I'm pretty sure @jramsay mentioned thinking about this..
Would it be possible to allow users that are in multiple approver groups to only give approval for one of their groups? We currently use gerrit and have some users (me included), that are code reviewers and functional reviewers. But most of the time I only code review and someone else does the functional review.
For this feature, I'm concerned about how we propagate changes in the project settings to preexisting open MR's. WDYT?
TL;DR
Let's keep the overall approval status of MR's in sync with changes from the project settings.
User updates approval rule in project settings
Given a MR that is approved by a member of an approval rule,
When the member is removed from the rule in the project settings,
Then the MR will no longer be approved for that rule.
Given a MR that is approved by a member of a group in an approval rule,
When the member is removed from the group,
Then the MR will no longer be approved for that rule.
User removes approval rule in project settings
Given an MR with approvals for all rules except one,
When that one rule is removed in the project settings,
Then the MR will be fully approved.
User adds approval rule in project settings
Given an open MR with all approval rules met,
When an approval rule is added in the project settings,
And the MR does not satisfy this new rule,
Then the MR should have the new approval rule,
And the MR will no longer be fully approved.
@jkarthik Sad to hear this, I hope it will be made possible in the future.
@pslaughter Shouldn't the last two depend on the "Can override approvers and approvals required per merge request" setting? I wouldn't activate this setting for my private projects and would then expect new/removed approval groups in the project to be reflected in all open MR's.
Shouldn't the last two depend on the "Can override approvers and approvals required per merge request" setting? I wouldn't activate this setting for my private projects and would then expect new/removed approval groups in the project to be reflected in all open MR's.
Such a good point! I'll update my comment... Done! WDYT @saithis?
@pslaughter Yes, it looks good to me now. But for the "User adds approval set in project settings" + "Can override in MR setting is true" case, I think it would also be ok to add the approval set with the configured number of required approvers. I think in this case both options are sometimes right and sometimes wrong. So I would choose whatever is easier to implement.
Edit: The "User decreases/increases number of required approvals für a approval set in project settings" + "Can override in MR setting is true" case would also be interesting
Sorry for the noise everyone. I had included some very trivial use cases in https://gitlab.com/gitlab-org/gitlab-ee/issues/1979#note_117272808 above, because I misunderstood "Override approvers and approvals required per merge request", which sounds like MR's have free reign to create / edit / remove their own rules, but it turns out this is not the case.
I've removed the trivial cases and added a TL;DR to the remaining concern in the comment.
Maybe we should think of renaming this setting since overriding approvers isn't actually allowed...
@pslaughter@jramsay When updating the approvals project setting we could prompt the user whenter to update all existing MRs or to leave them be. This can help prevent MRs from loosing/gaining approvals. What do you think?
we could prompt the user whenter to update all existing MRs or to leave them be. This can help prevent MRs from loosing/gaining approvals.
@jkarthik This is interesting. FWIW, letting MR's naturally lose or gain their overall approval status is the easiest implementation. When a rule is changed in the project settings, a user's individual stamp of approval doesn't change, but it would naturally change the result of an MR's overall approval status. At least, that's what I'm suggesting it should do
@lulalala May be able to confirm or deny my suspicion.
For this feature, I'm concerned about how we propagate changes in the project settings to preexisting open MR's. WDYT?
Thanks @pslaughter for raising this and for the suggestions @jkarthik@lulalala. In the spirit of minimum iteration, I think we need to also consider:
what is the currently implemented behavior?
The purpose of this issue isn't explicitly to improve or change the way approval rules are propagated to already open merge requests.
Based on my testing, the eligible approvers list in the project settings is not propagated, and the required approvers count is propagated up until the required approvers count is changed for the merge request.
will this logic need to be reimplemented anyway?
If the current logic doesn't need much change to continue functioning in it's current mode, or re-implementing the current behavior is very straight forwards, I think we should avoid scope increase that isn't explicitly required.
If the current logic does need to be entirely re-implemented and this is complex, we should implement the correct/preferred logic.
what is the correct/preferred logic for propagating changes to open merge?
The proposal Let's keep the overall approval status of MR's in sync with changes from the project settings is good, but likely complex in practice.
(a) Updating the approvals required for each rule is probably not so hard (current behaviour).
(b) Propagating changes to the eligible approvers is probably more works since for every open merge request we'll need to match the merge request rule to the project rule and only update them if the merge request eligible approvers are the same as the previous project eligible approvers for that rule so that customer approver rules aren't reset (if Can override approvers and approvals required per merge request is enabled)
I think this boils down to, should we increase the scope of this feature to include item 3(b) or not? @lulalala@oswaldo I'm interested on your thoughts, but would prefer this to be split into a separate issue that we try address in 11.6 or a later release rather than including in the first merge request. Smaller scope, and fewer changes per merge request should be what we are aiming for.
I suggest keeping these rules in sync, because it might actually be both the easiest and preferred implementation. Since removing and editing shared approval rules is out of scope (based on issue description and UX design), we might not need the complexity of duplicate data and syncing could happen "naturally".
On the other hand, it may turn out that this is more complex... I certainly defer to @lulalala's expertise
Since removing and editing shared approval rules is out of scope (based on issue description and UX design)
@jkarthik the description says it should be possible to add/remove approvers from the existing rules so that we can continue to support the existing Override approvers and approvals required per merge request feature. Looks like the designs need to be updated to consider this in the merge request edit interface.
...the eligible approvers list in the project settings is not propagated,
Clarification: This is true only when an MR has been submitted while the "override approvers" setting is checked. Otherwise, changes to the project level approval rules are reflected in the MR.
what is the currently implemented behavior?
The purpose of this issue isn't explicitly to improve or change the way approval rules are propagated to already open merge requests.
Based on my testing, the eligible approvers list in the project settings is not propagated, and the required approvers count is propagated up until the required approvers count is changed for the merge request.
I think your case can be a little misleading. Your case is a special case of MR being created when override is turned on. A more precise current behavior would be:
project settings is used when there is no MR level setting
When the override is turned on, creating MR will also create MR level setting (override) which is a copy of project level setting.
When the override is turned off, MR's MR level setting will still be there, and will be used.
In summary, project setting will propagate to only those MRs without MR level setting.
will this logic need to be reimplemented anyway?what is the correct/preferred logic for propagating changes to open merge?
(b) Propagating changes to the eligible approvers is probably more works since for every open merge request we'll need to match the merge request rule to the project rule and only update them if the merge request eligible approvers are the same as the previous project eligible approvers for that rule so that customer approver rules aren't reset
I feel the existing behavior is the easiest implementation, and there is no need to worry about matching project level and MR level settings. I think the new feature should behave similarly.
Migrate existing data into these new tables for this release (11.6). Keep newly created data in sync between old and new tables.
In the next release (11.7), implement the new feature (frontend/backend) changes using the new tables.
In 11.8 remove old tables
The data migration will be recreating rules and members from existing data, which amounts to around 630,000 records, so it needs to be done as a background migration. WDYT?
the description says it should be possible to add/remove approvers from the existing rules so that we can continue to support the existing Override approvers and approvals required per merge request feature. Looks like the designs need to be updated to consider this in the merge request edit interface.
@lulalala Do you have an estimation on the amount of time migrating these rows would take? If it takes less than an hour or so, we don't necessarily need a background migration.
In that case, I don't think we need to wait until 11.7 to implement the new functionality on top of the new data model either. Perhaps we could get the new feature into 11.6 behind a feature flag, that we wouldn't flip until the migration is completed and we don't care about the old data model anymore?
@jkarthik could you consider what the configuration interface looks like in GitLab Starter where only one rule is permitted?
@lulalala@DouweM can we include something in the usage ping so we can measure the adoption of this feature. It's part of the Q4 Product OKR to do this, and I should have called it out in the issue description (added to description). The most basic data point would be to add mean approval rules per project.
Since approvals is enabled by default for every project, meaning if no approval settings are currently set it is optional and every developer is an eligible approver, this means every project on a Starter instance or above has approvals enabled in a minimal sense. It's probably not helpful to include this in the calculation. Maybe we can add:
count of projects with more than one approval rule
count of projects with more than one required approval rule
could you consider what the configuration interface looks like in GitLab Starter where only one rule is permitted?
@jramsay@pslaughter For GitLab Starter I'm thinking we can display just one row of the table without the first column (Name). The same logic applies in the MR create/edit form as well – single row with the edit button. Everything else can remain the same including the empty state for the table and the MR widget.
Once there are fewer approvers left in the list than approvals required or
there are no more approvals required
allow other project members to approve the MR.
I think to adapt this logic in the multi-rule scenario, it would be:
Each rule is considered approved if "remaining approvals needed" is zero, or if approvers left is zero.
If all rules are approved, allow other project members to approve the MR.
@jramsay In the past, merge request's "approvals required count" cannot be lower than that of the project setting. In this issue the restriction is no longer there, we can set it to zero at MR level to nilify it.
However, in the future, could such restriction come back on a rule level? If that is the case, I want to proactively design something in advance to accommodate that. Thanks!
@lulalala can you help me understand better? The way I interpret our current capabilities as being features of one rule, so the rule that the required count set in the project settings is the minimum when overridden at the merge request should continue, but now on a per rule basis.
What this means that for each approval rule, is that it can't be reduced below the count required for that rule in the project settings. By way of example, if I have two rules: 1 product manager must approve and 2 engineers must approve, neither can be set to zero, they can only be increased. Does this make sense?
Override approvers and approvals required per merge request
Removing approval rules per merge request is not allowed. Project approval rules can be nullified by reducing the approvals required to zero.
I thought this means that MR override rule can set to zero, which seems to be the opposite to what you are describing?
@lulalala oops My intention was consistency with current behavior.
My understand was that it is currently possible to override the number of approvals and reduce it to zero, I think because the edit form allows you to change the number to zero (in fact even negative numbers). I've tested it more carefully and can see that if the required approval count is 1 anything less than 1 is ignored (e.g. -4 or 0).
I'll try to fit that in. One edge case is that, after MR rule is created, the project rules get destroyed and then a new project rule is created. I don't think there is a trivial way to handle this, but I think if people complain in the future we can then see how we can handle that.
Unfortunately, I had a family emergency come up Monday afternoon and I've been fairly unavailable since. With this, the impending holidays, and maintainer availability, I feel a little behind. I think there's a risk that the FE for this won't be merged by the 7th, but I'm going to do what I can to mitigate that risk.
Worst case, since this is behind a feature flag, will we be able to pick it after the 7th?
Currently I am a bit stuck on implementing the security around private groups. In the past, we use presenters to override approver_group method to filter the groups users see. However now there being multiple rules, we kind of requires some double presenter - a presenter which would also present each of the rules.
I found this a little complex. Would the following be an acceptable solution?
If a private group is inaccessible to the user, display a dummy group instead.
Don't expand group into its members, eliminating private group membership leak.
P.S. I feel I need to recheck/reimplement all of the past approver related issues. Hopefully there isn't too many.
@jramsay I discovered one edge case behavior difference, just wondering would this be a concern or confusion to user?
Some project (without MR override) sets approvals_required to be 1, but does not set up any approvers. The intention was probably wanting the MR author to find someone to approve. Without explicit approvers set, project member becomes implicit approvers.
When this is converted to the new schema, it is considered approved however. This is because:
A rule without approver is considered approved.
A MR without any rule is also considered approved.
If this is a not the expected behavior, I suggest we keep project level approvals_required setting, representing the total number of approvals required. (But we can ignore the MR level approvals_required setting as it gives little value)
Some project (without MR override) sets approvals_required to be 1, but does not set up any approvers.
@lulalala, I think the user's intention for this would be "There needs to be at least 1 approval (no matter who)". For an MR, I don't this rule should be satisfied until there's at least 1 approval. In other words, if a rule has no specific members, it matches all members.
if a rule has no specific members, it matches all members
This isn't very easy to implement this way. Having an overall check is easier.
This is also why I suggested to you that I'll be doing the approved? calculation at the backend.
You wouldn't have to any extra calculation.
@lulalala I agree with @pslaughter that this wouldn’t be the users intention when setting approvals required to 1 but not selecting any approves. It’d be really confusing to set a requirement that just gets ignored in this way.
In summary:
we shouldn’t introduce another setting for minimum approvals required across the whole merge request
if a rule has a minimum of 1 approval, and no approves set, the rule should require one approval by one of the eligible approves per existing logic.
This is simpler to reason about as a user because it is consistent with existing behavior, is the more easily expected behavior, and only has one level of configuration. If it isn’t possible to make achieve this, let’s have a conversation how we can maybe break this down and resolve in a second iteration in 12.0.
However this is just the the existing setting, not a new one.
the rule should require one approval by one of the eligible approves per existing logic.
Currently the rule is designed to only work with explicitly assigned approvers. Counting implicit approvers (project members) would make the implementation very complex, if not impossible. It is also very confusing. Imagine a member approves, and was recorded as approving the "security" rule, yet he is not related to security.
In that case, there would be two different "No. approvals required" input fields, one to use if you don't care about who approves (and there is no approver rule), and one to use if you do (and there is an approver rule).
The way he imagines it, it would be possible to create a new approval rule with no connected users or groups at all. It could be edited to add users or groups, but in that state, any approval of the MR by an "eligible approver" would create an approval_merge_request_rules_approved_approvers record for that particular rule, thus counting towards the "No. approvals required". In the settings UI under "Members", the rule could either list all eligible approvers (which may be a lot, so that may not be feasible), or display something like "Any eligible approver".
If that rule was called "Security", it would indeed be confusing, but I imagine the "Security" rule would actually have users or groups added to it, and the rule without users/groups would be called something like "Anyone".
In Premium, where multiple rules can be created, it would still be possible to create a rule like this if you want to have a minimum total amount of approvals of the MR. Again, any approval would count toward that rule through a new approval_merge_request_rules_approved_approvers record.
Right now, we migrate projects.approvals_before_merge to approval_required on the newly created approval rule(s), but if there aren't any, the value is "lost". With this setup, we could preserve the value and behavior by creating such an empty "Anyone" approval rule and setting the value there.
From a technical perspective, I imagine we'd want to add something like any_approver_allowed? to ApprovalMergeRequestRule if no users/groups are attached, and have ApprovalMergeRequestRule#approvers and ApprovalMergeRequestRule#unactioned_approvers return empty arrays. if any_approver_allowed?, it looks like ApprovalWrappedRule#approved? would need to be modified to only look at approvals_left <= 0, and not also unactioned_approvers.size <= 0, while ApprovalWrappedRule#approved_approvers would return all merge_request.approvals.map(&:user) instead of just those inside approvers.
What do you think?
Of course, implementing this may be much more complicated than what I'm suggesting here, and either way it would take more time than we have currently allocated to finishing this feature in 11.8. I strongly think we should try to "get this right" before putting in front of our customers, though, even if that requires some more iteration, and means we can't merge this in time for 11.7. Do you think it would be doable in time for the 11.8 feature freeze, if we move https://gitlab.com/gitlab-org/gitlab-ce/issues/19054 to 12.0?
I've booked some time on your calendar tomorrow to discuss this some more!
Having worked on this system for so long,
there are many interlinkings, and the implementation suggestions has to consider the ripple effects.
It's like adding another dimension to consider.
That's why all of the alarms in my head are ringing.
mental complexity increased
many many more lines of code are required
but a working system is there already
main concern with keeping the project-wide "No. approvals required" setting is that it would result in a confusing experience for users of GitLab Starter, where a project can only have a single approval rule
This can easily be bypassed by hiding that field in Starter plan.
The current design has two levels of computation:
The ApprovalRule object represents property related to the rule's user and groups.
The ApprovalState object contains a collection of rules. It draws from those rules calculate each approval related properties.
The approvals, being approving MR as a whole, fit right into the same level as ApprovalState.
Imagine we already have the rule system in place, and someone is asked to add "check overall approvals required".
Since it sits at the overall level, bending the system so an MR level rule can act as an overall level check feels just like a hack.
Usability-wise it is also unclear that an empty rule means anyone can approve.
In that case, there would be two different "No. approvals required" input fields, one to use if you don't care about who approves (and there is no approver rule), and one to use if you do (and there is an approver rule).
I don't know too much context here, but I am wondering what if we:
Still keep project level approvals_required
However once an explicit rule is created (which requires at least one approver), we ignore the project approvals_required
This way, in the UI we show approvals_required when there's no rule, and users can still create a rule, and once a rule is created, we hide and ignore approvals_required.
I think this should be clear that if a rule is set, users should follow the rule. If there's no rule, it just looks at approvals_required. We can think of that as a way to have a base rule which can only be used when there's no other rules.
I also think this should work better because if we allow setting empty approvers for a rule, what if there are multiple rules which don't have any approvers? Does it work like whoever approve, they effectively approve all rules without any explicit approvers? That also feels a bit strange.
If we really want to have something like this, maybe it'll be easier to understand and implement if we can set a project as an approver, similar to a group, which effectively means all the members in the project.
@lulalala What do you think? Will this be easier to implement?
I've updated my comment above, but here I want to go through another thought process I had, in order to illustrate how many factors I need to consider.
And to re-iterate, the current implementation already works fine.
If we have to change, one issue to consider is that the current migration is set to have no record if there is no approver. So we need somehow to provide one if there is no approver.
For plan A, we can go for the virtual record route, generating one rule on the fly. During update action, if I ever see a rule without approvers, I would intercept it to update the project's approvals_required accordingly.
But this wouldn't work if there exists multiple rules without approvers. The setting on one rule would overwrite another one.
For plan B, we create an empty record in the database when viewing the MR. We wraps empty record with another wrapper which behaves similar to what Douwe describes (e.g. #approvers would return [] since we wouldn't want to display all project members anyways).
So what are the criteria in creating this? It would be
project has no rule
project approvals_required greater than 0...
Then I realized that I missed the fact that I can't rely on Project#approvals_required setting. I'll need to create an empty rule for every project without rule, and ensure when a new project is created, it also create one such rule.
There is also a validation checking if all project rules are referenced by a MR rule. This is to ensure the approvals_required can only increase not decrease at MR level. If the project has the empty rule, each MR also would need an empty rule, else any updates to MR would fail. So the migration must cover MR too. Due to the size of the MR table, the migration time would probably take days.
After this, some MR's approval_rules_overwritten? status will switch to true. Would this have any impact? All places calling this would have to be evaluated. Could some leaky abstraction appear else where? I do need to check too. I'll stop here.
After the discussion with Douwe, I think it is doable, but doing all the check and code/specs rewrite is tiresome.
I'm wondering if this is becoming overly complex because of how we scoped this iteration. Could this actually be made simpler by allowing approvers to approve for only a specific rule?
@jramsay, @lulalala, @DouweM, and I discussed this issue on a call and here was the agreed approach:
(The following is similar for merge requests and projects)
In the BE, we'll have approval_rulesand an overall approvals_required. The overall approvals_required field only makes a difference if there are no rules associated with a project or merge request.
In the FE, when no rules exist, we will show an "empty rule" - no name, no members, but it has an approvals_required. This approvals_required actually sets the overall MR or project approvals_required.
To the user, this looks like a regular approval rule, but behind the curtain it's just the overall approvals_required which behaves like it does today.
If a user adds members to this rule, it becomes a regular approval rule which does not effect the overall BE's approvals_required. When approval_rules exist, the overall approvals_required field does not make a difference (and is not visible to the user).
If a rule is orphaned (the associated users or groups get deleted), it will still exist in the project and MR's until it's removed by the user. These rules will always be "approved", but they are in an invalid state and we should show that to the user somehow.
Based on my call with @pslaughter I've updated a few screens for Starter and Premium users. I think this helps to better communicate the default implicit rule (Developer+ and Code owners).
The empty state of approvals project settings in the previous proposal has been removed. Instead, the user will see an implicit approval rule that all developer+ members and code owners are eligible approvers. The default approvals required count for this rule is 0 and the user is free to increase the count.
A starter user, clicking the approval rule edit button, will see the following dialog. The "name" field will not be shown and I've updated the empty state copy to indicate the default implicit rule. Also, the save button label has been changed from "Add approvers" to "Save changes". No changes on this dialog for premium users.
Minor change: I've replaced the edit icon button (for approval rules in project settings) to a text button. When there is only one rule, the lonely icon button would be hard to discover for a user. This change needs to be made in the project settings page as well as the create/edit MR form page (for consistency).
Hi, may I have a question to the planned interaction of Starter vs. Premium difference (one vs. multiple allowed rules) with code owners? Current design suggests requiring code owners are one rule, but the file can contain multiple mutually exclusive rules inside, each rule with it's own min approval count.
@zakjan Great question, the issue for that specific feature states:
Until all matching code owners rules are satisfied by one approval (or more), the merge request cannot be merged.
So I think that each line in the CODEOWNERS file that matches changes in the merge request, should the required number of approvals. I'm going to discuss that number in the original issue https://gitlab.com/gitlab-org/gitlab-ee/issues/4418
(Recreating this comment as a discussion. Sorry for the noise!)
I had a call with @jramsay to clarify my confusion on one of the requirements.
Only the number of approvals required is editable on project-level approval groups. If "Override approvers and approvals required per merge request" is enabled, the MR author can add additional approval groups from the MR create/edit form.
Let's call MR rules that are backed by project settings "default MR approval rules".
Here's some notes on this behavior when Override approvers and approvals required is checked:
The approvals_required of default MR approval rules must be greater than or equal to the corresponding project setting's rule. This is similar to the existing behavior.
In GitLab Starter, the list of approvers is overridable at the MR level (existing behavior), but in GitLab Premium it is not. This means the GitLab Premium is technically more restrictive to MR authors.
What does this mean from an admin's perspective?
Thinking of our workflow at GitLab, we would probably configure our project's approval rules (i.e. "Frontend", "Backend", "Security") each with 0 approvals_required. This means that MR authors can opt into these rules by setting the desired rule's approvals_required to 1 or above.
Edge cases
Let's clarify the following behaviors in future iterations.
(Referencing point 1 above) Say a MR is created with default approval rules each with the default approvals_required value. What happens if an admin increases a rule's approvals_required in the project settings? Should this make my MR edit invalid until the necessary approvals_required are also increased? Currently, we magically increase the overall approvals_required on MR udpate when we see that it's less that the project settings.
(Referencing point 1 above) Say a MR is created with default approval rules each with the default approvals_required value. What happens if an admin delete's the corresponding rule in the project settings? Should the MR author be allowed to set approvals_required to 0?
In GitLab Starter, the list of approvers is overridable at the MR level (existing behavior), but in GitLab Premium it is not. This means the GitLab Premium is technically more restrictive to MR authors.
I talked about this point with @lulalala. It sounds like overriding approvers in GitLab Starter but not in GitLab Premium adds some BE complexity we'd like to avoid.
@tristan A regression was discovered, so we temporarily turned this feature off Once the patch release is deployed, we will be turning it back on. Sorry about that.
@jramsay The online help for this feature incorrectly states that multiple approval rules are available in ULTIMATE and not in PREMIUM. The online docs however are correct.
The new way of managing approvers does not add any value in GitLab Starter. It makes it more cumbersome. To make things worse, it can only be done using a mouse (selecting, adding).
Therefore we use the feature flag to disable the new way, but will be gone in 12.0.
The new way of managing approvers does not add any value in GitLab Starter. It makes it more cumbersome. To make things worse, it can only be done using a mouse (selecting, adding).
@maikelv Functionality wise, the new feature should have stayed the same for users on GitLab Starter. But we certainly didn't want to introduce a step backwards UX wise to those users. Would you be open to creating a new issue so we can discuss ways to improve the way approval rules are managed? What behaviour did you like in the old implementation? Perhaps we can even figure out a way that improves the feature for users of GitLab StarterandGitLab Premium.