Link reviews to a set of commits, instead of to a merge request
Problem to solve
People want to review code in all sorts of contexts. Prior to merging a merge request is an obvious case - and the only one we support - but less obvious are reviews of code that comes after it has been merged into master
. People want to do this - and track the details of what has and hasn't been reviewed like this - for all sorts of reasons, including code audits.
Further, reviews of code have value to people outside of a merge request context - if I'm reading app/models/project.rb
, we show details about who has authored each line of code, but equally interesting is the review history of the file, who last reviewed a particular line, etc.
Intended users
Basically everyone
Further details
Perhaps a DBA wants to go through a set of .sql
files (or GitLab migrations) and make suggestions for changes that can then be acted on by other people?
Perhaps a security engineer wants to go through the same set of files, but they're looking for SQL interpolations? Perhaps the codebase is huge and it's being handled by multiple people over a period of a few weeks. How do they avoid duplicating the effort? How do they keep track of which lines in which files have been reviewed, and which have not? How do they take account of new code being merged into master
while the review is ongoing?
GitLab does have the idea of "a review", which is an object linking a set of notes to a particular merge request. A review is what these people are doing, but there is no "merge request" in sight.
To the review, however, the most interesting thing about the merge request is the range of commits it is over - from which we generate the diff that is presented to the user. So, a merge request is a thin wrapper over a subset of commits in a branch. Note that those commits can move between branches as merges happen.
Also rebases and cherry-picks, but that's harder to detect, so let's ignore for now.
Proposal
We could make reviews be explicitly about a range of commits, rather than (just) being linked to a merge request. Reviews then follow commits around as they are merged from a feature branch into master, instead of them being confined to the context of a single merge request.
We can use this data to say interesting things like "the master
branch has X reviews in this time period", or "app/models/project.rb
on master
has X reviews in this time period". We can also say interesting things like "this line of app/models/project.rb
on master
was last reviewed by Nick Thomas", or "this line has never been reviewed". These are all useful things to be able to say about the master
branch.
Since reviews are no longer linked to MRs, we can also allow people to create new ones for code on master
. They'd do so by selecting a range of commits to display a diff for, or through being able to say "this file. All of it". Comments can then be added to this review in the same way we add comments to a review in a merge request. Previous reviews done in the same area are visible to you, and you could choose to either ignore code already reviewed by particular people, or to ignore code reviewed less than (say) 1 month ago.
There's also a fun interaction with approvals, if we stop them being about MRs and instead link them to reviews, but this issue is large enough already!
What does success look like, and how can we measure that?
As a security engineer, I can review an existing codebase for SQL vulnerabilities in a transparent manner, leaving a persistent record of my findings.
As a debugger, I can quickly and easily pull up information about past reviews of a line of code, without having to trek through a list of merge requests that introduced (and then changed) that line.
What is the type of buyer?
Reviews are GitLab Premium
Links / references
cc @oswaldo @jramsay @felipe_artur @DouweM @lulalala @kerrizor