tree 71e3abf746f35dab6b4caa3a9cc2fc3d17bd7100
parent cfa11501b14e52b8fe9c88bebd47354a75094a86
author Edwin Kempin <ekempin@google.com> 1666957246 +0200
committer Edwin Kempin <ekempin@google.com> 1667981293 +0100

Allow making code owner approvals sticky per file

Add a new configuration option that allows to make code owner approvals
sticky on the files they approve, even if these files are changed in
follow-up patch sets.

This feature is requested by our stakeholders to increase developer
productivity. Without sticky code owner approvals, all code owners need
to reapprove the change each time a new patch set is uploaded, which can
be very time consuming if many code owners need to approve a change.
It's also frustrating for code owners, since there re-approval is
required even if the new patch set doesn't touch any files they own. One
way to address this could be to make code owner approvals sticky on
change level (by configuring a copy condition on the label that is
required for code owner approvals), but this has the disadvantage that
the approval is sticky on change level and hence approves all files that
are owned by the code owner, including newly modified files in the new
patch set if they that are owned by the approver. This is a problem
since the approver didn't see these files (because they were not present
in the patch set that they approved). Hence there is the wish to make
code owner approvals sticky on file level. This way code owners only
need to reapprove a change when a new patch set newly modifies files
that are also owned by them. This can be a huge productivity gain (e.g.
if 1 file is newly modified only 1 code owner needs to reapprove instead
of all code owners). Ideally code owner approvals would only be sticky
on files as long as the files are not changed in follow-up patch sets,
but in a worst case this would require diffing all files in the change
against all the previous patch sets which would likely kill performance.
Hence we make code owner approvals sticky on files even if these files
are changed in follow-up patch sets. This is OK, since requiring code
owner approvals is optional and in addition to requiring (Code-Review)
approvals from 2 trusted users (usually an implicit approval from a
trusted uploader + an explicit approval from a trusted reviewer). This
means ensuring that two trusted users agree to the change doesn't look
at code owner approvals at all, hence making them sticky has no effect
on this.

With the new setting that makes code owner approvals sticky on files
previous code owner approvals on files only get invalidated if the
approval is revoked, changed (e.g. from 'Code-Review+1' to
'Code-Review-1') or re-applied, but not by the upload of a new patch set
regardless of whether the new patch sets changes these files.

Code owner approvals are sticky per file but not on change level. This
means they do not show up as (copied) approvals on the change screen
like regular sticky approvals, but only count for the computation of the
code owner file statuses. In contrast to this setting, making code owner
approvals sticky on change level (by setting a copy condition on the
label that is used for code owner approvals) would make the sticky
approvals count for all files in the current patch set that are owned by
the approvers, regardless of whether these files existed in the patch
sets that were originally approved.

Since code owner approvals that are sticky on file level are not shown
in the UI, users need to inspect the per-file code owner statuses (code
owner status icons in the file list on the change screen) to
know which files are code owner approved. For each file that is code
owner approved the API returns a reason that tells why this file is code
owner approved. If a file was code owner approved by a sticky approval
the reason says on which patch set the approval was applied (e.g. the
reason is "approved on patch set 1 by Foo who is a code owner"). In
contrast to this, if the file was code owner approved by an approval on
the current patch set the reason just says that the file is code owner
approved (e.g. the reason is "approved by Foo who is a code owner"). The
reasons are not shown on the change screen yet, but it's an open feature
request to implement this. This means by looking only at the file code
owner statuses users can't tell why a file is code owner approved. This
may confuse users that are not aware that code owner approvals can be
sticky on files: if a code owner approval is sticky on a file there is
no code owner approval on change level, but files with sticky approvals
are showns as code owner approved, which may look like a contradiction
if you are not aware of sticky code owner approvals. That's not ideal,
but it's accepted by the stakeholders that are requesting this feature,
because the productivity gain of making code owner approvals sticky
outweighs any potential user confusion.

Example:
If patch set 1 contains the files A, B and C and a user that owns the
files A and B approves the patch set by voting with `Code-Review+1`, A
and B are code owner approved for this patch set and all future patch
sets unless the approval is revoked, changed or re-applied. This means
if a second patch set is uploaded that touches files A and B, A and B
are still code owner approved. If a third patch set is uploaded that
adds file D that is also owned by the same code owner, file D is not
code owner approved since it is not present in patch set 1 on which the
user applied the code owner approval. If the user changes their vote on
patch set 3 from `Code-Review+1` to `Code-Review-1`, the new vote
invalidates the approval on patch set 1, so that in this example the
files A and B would no longer be code owner approved by this user. If
the user re-applies the `Code-Review+1` approval now all owned files
that are present in the current patch set, files A, B and D, are code
owner approved.

Whether code owner approvals should be sticky per file is configurable
by project. By default code owner approvals are not sticky.

The new 'enableStickyApprovals' configuration setting is a boolean
(false: code owner approvals are not sticky on files; true: code owner
approvals are sticky on files even if they are changed in follow-up
patch sets). In case we need to support more options in the future, we
can easily convert this setting to an enum, with values FALSE, TRUE and
further options (similar to the EnableImplicitApprovals enum). This
means it's possible to add more options in the future without needing to
migrate existing code owner configurations. One possibility for a new
option could be ONLY_IF_FILES_DID_NOT_CHANGE to make code owner
approvals sticky on files only if they were not changed in follow-up
patch sets.

To be able to see whether making code owner approvals sticky on files
has any impact on the performance of computing the code owner statuses
for the files in a change, a new field is added to the
compute_file_statuses metric that is set if sticky code owner approvals
are configured.

Signed-off-by: Edwin Kempin <ekempin@google.com>
Change-Id: I362c021f95190f3d14c87bff7d2694b867d3ab5a