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
diff --git a/java/com/google/gerrit/plugins/codeowners/backend/BUILD b/java/com/google/gerrit/plugins/codeowners/backend/BUILD
index 83671a0..04aaf14 100644
--- a/java/com/google/gerrit/plugins/codeowners/backend/BUILD
+++ b/java/com/google/gerrit/plugins/codeowners/backend/BUILD
@@ -6,6 +6,7 @@
srcs = glob(["**/*.java"]),
visibility = ["//visibility:public"],
deps = PLUGIN_DEPS_NEVERLINK + [
+ "//lib/errorprone:annotations",
"//plugins/code-owners/java/com/google/gerrit/plugins/codeowners/common",
"//plugins/code-owners/java/com/google/gerrit/plugins/codeowners/metrics",
"//plugins/code-owners/java/com/google/gerrit/plugins/codeowners/util",
diff --git a/java/com/google/gerrit/plugins/codeowners/backend/BackendModule.java b/java/com/google/gerrit/plugins/codeowners/backend/BackendModule.java
index 89f493f..cba9409 100644
--- a/java/com/google/gerrit/plugins/codeowners/backend/BackendModule.java
+++ b/java/com/google/gerrit/plugins/codeowners/backend/BackendModule.java
@@ -36,6 +36,7 @@
public class BackendModule extends FactoryModule {
@Override
protected void configure() {
+ factory(ChangedFilesByPatchSetCache.Factory.class);
factory(CodeOwnerApprovalCheckInput.Loader.Factory.class);
factory(CodeOwnersUpdate.Factory.class);
factory(CodeOwnerConfigScanner.Factory.class);
diff --git a/java/com/google/gerrit/plugins/codeowners/backend/ChangedFilesByPatchSetCache.java b/java/com/google/gerrit/plugins/codeowners/backend/ChangedFilesByPatchSetCache.java
new file mode 100644
index 0000000..3d5fc9c
--- /dev/null
+++ b/java/com/google/gerrit/plugins/codeowners/backend/ChangedFilesByPatchSetCache.java
@@ -0,0 +1,100 @@
+// Copyright (C) 2022 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.common.base.Preconditions.checkState;
+
+import com.google.common.collect.ImmutableList;
+import com.google.gerrit.entities.PatchSet;
+import com.google.gerrit.exceptions.StorageException;
+import com.google.gerrit.plugins.codeowners.backend.config.CodeOwnersPluginProjectConfigSnapshot;
+import com.google.gerrit.plugins.codeowners.common.ChangedFile;
+import com.google.gerrit.server.notedb.ChangeNotes;
+import com.google.gerrit.server.patch.DiffNotAvailableException;
+import com.google.inject.Inject;
+import com.google.inject.assistedinject.Assisted;
+import java.io.IOException;
+import java.util.HashMap;
+import java.util.Map;
+
+/**
+ * Computes and caches the {@link ChangedFile}s for the patch sets of a change.
+ *
+ * <p>The changed files for a patch set are computed lazily. This way we do not compute changed
+ * files unnecessarily that are never requested.
+ *
+ * <p>This class is not thread-safe.
+ */
+public class ChangedFilesByPatchSetCache {
+ interface Factory {
+ ChangedFilesByPatchSetCache create(
+ CodeOwnersPluginProjectConfigSnapshot codeOwnersConfig, ChangeNotes changeNotes);
+ }
+
+ private final ChangedFiles changedFiles;
+ private final CodeOwnersPluginProjectConfigSnapshot codeOwnersConfig;
+ private final ChangeNotes changeNotes;
+
+ private Map<PatchSet.Id, ImmutableList<ChangedFile>> cache = new HashMap<>();
+
+ @Inject
+ public ChangedFilesByPatchSetCache(
+ ChangedFiles changedFiles,
+ @Assisted CodeOwnersPluginProjectConfigSnapshot codeOwnersConfig,
+ @Assisted ChangeNotes changeNotes) {
+ this.changedFiles = changedFiles;
+ this.codeOwnersConfig = codeOwnersConfig;
+ this.changeNotes = changeNotes;
+ }
+
+ public ImmutableList<ChangedFile> get(PatchSet.Id patchSetId) {
+ return cache.computeIfAbsent(patchSetId, this::compute);
+ }
+
+ private ImmutableList<ChangedFile> compute(PatchSet.Id patchSetId) {
+ checkState(
+ patchSetId.changeId().equals(changeNotes.getChange().getId()),
+ "patch set %s belongs to other change than change %s",
+ patchSetId,
+ changeNotes.getChange().getId().get());
+ PatchSet patchSet = getPatchSet(patchSetId);
+ try {
+ return changedFiles.getFromDiffCache(
+ changeNotes.getProjectName(),
+ patchSet.commitId(),
+ codeOwnersConfig.getMergeCommitStrategy());
+ } catch (IOException | DiffNotAvailableException e) {
+ throw new StorageException(
+ String.format(
+ "failed to retrieve changed files for patch set %d of change %d"
+ + " in project %s (commit=%s)",
+ patchSetId.get(),
+ patchSetId.changeId().get(),
+ changeNotes.getProjectName(),
+ patchSet.commitId()),
+ e);
+ }
+ }
+
+ private PatchSet getPatchSet(PatchSet.Id patchSetId) {
+ PatchSet patchSet = changeNotes.getPatchSets().get(patchSetId);
+ if (patchSet == null) {
+ throw new StorageException(
+ String.format(
+ "patch set %s not found in change %d", patchSetId, changeNotes.getChangeId().get()));
+ }
+ return patchSet;
+ }
+}
diff --git a/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerApprovalCheck.java b/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerApprovalCheck.java
index 792f641..71c0044 100644
--- a/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerApprovalCheck.java
+++ b/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerApprovalCheck.java
@@ -35,6 +35,7 @@
import com.google.gerrit.entities.RefNames;
import com.google.gerrit.extensions.restapi.BadRequestException;
import com.google.gerrit.metrics.Timer0;
+import com.google.gerrit.metrics.Timer1;
import com.google.gerrit.plugins.codeowners.backend.config.CodeOwnersPluginConfiguration;
import com.google.gerrit.plugins.codeowners.backend.config.CodeOwnersPluginProjectConfigSnapshot;
import com.google.gerrit.plugins.codeowners.backend.config.RequiredApproval;
@@ -89,6 +90,7 @@
private final Provider<CodeOwnerResolver> codeOwnerResolverProvider;
private final CodeOwnerApprovalCheckInput.Loader.Factory inputLoaderFactory;
private final CodeOwnerMetrics codeOwnerMetrics;
+ private final ChangedFilesByPatchSetCache.Factory changedFilesByPatchSetCacheFactory;
@Inject
CodeOwnerApprovalCheck(
@@ -99,7 +101,8 @@
Provider<CodeOwnerConfigHierarchy> codeOwnerConfigHierarchyProvider,
Provider<CodeOwnerResolver> codeOwnerResolverProvider,
CodeOwnerApprovalCheckInput.Loader.Factory codeOwnerApprovalCheckInputLoaderFactory,
- CodeOwnerMetrics codeOwnerMetrics) {
+ CodeOwnerMetrics codeOwnerMetrics,
+ ChangedFilesByPatchSetCache.Factory changedFilesByPatchSetCacheFactory) {
this.repoManager = repoManager;
this.codeOwnersPluginConfiguration = codeOwnersPluginConfiguration;
this.changedFiles = changedFiles;
@@ -108,6 +111,7 @@
this.codeOwnerResolverProvider = codeOwnerResolverProvider;
this.inputLoaderFactory = codeOwnerApprovalCheckInputLoaderFactory;
this.codeOwnerMetrics = codeOwnerMetrics;
+ this.changedFilesByPatchSetCacheFactory = changedFilesByPatchSetCacheFactory;
}
/**
@@ -212,9 +216,12 @@
changeNotes.getChangeId().get(), changeNotes.getProjectName());
CodeOwnerConfigHierarchy codeOwnerConfigHierarchy = codeOwnerConfigHierarchyProvider.get();
CodeOwnerResolver codeOwnerResolver = codeOwnerResolverProvider.get().enforceVisibility(false);
+ CodeOwnersPluginProjectConfigSnapshot codeOwnersConfig =
+ codeOwnersPluginConfiguration.getProjectConfig(changeNotes.getProjectName());
try {
boolean isSubmittable =
- !getFileStatuses(codeOwnerConfigHierarchy, codeOwnerResolver, changeNotes)
+ !getFileStatuses(
+ codeOwnersConfig, codeOwnerConfigHierarchy, codeOwnerResolver, changeNotes)
.anyMatch(
fileStatus ->
(fileStatus.newPathStatus().isPresent()
@@ -247,17 +254,22 @@
*
* @param start number of file statuses to skip
* @param limit the max number of file statuses that should be returned (0 = unlimited)
- * @see #getFileStatuses(CodeOwnerConfigHierarchy, CodeOwnerResolver, ChangeNotes)
+ * @see #getFileStatuses(CodeOwnersPluginProjectConfigSnapshot, CodeOwnerConfigHierarchy,
+ * CodeOwnerResolver, ChangeNotes)
*/
public ImmutableSet<FileCodeOwnerStatus> getFileStatusesAsSet(
ChangeNotes changeNotes, int start, int limit) throws IOException, DiffNotAvailableException {
requireNonNull(changeNotes, "changeNotes");
- try (Timer0.Context ctx = codeOwnerMetrics.computeFileStatuses.start()) {
+ CodeOwnersPluginProjectConfigSnapshot codeOwnersConfig =
+ codeOwnersPluginConfiguration.getProjectConfig(changeNotes.getProjectName());
+ try (Timer1.Context<Boolean> ctx =
+ codeOwnerMetrics.computeFileStatuses.start(codeOwnersConfig.areStickyApprovalsEnabled())) {
logger.atFine().log(
"compute file statuses (project = %s, change = %d, start = %d, limit = %d)",
changeNotes.getProjectName(), changeNotes.getChangeId().get(), start, limit);
Stream<FileCodeOwnerStatus> fileStatuses =
getFileStatuses(
+ codeOwnersConfig,
codeOwnerConfigHierarchyProvider.get(),
codeOwnerResolverProvider.get().enforceVisibility(false),
changeNotes);
@@ -300,6 +312,7 @@
* returned
*/
private Stream<FileCodeOwnerStatus> getFileStatuses(
+ CodeOwnersPluginProjectConfigSnapshot codeOwnersConfig,
CodeOwnerConfigHierarchy codeOwnerConfigHierarchy,
CodeOwnerResolver codeOwnerResolver,
ChangeNotes changeNotes)
@@ -310,9 +323,6 @@
"prepare stream to compute file statuses (project = %s, change = %d)",
changeNotes.getProjectName(), changeNotes.getChangeId().get());
- CodeOwnersPluginProjectConfigSnapshot codeOwnersConfig =
- codeOwnersPluginConfiguration.getProjectConfig(changeNotes.getProjectName());
-
Account.Id patchSetUploader = changeNotes.getCurrentPatchSet().uploader();
ImmutableSet<Account.Id> exemptedAccounts = codeOwnersConfig.getExemptedAccounts();
logger.atFine().log("exemptedAccounts = %s", exemptedAccounts);
@@ -360,6 +370,8 @@
getFileStatus(
codeOwnerConfigHierarchy,
codeOwnerResolver,
+ codeOwnersConfig,
+ changedFilesByPatchSetCacheFactory.create(codeOwnersConfig, changeNotes),
branch,
revision.orElse(null),
changedFile,
@@ -430,6 +442,8 @@
getFileStatus(
codeOwnerConfigHierarchy,
codeOwnerResolver,
+ codeOwnersConfig,
+ changedFilesByPatchSetCacheFactory.create(codeOwnersConfig, changeNotes),
branch,
revision.orElse(null),
changedFile,
@@ -476,6 +490,8 @@
private FileCodeOwnerStatus getFileStatus(
CodeOwnerConfigHierarchy codeOwnerConfigHierarchy,
CodeOwnerResolver codeOwnerResolver,
+ CodeOwnersPluginProjectConfigSnapshot codeOwnersConfig,
+ ChangedFilesByPatchSetCache changedFilesByPatchSetCache,
BranchNameKey branch,
@Nullable ObjectId revision,
ChangedFile changedFile,
@@ -492,6 +508,8 @@
getPathCodeOwnerStatus(
codeOwnerConfigHierarchy,
codeOwnerResolver,
+ codeOwnersConfig,
+ changedFilesByPatchSetCache,
branch,
revision,
newPath,
@@ -510,6 +528,8 @@
getPathCodeOwnerStatus(
codeOwnerConfigHierarchy,
codeOwnerResolver,
+ codeOwnersConfig,
+ changedFilesByPatchSetCache,
branch,
revision,
changedFile.oldPath().get(),
@@ -526,6 +546,8 @@
private PathCodeOwnerStatus getPathCodeOwnerStatus(
CodeOwnerConfigHierarchy codeOwnerConfigHierarchy,
CodeOwnerResolver codeOwnerResolver,
+ CodeOwnersPluginProjectConfigSnapshot codeOwnersConfig,
+ ChangedFilesByPatchSetCache changedFilesByPatchSetCache,
BranchNameKey branch,
@Nullable ObjectId revision,
Path absolutePath,
@@ -554,10 +576,12 @@
boolean isGloballyApproved =
isApproved(
+ absolutePath,
input.globalCodeOwners(),
CodeOwnerKind.GLOBAL_CODE_OWNER,
- input.approvers(),
- input.implicitApprover().orElse(null),
+ codeOwnersConfig,
+ changedFilesByPatchSetCache,
+ input,
reason);
if (isGloballyApproved) {
@@ -626,10 +650,12 @@
}
if (isApproved(
+ absolutePath,
codeOwners,
codeOwnerKind,
- input.approvers(),
- input.implicitApprover().orElse(null),
+ codeOwnersConfig,
+ changedFilesByPatchSetCache,
+ input,
reason)) {
codeOwnerStatus.set(CodeOwnerStatus.APPROVED);
// No need to recurse if we are not checking all owners or all owners are
@@ -798,31 +824,36 @@
}
/**
- * Checks whether the given path was implicitly or explicitly approved.
+ * Checks whether the given path was approved implicitly, explicitly or by sticky approvals.
*
+ * @param absolutePath the absolute path for which it should be checked whether it is code owner
+ * approved
* @param codeOwners users that own the path
* @param codeOwnerKind the kind of the given {@code codeOwners}
- * @param approverAccountIds the IDs of the accounts that have approved the change
- * @param implicitApprover the ID of the account the could be an implicit approver (aka last patch
- * set uploader)
+ * @param codeOwnersConfig the code-owners plugin configuration that applies to the project that
+ * contains the change for which the code owner statuses are checked
+ * @param changedFilesByPatchSetCache cache that allows to lookup changed files by patch set
+ * @param input input data for checking if a path is code owner approved
* @param reason {@link AtomicReference} on which the reason is being set if the path is approved
* @return whether the path was approved
*/
private boolean isApproved(
+ Path absolutePath,
CodeOwnerResolverResult codeOwners,
CodeOwnerKind codeOwnerKind,
- ImmutableSet<Account.Id> approverAccountIds,
- @Nullable Account.Id implicitApprover,
+ CodeOwnersPluginProjectConfigSnapshot codeOwnersConfig,
+ ChangedFilesByPatchSetCache changedFilesByPatchSetCache,
+ CodeOwnerApprovalCheckInput input,
AtomicReference<String> reason) {
- if (implicitApprover != null) {
- if (codeOwners.codeOwnersAccountIds().contains(implicitApprover)
+ if (input.implicitApprover().isPresent()) {
+ if (codeOwners.codeOwnersAccountIds().contains(input.implicitApprover().get())
|| 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.
reason.set(
String.format(
"implicitly approved by the patch set uploader %s who is a %s%s",
- AccountTemplateUtil.getAccountTemplate(implicitApprover),
+ AccountTemplateUtil.getAccountTemplate(input.implicitApprover().get()),
codeOwnerKind.getDisplayName(),
codeOwners.ownedByAllUsers()
? String.format(" (all users are %ss)", codeOwnerKind.getDisplayName())
@@ -831,13 +862,14 @@
}
}
- if (!Collections.disjoint(approverAccountIds, codeOwners.codeOwnersAccountIds())
- || (codeOwners.ownedByAllUsers() && !approverAccountIds.isEmpty())) {
+ ImmutableSet<Account.Id> approvers = input.approvers();
+ if (!Collections.disjoint(approvers, codeOwners.codeOwnersAccountIds())
+ || (codeOwners.ownedByAllUsers() && !approvers.isEmpty())) {
// At least one of the code owners approved the change.
Optional<Account.Id> approver =
codeOwners.ownedByAllUsers()
- ? approverAccountIds.stream().findAny()
- : approverAccountIds.stream()
+ ? approvers.stream().findAny()
+ : approvers.stream()
.filter(accountId -> codeOwners.codeOwnersAccountIds().contains(accountId))
.findAny();
checkState(approver.isPresent(), "no approver found");
@@ -852,6 +884,58 @@
return true;
}
+ return codeOwnersConfig.areStickyApprovalsEnabled()
+ && isApprovedByStickyApproval(
+ absolutePath, codeOwners, codeOwnerKind, changedFilesByPatchSetCache, input, reason);
+ }
+
+ /**
+ * Checks whether the given path is code owner approved by a sticky approval on a previous patch
+ * set.
+ */
+ private boolean isApprovedByStickyApproval(
+ Path absolutePath,
+ CodeOwnerResolverResult codeOwners,
+ CodeOwnerKind codeOwnerKind,
+ ChangedFilesByPatchSetCache changedFilesByPatchSetCache,
+ CodeOwnerApprovalCheckInput input,
+ AtomicReference<String> reason) {
+ for (PatchSet.Id patchSetId : input.previouslyApprovedPatchSetsInReverseOrder()) {
+ if (changedFilesByPatchSetCache.get(patchSetId).stream()
+ .anyMatch(
+ changedFile ->
+ changedFile.hasNewPath(absolutePath) || changedFile.hasOldPath(absolutePath))) {
+ logger.atFine().log(
+ "previously approved patch set %d contains path %s", patchSetId.get(), absolutePath);
+ Optional<Account.Id> approver =
+ input.approversFromPreviousPatchSets().get(patchSetId).stream()
+ .filter(
+ accountId ->
+ codeOwners.codeOwnersAccountIds().contains(accountId)
+ || codeOwners.ownedByAllUsers())
+ .findAny();
+ if (!approver.isPresent()) {
+ logger.atFine().log(
+ "none of the approvals on previous patch set %d is from a user that owns path %s"
+ + " (approvers=%s)",
+ patchSetId.get(),
+ absolutePath,
+ input.approversFromPreviousPatchSets().get(patchSetId));
+ continue;
+ }
+
+ reason.set(
+ String.format(
+ "approved on patch set %d by %s who is a %s%s",
+ patchSetId.get(),
+ AccountTemplateUtil.getAccountTemplate(approver.get()),
+ codeOwnerKind.getDisplayName(),
+ codeOwners.ownedByAllUsers()
+ ? String.format(" (all users are %ss)", codeOwnerKind.getDisplayName())
+ : ""));
+ return true;
+ }
+ }
return false;
}
diff --git a/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerApprovalCheckInput.java b/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerApprovalCheckInput.java
index cd2ef0e..9335e5b 100644
--- a/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerApprovalCheckInput.java
+++ b/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerApprovalCheckInput.java
@@ -14,14 +14,23 @@
package com.google.gerrit.plugins.codeowners.backend;
+import static com.google.common.collect.ImmutableMap.toImmutableMap;
import static com.google.common.collect.ImmutableSet.toImmutableSet;
+import static com.google.common.collect.ImmutableSetMultimap.toImmutableSetMultimap;
+import static java.util.Comparator.comparing;
import com.google.auto.value.AutoValue;
+import com.google.auto.value.extension.memoized.Memoized;
+import com.google.common.base.Predicate;
import com.google.common.collect.ImmutableList;
+import com.google.common.collect.ImmutableMap;
+import com.google.common.collect.ImmutableMultimap;
import com.google.common.collect.ImmutableSet;
+import com.google.common.collect.ImmutableSetMultimap;
import com.google.common.collect.ImmutableSortedSet;
import com.google.common.flogger.FluentLogger;
import com.google.gerrit.entities.Account;
+import com.google.gerrit.entities.PatchSet;
import com.google.gerrit.entities.PatchSetApproval;
import com.google.gerrit.metrics.Timer0;
import com.google.gerrit.plugins.codeowners.backend.config.CodeOwnersPluginProjectConfigSnapshot;
@@ -33,6 +42,8 @@
import com.google.gerrit.server.util.LabelVote;
import com.google.inject.Inject;
import com.google.inject.assistedinject.Assisted;
+import java.util.HashMap;
+import java.util.Map;
import java.util.Optional;
/**
@@ -63,6 +74,23 @@
public abstract ImmutableSet<Account.Id> approvers();
/**
+ * Gets a map of previous patch sets to the IDs of the accounts that have an approval on that
+ * patch set that is sticky and possibly counts as code owner approval (if they are code owners).
+ *
+ * <p>If self approvals are ignored the patch set uploader is filtered out for all patch sets
+ * since in this case the approval of the patch set uploader is ignored even if they are a code
+ * owner.
+ */
+ public abstract ImmutableMultimap<PatchSet.Id, Account.Id> approversFromPreviousPatchSets();
+
+ @Memoized
+ public ImmutableSortedSet<PatchSet.Id> previouslyApprovedPatchSetsInReverseOrder() {
+ return ImmutableSortedSet.orderedBy(comparing(PatchSet.Id::get).reversed())
+ .addAll(approversFromPreviousPatchSets().keySet())
+ .build();
+ }
+
+ /**
* Account from which an implicit code owner approval should be assumed.
*
* @see CodeOwnersPluginProjectConfigSnapshot#areImplicitApprovalsEnabled()
@@ -119,6 +147,7 @@
return create(
/* reviewers= */ ImmutableSet.of(),
/* approvers= */ accounts,
+ /* approversFromPreviousPatchSets= */ ImmutableMultimap.of(),
// 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
@@ -152,6 +181,7 @@
private static CodeOwnerApprovalCheckInput create(
ImmutableSet<Account.Id> reviewers,
ImmutableSet<Account.Id> approvers,
+ ImmutableMultimap<PatchSet.Id, Account.Id> approversFromPreviousPatchSets,
Optional<Account.Id> implicitApprover,
ImmutableSet<PatchSetApproval> overrides,
CodeOwnerResolverResult globalCodeOwners,
@@ -160,6 +190,7 @@
return new AutoValue_CodeOwnerApprovalCheckInput(
reviewers,
approvers,
+ approversFromPreviousPatchSets,
implicitApprover,
overrides,
globalCodeOwners,
@@ -207,6 +238,7 @@
return CodeOwnerApprovalCheckInput.create(
getReviewers(),
getApprovers(),
+ getApproversFromPreviousPatchSets(),
getImplicitApprover(),
getOverrides(),
getGlobalCodeOwners(),
@@ -284,6 +316,108 @@
}
/**
+ * Gets a map of previous patch sets to the IDs of the accounts that have an approval on that
+ * patch set that is sticky and possibly counts as code owner approval (if they are code
+ * owners).
+ *
+ * <p>If self approvals are ignored the patch set uploader is filtered out for all patch sets
+ * since in this case the approval of the patch set uploader is ignored even if they are a code
+ * owner.
+ */
+ private ImmutableMultimap<PatchSet.Id, Account.Id> getApproversFromPreviousPatchSets() {
+ if (!codeOwnersConfig.areStickyApprovalsEnabled()) {
+ logger.atFine().log("sticky approvals are disabled");
+ return ImmutableMultimap.of();
+ }
+
+ // Filter out approvals on the current patch set, since here we are only interested in code
+ // owner approvals on previous patch sets that should be considered as sticky.
+ PatchSet.Id currentPatchSetId = changeNotes.getCurrentPatchSet().id();
+ ImmutableSetMultimap<PatchSet.Id, Account.Id> approversFromPreviousPatchSets =
+ getLastCodeOwnerApprovalsByAccount().values().stream()
+ .filter(psa -> psa.patchSetId().get() < currentPatchSetId.get())
+ .collect(
+ toImmutableSetMultimap(
+ PatchSetApproval::patchSetId, PatchSetApproval::accountId));
+ logger.atFine().log(
+ "sticky approvals are enabled, approversFromPreviousPatchSets=%s",
+ approversFromPreviousPatchSets);
+ return approversFromPreviousPatchSets;
+ }
+
+ /**
+ * Returns the last code owner approvals by account.
+ *
+ * <p>The returned map contains for each user their last approval on the change that counts as a
+ * code owner approval. Approvals that are invalidated by code owner votes on newer patch sets
+ * are filtered out.
+ */
+ private ImmutableMap<Account.Id, PatchSetApproval> getLastCodeOwnerApprovalsByAccount() {
+ RequiredApproval requiredApproval = codeOwnersConfig.getRequiredApproval();
+
+ Map<Account.Id, PatchSetApproval> lastCodeOwnerVotesByAccount = new HashMap<>();
+ ImmutableSetMultimap<PatchSet.Id, PatchSetApproval> allCodeOwnerApprovals =
+ changeNotes.getApprovals().all().entries().stream()
+ // Only look at approvals on the label that is configured for code owner approvals.
+ .filter(e -> e.getValue().label().equals(requiredApproval.labelType().getName()))
+ .collect(toImmutableSetMultimap(Map.Entry::getKey, Map.Entry::getValue));
+ logger.atFine().log("allCodeOwnerApprovals=%s", allCodeOwnerApprovals);
+ // Iterate over the patch sets in reverse order (latest patch set first).
+ for (PatchSet.Id patchSetId : getPatchSetIdsInReverseOrder()) {
+ // Only store the code owner approval if we didn't find a code owner approval for that
+ // account on a newer patch set yet.
+ // If a code owner approval on a newer patch set exist, it invalidated the code owner
+ // approval on the older patch set and we can ignore it.
+ allCodeOwnerApprovals
+ .get(patchSetId)
+ .forEach(psa -> lastCodeOwnerVotesByAccount.putIfAbsent(psa.accountId(), psa));
+ }
+
+ ImmutableMap<Account.Id, PatchSetApproval> lastCodeOwnerApprovalsByAccount =
+ lastCodeOwnerVotesByAccount.entrySet().stream()
+ // Remove all approvals which do not count as a code owner approval because the voting
+ // value is insufficient.
+ .filter(e -> requiredApproval.isApprovedBy(e.getValue()))
+ .filter(filterOutSelfApprovalsIfSelfApprovalsAreIgnored())
+ .collect(toImmutableMap(Map.Entry::getKey, Map.Entry::getValue));
+ logger.atFine().log(
+ "lastCodeOwnerApprovalsByAccount=%s, lastCodeOwnerVotesByAccount=%s",
+ lastCodeOwnerApprovalsByAccount, lastCodeOwnerVotesByAccount);
+ return lastCodeOwnerApprovalsByAccount;
+ }
+
+ /**
+ * Creates a filter that filters out self approvals by the patch set uploader if self approvals
+ * are ignored
+ */
+ private Predicate<Map.Entry<Account.Id, PatchSetApproval>>
+ filterOutSelfApprovalsIfSelfApprovalsAreIgnored() {
+ RequiredApproval requiredApproval = codeOwnersConfig.getRequiredApproval();
+ if (!requiredApproval.labelType().isIgnoreSelfApproval()) {
+ logger.atFine().log("s");
+ return e -> true;
+ }
+
+ Account.Id patchSetUploader = changeNotes.getCurrentPatchSet().uploader();
+ return e -> {
+ if (e.getKey().equals(patchSetUploader)) {
+ logger.atFine().log(
+ "Removing approvals of the patch set uploader %s since the label of the required"
+ + " approval (%s) is configured to ignore self approvals",
+ patchSetUploader, requiredApproval.labelType());
+ return false;
+ }
+ return true;
+ };
+ }
+
+ private ImmutableSortedSet<PatchSet.Id> getPatchSetIdsInReverseOrder() {
+ return ImmutableSortedSet.orderedBy(comparing(PatchSet.Id::get).reversed())
+ .addAll(changeNotes.getPatchSets().keySet())
+ .build();
+ }
+
+ /**
* Gets the approvals from the current patch set that count as code owner overrides.
*
* <p>If self approvals are ignored an override of the patch set uploader is filtered out since
diff --git a/java/com/google/gerrit/plugins/codeowners/backend/config/CodeOwnersPluginProjectConfigSnapshot.java b/java/com/google/gerrit/plugins/codeowners/backend/config/CodeOwnersPluginProjectConfigSnapshot.java
index 89fc56e..f02e54d 100644
--- a/java/com/google/gerrit/plugins/codeowners/backend/config/CodeOwnersPluginProjectConfigSnapshot.java
+++ b/java/com/google/gerrit/plugins/codeowners/backend/config/CodeOwnersPluginProjectConfigSnapshot.java
@@ -95,6 +95,7 @@
private Map<String, Optional<PathExpressions>> pathExpressionsByBranch = new HashMap<>();
@Nullable private Optional<PathExpressions> pathExpressions;
@Nullable private Boolean implicitApprovalsEnabled;
+ @Nullable private Boolean stickyApprovalsEnabled;
@Nullable private RequiredApproval requiredApproval;
@Nullable private ImmutableSortedSet<RequiredApproval> overrideApprovals;
@@ -618,6 +619,27 @@
}
/**
+ * Checks whether sticky code owner approvals are enabled.
+ *
+ * <p>If enabled, a code owner approval on a previous patch set is sticky (if the approver didn't
+ * alter or remove it on a later patch set).
+ */
+ public boolean areStickyApprovalsEnabled() {
+ if (stickyApprovalsEnabled == null) {
+ stickyApprovalsEnabled = readStickyApprovalsEnabled();
+ }
+ return stickyApprovalsEnabled;
+ }
+
+ private boolean readStickyApprovalsEnabled() {
+ boolean enableStickyApprovals = generalConfig.enableStickyApprovals(projectName, pluginConfig);
+ logger.atFine().log(
+ "sticky approvals on project %s are %s",
+ projectName, enableStickyApprovals ? "enabled" : "disabled");
+ return enableStickyApprovals;
+ }
+
+ /**
* Returns the approval that is required from code owners to approve the files in a change.
*
* <p>Defines which approval counts as code owner approval.
diff --git a/java/com/google/gerrit/plugins/codeowners/backend/config/GeneralConfig.java b/java/com/google/gerrit/plugins/codeowners/backend/config/GeneralConfig.java
index 00574bc..810486f 100644
--- a/java/com/google/gerrit/plugins/codeowners/backend/config/GeneralConfig.java
+++ b/java/com/google/gerrit/plugins/codeowners/backend/config/GeneralConfig.java
@@ -80,6 +80,7 @@
public static final String KEY_GLOBAL_CODE_OWNER = "globalCodeOwner";
public static final String KEY_EXEMPTED_USER = "exemptedUser";
public static final String KEY_ENABLE_IMPLICIT_APPROVALS = "enableImplicitApprovals";
+ public static final String KEY_ENABLE_STICKY_APPROVALS = "enableStickyApprovals";
public static final String KEY_OVERRIDE_INFO_URL = "overrideInfoUrl";
public static final String KEY_INVALID_CODE_OWNER_CONFIG_INFO_URL =
"invalidCodeOwnerConfigInfoUrl";
@@ -875,6 +876,22 @@
}
/**
+ * Gets whether sticky code owner approvals are enabled from the given plugin config with fallback
+ * to {@code gerrit.config}.
+ *
+ * <p>If enabled, a code owner approval on a previous patch set is sticky (if the approver didn't
+ * alter or remove it on a later patch set).
+ *
+ * @param project the name of the project for which the configuration should be read
+ * @param pluginConfig the plugin config from which the configuration should be read.
+ * @return whether a sticky code owner approvals are enabled
+ */
+ boolean enableStickyApprovals(Project.NameKey project, Config pluginConfig) {
+ return getBooleanConfig(
+ project, pluginConfig, KEY_ENABLE_STICKY_APPROVALS, /* defaultValue= */ false);
+ }
+
+ /**
* Gets the users which are configured as global code owners from the given plugin config with
* fallback to {@code gerrit.config}.
*
diff --git a/java/com/google/gerrit/plugins/codeowners/metrics/CodeOwnerMetrics.java b/java/com/google/gerrit/plugins/codeowners/metrics/CodeOwnerMetrics.java
index d6ac638..fa90564 100644
--- a/java/com/google/gerrit/plugins/codeowners/metrics/CodeOwnerMetrics.java
+++ b/java/com/google/gerrit/plugins/codeowners/metrics/CodeOwnerMetrics.java
@@ -34,7 +34,7 @@
// latency metrics
public final Timer1<String> addChangeMessageOnAddReviewer;
public final Timer0 computeFileStatus;
- public final Timer0 computeFileStatuses;
+ public final Timer1<Boolean> computeFileStatuses;
public final Timer0 computeOwnedPaths;
public final Timer0 computePatchSetApprovals;
public final Timer0 extendChangeMessageOnPostReview;
@@ -90,7 +90,10 @@
this.computeFileStatuses =
createTimer(
"compute_file_statuses",
- "Latency for computing file statuses for all files in a change");
+ "Latency for computing file statuses for all files in a change",
+ Field.ofBoolean("sticky_approvals", (metadataBuilder, stickyApprovals) -> {})
+ .description("Whether sticky approvals on file level are enabled.")
+ .build());
this.computeOwnedPaths =
createTimer(
"compute_owned_paths",
diff --git a/javatests/com/google/gerrit/plugins/codeowners/backend/CodeOwnerApprovalCheckInputTest.java b/javatests/com/google/gerrit/plugins/codeowners/backend/CodeOwnerApprovalCheckInputTest.java
index 3c41f14..0054f2e 100644
--- a/javatests/com/google/gerrit/plugins/codeowners/backend/CodeOwnerApprovalCheckInputTest.java
+++ b/javatests/com/google/gerrit/plugins/codeowners/backend/CodeOwnerApprovalCheckInputTest.java
@@ -17,13 +17,19 @@
import static com.google.common.collect.ImmutableSet.toImmutableSet;
import static com.google.common.truth.Truth.assertThat;
import static com.google.common.truth.Truth8.assertThat;
+import static com.google.gerrit.server.group.SystemGroupBackend.REGISTERED_USERS;
+import com.google.common.collect.ArrayListMultimap;
+import com.google.common.collect.ImmutableMap;
import com.google.common.collect.ImmutableSet;
import com.google.gerrit.acceptance.TestAccount;
import com.google.gerrit.acceptance.config.GerritConfig;
+import com.google.gerrit.acceptance.testsuite.project.ProjectOperations;
+import com.google.gerrit.acceptance.testsuite.project.TestProjectUpdate;
import com.google.gerrit.acceptance.testsuite.request.RequestScopeOperations;
import com.google.gerrit.entities.Account;
import com.google.gerrit.entities.Change;
+import com.google.gerrit.entities.PatchSet;
import com.google.gerrit.entities.PatchSetApproval;
import com.google.gerrit.entities.Project;
import com.google.gerrit.extensions.api.changes.ChangeApi;
@@ -34,6 +40,7 @@
import com.google.gerrit.plugins.codeowners.backend.config.CodeOwnersPluginConfiguration;
import com.google.gerrit.plugins.codeowners.backend.config.CodeOwnersPluginProjectConfigSnapshot;
import com.google.gerrit.server.notedb.ChangeNotes;
+import com.google.gerrit.server.query.change.ChangeData;
import com.google.inject.Inject;
import org.junit.Before;
import org.junit.Test;
@@ -41,6 +48,7 @@
/** Tests for {@link CodeOwnerApprovalCheckInput}. */
public class CodeOwnerApprovalCheckInputTest extends AbstractCodeOwnersTest {
@Inject private ChangeNotes.Factory changeNotesFactory;
+ @Inject private ProjectOperations projectOperations;
@Inject private RequestScopeOperations requestScopeOperations;
private CodeOwnerApprovalCheckInput.Loader.Factory inputLoaderFactory;
@@ -48,6 +56,7 @@
private CodeOwnersPluginProjectConfigSnapshot codeOwnersConfig;
private TestAccount user2;
private Change.Id changeId;
+ private Change.Key changeKey;
private Account.Id changeOwner;
@Before
@@ -63,7 +72,9 @@
@Before
public void setUp() throws Exception {
user2 = accountCreator.user2();
- changeId = createChange().getChange().getId();
+ ChangeData changeData = createChange().getChange();
+ changeId = changeData.getId();
+ changeKey = changeData.change().getKey();
changeOwner = admin.id();
}
@@ -143,6 +154,448 @@
assertThat(loadInput().approvers()).containsExactly(user.id(), user2.id());
}
+ /** Test that current approvals do not count for computing previous approvers. */
+ @Test
+ @GerritConfig(name = "plugin.code-owners.enableStickyApprovals", value = "true")
+ public void noPreviousApprovers() throws Exception {
+ // self approve current patch set
+ requestScopeOperations.setApiUser(changeOwner);
+ recommend(changeId.toString());
+
+ // approve as user current patch set
+ requestScopeOperations.setApiUser(user.id());
+ recommend(changeId.toString());
+
+ // approve as user2 current patch set
+ requestScopeOperations.setApiUser(user2.id());
+ recommend(changeId.toString());
+
+ assertThat(loadInput().approversFromPreviousPatchSets()).isEmpty();
+ }
+
+ /** Test that previous approvals on other labels do not count for computing previous approvers. */
+ @Test
+ @GerritConfig(name = "plugin.code-owners.enableStickyApprovals", value = "true")
+ public void noPreviousApproversIfApprovalIsOnUnrelatedLabel() throws Exception {
+ // Create Foo-Review label.
+ LabelDefinitionInput input = new LabelDefinitionInput();
+ input.values = ImmutableMap.of("+1", "Approved", " 0", "Not Approved");
+ gApi.projects().name(project.get()).label("Foo-Review").create(input).get();
+
+ // Allow to vote on the Foo-Review label.
+ projectOperations
+ .project(project)
+ .forUpdate()
+ .add(
+ TestProjectUpdate.allowLabel("Foo-Review")
+ .range(0, 1)
+ .ref("refs/heads/*")
+ .group(REGISTERED_USERS)
+ .build())
+ .update();
+
+ // approve on Foo-Review label
+ requestScopeOperations.setApiUser(user.id());
+ gApi.changes().id(changeId.get()).current().review(new ReviewInput().label("Foo-Review", 1));
+
+ // create a second patch set
+ requestScopeOperations.setApiUser(changeOwner);
+ amendChange(changeKey.get()).assertOkStatus();
+
+ assertThat(loadInput().approversFromPreviousPatchSets()).isEmpty();
+ }
+
+ /**
+ * Test that previous votes with insufficient values do not count for computing previous
+ * approvers.
+ */
+ @Test
+ @GerritConfig(name = "plugin.code-owners.requiredApproval", value = "Code-Review+2")
+ @GerritConfig(name = "plugin.code-owners.enableStickyApprovals", value = "true")
+ public void noPreviousApproversIfVoteIsNotAnApproval() throws Exception {
+ // vote with Code-Review+1, but only Code-Review+2 counts as a code owner approval
+ requestScopeOperations.setApiUser(user.id());
+ recommend(changeId.toString());
+
+ // create a second patch set
+ requestScopeOperations.setApiUser(changeOwner);
+ amendChange(changeKey.get()).assertOkStatus();
+
+ assertThat(loadInput().approversFromPreviousPatchSets()).isEmpty();
+ }
+
+ /** Test that previous approvals are ignored if sticky approvals are disabled. */
+ @Test
+ @GerritConfig(name = "plugin.code-owners.enableStickyApprovals", value = "false")
+ public void noPreviousApproversIfEnableStickyApprovalsDisabled() throws Exception {
+ // self approve
+ requestScopeOperations.setApiUser(changeOwner);
+ recommend(changeId.toString());
+
+ // approve as user
+ requestScopeOperations.setApiUser(user.id());
+ recommend(changeId.toString());
+
+ // approve as user2
+ requestScopeOperations.setApiUser(user2.id());
+ recommend(changeId.toString());
+
+ // create a second patch set
+ requestScopeOperations.setApiUser(changeOwner);
+ amendChange(changeKey.get()).assertOkStatus();
+
+ assertThat(loadInput().approversFromPreviousPatchSets()).isEmpty();
+ }
+
+ /** Test that the approvals on the previous patch set count for computing previous approvers. */
+ @Test
+ @GerritConfig(name = "plugin.code-owners.enableStickyApprovals", value = "true")
+ public void withPreviousApprovers() throws Exception {
+ // self approve
+ requestScopeOperations.setApiUser(changeOwner);
+ recommend(changeId.toString());
+
+ // approve as user
+ requestScopeOperations.setApiUser(user.id());
+ recommend(changeId.toString());
+
+ // approve as user2
+ requestScopeOperations.setApiUser(user2.id());
+ recommend(changeId.toString());
+
+ // create a second patch set
+ requestScopeOperations.setApiUser(changeOwner);
+ amendChange(changeKey.get()).assertOkStatus();
+
+ ArrayListMultimap<PatchSet.Id, Account.Id> expectedPreviousApprovers =
+ ArrayListMultimap.create();
+ expectedPreviousApprovers.putAll(
+ PatchSet.id(changeId, 1), ImmutableSet.of(changeOwner, user.id(), user2.id()));
+ assertThat(loadInput().approversFromPreviousPatchSets())
+ .containsExactlyEntriesIn(expectedPreviousApprovers);
+ }
+
+ /**
+ * Test that a self-approval on the previous patch set is ignored for computing previous
+ * approvers.
+ */
+ @Test
+ @GerritConfig(name = "plugin.code-owners.enableStickyApprovals", value = "true")
+ public void withPreviousApprovers_selfApprovalsIgnored() throws Exception {
+ disableSelfCodeReviewApprovals();
+
+ // self approve
+ requestScopeOperations.setApiUser(changeOwner);
+ recommend(changeId.toString());
+
+ // approve as user
+ requestScopeOperations.setApiUser(user.id());
+ recommend(changeId.toString());
+
+ // approve as user2
+ requestScopeOperations.setApiUser(user2.id());
+ recommend(changeId.toString());
+
+ // create a second patch set
+ requestScopeOperations.setApiUser(changeOwner);
+ amendChange(changeKey.get()).assertOkStatus();
+
+ ArrayListMultimap<PatchSet.Id, Account.Id> expectedPreviousApprovers =
+ ArrayListMultimap.create();
+ expectedPreviousApprovers.putAll(
+ PatchSet.id(changeId, 1), ImmutableSet.of(user.id(), user2.id()));
+ assertThat(loadInput().approversFromPreviousPatchSets())
+ .containsExactlyEntriesIn(expectedPreviousApprovers);
+ }
+
+ /**
+ * Test that the approvals on different previous patch sets count for computing previous
+ * approvers.
+ */
+ @Test
+ @GerritConfig(name = "plugin.code-owners.enableStickyApprovals", value = "true")
+ public void withPreviousApproversOnDifferentPatchSets() throws Exception {
+ // self approve
+ requestScopeOperations.setApiUser(changeOwner);
+ recommend(changeId.toString());
+
+ // create a second patch set
+ requestScopeOperations.setApiUser(changeOwner);
+ amendChange(changeKey.get()).assertOkStatus();
+
+ // approve as user
+ requestScopeOperations.setApiUser(user.id());
+ recommend(changeId.toString());
+
+ // create a third patch set
+ requestScopeOperations.setApiUser(changeOwner);
+ amendChange(changeKey.get()).assertOkStatus();
+
+ // approve as user2
+ requestScopeOperations.setApiUser(user2.id());
+ recommend(changeId.toString());
+
+ // create a 4th patch set
+ requestScopeOperations.setApiUser(changeOwner);
+ amendChange(changeKey.get()).assertOkStatus();
+
+ ArrayListMultimap<PatchSet.Id, Account.Id> expectedPreviousApprovers =
+ ArrayListMultimap.create();
+ expectedPreviousApprovers.put(PatchSet.id(changeId, 1), changeOwner);
+ expectedPreviousApprovers.put(PatchSet.id(changeId, 2), user.id());
+ expectedPreviousApprovers.put(PatchSet.id(changeId, 3), user2.id());
+ assertThat(loadInput().approversFromPreviousPatchSets())
+ .containsExactlyEntriesIn(expectedPreviousApprovers);
+ }
+
+ /** Test that a self-approval on an old patch set is ignored for computing previous approvers. */
+ @Test
+ @GerritConfig(name = "plugin.code-owners.enableStickyApprovals", value = "true")
+ public void withPreviousApproversOnDifferentPatchSets_selfApprovalsIgnored() throws Exception {
+ disableSelfCodeReviewApprovals();
+
+ // self approve
+ requestScopeOperations.setApiUser(changeOwner);
+ recommend(changeId.toString());
+
+ // create a second patch set
+ requestScopeOperations.setApiUser(changeOwner);
+ amendChange(changeKey.get()).assertOkStatus();
+
+ // approve as user
+ requestScopeOperations.setApiUser(user.id());
+ recommend(changeId.toString());
+
+ // create a third patch set
+ requestScopeOperations.setApiUser(changeOwner);
+ amendChange(changeKey.get()).assertOkStatus();
+
+ // approve as user2
+ requestScopeOperations.setApiUser(user2.id());
+ recommend(changeId.toString());
+
+ // create a 4th patch set
+ requestScopeOperations.setApiUser(changeOwner);
+ amendChange(changeKey.get()).assertOkStatus();
+
+ ArrayListMultimap<PatchSet.Id, Account.Id> expectedPreviousApprovers =
+ ArrayListMultimap.create();
+ expectedPreviousApprovers.put(PatchSet.id(changeId, 2), user.id());
+ expectedPreviousApprovers.put(PatchSet.id(changeId, 3), user2.id());
+ assertThat(loadInput().approversFromPreviousPatchSets())
+ .containsExactlyEntriesIn(expectedPreviousApprovers);
+ }
+
+ /**
+ * Test that sticky approvals do not count for computing previous approvers (because if the
+ * approval is sticky it's a current approval).
+ */
+ @Test
+ @GerritConfig(name = "plugin.code-owners.enableStickyApprovals", value = "true")
+ public void noPreviousApproversIfApprovalIsCopied() throws Exception {
+ // Make Code-Review approvals sticky
+ LabelDefinitionInput input = new LabelDefinitionInput();
+ input.copyCondition = "is:ANY";
+ gApi.projects().name(allProjects.get()).label("Code-Review").update(input);
+
+ // approve as user
+ requestScopeOperations.setApiUser(user.id());
+ recommend(changeId.toString());
+
+ // create a second patch set
+ requestScopeOperations.setApiUser(changeOwner);
+ amendChange(changeKey.get()).assertOkStatus();
+
+ assertThat(loadInput().approversFromPreviousPatchSets()).isEmpty();
+ }
+
+ /**
+ * Test that a previous approval still counts for computing previous approvers if the approver
+ * comments on the current patch set without applying a vote.
+ */
+ @Test
+ @GerritConfig(name = "plugin.code-owners.enableStickyApprovals", value = "true")
+ public void previousApproversIsPreservedWhenThePreviousApproverCommentsOnTheChange()
+ throws Exception {
+ // approve as user
+ requestScopeOperations.setApiUser(user.id());
+ recommend(changeId.toString());
+
+ // create a second patch set
+ requestScopeOperations.setApiUser(changeOwner);
+ amendChange(changeKey.get()).assertOkStatus();
+
+ // check that the previous approver on patch set 1 is found
+ ArrayListMultimap<PatchSet.Id, Account.Id> expectedPreviousApprovers =
+ ArrayListMultimap.create();
+ expectedPreviousApprovers.put(PatchSet.id(changeId, 1), user.id());
+ assertThat(loadInput().approversFromPreviousPatchSets())
+ .containsExactlyEntriesIn(expectedPreviousApprovers);
+
+ // comment on the change
+ requestScopeOperations.setApiUser(user.id());
+ ReviewInput reviewInput = ReviewInput.noScore();
+ reviewInput.message = "a comment";
+ gApi.changes().id(changeId.get()).current().review(reviewInput);
+
+ assertThat(loadInput().approversFromPreviousPatchSets())
+ .containsExactlyEntriesIn(expectedPreviousApprovers);
+ }
+
+ /**
+ * Test that a previous approval doesn't count for computing previous approvers if the approver
+ * downgrades the vote.
+ */
+ @Test
+ @GerritConfig(name = "plugin.code-owners.enableStickyApprovals", value = "true")
+ @GerritConfig(name = "plugin.code-owners.requiredApproval", value = "Code-Review+2")
+ public void noPreviousApproversIfApprovalIsDowngraded() throws Exception {
+ // Allow all users to vote with Code-Review+2.
+ projectOperations
+ .project(project)
+ .forUpdate()
+ .add(
+ TestProjectUpdate.allowLabel("Code-Review")
+ .range(0, 2)
+ .ref("refs/heads/*")
+ .group(REGISTERED_USERS)
+ .build())
+ .update();
+
+ // approve as user
+ requestScopeOperations.setApiUser(user.id());
+ approve(changeId.toString());
+
+ // create a second patch set
+ requestScopeOperations.setApiUser(changeOwner);
+ amendChange(changeKey.get()).assertOkStatus();
+
+ // check that the previous approver on patch set 1 is found
+ ArrayListMultimap<PatchSet.Id, Account.Id> expectedPreviousApprovers =
+ ArrayListMultimap.create();
+ expectedPreviousApprovers.put(PatchSet.id(changeId, 1), user.id());
+ assertThat(loadInput().approversFromPreviousPatchSets())
+ .containsExactlyEntriesIn(expectedPreviousApprovers);
+
+ // change vote from Code-Review+2 to Code-Review+1 as user
+ requestScopeOperations.setApiUser(user.id());
+ recommend(changeId.toString());
+
+ // the Code-Review+1 vote on the current patch set overrode the previous approval
+ assertThat(loadInput().approversFromPreviousPatchSets()).isEmpty();
+
+ // create a third patch set
+ requestScopeOperations.setApiUser(changeOwner);
+ amendChange(changeKey.get()).assertOkStatus();
+
+ // the Code-Review+1 vote on the previous patch set overrode the previous approval
+ assertThat(loadInput().approversFromPreviousPatchSets()).isEmpty();
+ }
+
+ /**
+ * Test that a previous approval doesn't count for computing previous approvers if the approver
+ * re-applies the approval (because now it's a current approval).
+ */
+ @Test
+ @GerritConfig(name = "plugin.code-owners.enableStickyApprovals", value = "true")
+ public void noPreviousApproversIfApprovalIsReapplied() throws Exception {
+ // approve as user
+ requestScopeOperations.setApiUser(user.id());
+ recommend(changeId.toString());
+
+ // create a second patch set
+ requestScopeOperations.setApiUser(changeOwner);
+ amendChange(changeKey.get()).assertOkStatus();
+
+ // check that the previous approver on patch set 1 is found
+ ArrayListMultimap<PatchSet.Id, Account.Id> expectedPreviousApprovers =
+ ArrayListMultimap.create();
+ expectedPreviousApprovers.put(PatchSet.id(changeId, 1), user.id());
+ assertThat(loadInput().approversFromPreviousPatchSets())
+ .containsExactlyEntriesIn(expectedPreviousApprovers);
+
+ // re-apply the approval
+ requestScopeOperations.setApiUser(user.id());
+ recommend(changeId.toString());
+
+ assertThat(loadInput().approversFromPreviousPatchSets()).isEmpty();
+ }
+
+ /**
+ * Test that only the last previous approval of a user counts for computing previous approvers.
+ */
+ @Test
+ @GerritConfig(name = "plugin.code-owners.enableStickyApprovals", value = "true")
+ public void onlyLastPreviousApprovalOfAUserIsConsideredForComputingPreviousApprovers()
+ throws Exception {
+ // approve as user
+ requestScopeOperations.setApiUser(user.id());
+ recommend(changeId.toString());
+
+ // create a second patch set
+ requestScopeOperations.setApiUser(changeOwner);
+ amendChange(changeKey.get()).assertOkStatus();
+
+ // re-approve as user
+ requestScopeOperations.setApiUser(user.id());
+ recommend(changeId.toString());
+
+ // create a third patch set
+ requestScopeOperations.setApiUser(changeOwner);
+ amendChange(changeKey.get()).assertOkStatus();
+
+ ArrayListMultimap<PatchSet.Id, Account.Id> expectedPreviousApprovers =
+ ArrayListMultimap.create();
+ expectedPreviousApprovers.put(PatchSet.id(changeId, 2), user.id());
+ assertThat(loadInput().approversFromPreviousPatchSets())
+ .containsExactlyEntriesIn(expectedPreviousApprovers);
+ }
+
+ @Test
+ @GerritConfig(name = "plugin.code-owners.enableStickyApprovals", value = "true")
+ public void noPreviouslyApprovedPatchSets() throws Exception {
+ // approve current patch set
+ requestScopeOperations.setApiUser(user.id());
+ recommend(changeId.toString());
+
+ assertThat(loadInput().previouslyApprovedPatchSetsInReverseOrder()).isEmpty();
+ }
+
+ @Test
+ @GerritConfig(name = "plugin.code-owners.enableStickyApprovals", value = "true")
+ public void previouslyApprovedPatchSetsAreReturnedInReverseOrder() throws Exception {
+ // create a second patch set
+ requestScopeOperations.setApiUser(changeOwner);
+ amendChange(changeKey.get()).assertOkStatus();
+
+ // approve as user
+ requestScopeOperations.setApiUser(user.id());
+ recommend(changeId.toString());
+
+ // create a third patch set
+ requestScopeOperations.setApiUser(changeOwner);
+ amendChange(changeKey.get()).assertOkStatus();
+
+ // approve as user2, ignored since overridden on patch set 4
+ requestScopeOperations.setApiUser(user2.id());
+ recommend(changeId.toString());
+
+ // create a 4th patch set
+ requestScopeOperations.setApiUser(changeOwner);
+ amendChange(changeKey.get()).assertOkStatus();
+
+ // re- approve as user2
+ requestScopeOperations.setApiUser(user2.id());
+ recommend(changeId.toString());
+
+ // create a 5th patch set
+ requestScopeOperations.setApiUser(changeOwner);
+ amendChange(changeKey.get()).assertOkStatus();
+
+ assertThat(loadInput().previouslyApprovedPatchSetsInReverseOrder())
+ .containsExactly(PatchSet.id(changeId, 4), PatchSet.id(changeId, 2));
+ }
+
@Test
public void noImplicitApprover() {
assertThat(loadInput().implicitApprover()).isEmpty();
diff --git a/javatests/com/google/gerrit/plugins/codeowners/backend/CodeOwnerApprovalCheckWithStickyApprovalsTest.java b/javatests/com/google/gerrit/plugins/codeowners/backend/CodeOwnerApprovalCheckWithStickyApprovalsTest.java
new file mode 100644
index 0000000..fc73235
--- /dev/null
+++ b/javatests/com/google/gerrit/plugins/codeowners/backend/CodeOwnerApprovalCheckWithStickyApprovalsTest.java
@@ -0,0 +1,696 @@
+// Copyright (C) 2022 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.assertThatCollection;
+import static com.google.gerrit.server.group.SystemGroupBackend.REGISTERED_USERS;
+
+import com.google.common.collect.ImmutableMap;
+import com.google.common.collect.ImmutableSet;
+import com.google.gerrit.acceptance.PushOneCommit;
+import com.google.gerrit.acceptance.TestAccount;
+import com.google.gerrit.acceptance.config.GerritConfig;
+import com.google.gerrit.acceptance.testsuite.project.ProjectOperations;
+import com.google.gerrit.acceptance.testsuite.project.TestProjectUpdate;
+import com.google.gerrit.acceptance.testsuite.request.RequestScopeOperations;
+import com.google.gerrit.entities.Change;
+import com.google.gerrit.entities.RefNames;
+import com.google.gerrit.plugins.codeowners.acceptance.AbstractCodeOwnersTest;
+import com.google.gerrit.plugins.codeowners.acceptance.testsuite.CodeOwnerConfigOperations;
+import com.google.gerrit.plugins.codeowners.backend.config.GeneralConfig;
+import com.google.gerrit.plugins.codeowners.common.CodeOwnerStatus;
+import com.google.gerrit.plugins.codeowners.util.JgitPath;
+import com.google.gerrit.server.notedb.ChangeNotes;
+import com.google.gerrit.server.util.AccountTemplateUtil;
+import com.google.gerrit.testing.ConfigSuite;
+import com.google.inject.Inject;
+import java.nio.file.Path;
+import java.nio.file.Paths;
+import java.util.Map;
+import org.eclipse.jgit.lib.Config;
+import org.junit.Before;
+import org.junit.Test;
+
+/** Tests for {@link CodeOwnerApprovalCheck} with sticky approvals enabled. */
+public class CodeOwnerApprovalCheckWithStickyApprovalsTest extends AbstractCodeOwnersTest {
+ @Inject private ChangeNotes.Factory changeNotesFactory;
+ @Inject private RequestScopeOperations requestScopeOperations;
+ @Inject private ProjectOperations projectOperations;
+
+ private CodeOwnerApprovalCheck codeOwnerApprovalCheck;
+ private CodeOwnerConfigOperations codeOwnerConfigOperations;
+
+ /** Returns a {@code gerrit.config} that configures all users as fallback code owners. */
+ @ConfigSuite.Default
+ public static Config defaultConfig() {
+ Config cfg = new Config();
+ cfg.setBoolean(
+ "plugin", "code-owners", GeneralConfig.KEY_ENABLE_STICKY_APPROVALS, /* value= */ true);
+ return cfg;
+ }
+
+ @Before
+ public void setUpCodeOwnersPlugin() throws Exception {
+ codeOwnerApprovalCheck = plugin.getSysInjector().getInstance(CodeOwnerApprovalCheck.class);
+ codeOwnerConfigOperations =
+ plugin.getSysInjector().getInstance(CodeOwnerConfigOperations.class);
+ }
+
+ @Test
+ public void notApproved_noStickyApproval() throws Exception {
+ 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 is not approved.
+ ImmutableSet<FileCodeOwnerStatus> fileCodeOwnerStatuses = getFileCodeOwnerStatuses(changeId);
+ assertThatCollection(fileCodeOwnerStatuses)
+ .containsExactly(
+ FileCodeOwnerStatus.addition(path, CodeOwnerStatus.INSUFFICIENT_REVIEWERS));
+
+ // create a second patch set so that there is a previous patch set
+ amendChange(changeId, "Change Adding A File", JgitPath.of(path).get(), "new file content")
+ .assertOkStatus();
+
+ // Verify that the file is not approved.
+ fileCodeOwnerStatuses = getFileCodeOwnerStatuses(changeId);
+ assertThatCollection(fileCodeOwnerStatuses)
+ .containsExactly(
+ FileCodeOwnerStatus.addition(path, CodeOwnerStatus.INSUFFICIENT_REVIEWERS));
+ }
+
+ @Test
+ public void notApproved_byPreviousApprovalOfNonCodeOwner() throws Exception {
+ Path path = Paths.get("/foo/bar.baz");
+ String changeId =
+ createChange("Change Adding A File", JgitPath.of(path).get(), "file content").getChangeId();
+
+ // code owner approve as user who is not a code owner
+ recommend(changeId);
+
+ // create a second patch set so that the approval becomes an approval on a previous patch set
+ amendChange(changeId, "Change Adding A File", JgitPath.of(path).get(), "new file content")
+ .assertOkStatus();
+
+ // Verify that the file is not approved.
+ ImmutableSet<FileCodeOwnerStatus> fileCodeOwnerStatuses = getFileCodeOwnerStatuses(changeId);
+ assertThatCollection(fileCodeOwnerStatuses)
+ .containsExactly(
+ FileCodeOwnerStatus.addition(path, CodeOwnerStatus.INSUFFICIENT_REVIEWERS));
+ }
+
+ @Test
+ public void approved_byStickyApprovalOnPreviousPatchSet() throws Exception {
+ TestAccount codeOwner =
+ accountCreator.create(
+ "codeOwner", "codeOwner@example.com", "CodeOwner", /* displayName= */ null);
+ setAsRootCodeOwners(codeOwner);
+
+ Path path = Paths.get("/foo/bar.baz");
+ String changeId =
+ createChange("Change Adding A File", JgitPath.of(path).get(), "file content").getChangeId();
+
+ // code owner approve
+ requestScopeOperations.setApiUser(codeOwner.id());
+ recommend(changeId);
+
+ // create a second patch set so that the approval becomes an approval on a previous patch set
+ amendChange(changeId, "Change Adding A File", JgitPath.of(path).get(), "new file content")
+ .assertOkStatus();
+
+ // Verify that the file is approved.
+ ImmutableSet<FileCodeOwnerStatus> fileCodeOwnerStatuses = getFileCodeOwnerStatuses(changeId);
+ assertThatCollection(fileCodeOwnerStatuses)
+ .containsExactly(
+ FileCodeOwnerStatus.addition(
+ path,
+ CodeOwnerStatus.APPROVED,
+ String.format(
+ "approved on patch set 1 by %s who is a code owner",
+ AccountTemplateUtil.getAccountTemplate(codeOwner.id()))));
+ }
+
+ @Test
+ public void approved_byStickyApprovalOnOldPatchSet() throws Exception {
+ TestAccount codeOwner =
+ accountCreator.create(
+ "codeOwner", "codeOwner@example.com", "CodeOwner", /* displayName= */ null);
+ setAsRootCodeOwners(codeOwner);
+
+ Path path = Paths.get("/foo/bar.baz");
+ String changeId =
+ createChange("Change Adding A File", JgitPath.of(path).get(), "file content").getChangeId();
+
+ // code owner approve
+ requestScopeOperations.setApiUser(codeOwner.id());
+ recommend(changeId);
+
+ // create several new patch sets so that the approval becomes an approval on an old patch set
+ amendChange(changeId, "Change Adding A File", JgitPath.of(path).get(), "new file content")
+ .assertOkStatus();
+ amendChange(changeId, "Change Adding A File", JgitPath.of(path).get(), "newer file content")
+ .assertOkStatus();
+ amendChange(changeId, "Change Adding A File", JgitPath.of(path).get(), "newest file content")
+ .assertOkStatus();
+
+ // Verify that the file is approved.
+ ImmutableSet<FileCodeOwnerStatus> fileCodeOwnerStatuses = getFileCodeOwnerStatuses(changeId);
+ assertThatCollection(fileCodeOwnerStatuses)
+ .containsExactly(
+ FileCodeOwnerStatus.addition(
+ path,
+ CodeOwnerStatus.APPROVED,
+ String.format(
+ "approved on patch set 1 by %s who is a code owner",
+ AccountTemplateUtil.getAccountTemplate(codeOwner.id()))));
+ }
+
+ @Test
+ public void approved_byStickyApprovalsOfDifferentUsersOnDifferentPreviousPatchSets()
+ throws Exception {
+ TestAccount codeOwner1 =
+ accountCreator.create(
+ "codeOwner1", "codeOwner1@example.com", "CodeOwner1", /* displayName= */ null);
+ setAsCodeOwners("/foo/", codeOwner1);
+ TestAccount codeOwner2 =
+ accountCreator.create(
+ "codeOwner2", "codeOwner2@example.com", "CodeOwner2", /* displayName= */ null);
+ setAsCodeOwners("/bar/", codeOwner2);
+
+ Path path1 = Paths.get("/foo/bar.baz");
+ Path path2 = Paths.get("/bar/foo.baz");
+ String changeId =
+ createChange(
+ "Change Adding A File",
+ ImmutableMap.of(
+ JgitPath.of(path1).get(),
+ "file content",
+ JgitPath.of(path2).get(),
+ "file content"))
+ .getChangeId();
+
+ // code owner approve first path
+ requestScopeOperations.setApiUser(codeOwner1.id());
+ recommend(changeId);
+
+ // create a second patch set so that the approval becomes an approval on a previous patch set
+ amendChange(
+ changeId,
+ "Change Adding A File",
+ ImmutableMap.of(
+ JgitPath.of(path1).get(),
+ "new file content",
+ JgitPath.of(path2).get(),
+ "new file content"))
+ .assertOkStatus();
+
+ // Verify that the path1 is approved, but path2 isn't.
+ ImmutableSet<FileCodeOwnerStatus> fileCodeOwnerStatuses = getFileCodeOwnerStatuses(changeId);
+ assertThatCollection(fileCodeOwnerStatuses)
+ .containsExactly(
+ FileCodeOwnerStatus.addition(
+ path1,
+ CodeOwnerStatus.APPROVED,
+ String.format(
+ "approved on patch set 1 by %s who is a code owner",
+ AccountTemplateUtil.getAccountTemplate(codeOwner1.id()))),
+ FileCodeOwnerStatus.addition(path2, CodeOwnerStatus.INSUFFICIENT_REVIEWERS));
+
+ // code owner approve second path
+ requestScopeOperations.setApiUser(codeOwner2.id());
+ recommend(changeId);
+
+ // create another patch set so that the second approval becomes an approval on a previous patch
+ // set
+ amendChange(
+ changeId,
+ "Change Adding A File",
+ ImmutableMap.of(
+ JgitPath.of(path1).get(),
+ "newer file content",
+ JgitPath.of(path2).get(),
+ "newer file content"))
+ .assertOkStatus();
+
+ // Verify that both paths approved now.
+ fileCodeOwnerStatuses = getFileCodeOwnerStatuses(changeId);
+ assertThatCollection(fileCodeOwnerStatuses)
+ .containsExactly(
+ FileCodeOwnerStatus.addition(
+ path1,
+ CodeOwnerStatus.APPROVED,
+ String.format(
+ "approved on patch set 1 by %s who is a code owner",
+ AccountTemplateUtil.getAccountTemplate(codeOwner1.id()))),
+ FileCodeOwnerStatus.addition(
+ path2,
+ CodeOwnerStatus.APPROVED,
+ String.format(
+ "approved on patch set 2 by %s who is a code owner",
+ AccountTemplateUtil.getAccountTemplate(codeOwner2.id()))));
+ }
+
+ @Test
+ public void notApproved_byPreviousApprovalThatHasBeenDeleted() throws Exception {
+ TestAccount codeOwner =
+ accountCreator.create(
+ "codeOwner", "codeOwner@example.com", "CodeOwner", /* displayName= */ null);
+ setAsRootCodeOwners(codeOwner);
+
+ Path path = Paths.get("/foo/bar.baz");
+ String changeId =
+ createChange("Change Adding A File", JgitPath.of(path).get(), "file content").getChangeId();
+
+ // code owner approve
+ requestScopeOperations.setApiUser(codeOwner.id());
+ recommend(changeId);
+
+ // delete the approval
+ adminRestSession
+ .delete(
+ "/changes/"
+ + changeId
+ + "/reviewers/"
+ + codeOwner.id().toString()
+ + "/votes/Code-Review")
+ .assertNoContent();
+
+ // create a second patch set so that the deleted approval becomes an approval on a previous
+ // patch set
+ amendChange(changeId, "Change Adding A File", JgitPath.of(path).get(), "new file content")
+ .assertOkStatus();
+
+ // Verify that the file is not approved. The expected status is PENDING since the code owner is
+ // a reviewer now.
+ ImmutableSet<FileCodeOwnerStatus> fileCodeOwnerStatuses = getFileCodeOwnerStatuses(changeId);
+ assertThatCollection(fileCodeOwnerStatuses)
+ .containsExactly(
+ FileCodeOwnerStatus.addition(
+ path,
+ CodeOwnerStatus.PENDING,
+ String.format(
+ "reviewer %s is a code owner",
+ AccountTemplateUtil.getAccountTemplate(codeOwner.id()))));
+ }
+
+ @Test
+ @GerritConfig(name = "plugin.code-owners.requiredApproval", value = "Code-Review+2")
+ public void notApproved_byPreviousApprovalThatHasBeenDowngraded() throws Exception {
+ // Allow all users to vote with Code-Review+2.
+ projectOperations
+ .project(project)
+ .forUpdate()
+ .add(
+ TestProjectUpdate.allowLabel("Code-Review")
+ .range(0, 2)
+ .ref("refs/heads/*")
+ .group(REGISTERED_USERS)
+ .build())
+ .update();
+
+ TestAccount codeOwner =
+ accountCreator.create(
+ "codeOwner", "codeOwner@example.com", "CodeOwner", /* displayName= */ null);
+ setAsRootCodeOwners(codeOwner);
+
+ Path path = Paths.get("/foo/bar.baz");
+ String changeId =
+ createChange("Change Adding A File", JgitPath.of(path).get(), "file content").getChangeId();
+
+ // code owner approve
+ requestScopeOperations.setApiUser(codeOwner.id());
+ approve(changeId);
+
+ // create a second patch set so that the deleted approval becomes an approval on a previous
+ // patch set
+ amendChange(changeId, "Change Adding A File", JgitPath.of(path).get(), "new file content")
+ .assertOkStatus();
+
+ // downgrade approval
+ recommend(changeId);
+
+ // Verify that the file is not approved. The expected status is PENDING since the code owner is
+ // a reviewer now.
+ ImmutableSet<FileCodeOwnerStatus> fileCodeOwnerStatuses = getFileCodeOwnerStatuses(changeId);
+ assertThatCollection(fileCodeOwnerStatuses)
+ .containsExactly(
+ FileCodeOwnerStatus.addition(
+ path,
+ CodeOwnerStatus.PENDING,
+ String.format(
+ "reviewer %s is a code owner",
+ AccountTemplateUtil.getAccountTemplate(codeOwner.id()))));
+
+ // create another patch set so that the downgraded approval becomes an approval on a previous
+ // patch set
+ amendChange(changeId, "Change Adding A File", JgitPath.of(path).get(), "newer file content")
+ .assertOkStatus();
+
+ fileCodeOwnerStatuses = getFileCodeOwnerStatuses(changeId);
+ assertThatCollection(fileCodeOwnerStatuses)
+ .containsExactly(
+ FileCodeOwnerStatus.addition(
+ path,
+ CodeOwnerStatus.PENDING,
+ String.format(
+ "reviewer %s is a code owner",
+ AccountTemplateUtil.getAccountTemplate(codeOwner.id()))));
+ }
+
+ @Test
+ public void approved_reapprovalTrumpsPreviousApproval() throws Exception {
+ TestAccount codeOwner =
+ accountCreator.create(
+ "codeOwner", "codeOwner@example.com", "CodeOwner", /* displayName= */ null);
+ setAsRootCodeOwners(codeOwner);
+
+ Path path = Paths.get("/foo/bar.baz");
+ String changeId =
+ createChange("Change Adding A File", JgitPath.of(path).get(), "file content").getChangeId();
+
+ // code owner approve
+ requestScopeOperations.setApiUser(codeOwner.id());
+ recommend(changeId);
+
+ // create a second patch set so that the deleted approval becomes an approval on a previous
+ // patch set
+ amendChange(changeId, "Change Adding A File", JgitPath.of(path).get(), "new file content")
+ .assertOkStatus();
+
+ // re-approve
+ recommend(changeId);
+
+ // Verify that the file is approved by the current approval.
+ ImmutableSet<FileCodeOwnerStatus> fileCodeOwnerStatuses = getFileCodeOwnerStatuses(changeId);
+ assertThatCollection(fileCodeOwnerStatuses)
+ .containsExactly(
+ FileCodeOwnerStatus.addition(
+ path,
+ CodeOwnerStatus.APPROVED,
+ String.format(
+ "approved by %s who is a code owner",
+ AccountTemplateUtil.getAccountTemplate(codeOwner.id()))));
+
+ // create another patch set so that the re-approval becomes an approval on a previous patch set
+ amendChange(changeId, "Change Adding A File", JgitPath.of(path).get(), "newer file content")
+ .assertOkStatus();
+
+ fileCodeOwnerStatuses = getFileCodeOwnerStatuses(changeId);
+ assertThatCollection(fileCodeOwnerStatuses)
+ .containsExactly(
+ FileCodeOwnerStatus.addition(
+ path,
+ CodeOwnerStatus.APPROVED,
+ String.format(
+ "approved on patch set 2 by %s who is a code owner",
+ AccountTemplateUtil.getAccountTemplate(codeOwner.id()))));
+ }
+
+ @Test
+ @GerritConfig(name = "plugin.code-owners.enableImplicitApprovals", value = "true")
+ public void approved_implicitApprovalTrumpsPreviousApproval() throws Exception {
+ TestAccount implicitCodeOwner = admin; // the changes is created by the admit user
+ setAsRootCodeOwners(implicitCodeOwner);
+
+ Path path = Paths.get("/foo/bar.baz");
+ String changeId =
+ createChange("Change Adding A File", JgitPath.of(path).get(), "file content").getChangeId();
+
+ // code owner approve
+ requestScopeOperations.setApiUser(implicitCodeOwner.id());
+ recommend(changeId);
+
+ // create a second patch set so that the deleted approval becomes an approval on a previous
+ // patch set
+ amendChange(changeId, "Change Adding A File", JgitPath.of(path).get(), "new file content")
+ .assertOkStatus();
+
+ // Verify that the file is approved by the current implicit approval.
+ ImmutableSet<FileCodeOwnerStatus> fileCodeOwnerStatuses = getFileCodeOwnerStatuses(changeId);
+ assertThatCollection(fileCodeOwnerStatuses)
+ .containsExactly(
+ FileCodeOwnerStatus.addition(
+ path,
+ CodeOwnerStatus.APPROVED,
+ String.format(
+ "implicitly approved by the patch set uploader %s who is a code owner",
+ AccountTemplateUtil.getAccountTemplate(implicitCodeOwner.id()))));
+ }
+
+ @Test
+ public void notApproved_fileThatIsNotPresentInApprovedPatchSetIsNotCoveredByTheApproval()
+ throws Exception {
+ TestAccount codeOwner =
+ accountCreator.create(
+ "codeOwner", "codeOwner@example.com", "CodeOwner", /* displayName= */ null);
+ setAsRootCodeOwners(codeOwner);
+
+ Path path = Paths.get("/foo/bar.baz");
+ String changeId =
+ createChange("Change Adding A File", JgitPath.of(path).get(), "file content").getChangeId();
+
+ // code owner approve
+ requestScopeOperations.setApiUser(codeOwner.id());
+ recommend(changeId);
+
+ // create a second patch set that adds a new file
+ Path path2 = Paths.get("/foo/abc.xyz");
+ amendChange(
+ changeId,
+ "Change Adding A File",
+ ImmutableMap.of(
+ JgitPath.of(path).get(),
+ "new file content",
+ JgitPath.of(path2).get(),
+ "file content"))
+ .assertOkStatus();
+
+ // Verify that the new file is not approved. The expected status is PENDING since the code owner
+ // is a reviewer.
+ ImmutableSet<FileCodeOwnerStatus> fileCodeOwnerStatuses = getFileCodeOwnerStatuses(changeId);
+ assertThatCollection(fileCodeOwnerStatuses)
+ .containsExactly(
+ FileCodeOwnerStatus.addition(
+ path,
+ CodeOwnerStatus.APPROVED,
+ String.format(
+ "approved on patch set 1 by %s who is a code owner",
+ AccountTemplateUtil.getAccountTemplate(codeOwner.id()))),
+ FileCodeOwnerStatus.addition(
+ path2,
+ CodeOwnerStatus.PENDING,
+ String.format(
+ "reviewer %s is a code owner",
+ AccountTemplateUtil.getAccountTemplate(codeOwner.id()))));
+
+ // re-approve to cover all files
+ recommend(changeId);
+
+ fileCodeOwnerStatuses = getFileCodeOwnerStatuses(changeId);
+ assertThatCollection(fileCodeOwnerStatuses)
+ .containsExactly(
+ FileCodeOwnerStatus.addition(
+ path,
+ CodeOwnerStatus.APPROVED,
+ String.format(
+ "approved by %s who is a code owner",
+ AccountTemplateUtil.getAccountTemplate(codeOwner.id()))),
+ FileCodeOwnerStatus.addition(
+ path2,
+ CodeOwnerStatus.APPROVED,
+ String.format(
+ "approved by %s who is a code owner",
+ AccountTemplateUtil.getAccountTemplate(codeOwner.id()))));
+ }
+
+ @Test
+ public void approved_byStickyApprovalOnPreviousPatchSet_everyoneIsCodeOwner() throws Exception {
+ // Create a code owner config file that makes everyone a code owner.
+ codeOwnerConfigOperations
+ .newCodeOwnerConfig()
+ .project(project)
+ .branch("master")
+ .folderPath("/")
+ .addCodeOwnerEmail("*")
+ .create();
+
+ Path path = Paths.get("/foo/bar.baz");
+ String changeId =
+ createChange("Change Adding A File", JgitPath.of(path).get(), "file content").getChangeId();
+
+ // code owner approve
+ requestScopeOperations.setApiUser(user.id());
+ recommend(changeId);
+
+ // create a second patch set so that the approval becomes an approval on a previous patch set
+ amendChange(changeId, "Change Adding A File", JgitPath.of(path).get(), "new file content")
+ .assertOkStatus();
+
+ // Verify that the file is approved.
+ ImmutableSet<FileCodeOwnerStatus> fileCodeOwnerStatuses = getFileCodeOwnerStatuses(changeId);
+ assertThatCollection(fileCodeOwnerStatuses)
+ .containsExactly(
+ FileCodeOwnerStatus.addition(
+ path,
+ CodeOwnerStatus.APPROVED,
+ String.format(
+ "approved on patch set 1 by %s who is a code owner (all users are code owners)",
+ AccountTemplateUtil.getAccountTemplate(user.id()))));
+ }
+
+ @Test
+ public void approved_byStickyApprovalOfDefaultCodeOnPreviousPatchSet() throws Exception {
+ TestAccount codeOwner =
+ accountCreator.create(
+ "codeOwner", "codeOwner@example.com", "CodeOwner", /* displayName= */ null);
+ setAsDefaultCodeOwners(codeOwner);
+
+ Path path = Paths.get("/foo/bar.baz");
+ String changeId =
+ createChange("Change Adding A File", JgitPath.of(path).get(), "file content").getChangeId();
+
+ // code owner approve
+ requestScopeOperations.setApiUser(codeOwner.id());
+ recommend(changeId);
+
+ // create a second patch set so that the approval becomes an approval on a previous patch set
+ amendChange(changeId, "Change Adding A File", JgitPath.of(path).get(), "new file content")
+ .assertOkStatus();
+
+ // Verify that the file is approved.
+ ImmutableSet<FileCodeOwnerStatus> fileCodeOwnerStatuses = getFileCodeOwnerStatuses(changeId);
+ assertThatCollection(fileCodeOwnerStatuses)
+ .containsExactly(
+ FileCodeOwnerStatus.addition(
+ path,
+ CodeOwnerStatus.APPROVED,
+ String.format(
+ "approved on patch set 1 by %s who is a default code owner",
+ AccountTemplateUtil.getAccountTemplate(codeOwner.id()))));
+ }
+
+ @Test
+ public void approved_byStickyApprovalOfDefaultCodeOnPreviousPatchSet_everyoneIsDefaultCodeOwner()
+ throws Exception {
+ // Create a code owner config file that makes everyone a default code owner.
+ codeOwnerConfigOperations
+ .newCodeOwnerConfig()
+ .project(project)
+ .branch(RefNames.REFS_CONFIG)
+ .folderPath("/")
+ .addCodeOwnerEmail("*")
+ .create();
+
+ Path path = Paths.get("/foo/bar.baz");
+ String changeId =
+ createChange("Change Adding A File", JgitPath.of(path).get(), "file content").getChangeId();
+
+ // code owner approve
+ requestScopeOperations.setApiUser(user.id());
+ recommend(changeId);
+
+ // create a second patch set so that the approval becomes an approval on a previous patch set
+ amendChange(changeId, "Change Adding A File", JgitPath.of(path).get(), "new file content")
+ .assertOkStatus();
+
+ // Verify that the file is approved.
+ ImmutableSet<FileCodeOwnerStatus> fileCodeOwnerStatuses = getFileCodeOwnerStatuses(changeId);
+ assertThatCollection(fileCodeOwnerStatuses)
+ .containsExactly(
+ FileCodeOwnerStatus.addition(
+ path,
+ CodeOwnerStatus.APPROVED,
+ String.format(
+ "approved on patch set 1 by %s who is a default code owner"
+ + " (all users are default code owners)",
+ AccountTemplateUtil.getAccountTemplate(user.id()))));
+ }
+
+ @Test
+ @GerritConfig(name = "plugin.code-owners.globalCodeOwner", value = "bot@example.com")
+ public void approved_byStickyApprovalOfGlobalCodeOnPreviousPatchSet() throws Exception {
+ // Create a bot user that is a global code owner.
+ TestAccount bot =
+ accountCreator.create("bot", "bot@example.com", "Bot", /* displayName= */ null);
+
+ Path path = Paths.get("/foo/bar.baz");
+ String changeId =
+ createChange("Change Adding A File", JgitPath.of(path).get(), "file content").getChangeId();
+
+ // code owner approve
+ requestScopeOperations.setApiUser(bot.id());
+ recommend(changeId);
+
+ // create a second patch set so that the approval becomes an approval on a previous patch set
+ amendChange(changeId, "Change Adding A File", JgitPath.of(path).get(), "new file content")
+ .assertOkStatus();
+
+ // Verify that the file is approved.
+ ImmutableSet<FileCodeOwnerStatus> fileCodeOwnerStatuses = getFileCodeOwnerStatuses(changeId);
+ assertThatCollection(fileCodeOwnerStatuses)
+ .containsExactly(
+ FileCodeOwnerStatus.addition(
+ path,
+ CodeOwnerStatus.APPROVED,
+ String.format(
+ "approved on patch set 1 by %s who is a global code owner",
+ AccountTemplateUtil.getAccountTemplate(bot.id()))));
+ }
+
+ @Test
+ @GerritConfig(name = "plugin.code-owners.globalCodeOwner", value = "*")
+ public void approved_byStickyApprovalOfGlobalCodeOnPreviousPatchSet_everyoneIsGlobalCodeOwner()
+ throws Exception {
+ Path path = Paths.get("/foo/bar.baz");
+ String changeId =
+ createChange("Change Adding A File", JgitPath.of(path).get(), "file content").getChangeId();
+
+ // code owner approve
+ requestScopeOperations.setApiUser(user.id());
+ recommend(changeId);
+
+ // create a second patch set so that the approval becomes an approval on a previous patch set
+ amendChange(changeId, "Change Adding A File", JgitPath.of(path).get(), "new file content")
+ .assertOkStatus();
+
+ // Verify that the file is approved.
+ ImmutableSet<FileCodeOwnerStatus> fileCodeOwnerStatuses = getFileCodeOwnerStatuses(changeId);
+ assertThatCollection(fileCodeOwnerStatuses)
+ .containsExactly(
+ FileCodeOwnerStatus.addition(
+ path,
+ CodeOwnerStatus.APPROVED,
+ String.format(
+ "approved on patch set 1 by %s who is a global code owner"
+ + " (all users are global code owners)",
+ AccountTemplateUtil.getAccountTemplate(user.id()))));
+ }
+
+ private ImmutableSet<FileCodeOwnerStatus> getFileCodeOwnerStatuses(String changeId)
+ throws Exception {
+ return codeOwnerApprovalCheck.getFileStatusesAsSet(
+ getChangeNotes(changeId), /* start= */ 0, /* limit= */ 0);
+ }
+
+ private ChangeNotes getChangeNotes(String changeId) throws Exception {
+ return changeNotesFactory.create(project, Change.id(gApi.changes().id(changeId).get()._number));
+ }
+
+ private PushOneCommit.Result amendChange(
+ String changeId, String subject, Map<String, String> files) throws Exception {
+ PushOneCommit push = pushFactory.create(admin.newIdent(), testRepo, subject, files, changeId);
+ return push.to("refs/for/master");
+ }
+}
diff --git a/javatests/com/google/gerrit/plugins/codeowners/backend/config/GeneralConfigTest.java b/javatests/com/google/gerrit/plugins/codeowners/backend/config/GeneralConfigTest.java
index 86801cc..970aa74 100644
--- a/javatests/com/google/gerrit/plugins/codeowners/backend/config/GeneralConfigTest.java
+++ b/javatests/com/google/gerrit/plugins/codeowners/backend/config/GeneralConfigTest.java
@@ -20,6 +20,7 @@
import static com.google.gerrit.plugins.codeowners.backend.config.GeneralConfig.KEY_ENABLE_ASYNC_MESSAGE_ON_ADD_REVIEWER;
import static com.google.gerrit.plugins.codeowners.backend.config.GeneralConfig.KEY_ENABLE_CODE_OWNER_CONFIG_FILES_WITH_FILE_EXTENSIONS;
import static com.google.gerrit.plugins.codeowners.backend.config.GeneralConfig.KEY_ENABLE_IMPLICIT_APPROVALS;
+import static com.google.gerrit.plugins.codeowners.backend.config.GeneralConfig.KEY_ENABLE_STICKY_APPROVALS;
import static com.google.gerrit.plugins.codeowners.backend.config.GeneralConfig.KEY_ENABLE_VALIDATION_ON_BRANCH_CREATION;
import static com.google.gerrit.plugins.codeowners.backend.config.GeneralConfig.KEY_ENABLE_VALIDATION_ON_COMMIT_RECEIVED;
import static com.google.gerrit.plugins.codeowners.backend.config.GeneralConfig.KEY_ENABLE_VALIDATION_ON_SUBMIT;
@@ -1642,6 +1643,63 @@
}
@Test
+ public void cannotGetEnableStickyApprovalsForNullProject() throws Exception {
+ NullPointerException npe =
+ assertThrows(
+ NullPointerException.class,
+ () -> generalConfig.enableStickyApprovals(/* project= */ null, new Config()));
+ assertThat(npe).hasMessageThat().isEqualTo("project");
+ }
+
+ @Test
+ public void cannotGetEnableStickyApprovalsForNullPluginConfig() throws Exception {
+ NullPointerException npe =
+ assertThrows(
+ NullPointerException.class,
+ () -> generalConfig.enableStickyApprovals(project, /* pluginConfig= */ null));
+ assertThat(npe).hasMessageThat().isEqualTo("pluginConfig");
+ }
+
+ @Test
+ public void noEnableStickyApprovals() throws Exception {
+ assertThat(generalConfig.enableStickyApprovals(project, new Config())).isFalse();
+ }
+
+ @Test
+ @GerritConfig(name = "plugin.code-owners.enableStickyApprovals", value = "true")
+ public void
+ enableStickyApprovalsConfigurationIsRetrievedFromGerritConfigIfNotSpecifiedOnProjectLevel()
+ throws Exception {
+ assertThat(generalConfig.enableStickyApprovals(project, new Config())).isTrue();
+ }
+
+ @Test
+ @GerritConfig(name = "plugin.code-owners.enableStickyApprovals", value = "true")
+ public void
+ enableStickyApprovalsConfigurationInPluginConfigOverridesEnableStickyApprovalsConfigurationInGerritConfig()
+ throws Exception {
+ Config cfg = new Config();
+ cfg.setString(
+ SECTION_CODE_OWNERS, /* subsection= */ null, KEY_ENABLE_STICKY_APPROVALS, "false");
+ assertThat(generalConfig.enableStickyApprovals(project, cfg)).isFalse();
+ }
+
+ @Test
+ @GerritConfig(name = "plugin.code-owners.enableStickyApprovals", value = "true")
+ public void invalidEnableStickyApprovalsConfigurationInPluginConfigIsIgnored() throws Exception {
+ Config cfg = new Config();
+ cfg.setString(
+ SECTION_CODE_OWNERS, /* subsection= */ null, KEY_ENABLE_STICKY_APPROVALS, "INVALID");
+ assertThat(generalConfig.enableStickyApprovals(project, cfg)).isTrue();
+ }
+
+ @Test
+ @GerritConfig(name = "plugin.code-owners.enableStickyApprovals", value = "INVALID")
+ public void invalidEnableStickyApprovalsConfigurationInGerritConfigIsIgnored() throws Exception {
+ assertThat(generalConfig.enableStickyApprovals(project, new Config())).isFalse();
+ }
+
+ @Test
public void cannotGetGlobalCodeOwnersForNullPluginConfig() throws Exception {
NullPointerException npe =
assertThrows(
diff --git a/resources/Documentation/config-guide.md b/resources/Documentation/config-guide.md
index 2a20478..3eaefca 100644
--- a/resources/Documentation/config-guide.md
+++ b/resources/Documentation/config-guide.md
@@ -30,6 +30,9 @@
approval](config.html#pluginCodeOwnersRequiredApproval) and [override
approval](config.html#pluginCodeOwnersOverrideApproval).
+Alternatively code owner approvals can be made sticky per file by [enabling
+sticky approvals in the plugin configuration](config.html#pluginCodeOwnersEnableStickyApprovals).
+
### <a id="implicitApprovals">Implicit code owner approvals
It's possible to [enable implicit approvals](config.html#pluginCodeOwnersEnableImplicitApprovals)
diff --git a/resources/Documentation/config.md b/resources/Documentation/config.md
index b4ea424..80c5b59 100644
--- a/resources/Documentation/config.md
+++ b/resources/Documentation/config.md
@@ -261,6 +261,66 @@
in `@PLUGIN@.config`.\
By default `FALSE`.
+<a id="pluginCodeOwnersEnableStickyApprovals">plugin.@PLUGIN@.enableStickyApprovals</a>
+: Whether code owner approvals should be sticky on the files they approve,
+ even if these files are changed in follow-up patch sets.\
+ \
+ With this setting 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](../../../Documentation/config-labels.html#label_copyCondition)
+ 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](how-to-use.html#perFileCodeOwnerStatuses) to know which files
+ are code owner approved.\
+ \
+ 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.\
+ \
+ Enabling sticky code owner approvals on file level can improve the
+ overall developer productivity significantly, since it makes
+ re-approvals on new patch sets unecessary in many cases. With sticky
+ code owner approvals on files new patch sets only need to be re-approved
+ if they touch additional files, and then only by the users that own
+ these files. In contrast to this, if code owner approvals are not
+ sticky (neither on file level via this setting, nor on change level via
+ a copy condition) a new patch set must always be re-approved by all code
+ owners that approved any of the contained files, no matter if these
+ files are touched in the new patch set.\
+ \
+ Can be overridden per project by setting
+ [codeOwners.enableStickyApprovals](#codeOwnersEnableStickyApprovals)
+ in `@PLUGIN@.config`.\
+ \
+ By default `false`.
+
<a id="pluginCodeOwnersGlobalCodeOwner">plugin.@PLUGIN@.globalCodeOwner</a>
: The email of a user that should be a code owner globally across all
branches.\
@@ -854,11 +914,24 @@
If implicit code owner approvals are disabled, code owners can still
self-approve their own changes by voting on the change.\
Overrides the global setting
- [plugin.@PLUGIN@.enableImplicitApprovals](#pluginCodeOwnersenableImplicitApprovals)
+ [plugin.@PLUGIN@.enableImplicitApprovals](#pluginCodeOwnersEnableImplicitApprovals)
in `gerrit.config` and the `codeOwners.enableImplicitApprovals` setting
from parent projects.\
If not set, the global setting
- [plugin.@PLUGIN@.enableImplicitApprovals](#pluginCodeOwnersenableImplicitApprovals)
+ [plugin.@PLUGIN@.enableImplicitApprovals](#pluginCodeOwnersEnableImplicitApprovals)
+ in `gerrit.config` is used.
+
+<a id="codeOwnersEnableStickyApprovals">codeOwners.enableStickyApprovals</a>
+: Whether code owner approvals should be sticky on the files they approve,
+ even if these files are changed in follow-up patch sets.\
+ For details see
+ [plugin.@PLUGIN@.enableStickyApprovals](#pluginCodeOwnersEnableStickyApprovals).\
+ Overrides the global setting
+ [plugin.@PLUGIN@.enableStickyApprovals](#pluginCodeOwnersEnableStickyApprovals)
+ in `gerrit.config` and the `codeOwners.enableStickyApprovals` setting
+ from parent projects.\
+ If not set, the global setting
+ [plugin.@PLUGIN@.enableStickyApprovals](#pluginCodeOwnersEnableStickyApprovals)
in `gerrit.config` is used.
<a id="codeOwnersGlobalCodeOwner">codeOwners.globalCodeOwner</a>
diff --git a/resources/Documentation/how-to-use.md b/resources/Documentation/how-to-use.md
index 552f675..1635034 100644
--- a/resources/Documentation/how-to-use.md
+++ b/resources/Documentation/how-to-use.md
@@ -191,7 +191,7 @@
\
-### <a id="perFilCodeOwnerStatuses">Per file code owner statuses
+### <a id="perFileCodeOwnerStatuses">Per file code owner statuses
The `@PLUGIN@` plugin also shows the code owner statuses per file in the file
list.
diff --git a/resources/Documentation/metrics.md b/resources/Documentation/metrics.md
index 7c15669..b5f7765 100644
--- a/resources/Documentation/metrics.md
+++ b/resources/Documentation/metrics.md
@@ -16,6 +16,8 @@
Latency for computing the file status for one file.
* `compute_file_statuses`:
Latency for computing file statuses for all files in a change.
+ * `sticky_approvals`:
+ Whether sticky approvals on file level are enabled.
* `compute_owned_paths`:
Latency for computing file statuses.
* `compute_owned_paths`:
