Many 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.
@DouweM@oswaldo in addition to the UX considerations, I'd like to have some discussion around how we might approach building this. I know we just made improvements to approvers logic by allowing self approval, and I don't want this feature to pile on large amounts of technical debt.
I'll add some notes about how I think code owners can interact.
@DouweM@jramsay This is still a high level idea, but here we go:
What we have now
approvers table which configure the users who can approve a MR, both on project and MR level (MR configuration takes precedence over projects).
approver_groups table which configure the groups users who can approve a MR, both on project and MR level (MR configuration also takes precedence over projects).
approvals which persists who approved and which MR this approval went in.
Proposal
Introduce a approvals_rules table with:
target_id - polymorphic relation for project and merge requests
name with a default name for the initial rule (suggestion: Default)
approvals_required (which can map the values we currently have for approvals_before_merge on projects and merge_requests)
Change the way we persist approvers and approver_groups:
target_id points to a approver_rule instead projects and merge_requests
The migration
Every project with approvers (users or groups) should have a default project approval rule
Every merge request with approvers (users or groups) should have a default merge request approval rule
The merge request approval rule will take precedence over the project approval rule
The initially created approval rules will take the current configured approvals_before_merge from projects and merge requests
What the user will see is a transparent transition, if the project has a default configuration for approvers, we'll see a default rule named Default (or other name) with the same users, groups and minimum amount of approvals to merge.
I may be overlooking something, or not foreseeing the integration with the current 'code owners' feature. But that's a initial thought.
So imagining a project with a default rule with 2 users and 1 approval required, then we create a new one with 3 users and 2 required, we should present on the MR:
@oswaldo Would the rules need to have names? In your example, I think we could say:
1 approval required from [avatar] [avatar]
2 approvals required from [avatar] [avatar] [avatar]
We could turn that into a sentence, although that may get very long if there are a lot of rules. Either way, this is really a question for UX :) I like your technical proposal!
@DouweM The idea with it is giving the ability to name it something like Technical, Editorial, basically labelling the approval step. But we can also make it work without it :)
Edit: Instead Default we can make the initial one something like Overall.
Thanks @oswaldo@DouweM – the other element to consider is requiring approval by code owners and how to represent this. Could this be represented as one approvals_rules for each distinct code owners rule?
A distinct rule would be defined by the list of owners, not the file pattern. For example the following CODEOWNERS file would only ever have one distinct approvals_rules created for code owners.
If the approval is required by code owners then approvals_required: 0 else approvals_required: 1approvals_required: 1 else approvals_required: 0.
We'd probably need special UI handling, but this allows us to ensure that each code owner rule is met, and also render approval requirements in a structured manner.
@jramsay Here are some mockups of how approval rules can be implemented. I would love to hear your thoughts and please do tell me if I've missed anything.
Could this be represented as one approvals_rules for each distinct code owners rule?
Yep! According to https://gitlab.com/gitlab-org/gitlab-ee/merge_requests/7933, it seems we can update an exclusive approvals_rule for code owners (or be more specific and create a approvals_rule for each code owner entry). Then we could automatically set to 1 required, and/or let the user edit it via UI. cc'ing @lulalala as he is diving into this part of the codebase.
The good point about having this table is that it makes it generic enough to create how many rules we need to. So if we need to match with the 'code owners' entries, we're able to do so.
This means we can't simply list every code owner one by one. Also I think it would help to list the pattern after the list of eligible approvers so that it is apparent why they are needed.
@jramsay In the example you provide, would it make sense to collapse the rules into only jacob@example.com must approve because jane@example.com's approval is not going to make a difference either way.
would it make sense to collapse the rules into only jacob@example.com must approve because jane@example.com's approval is not going to make a difference either way
@DouweM I don't follow. Because README.md is changed the * rule must be met, which requires either John or Jane to approve. At a minimum 2 different people will need to approve, either:
If the approval is required by code owners then approvals_required: 0 else approvals_required: 1.
I don't quite follow this. Isn't it another way around?
Some questions:
Multiple versions of code owners
At project level We can have different code owners file in different branches. Which version do we choose?
When user is project approver and code owner
How do we represent this, as there is a need to have the user displayed as two distinct rules.
Here I actually want to propose to not have the code owners source be editable. Code owners can be displayed as approvers, but can not be removed by MR author. I feel it would make this and #1012 (closed) easier to implement.
Overlaping with approval-groups?
Seems approval_rule would have many approvers. Wouldn't this slightly overlaps the role of approval group? If we want multiple approvers per rule, can't we use approval group instead?
The UI design does not cover the case where one single user must approve something.
At project level We can have different code owners file in different branches. Which version do we choose?
We use the CODEOWNERS file from the target branch – where the change is being merged into
When user is project approver and code owner
I agree that they shouldn't be removable or editable. With regards to how to present this, I think each distinct code owners rule should be represented as an approval rule in addition to any approval rules configured for the project.
Overlaping with approval-groups
I don't fully understand the question. Each approval_rule can have multiple users and/or multiple groups (although we don't support groups in the codeowners file yet). Requiring one user or one group feels unfriendly and inflexible and would be frustrating to use in practice.
I've made a summary on code owner rules, and offer two ways to implement it. @oswaldo / @DouweM could you take a look?
Note: code owner is abbreviated as CO
Currently when MR approver override is off, we read settings from project level directly, since it is the same for all MRs.
We can convert CO file into approval rules. But since this differs from MR to MR, it is a MR level setting.
Assuming we have project level setting like this:
After we add code owner into the mix, there are two ways to implement this:
Keep those CO rules in memory. Each time we need them we recompute them.
Persist those rules in database at MR level.
To keep in memory
If we choose to keep those CO rules in memory:
It makes more sense to not allow those to be removed, even when author is allowed to override.
Recomputing everytime may be slow.
Less db space needed.
To persist at MR level
If we choose to persist, each MR will have a few rows in the rules table, even when overriding is off.
This being the case, we should also always make a copy of project setting and save as MR setting. This way we only load from MR setting, lowering the complexity.
CO Rule being readonly
Regardless of which implementation to take, CO rules should only be managed by the system, not editable by the user.
Representing a rule and its associated owners as various form input fields is complex, but does not add much value. They are refreshed by code push anyways. So allow user to delete CO rules and later discover it comes back is just confusing. Besides, extra logic is needed to keep edge case behavior consistent.
Side note:
A couple of descriptions in the current doc are inaccurate:
If the approvals settings are overridden for the particular merge request, then the set of explicit approvers is the union of the default approvers and the extra approvers set in the merge request.
Currently it is either the MR level approvers, or project level approver if not overidden. It is never the union of the two.
If the approvers are changed via the project's settings after a merge request is created, the merge request retains the previous approvers, but you can always change them by editing the merge request.
@lulalala Thanks for writing this up, and for the hand-written graphs :)
A few questions about the "to keep in memory" approach:
How expensive would live recomputation be? What order of time are we talking about? milliseconds? hundreds of milliseconds? seconds? Large projects will have large MRs, and large codeowners files. Would this live computation timing still be acceptable?
After an MR is merged, and the target branch is updated or even removed, can we still display historically accurate "required approvers" information on the MR? It would be confusing if at the time, the MR got exactly the approvals it needed, but 3 months later it looks like it didn't.
Right now, I'm leaning towards the "persist at MR level" option. Are you aware of any downsides of that approach that may outweigh those of the other one?
I did a quick benchmark with existing CO suggestion feature:
(ms)
user
system
SQL query
wall
load CO file
2
1
15
load touched file paths
1
0
0.3
0.7 (O(1))
check feature permission
1
0
0.2
1 (O(1))
compute owners
extract/load user
1
0
0.3
15 (seems < O(n))
overall
8
1
47
I don't know how representative is the wall time recorded on dev machine.
As the CO file is small and touched files are few.
When rules table is involved, more virtual AR objects will be initialized, so will probably take longer.
And since this is done per merge_requests#show, it does indeed feel wasteful.
can we still display historically accurate "required approvers" information
Approvals are not change so that's cool.
Approvers can change if target branch's CO changes. The thing is that is is already the case now. In projects without override, a MR references project approver, and one year later the setting can change. But that shouldn't be a problem because approvals are not changed.
After this I also feel that we should persist.
Data migration is needed to insert rules on past data
Each MR will take a few rows in the rules table and a few rows in the approvers table, even when override is off.
Project approver updates and CO updates no longer reflected for projects without override (in a way this is good since the behavior will now be consistent with projects with override)
@jkarthik thanks for showing me those wire frames. A few thoughts:
adding an extra layer of configuration shouldn't increase the complexity of approving a merge requests for simple configurations (e.g. only one approval group)
multiple approval groups can be used in many configurations, e.g. one group might have approvals_required > 1 and the rest are zero making them optional
I recall seeing that you had a required_approvals field for each rule and also a total for the merge request. This second field is out of scope for this iteration, but a feature proposal we should create to capture interest.
Here's a few of the situations I mentioned to consider:
merge request author when they create the merge request trying to work out who to ping first for a review
merge request author after approvals have started trying to understand who else needs to approve
reviewer should know which attributes of the merge request they are responsible for reviewing. That is if they are in three different groups that have different review responsibilities, they need to know by clicking approve they are indicating they have fulfilled all three responsibilities
engineering manager trying to work out how to get a merge request merged quickly – which approvers should they ping for maximum efficiency (the more rules an approver is eligible for, the more efficient the review)
When an MR is updated, we currently wipe out all approvals (if that project setting is enabled), which means that a frontend approver would need to re-approve an MR even if the latest push only touched the backend, and the code they reviewed and approved didn't actually change.
Once we automatically determine required approvers based on codeowners, I think we could make this behavior a lot smarter, and only require re-approval from the people who own the code was actually changed, leaving all other approvals alone.
@lulalala How difficult would this be to implement on top of the architecture we've been discussing here?
@jkarthik here's an issue with proposed design improvements to the merge request approvals settings: https://gitlab.com/gitlab-org/gitlab-ee/issues/6346 – this might be useful inspiration and can probably be resolved by the interface changes made for multiple blocking approvals.
@jramsay With the feedback from the last set of wireframes, I decided to make high fidelity mockups.
Add/Edit approver groups
Create/Edit MR Form
Adding approvers
Confim approver group removal
I felt there were too many moving parts in the previous iteration of this functionality (form+modal). I've simplified this UI even further from the previous iteration. Approver groups and members can be added directly from the table. Code owners are listed at the top and only their "approvals required" is editable. The MR author can create approver groups with 0 "approvals required" - optional approver groups.
MR Widget
Collapsed
Expanded
The approval section in the MR widget is split into two halves. The top half has information regarding approval status and approvers. The bottom half has the appropriate call to action for the currently viewing user.
The primary problem that I tried to solve in the MR widget is the discovery of the "best" reviewer. Instead of listing approver groups, I'm listing all the approvers sorted first by the number of groups they belong to and second alphabetically. My assumption is that a user in more than one approver group is likely a better reviewer for the MR author or EM to reach out to.
The list of approvers will only show those who are eligible and required to approve. For example, it will not show optional approvers, project members who are Developer+, etc. Another example that I can give is, let's say a UX approver group has 5 members with 1 approval required. The list will initially show all 5 members of UX. If the MR get approved by any one of them, then all them are hidden from the list (as long as they are not members in another approver group).
Approved
Additional approval
Non-member
@matejlatin@dimitrieh I would love to hear your feedback on this as well (if you have capacity )
@jkarthik thanks for sharing! Some marked up screens first:
Editing approval rules
Merge widget expanded
Customizing approver groups on a merge request
We need to try and simplify this. There is simply too many options and configurations to customize per merge request.
I think we can remove the whole code owners section and replace it with one read only line that says Code owner approval is required.
Also, let's completely avoid adding/removing custom approval groups on a per merge request basis. The current option we have on the project merge request settings is Can override approvers and approvals required per merge request – let's interpret this as:
adding/removing approvers from the project configured approval groups
changing the number of approvals required on the project configured approval groups
I think these changes can dramatically simplify the merge request editing interface.
Merge request widget
I think there is opportunity to better layer the disclosure of information here.
The summary state is nice and simple – is what was previously designed/is it 1:1 with current widget. We should do a careful review of all the approval states currently so that we don't get caught out.
The details state is hard for me to interpret and looks like it would grow really quickly. I think we should explore ways to present this information in less vertical space, and focus on answering the question of which approval rules have not been met – e.g. is it frontend, backend, QA, or code owners. I'm imagining a horizontal summary: ✅ frontend, ✅ backend, 🔶 QA, 🔶 code owners. This information could even be bubbled up to the summary state.
Also, I think code owners should be summarized as one approval group unless the user needs more detail.
An unresolved problem is who should I ping to review the merge request next? The current merge widget shows who the eligible approvers are and I can just ping the first person I see to look at it. How do I do that in this design. If I'm just a developer doing a code review, I just want the widget to show me some faces of people who should review the merge request next.
Managing project merge request settings
Can you create a click through of how you think this will work? It's important that this doesn't feel intimidating, and also we shouldn't encourage users to create needlessly complicated rules either.
Hey @jkarthik the following bit is confusing to me:
It says that it still needs to be approved by 2 approvers, are these two marked by the green icons? I would expect it to be the other way around, the green ones have already approved...
Could we skip the scrollbar on the list of approvers? It creates a scrollable area on a scrollable page and it gets tricky to use. The section is expandable so would it really hurt if the list of approvers is long (if there was no scrollbar)?
I've posted updated designs below as a separate discussion @matejlatin.
Regarding the confusing approval count and icons, I've opted to list the users in a grouped way. This helps when the MR author has to figure out whom to contact for the next stage of code review.
I've removed the nested scrollbars since they are not needed anymore.
Regarding the group name label, yes, they are labels for the text inputs below.
Regarding code owners being uneditable, I think users who are creating merge requests will likely the code owners features. Only the users who are owners of the changed file are listed here. So, I don't think we need any help text here.
Thanks for the feedback @jramsay@matejlatin. I've updated the mockups and addressed all the concerns that were raised.
Approval settings
Project settings
Adding approvers
Confirm removal
Create/edit merge request
Approver groups are fully managed from Project settings and are customizable from the create/edit MR form.
MR widget
Collapsed
Expanded
In the collapsed state, there is a clear summary of pending approvals. By expanding the widget section, the user can see the list of users and figure out whom to contact for the next stage of code review.
Approved
Additional approval
Non-member
Same user in multiple approval groups
Not approved
1 approved
I've added explicit approve buttons for each group
If a user is present in more than one group, they can choose the group to cast their approval.
Regarding the feature name, I've avoided using the term "Approver groups" explicitly. Users will simply see the feature as "Approvals".
Buttons are often embedded in or shown next to components like modals or editors. Similar to ordering, buttons with dismissive actions are left aligned and buttons with affirmative actions are right aligned.
@jkarthik we're getting closer! As I mentioned when we spoke earlier today, let's try and simplify this as much as possible. I think we should have the ambition to make approvals simpler than they currently are for a single approval group (Starter and many Premium users) even though we are adding more power for the most sophisticated Premium users.
Project settings
Widget
Widget
Project settings
This is feeling very close. Can we use a different pattern to avoid the always present empty row?
Scenarios that have yet to be considered:
Starter (0 or 1 approval group)
Premium (0 or n approval groups)
Perhaps always show 1 row that is empty by default to support one approval group, and then have a button to add more groups.
Also, when there is only one approval group, we the Name field doesn't make sense. Can you consider how we avoid this – definitely for Starter, but also for Premium too.
Merge request settings
Code review approvers don't need to be shown in this table/editable form because there is no way to configure them. It's confusing for a row to be read only. Instead we should have a note below the form indicating the Code owner approval is required.
Merge request widget
Summary state
I think we need to provide more information in the summary state.
the list of people who can approve the merge request has been removed. This is really helpful information so that I can mention or assign a person to do the next review.
approval progress is shown by the number of people that have been approved, but I don't know which approval groups have been satisfied. It would be helpful to understand progress both on person and group level.
Details/expanded state
Can we simplify the table? The approvals required column inserts a really large gap between the name and the people. Maybe the row could look like:
Thanks for collapsing the code owners into a single row, but I think we also need some way to expand the full detail of the code owners. Maybe a modal? As I suggest this, three levels of disclosure also feels like a bad smell.
Proposal: when approval by code owners is:
optional show it as one summary row with the list of all the faces
required show each code owner rule as a separate row (kind of like your original design)
Bonus idea: add filter to merge request diffs to filter to the files I am code owner for.
Scenarios that have yet to be considered
one approval group (no code owners)
the widget doesn't need to show anything about groups
the widget can focus 100% on people
the widget shouldn't need a details/expanded state
@lulalala@oswaldo how do you think additional approvals should work? Currently if approvals required for the merge request is enough, anyone who is a developer can approve additionally.
If there are multiple approval groups, I think this should permitted once all approval requirements are satisfied. But where are these approvals stored in the data model? And where are they shown from a UX perspective? /cc @jkarthik@DouweM
@jramsay In my suggestion (https://gitlab.com/gitlab-org/gitlab-ee/issues/7790#note_109338345) I don't mention the approvals storage. Thinking a bit about it, I feel we should have a 1:N relation between the actual approval and the "approver rule". That is, when approving a specific rule, the storage won't just say:
X user approved Y merge request
Instead, we'll have X user approved Y merge request under Z rule
My thinking is that even if a user isn't from the Frontend (example) group anymore, we'll still say that this user approved the Frontend rule.
Before moving to the additional approvals, a question:
Given user X is from Frontend and Developer group, and a MR needs 2 approvals of each, and user X approves just Frontend, should we automatically make the approval valid for both Frontend and Developer rules?
Personally, I feel it's a bit confusing.
Now moving to the "additional approvals". Given we'll know how many approvals each rule has at a given time we can easily "flip the switch" to the Additional approval, that is, it's an approval that's not tied to any persisted rule. WDYT @DouweM?
Given user X is from Frontend and Developer group, and a MR needs 2 approvals of each, and user X approves just Frontend, should we automatically make the approval valid for both Frontend and Developer rules?
@oswaldo I agree. Yes – I left the same feedback for @jkarthik this morning and updated the description in https://gitlab.com/gitlab-org/gitlab-ee/issues/1979 to be explicit that from a user perspective they approve the merge request as a whole. A user shouldn't be able to approve the Frontend rule without approving every other rule they are also an eligibile approver for.
Once we automatically determine required approvers based on codeowners, I think we could make this behavior a lot smarter, and only require re-approval from the people who own the code was actually changed, leaving all other approvals alone.
@DouweM I think it should be easy regardless of which approver implementation is chosen. We can find paths of the changed files, find owners for those paths, and remove approvals from those owners.
@jkarthik can you try breaking up the Project Settings table into maybe blocks that have their own border so there is a clearer structure. This will take more horizontal space, but I think it will look more complete and less cramped. Thanks!
A user shouldn't be able to approve the Frontend rule without approving every other rule they are also an eligibile approver for.
@jramsay If I understand correctly, that means that we wouldn't want to store "X user approved Y merge request under Z rule" as @oswaldo suggests, but instead stay with "X user approved Y merge request".
I think we could allow "additional approvals" from anyone at any time, we just won't count them towards any rule if they're not part of that rule, and of course the MR can't be merged until all rule conditions are satisfied. All approvals would be stored in the same way; we can calculate rule satisfaction at runtime.
I think we could allow "additional approvals" from anyone at any time, we just won't count them towards any rule if they're not part of that rule, and of course the MR can't be merged until all rule conditions are satisfied. All approvals would be stored in the same way; we can calculate rule satisfaction at runtime.
@DouweM My only concern on this is the scenario where the user is not from a group anymore. We won't be able to recognise which approval rule a certain previous approval it satisfies anymore, but instead we'd count it towards an 'Additional approval'. Which also works though.
@oswaldo Hmm, that's a good point. That may make it seem like an MR that was fully approved at the time and is now merged, no longer matches the approval rules after the fact...
While an MR is open, I would expect that someone's approval would no longer count towards a rule if they are removed from the approver group, or if the codeowners rules in the target branch are edited to no longer require their approval...
Should approvals have a many-to-many relationship to rules then, so that one user's approval can count towards multiple rules, with this many-to-many relationship automatically being updated in the cases described while the MR is open, but never after it is closed/merged?
Should approvals have a many-to-many relationship to rules then, so that one user's approval can count towards multiple rules, with this many-to-many relationship automatically being updated in the cases described while the MR is open, but never after it is closed/merged?
@DouweM I like this because we'd have both consistency and trackability (and probably will see some performance benefits for doing so). WDYT @lulalala?
I am thinking about CO files turning into rules. As a brainstorm, I am wondering what you think about the following:
In the previous schema, everytime when a rule changes, we have to delete the rule and its associated approvers, and then repopulate new rule and its approvers.
New schema can have some possible advantages:
We can avoid touching the approvers table when rules changes
This allows approver without rules (like existing).
It is possible to have CO approvers persisted, while keeping CO rules virtual.
approvers persisted is good for rendering each MR#show render
rules are only needed during updates, which is less frequent, so it is okay to recompute everytime (we don't show all rules during MR#show right?)
Trackability of approval on CO rules has less value, if we go by the distinct owner. We only know one of [userA, userB] must approve, but we don't know why.
@lulalala I'm not totally sure I understand the proposed schema :)
How does approvals fit into this? If CO rules can be virtual, what rule_id would the non-virtual approvers_to_rules reference? Or are those virtual as well? And I think we will show all rules on the MR show page, because we need them to display what approvals we're still missing.
@DouweM Approvals has_and_belongs_to_many Rules, as you suggested. Sorry after some deeper thinking, my suggestion just introduced one more abstraction layer without actual benefit.
Closing this issue and removing the missed deliverable labels since we definitely made a lot of progress working through the design and planning for this Thanks everyone!