commit 1d01a4f76ba3a60d9fb3f83b734b05e6bd951731
author Edwin Kempin <ekempin@google.com> Fri Feb 19 13:36:10 2021 +0100
committer Edwin Kempin <ekempin@google.com> Fri Feb 19 15:01:05 2021 +0100
tree 542a917bde49b3398169482a0e85a33acfd45179
parent 9438fbf0f4434774e775014104cce096d1452adb

Replace bootstrapping mode with PROJECT_OWNERS option for fallback code owners

We currently have a bootstrapping mode that applies if a branch doesn't
contain any OWNERS file yet. In this case the project owners are
automatically code owners so that they can code-owner approve any
change. As soon as any OWNERS file is added, the bootstrapping logic no
longer applies.

In order to decide if we are in the bootstrapping mode we must
potentially scan the whole branch for OWNERS files (the scan is aborted
as soon as an OWNERS file is found, but if a branch doesn't contain any
OWNERS files we need to check all folders). For large repositories that
do not contain any OWNERS files this scan can take a long time (> 10s).
This affects all operations that need to check if we are in
bootstrapping mode (e.g. get file code owner statuses, check the code
owner submit requirement).

Instead of trying to make this scan faster or cache its result, it's
better to omit it completely by dropping the bootstrapping mode and
replace it with a new PROJECT_OWNERS option for fallback code owners.

Fallback code owners is a policy that controls who should own files that
don't have any code owners. So far the fallback code owners setting
supports the options NONE (files that don't have code owners are owned
by no one) and ALL_USERS (files that don't have code owners are owned by
all users). The new PROJECT_OWNERS options means that files that don't
have code owners are owned by the project owners. That's similar to how
the bootstrapping mode work, except that project owners are still code
owners of files that don't have code owners if the branch contains other
non-relevant OWNERS files. Since fallback code owners is a
configuration setting we no longer need to scan the branch for OWNERS
files to know if project owners should be applied as fallback code
owners, and hence the performance issue that is described above
vanishes.

Another advantage of having a fallback code owners option for project
owners instead of the bootstrapping mode is that it's easier to
understand ("files that don't have any code owners are owned by the
project owners" is easier than "if the branch does not contain any
OWNERS file then the project owners are considered as code owners").

Implementation-wise the main logic change is in CodeOwnerApprovalCheck.
The code to detect the bootstrapping mode was removed and the code to
handle the bootstrapping mode was repurposed to compute the code owner
statuses if fallback code owners is set to the new PROJECT_OWNERS option.

All tests for the bootstrapping mode that were located in
CodeOwnerApprovalCheckTest have been moved to a new
CodeOwnerApprovalCheckWithProjectOwnersAsFallbackCodeOwnersTest test
class (which follows the example of the
CodeOwnerApprovalCheckWithAllUsersAsFallbackCodeOwnersTest). Since there
are too many tests CodeOwnerApprovalCheckTest it makes sense to split
them out further, even if it means duplicating some of the test code.
The tests for the bootstrapping mode could easily be repurposed to test
the new PROJECT_OWNERS option for fallback code owners, as all that
needed to be changed was setting fallbackCodeOwners = PROJECT_OWNERS
which we do in the setup of the test class.

Since there is no bootstrapping mode anymore, other tests no longer need
to create an arbitrary code owner config file to avoid the bootstrapping
mode, which allowed us to delete the creation of an arbitray code owner
config from many tests.

It turned out that some test implicitly relied on the bootstrapping mode
(e.g. some validations tests relied on being able to submit changes
after an approval of a project owner). For these tests we now create a
root or default code owner config that defines the admin user as code
owner (mostly we did this via a root OWNERS file, but for tests that
already had a root OWNERS file we just added a default OWNERS file in
refs/meta/config). Except in some cases it was easier to just set
fallbackCodeOwners = PROJECT_OWNERS via the @GerritConfig annotation on
the test (e.g. the validation tests that use a failing code owner
backend to provoke an internal server error as the failing code owner
backend prvents the creation of a code owner config, or the test for the
BatchModule which doesn't have the CodeOwnerConfigOperations class
bound). Another special case are tests for validating the
code-owners.config file in refs/meta/config, here it was easiest to just
disable requiring code owner approvals for the refs/meta/config branch.

Since the bootstrapping mode is removed it also make no sense to still
tell the frontend about it (via the noCodeOwnersDefined field in
CodeOwnerProjectConfigInfo). Since this field was nullable callers
already needed to handle the case where it was not set, hence dropping
this field should not break any caller. FWIW the frontend used this
field to display a special message in case the branch of a change was in
bootstrapping mode. This logic is no longer needed, but can be removed
later.

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