Merge changes I80b916f6,Ib1dc87a6,I42092800,I5c8486ef,I4c8338bc, ...
* changes:
Add test helper methods to set users as code owner
Use method to create arbitrary code owner config to avoid bootstrapping mode
Remove duplicate methods to create non-parsable code owner config
Allow to ignore self approvals for overrides
Document that overrides are sticky depending on the label configuration
Allow to ignore self approvals for required approval
diff --git a/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerApprovalCheck.java b/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerApprovalCheck.java
index 4055d28..2751067 100644
--- a/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerApprovalCheck.java
+++ b/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerApprovalCheck.java
@@ -20,6 +20,7 @@
import com.google.common.collect.ImmutableSet;
import com.google.common.flogger.FluentLogger;
+import com.google.gerrit.common.Nullable;
import com.google.gerrit.entities.Account;
import com.google.gerrit.entities.BranchNameKey;
import com.google.gerrit.entities.Change;
@@ -231,12 +232,99 @@
}
}
+ /**
+ * Gets the code owner status for all files/paths that were changed in the current revision of the
+ * given change assuming that there is only an approval from the given account.
+ *
+ * <p>This method doesn't take approvals from other users and global code owners into account.
+ *
+ * <p>The purpose of this method is to find the files/paths in a change that are owned by the
+ * given account.
+ *
+ * @param changeNotes the notes of the change for which the current code owner statuses should be
+ * returned
+ * @param accountId the ID of the account for which an approval should be assumed
+ */
+ public Stream<FileCodeOwnerStatus> getFileStatusesForAccount(
+ ChangeNotes changeNotes, Account.Id accountId)
+ throws ResourceConflictException, IOException, PatchListNotAvailableException {
+ requireNonNull(changeNotes, "changeNotes");
+ requireNonNull(accountId, "accountId");
+ try (TraceTimer traceTimer =
+ TraceContext.newTimer(
+ "Compute file statuses for account",
+ Metadata.builder()
+ .projectName(changeNotes.getProjectName().get())
+ .changeId(changeNotes.getChangeId().get())
+ .build())) {
+ RequiredApproval requiredApproval =
+ codeOwnersPluginConfiguration.getRequiredApproval(changeNotes.getProjectName());
+ logger.atFine().log("requiredApproval = %s", requiredApproval);
+
+ BranchNameKey branch = changeNotes.getChange().getDest();
+ ObjectId revision = getDestBranchRevision(changeNotes.getChange());
+ logger.atFine().log("dest branch %s has revision %s", branch.branch(), revision.name());
+
+ // If the branch doesn't contain any code owner config file yet, we apply special logic
+ // (project owners count as code owners) to allow bootstrapping the code owner configuration
+ // in the branch.
+ boolean isBootstrapping =
+ !codeOwnerConfigScannerFactory.create().containsAnyCodeOwnerConfigFile(branch);
+ boolean isProjectOwner = isProjectOwner(changeNotes.getProjectName(), accountId);
+ logger.atFine().log(
+ "isBootstrapping = %s (isProjectOwner = %s)", isBootstrapping, isProjectOwner);
+ if (isBootstrapping && isProjectOwner) {
+ // Return all paths as approved.
+ return changedFiles
+ .compute(changeNotes.getProjectName(), changeNotes.getCurrentPatchSet().commitId())
+ .stream()
+ .map(
+ changedFile ->
+ FileCodeOwnerStatus.create(
+ changedFile,
+ changedFile
+ .newPath()
+ .map(
+ newPath ->
+ PathCodeOwnerStatus.create(newPath, CodeOwnerStatus.APPROVED)),
+ changedFile
+ .oldPath()
+ .map(
+ oldPath ->
+ PathCodeOwnerStatus.create(
+ oldPath, CodeOwnerStatus.APPROVED))));
+ }
+
+ return changedFiles
+ .compute(changeNotes.getProjectName(), changeNotes.getCurrentPatchSet().commitId())
+ .stream()
+ .map(
+ changedFile ->
+ getFileStatus(
+ branch,
+ revision,
+ /* globalCodeOwners= */ CodeOwnerResolverResult.createEmpty(),
+ // Do not check for implicit approvals since implicit approvals of other users
+ // should be ignored. For the given account we do not need to check for
+ // implicit approvals since all owned files are already covered by the
+ // explicit approval.
+ /* enableImplicitApprovalFromUploader= */ false,
+ /* patchSetUploader= */ null,
+ /* reviewerAccountIds= */ ImmutableSet.of(),
+ // Assume an explicit approval of the given account.
+ /* approverAccountIds= */ ImmutableSet.of(accountId),
+ /* hasOverride= */ false,
+ /* isBootstrapping= */ false,
+ changedFile));
+ }
+ }
+
private FileCodeOwnerStatus getFileStatus(
BranchNameKey branch,
ObjectId revision,
CodeOwnerResolverResult globalCodeOwners,
boolean enableImplicitApprovalFromUploader,
- Account.Id patchSetUploader,
+ @Nullable Account.Id patchSetUploader,
ImmutableSet<Account.Id> reviewerAccountIds,
ImmutableSet<Account.Id> approverAccountIds,
boolean hasOverride,
@@ -295,7 +383,7 @@
ObjectId revision,
CodeOwnerResolverResult globalCodeOwners,
boolean enableImplicitApprovalFromUploader,
- Account.Id patchSetUploader,
+ @Nullable Account.Id patchSetUploader,
ImmutableSet<Account.Id> reviewerAccountIds,
ImmutableSet<Account.Id> approverAccountIds,
boolean hasOverride,
@@ -341,7 +429,7 @@
BranchNameKey branch,
CodeOwnerResolverResult globalCodeOwners,
boolean enableImplicitApprovalFromUploader,
- Account.Id patchSetUploader,
+ @Nullable Account.Id patchSetUploader,
ImmutableSet<Account.Id> reviewerAccountIds,
ImmutableSet<Account.Id> approverAccountIds,
Path absolutePath) {
@@ -387,7 +475,7 @@
CodeOwnerResolverResult globalCodeOwners,
ImmutableSet<Account.Id> approverAccountIds,
boolean enableImplicitApprovalFromUploader,
- Account.Id patchSetUploader) {
+ @Nullable Account.Id patchSetUploader) {
return (enableImplicitApprovalFromUploader
&& isImplicitlyApprovedBootstrappingMode(
projectName, absolutePath, globalCodeOwners, patchSetUploader))
@@ -400,6 +488,8 @@
Path absolutePath,
CodeOwnerResolverResult globalCodeOwners,
Account.Id patchSetUploader) {
+ requireNonNull(
+ patchSetUploader, "patchSetUploader must be set if implicit approvals are enabled");
if (isProjectOwner(projectName, patchSetUploader)) {
// The uploader of the patch set is a project owner and thus a code owner. This means there
// is an implicit code owner approval from the patch set uploader so that the path is
@@ -474,7 +564,7 @@
BranchNameKey branch,
CodeOwnerResolverResult globalCodeOwners,
boolean enableImplicitApprovalFromUploader,
- Account.Id patchSetUploader,
+ @Nullable Account.Id patchSetUploader,
ObjectId revision,
ImmutableSet<Account.Id> reviewerAccountIds,
ImmutableSet<Account.Id> approverAccountIds,
@@ -633,14 +723,17 @@
CodeOwnerResolverResult codeOwners,
ImmutableSet<Account.Id> approverAccountIds,
boolean enableImplicitApprovalFromUploader,
- Account.Id patchSetUploader) {
- if (enableImplicitApprovalFromUploader
- && (codeOwners.codeOwnersAccountIds().contains(patchSetUploader)
- || codeOwners.ownedByAllUsers())) {
- // If the uploader of the patch set owns the path, there is an implicit code owner
- // approval from the patch set uploader so that the path is automatically approved.
- logger.atFine().log("%s was implicitly approved by the patch set uploader", absolutePath);
- return true;
+ @Nullable Account.Id patchSetUploader) {
+ if (enableImplicitApprovalFromUploader) {
+ requireNonNull(
+ patchSetUploader, "patchSetUploader must be set if implicit approvals are enabled");
+ if (codeOwners.codeOwnersAccountIds().contains(patchSetUploader)
+ || codeOwners.ownedByAllUsers()) {
+ // If the uploader of the patch set owns the path, there is an implicit code owner
+ // approval from the patch set uploader so that the path is automatically approved.
+ logger.atFine().log("%s was implicitly approved by the patch set uploader", absolutePath);
+ return true;
+ }
}
if (!Collections.disjoint(approverAccountIds, codeOwners.codeOwnersAccountIds())
diff --git a/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerResolverResult.java b/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerResolverResult.java
index a03f0e1..b5b71c0 100644
--- a/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerResolverResult.java
+++ b/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerResolverResult.java
@@ -79,4 +79,13 @@
return new AutoValue_CodeOwnerResolverResult(
codeOwners, ownedByAllUsers, hasUnresolvedCodeOwners, hasUnresolvedImports);
}
+
+ /** Creates a empty {@link CodeOwnerResolverResult} instance. */
+ public static CodeOwnerResolverResult createEmpty() {
+ return new AutoValue_CodeOwnerResolverResult(
+ /* codeOwners= */ ImmutableSet.of(),
+ /* ownedByAllUsers= */ false,
+ /* hasUnresolvedCodeOwners= */ false,
+ /* hasUnresolvedImports= */ false);
+ }
}
diff --git a/javatests/com/google/gerrit/plugins/codeowners/backend/CodeOwnerApprovalCheckForAccountTest.java b/javatests/com/google/gerrit/plugins/codeowners/backend/CodeOwnerApprovalCheckForAccountTest.java
new file mode 100644
index 0000000..e392e47
--- /dev/null
+++ b/javatests/com/google/gerrit/plugins/codeowners/backend/CodeOwnerApprovalCheckForAccountTest.java
@@ -0,0 +1,265 @@
+// Copyright (C) 2020 The Android Open Source Project
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package com.google.gerrit.plugins.codeowners.backend;
+
+import static com.google.gerrit.plugins.codeowners.testing.FileCodeOwnerStatusSubject.assertThatStream;
+
+import com.google.gerrit.acceptance.TestAccount;
+import com.google.gerrit.acceptance.config.GerritConfig;
+import com.google.gerrit.acceptance.testsuite.request.RequestScopeOperations;
+import com.google.gerrit.entities.Account;
+import com.google.gerrit.entities.Change;
+import com.google.gerrit.plugins.codeowners.JgitPath;
+import com.google.gerrit.plugins.codeowners.acceptance.AbstractCodeOwnersTest;
+import com.google.gerrit.plugins.codeowners.acceptance.testsuite.CodeOwnerConfigOperations;
+import com.google.gerrit.plugins.codeowners.api.CodeOwnerStatus;
+import com.google.gerrit.plugins.codeowners.testing.FileCodeOwnerStatusSubject;
+import com.google.gerrit.server.notedb.ChangeNotes;
+import com.google.inject.Inject;
+import java.nio.file.Path;
+import java.nio.file.Paths;
+import java.util.stream.Stream;
+import org.junit.Before;
+import org.junit.Test;
+
+/** Tests for {@link CodeOwnerApprovalCheck#getFileStatusesForAccount(ChangeNotes, Account.Id)}. */
+public class CodeOwnerApprovalCheckForAccountTest extends AbstractCodeOwnersTest {
+ @Inject private ChangeNotes.Factory changeNotesFactory;
+ @Inject private RequestScopeOperations requestScopeOperations;
+
+ private CodeOwnerApprovalCheck codeOwnerApprovalCheck;
+ private CodeOwnerConfigOperations codeOwnerConfigOperations;
+
+ @Before
+ public void setUpCodeOwnersPlugin() throws Exception {
+ codeOwnerApprovalCheck = plugin.getSysInjector().getInstance(CodeOwnerApprovalCheck.class);
+ codeOwnerConfigOperations =
+ plugin.getSysInjector().getInstance(CodeOwnerConfigOperations.class);
+ }
+
+ @Test
+ public void notApprovedByUser() throws Exception {
+ // create arbitrary code owner config to avoid entering the bootstrapping code path in
+ // CodeOwnerApprovalCheck
+ createArbitraryCodeOwnerConfigFile();
+
+ Path path = Paths.get("/foo/bar.baz");
+ String changeId =
+ createChange("Change Adding A File", JgitPath.of(path).get(), "file content").getChangeId();
+
+ // Verify that the file would not be approved by the user.
+ Stream<FileCodeOwnerStatus> fileCodeOwnerStatuses =
+ codeOwnerApprovalCheck.getFileStatusesForAccount(getChangeNotes(changeId), user.id());
+ FileCodeOwnerStatusSubject fileCodeOwnerStatusSubject =
+ assertThatStream(fileCodeOwnerStatuses).onlyElement();
+ fileCodeOwnerStatusSubject.hasNewPathStatus().value().hasPathThat().isEqualTo(path);
+ fileCodeOwnerStatusSubject
+ .hasNewPathStatus()
+ .value()
+ .hasStatusThat()
+ .isEqualTo(CodeOwnerStatus.INSUFFICIENT_REVIEWERS);
+ }
+
+ @Test
+ public void approvalFromOtherCodeOwnerHasNoEffect() throws Exception {
+ TestAccount codeOwner =
+ accountCreator.create(
+ "codeOwner", "codeOwner@example.com", "CodeOwner", /* displayName= */ null);
+
+ codeOwnerConfigOperations
+ .newCodeOwnerConfig()
+ .project(project)
+ .branch("master")
+ .folderPath("/foo/")
+ .addCodeOwnerEmail(codeOwner.email())
+ .create();
+
+ Path path = Paths.get("/foo/bar.baz");
+ String changeId =
+ createChange("Change Adding A File", JgitPath.of(path).get(), "file content").getChangeId();
+
+ // Add a Code-Review+1 (= code owner approval) from the code owner.
+ requestScopeOperations.setApiUser(codeOwner.id());
+ recommend(changeId);
+
+ // Verify that the file would not be approved by the user.
+ Stream<FileCodeOwnerStatus> fileCodeOwnerStatuses =
+ codeOwnerApprovalCheck.getFileStatusesForAccount(getChangeNotes(changeId), user.id());
+ FileCodeOwnerStatusSubject fileCodeOwnerStatusSubject =
+ assertThatStream(fileCodeOwnerStatuses).onlyElement();
+ fileCodeOwnerStatusSubject.hasNewPathStatus().value().hasPathThat().isEqualTo(path);
+ fileCodeOwnerStatusSubject
+ .hasNewPathStatus()
+ .value()
+ .hasStatusThat()
+ .isEqualTo(CodeOwnerStatus.INSUFFICIENT_REVIEWERS);
+ }
+
+ @Test
+ public void approvedByUser() throws Exception {
+ codeOwnerConfigOperations
+ .newCodeOwnerConfig()
+ .project(project)
+ .branch("master")
+ .folderPath("/foo/")
+ .addCodeOwnerEmail(user.email())
+ .create();
+
+ Path path = Paths.get("/foo/bar.baz");
+ String changeId =
+ createChange("Change Adding A File", JgitPath.of(path).get(), "file content").getChangeId();
+
+ // Verify that the file would be approved by the user.
+ Stream<FileCodeOwnerStatus> fileCodeOwnerStatuses =
+ codeOwnerApprovalCheck.getFileStatusesForAccount(getChangeNotes(changeId), user.id());
+ FileCodeOwnerStatusSubject fileCodeOwnerStatusSubject =
+ assertThatStream(fileCodeOwnerStatuses).onlyElement();
+ fileCodeOwnerStatusSubject.hasNewPathStatus().value().hasPathThat().isEqualTo(path);
+ fileCodeOwnerStatusSubject
+ .hasNewPathStatus()
+ .value()
+ .hasStatusThat()
+ .isEqualTo(CodeOwnerStatus.APPROVED);
+ }
+
+ @Test
+ public void notApprovedByUser_bootstrapping() throws Exception {
+ // since no code owner config exists we are entering the bootstrapping code path in
+ // CodeOwnerApprovalCheck
+
+ Path path = Paths.get("/foo/bar.baz");
+ String changeId =
+ createChange("Change Adding A File", JgitPath.of(path).get(), "file content").getChangeId();
+
+ // Verify that the file would not be approved by the user since the user is not a project owner.
+ Stream<FileCodeOwnerStatus> fileCodeOwnerStatuses =
+ codeOwnerApprovalCheck.getFileStatusesForAccount(getChangeNotes(changeId), user.id());
+ FileCodeOwnerStatusSubject fileCodeOwnerStatusSubject =
+ assertThatStream(fileCodeOwnerStatuses).onlyElement();
+ fileCodeOwnerStatusSubject.hasNewPathStatus().value().hasPathThat().isEqualTo(path);
+ fileCodeOwnerStatusSubject
+ .hasNewPathStatus()
+ .value()
+ .hasStatusThat()
+ .isEqualTo(CodeOwnerStatus.INSUFFICIENT_REVIEWERS);
+ }
+
+ @Test
+ public void approvedByProjectOwner_bootstrapping() throws Exception {
+ // since no code owner config exists we are entering the bootstrapping code path in
+ // CodeOwnerApprovalCheck
+
+ Path path = Paths.get("/foo/bar.baz");
+ String changeId =
+ createChange("Change Adding A File", JgitPath.of(path).get(), "file content").getChangeId();
+
+ // Verify that the file would be approved by the 'admin' user since the 'admin' user is a
+ // project owner.
+ Stream<FileCodeOwnerStatus> fileCodeOwnerStatuses =
+ codeOwnerApprovalCheck.getFileStatusesForAccount(getChangeNotes(changeId), admin.id());
+ FileCodeOwnerStatusSubject fileCodeOwnerStatusSubject =
+ assertThatStream(fileCodeOwnerStatuses).onlyElement();
+ fileCodeOwnerStatusSubject.hasNewPathStatus().value().hasPathThat().isEqualTo(path);
+ fileCodeOwnerStatusSubject
+ .hasNewPathStatus()
+ .value()
+ .hasStatusThat()
+ .isEqualTo(CodeOwnerStatus.APPROVED);
+ }
+
+ @GerritConfig(name = "plugin.code-owners.fallbackCodeOwners", value = "ALL_USERS")
+ @Test
+ public void approvedByFallbackCodeOwner() throws Exception {
+ // create arbitrary code owner config to avoid entering the bootstrapping code path in
+ // CodeOwnerApprovalCheck
+ createArbitraryCodeOwnerConfigFile();
+
+ Path path = Paths.get("/foo/bar.baz");
+ String changeId =
+ createChange("Change Adding A File", JgitPath.of(path).get(), "file content").getChangeId();
+
+ // Verify that the file would be approved by the user since the user is a fallback code owner.
+ Stream<FileCodeOwnerStatus> fileCodeOwnerStatuses =
+ codeOwnerApprovalCheck.getFileStatusesForAccount(getChangeNotes(changeId), user.id());
+ FileCodeOwnerStatusSubject fileCodeOwnerStatusSubject =
+ assertThatStream(fileCodeOwnerStatuses).onlyElement();
+ fileCodeOwnerStatusSubject.hasNewPathStatus().value().hasPathThat().isEqualTo(path);
+ fileCodeOwnerStatusSubject
+ .hasNewPathStatus()
+ .value()
+ .hasStatusThat()
+ .isEqualTo(CodeOwnerStatus.APPROVED);
+ }
+
+ @GerritConfig(name = "plugin.code-owners.fallbackCodeOwners", value = "ALL_USERS")
+ @Test
+ public void notApprovedByFallbackCodeOwnerIfCodeOwerIsDefined() throws Exception {
+ TestAccount codeOwner =
+ accountCreator.create(
+ "codeOwner", "codeOwner@example.com", "CodeOwner", /* displayName= */ null);
+
+ codeOwnerConfigOperations
+ .newCodeOwnerConfig()
+ .project(project)
+ .branch("master")
+ .folderPath("/foo/")
+ .addCodeOwnerEmail(codeOwner.email())
+ .create();
+
+ Path path = Paths.get("/foo/bar.baz");
+ String changeId =
+ createChange("Change Adding A File", JgitPath.of(path).get(), "file content").getChangeId();
+
+ // Verify that the file would not be approved by the user since fallback code owners do not
+ // apply.
+ Stream<FileCodeOwnerStatus> fileCodeOwnerStatuses =
+ codeOwnerApprovalCheck.getFileStatusesForAccount(getChangeNotes(changeId), user.id());
+ FileCodeOwnerStatusSubject fileCodeOwnerStatusSubject =
+ assertThatStream(fileCodeOwnerStatuses).onlyElement();
+ fileCodeOwnerStatusSubject.hasNewPathStatus().value().hasPathThat().isEqualTo(path);
+ fileCodeOwnerStatusSubject
+ .hasNewPathStatus()
+ .value()
+ .hasStatusThat()
+ .isEqualTo(CodeOwnerStatus.INSUFFICIENT_REVIEWERS);
+ }
+
+ @GerritConfig(name = "plugin.code-owners.fallbackCodeOwners", value = "ALL_USERS")
+ @Test
+ public void approvedByFallbackCodeOwner_bootstrappingMode() throws Exception {
+ // since no code owner config exists we are entering the bootstrapping code path in
+ // CodeOwnerApprovalCheck
+
+ Path path = Paths.get("/foo/bar.baz");
+ String changeId =
+ createChange("Change Adding A File", JgitPath.of(path).get(), "file content").getChangeId();
+
+ // Verify that the file would be approved by the user since the user is a fallback code owner.
+ Stream<FileCodeOwnerStatus> fileCodeOwnerStatuses =
+ codeOwnerApprovalCheck.getFileStatusesForAccount(getChangeNotes(changeId), user.id());
+ FileCodeOwnerStatusSubject fileCodeOwnerStatusSubject =
+ assertThatStream(fileCodeOwnerStatuses).onlyElement();
+ fileCodeOwnerStatusSubject.hasNewPathStatus().value().hasPathThat().isEqualTo(path);
+ fileCodeOwnerStatusSubject
+ .hasNewPathStatus()
+ .value()
+ .hasStatusThat()
+ .isEqualTo(CodeOwnerStatus.APPROVED);
+ }
+
+ private ChangeNotes getChangeNotes(String changeId) throws Exception {
+ return changeNotesFactory.create(project, Change.id(gApi.changes().id(changeId).get()._number));
+ }
+}
diff --git a/javatests/com/google/gerrit/plugins/codeowners/backend/CodeOwnerApprovalCheckTest.java b/javatests/com/google/gerrit/plugins/codeowners/backend/CodeOwnerApprovalCheckTest.java
index 3adaaab..d5cafb9 100644
--- a/javatests/com/google/gerrit/plugins/codeowners/backend/CodeOwnerApprovalCheckTest.java
+++ b/javatests/com/google/gerrit/plugins/codeowners/backend/CodeOwnerApprovalCheckTest.java
@@ -51,8 +51,13 @@
import org.junit.Test;
/**
- * Tests for {@link CodeOwnerApprovalCheck}. Further tests with fallback code owners are implemented
- * in {@link CodeOwnerApprovalCheckWithAllUsersAsFallbackCodeOwnersTest}}.
+ * Tests for {@link CodeOwnerApprovalCheck}.
+ *
+ * <p>Further tests with fallback code owners are implemented in {@link
+ * CodeOwnerApprovalCheckWithAllUsersAsFallbackCodeOwnersTest} and the functionality of {@link
+ * CodeOwnerApprovalCheck#getFileStatusesForAccount(ChangeNotes,
+ * com.google.gerrit.entities.Account.Id)} is covered by {@link
+ * CodeOwnerApprovalCheckForAccountTest}.
*/
public class CodeOwnerApprovalCheckTest extends AbstractCodeOwnersTest {
@Inject private ChangeNotes.Factory changeNotesFactory;
diff --git a/resources/Documentation/config-guide.md b/resources/Documentation/config-guide.md
new file mode 100644
index 0000000..8a55a6c
--- /dev/null
+++ b/resources/Documentation/config-guide.md
@@ -0,0 +1,222 @@
+# Config Guide
+
+The `@PLUGIN@` plugin has many configuration parameters that can be used to
+customize its behavior. These configuration parameters are described in the
+[config](config.html) documentation. This guide gives some additional
+recommendations for the configuration, but doesn't cover all configuration
+parameters.
+
+## <a id="requiredConfiguration">Required Configuration
+
+**Before** installing/enabling the plugin, or enabling the code owners
+functionality for further projects, it is important to do some basic
+configuration. This includes choosing a [code owner backend](backends.html),
+defining the approvals that count as code owner approval and as code owner
+override, opting-out projects or branches and configuring the allowed email
+domain. All this configuration is covered in detail by the [setup
+guide](setup-guide.html).
+
+## <a id="workflow">Workflow Configuration
+
+Some of the configuration parameters have an effect on the user workflow.
+
+### <a id="stickyApprovals">Make code owner approvals / overrides sticky
+
+Code owner approvals and code owner overrides can be made sticky by enabling
+[copy rules](../../../Documentation/config-labels.html#label_copyAnyScore) in
+the definitions of the labels that are configured as [required
+approval](config.html#pluginCodeOwnersRequiredApproval) and [override
+approval](config.html#pluginCodeOwnersOverrideApproval).
+
+### <a id="implicitApprovals">Implicit code owner approvals
+
+It's possible to [enable implicit approvals](config.html#pluginCodeOwnersEnableImplicitApprovals)
+of code owners on their own changes. If enabled and the uploader of a patch set
+is a code owner, an approval of the uploader is assumed for all owned files.
+This means if a code owner uploads a change / patch set that only touches files
+that they own, no approval from other code owners is required for submitting the
+change.
+
+If implicit approvals are enabled, paths can be exempted from requiring code
+owner approvals by assigning the code ownership to [all
+users](backend-find-owners.html#allUsers), as then any modification to the path
+is always implicitly approved by the uploader.
+
+**NOTE:** If implicit approvals are disabled, users can still self-approve their
+own changes by voting on the required label.
+
+**IMPORTANT**: Enabling implicit approvals is considered unsafe, see [security
+pitfalls](#securityImplicitApprovals) below.
+
+### <a id="mergeCommits">Required code owner approvals on merge commits
+
+For merge commits the list of modified files depends on the base against which
+the merge commit is compared:
+
+1. comparison against the destination branch (aka first parent commit):
+ All files which differ between the merge commit and the destination branch.
+ This includes all files which have been modified in the source branch since
+ the last merge into the destination branch has been done.
+
+2. comparison against the Auto-Merge (Auto-Merge = result of automatically
+ merging the source branch into the destination branch):
+ Only shows files for which a conflict resolution has been done.
+
+Which files a users sees on the change screen depends on their base selection.
+
+For the `@PLUGIN@` plugin it can be configured [which files of a merge commit
+require code owner approvals](config.html#pluginCodeOwnersMergeCommitStrategy),
+all files that differ with the destination branch (case 1) or only files that
+differ with the Auto-Merge (case 2). If case 1 is configured, all file diffs
+that have been approved in one branch must be re-approved when they are merged
+into another branch. If case 2 is configured, only conflict resolutions have to
+be approved when a merge is done.
+
+**IMPORTANT**: Requiring code owner approvals only for files that differ with
+the Auto-Merge (case 2) is considered unsafe, see [security
+pitfalls](#securityMergeCommits) below.
+
+## <a id="codeOwners">Recommendations for defining code owners
+
+Code owners can be defined on different levels, which differ by scope. This
+section gives an overview of the different levels and explains when they should
+be used.
+
+1. Folder and file code owners:
+ These are the code owners that are defined in the [code owner config
+ files](user-guide.html#codeOwnerConfigFiles) that are stored in the source
+ tree of the repository. They can either apply to a whole
+ [folder](backend-find-owners.html#userEmails) (folder code owners) or to
+ [matched files](backend-find-owners.html#perFile) (file code owners).\
+ This is the normal way to define code owners. This code owner definition is
+ discoverable since it is stored in human-readable code owner config file in
+ the source tree of the repository.\
+ Folder and file code owners can differ from branch to branch since they are
+ defined in the source tree.\
+ Folder and file code owners are usually users that are expert for a code area
+ and that should review and approve all changes to this code.
+2. Root code owners:
+ Root code owners are folder code owners (see 1.) that are defined in the code
+ owner config file that is stored in the root directory of a branch.\
+ Usually root code owners are the most experienced developers that can approve
+ changes to all the code base if needed, but that should only review and
+ approve changes if no other, more specific, code owner is available.\
+ Root code owners can differ from branch to branch.
+3. Default code owners:
+ [Default code owners](backend-find-owners.html#defaultCodeOwnerConfiguration)
+ are stored in the code owner config file in the `refs/meta/config` branch
+ that apply for all branches (unless inheritance is ignored).\
+ The same as root code owners these are experienced developers that can
+ approve changes to all the code base if needed.\
+ However in contrast to root code owners that apply to all branches (including
+ newly created branches), and hence can be used if code owners should be kept
+ consistent across all branches.\
+ A small disadvantage is that this code owner definition is not very well
+ discoverable since it is stored in the `refs/meta/config` branch, but default
+ code owners are suggested to users the same way as other code owners.
+4. Global code owners:
+ [Global code owners](config.html#pluginCodeOwnersGlobalCodeOwner) are defined
+ in the plugin configuration and apply to all projects or all child projects.\
+ They are intended to configure bots as code owners that need to operate on
+ all or multiple projects.\
+ Global code owners still apply if parent code owners are ignored.
+5. Fallback code owners:
+ [Fallback code owners](config.html#pluginCodeOwnersFallbackCodeOwners) is a
+ policy configuration that controls who should own paths that have no code
+ owners defined.\
+ Fallback code owners are not included in the code owner suggestion.\
+ Configuring all users as fallback code owners may allow bypassing the code
+ owners check (see [security pitfalls](#securityFallbackCodeOwners) below).
+
+In addition users can be allowed to [override the code owner submit
+check](user-guide.html#codeOwnerOverride). This permission is normally granted
+to users that that need to react to emergencies and need to submit changes
+quickly (e.g sheriffs) or users that need to make large-scale changes across
+many repositories.
+
+## <a id="externalValidationOfCodeOwnerConfigs">External validation of code owner config files
+
+By default, when code owner config files are modified they are
+[validated](validation.html) on push. If any issues in the modified code owner
+config files are found, the push is rejected. This is important since
+non-parsable code owner config files make submissions fail which likely blocks
+the development teams, and hence needs to be prevented.
+
+However rejecting pushes in case of invalid code owner config files is not an
+ideal workflow for everyone. Instead it may be wanted that the push always
+succeeds and that issues with modified code owner config files are then detected
+and reported by a CI bot. The CI bot would then post its findings as checks on
+the open change which prevent the change submission. To enable this the
+validation of code owner config files on push can be
+[disabled](config.html#pluginCodeOwnersEnableValidationOnCommitReceived), but
+then the host admins should setup a bot to do the validation of modified code
+owner config files externally. For this the bot could use the [Check Code Owner
+Config Files In Revision](rest-api.html#check-code-owner-config-files-in-revision)
+REST endpoint.
+
+## <a id="differentCodeOwnerConfigurations">Use different code owner configurations in a fork
+
+If a respository is forked and code owners are used in the original repository,
+the code owner configuration of the original repository shouldn't apply for the
+fork (the fork should have different code owners, and if the fork is stored on
+another Gerrit host it's also likely that the original code owners cannot be
+resolved on that host). In this case it is possible to [configure a file
+extension](config.html#pluginCodeOwnersFileExtension) for code owner config
+files in the fork so that its code owner config files do not clash with the
+original code owner config files.
+
+## <a id="securityPitfalls">Security pitfalls
+
+While requiring code owner approvals is primarily considered as a code quality
+feature and not a security feature, many admins / projects owners are concerned
+about possibilities to bypass code owner approvals. These admins / projects
+owners should be aware that some configuration settings may make it possible to
+bypass code owner approvals, and hence using them is not recommended.
+
+### <a id="securityImplicitApprovals">Implicit approvals
+
+If [implicit approvals](#implicitApprovals) are enabled, it is important that
+code owners are aware of their implicit approval when they upload new patch sets
+for other users. E.g. if a contributor pushes a change to a wrong branch and a
+code owner helps them to get it rebased onto the correct branch, the rebased
+change has implicit approvals from the code owner, since the code owner is the
+uploader. To avoid situations like this it is recommended to not enable implicit
+approvals.
+
+### <a id="securityMergeCommits">Required code owner approvals on merge commits
+
+If any branch doesn't require code owner approvals or if the code owners in any
+branch are not trusted, it is not safe to [configure for merge commits that they
+only require code owner approvals for files that differ with the
+Auto-Merge](#mergeCommits). E.g. if there is a branch that doesn't require code
+owner approvals, with this setting the code owners check can be bypassed by:
+
+1. setting the branch that doesn't require code owner approvals to the same
+ commit as the main branch that does require code owner approvals
+2. making a change in the branch that doesn't require code owner approvals
+3. merging this change back into the main branch that does require code owner
+ approvals
+4. since it's a clean merge, all files are merged automatically and no code
+ owner approval is required
+
+### <a id="securityFallbackCodeOwners">Setting all users as fallback code owners
+
+As soon as the code owners functionality is enabled for a project / branch, all
+files in it require code owner approvals. This means if any path doesn't have
+any code owners defined, submitting changes to the path is only possible with
+
+1. a code owner override
+2. an approval from a fallback code owners (only if enabled)
+
+[Configuring all users as fallback code
+owners](config.html#pluginCodeOwnersFallbackCodeOwners) is problematic, as it
+can happen easily that code owner config files are misconfigured so that some
+paths are accidentally not covered by code owners. In this case, the affected
+paths would suddenly be open to all users, which may not be wanted. This is why
+configuring all users as fallback code owners is not recommended.
+
+---
+
+Back to [@PLUGIN@ documentation index](index.html)
+
+Part of [Gerrit Code Review](../../../Documentation/index.html)
diff --git a/resources/Documentation/config.md b/resources/Documentation/config.md
index d80939f..32d3ae9 100644
--- a/resources/Documentation/config.md
+++ b/resources/Documentation/config.md
@@ -3,6 +3,9 @@
The global configuration of the @PLUGIN@ plugin is stored in the `gerrit.config`
file in the `plugin.@PLUGIN@` subsection.
+This page describes all available configuration parameters. For configuration
+recommendations please consult the [config guide](#config-guide.html).
+
## <a id="projectLevelConfigFile">
In addition some configuration can be done on the project level in
`@PLUGIN@.config` files that are stored in the `refs/meta/config` branches of
@@ -55,11 +58,11 @@
<a id="pluginCodeOwnersFileExtension">plugin.@PLUGIN@.fileExtension</a>
: The file extension that should be used for code owner config files.\
- Allows to use different owner configurations for upstream and internal
- in the same repository. E.g. if upstream uses `OWNERS` code owner config
- files (no file extension configured) one could set `internal` as file
- extension internally so that internally `OWNERS.internal` files are used
- and the existing `OWNERS` files are ignored.\
+ Allows to use a different code owner configuration in a fork. E.g. if
+ the original repository uses `OWNERS` code owner config files (no file
+ extension configured) one could set `fork` as file extension in the fork
+ so that the fork uses `OWNERS.fork` files and the existing `OWNERS`
+ files are ignored.\
Can be overridden per project by setting
[codeOwners.fileExtension](#codeOwnersFileExtension) in
`@PLUGIN@.config`.\
@@ -330,11 +333,11 @@
<a id="codeOwnersFileExtension">codeOwners.fileExtension</a>
: The file extension that should be used for the code owner config files
in this project.\
- Allows to use different owner configurations for upstream and internal
- in the same repository. E.g. if upstream uses `OWNERS` code owner config
- files (no file extension configured) one could set `internal` as file
- extension internally so that internally `OWNERS.internal` files are used
- and the existing `OWNERS` files are ignored.\
+ Allows to use a different code owner configuration in a fork. E.g. if
+ the original repository uses `OWNERS` code owner config files (no file
+ extension configured) one could set `fork` as file extension in the fork
+ so that the fork uses `OWNERS.fork` files and the existing `OWNERS`
+ files are ignored.\
Overrides the global setting
[plugin.@PLUGIN@.fileExtension](#pluginCodeOwnersFileExtension) in
`gerrit.config`.\
diff --git a/resources/Documentation/setup-guide.md b/resources/Documentation/setup-guide.md
index c662be7..7449164 100644
--- a/resources/Documentation/setup-guide.md
+++ b/resources/Documentation/setup-guide.md
@@ -27,6 +27,9 @@
* [How to update the code-owners.config file for a project](#updateCodeOwnersConfig)
* [How to check if the code owners functionality is enabled for a project or branch](#checkIfEnabled)
+Recommendations about further configuration parameters can be found in the
+[config guide](config-guide.html).
+
### <a id="configureCodeOwnersBackend">1. Configure the code owners backend that should be used
The `code-owners` plugin supports multiple [code owner backends](backends.html)
diff --git a/resources/Documentation/user-guide.md b/resources/Documentation/user-guide.md
index 1471348..340d3a5 100644
--- a/resources/Documentation/user-guide.md
+++ b/resources/Documentation/user-guide.md
@@ -7,6 +7,11 @@
This user guide explains the functionality of the `@PLUGIN@` plugin. For a
walkthrough of the UI please refer to the [intro](how-to-use.html) page.
+**TIP:** You may also want to check out the [presentation about code
+owners](https://docs.google.com/presentation/d/1DupBnGr3apIx-jzxi9cHzSgkI-2c1ouGu1teQ4khSfc)
+from the [Gerrit Contributor Summit
+2020](https://docs.google.com/document/d/1WauJfNxracjBK3PxuVnwNIppESGMBtZwxMYjxxeDN6M).
+
**NOTE:** How to setup the code owners functionality is explained in the
[setup guide](setup-guide.html).
diff --git a/resources/Documentation/validation.md b/resources/Documentation/validation.md
index d6160d6..b3b6405 100644
--- a/resources/Documentation/validation.md
+++ b/resources/Documentation/validation.md
@@ -20,6 +20,11 @@
etc.) are severe errors and block the submission of all changes for which the
affected configuration files are relevant.
+**NOTE:** It's possible to disable the validation of code owner config files on
+push and setup an [external
+validation](config-guide.html#externalValidationOfCodeOwnerConfigs) by a CI bot
+instead. In this case findings would be posted on the change.
+
All validations are best effort to prevent invalid configurations from
entering the repository, but not all possible issues can be prevented. Doing the
validation is useful since it prevents most issues and also gives quick feedback
