Allow to enforce implicit approvals even if self approvals are disallowed
At the moment implicit approvals are disabled if the configured required
label disallows self approvals. Add a new option for the
enableImplicitApprovals configuration setting that allows to enable
implicit approvals even if the configured required label disallows self
approvals.
Signed-off-by: Edwin Kempin <ekempin@google.com>
Change-Id: I723097069467ee16f62d7c75d5a4fb4be16e487b
diff --git a/java/com/google/gerrit/plugins/codeowners/backend/EnableImplicitApprovals.java b/java/com/google/gerrit/plugins/codeowners/backend/EnableImplicitApprovals.java
new file mode 100644
index 0000000..bcad5e8
--- /dev/null
+++ b/java/com/google/gerrit/plugins/codeowners/backend/EnableImplicitApprovals.java
@@ -0,0 +1,33 @@
+// Copyright (C) 2021 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;
+
+/** Enum to control whether implicit code-owner approvals by the patch set uploader are enabled. */
+public enum EnableImplicitApprovals {
+ /** Implicit code-owner approvals of the the patch set uploader are disabled. */
+ FALSE,
+
+ /**
+ * Implicit code-owner approvals of the patch set uploader are enabled, but only if the configured
+ * required label allows self approvals.
+ */
+ TRUE,
+
+ /**
+ * Implicit code-owner approvals of the patch set uploader are enabled, even if the configured
+ * required label disallows self approvals.
+ */
+ FORCED;
+}
diff --git a/java/com/google/gerrit/plugins/codeowners/backend/config/CodeOwnersPluginConfiguration.java b/java/com/google/gerrit/plugins/codeowners/backend/config/CodeOwnersPluginConfiguration.java
index 4b70846..cc62a94 100644
--- a/java/com/google/gerrit/plugins/codeowners/backend/config/CodeOwnersPluginConfiguration.java
+++ b/java/com/google/gerrit/plugins/codeowners/backend/config/CodeOwnersPluginConfiguration.java
@@ -29,6 +29,7 @@
import com.google.gerrit.extensions.restapi.MethodNotAllowedException;
import com.google.gerrit.plugins.codeowners.backend.CodeOwnerBackend;
import com.google.gerrit.plugins.codeowners.backend.CodeOwnerReference;
+import com.google.gerrit.plugins.codeowners.backend.EnableImplicitApprovals;
import com.google.gerrit.plugins.codeowners.backend.FallbackCodeOwners;
import com.google.gerrit.plugins.codeowners.common.CodeOwnerConfigValidationPolicy;
import com.google.gerrit.plugins.codeowners.common.MergeCommitStrategy;
@@ -213,15 +214,30 @@
*/
public boolean areImplicitApprovalsEnabled(Project.NameKey project) {
requireNonNull(project, "project");
- LabelType requiredLabel = getRequiredApproval(project).labelType();
- if (requiredLabel.isIgnoreSelfApproval()) {
- logger.atFine().log(
- "ignoring implicit approval configuration on project %s since the label of the required"
- + " approval (%s) is configured to ignore self approvals",
- project, requiredLabel);
- return false;
+ EnableImplicitApprovals enableImplicitApprovals =
+ generalConfig.getEnableImplicitApprovals(project, getPluginConfig(project));
+ switch (enableImplicitApprovals) {
+ case FALSE:
+ logger.atFine().log("implicit approvals on project %s are disabled", project);
+ return false;
+ case TRUE:
+ LabelType requiredLabel = getRequiredApproval(project).labelType();
+ if (requiredLabel.isIgnoreSelfApproval()) {
+ logger.atFine().log(
+ "ignoring implicit approval configuration on project %s since the label of the required"
+ + " approval (%s) is configured to ignore self approvals",
+ project, requiredLabel);
+ return false;
+ }
+ return true;
+ case FORCED:
+ logger.atFine().log("implicit approvals on project %s are enforced", project);
+ return true;
}
- return generalConfig.getEnableImplicitApprovals(getPluginConfig(project));
+ throw new IllegalStateException(
+ String.format(
+ "unknown value %s for enableImplicitApprovals configuration in project %s",
+ enableImplicitApprovals, project));
}
/**
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 3e5538c..187a7ab 100644
--- a/java/com/google/gerrit/plugins/codeowners/backend/config/GeneralConfig.java
+++ b/java/com/google/gerrit/plugins/codeowners/backend/config/GeneralConfig.java
@@ -26,6 +26,7 @@
import com.google.gerrit.entities.Project;
import com.google.gerrit.extensions.annotations.PluginName;
import com.google.gerrit.plugins.codeowners.backend.CodeOwnerReference;
+import com.google.gerrit.plugins.codeowners.backend.EnableImplicitApprovals;
import com.google.gerrit.plugins.codeowners.backend.FallbackCodeOwners;
import com.google.gerrit.plugins.codeowners.common.CodeOwnerConfigValidationPolicy;
import com.google.gerrit.plugins.codeowners.common.MergeCommitStrategy;
@@ -463,17 +464,44 @@
* Gets whether an implicit code owner approval from the last uploader is assumed from the given
* plugin config with fallback to {@code gerrit.config}.
*
+ * @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 an implicit code owner approval from the last uploader is assumed
*/
- boolean getEnableImplicitApprovals(Config pluginConfig) {
+ EnableImplicitApprovals getEnableImplicitApprovals(Project.NameKey project, Config pluginConfig) {
+ requireNonNull(project, "project");
requireNonNull(pluginConfig, "pluginConfig");
- return pluginConfig.getBoolean(
- SECTION_CODE_OWNERS,
- null,
- KEY_ENABLE_IMPLICIT_APPROVALS,
- pluginConfigFromGerritConfig.getBoolean(KEY_ENABLE_IMPLICIT_APPROVALS, false));
+ String enableImplicitApprovalsString =
+ pluginConfig.getString(SECTION_CODE_OWNERS, null, KEY_ENABLE_IMPLICIT_APPROVALS);
+ if (enableImplicitApprovalsString != null) {
+ try {
+ return pluginConfig.getEnum(
+ SECTION_CODE_OWNERS,
+ null,
+ KEY_ENABLE_IMPLICIT_APPROVALS,
+ EnableImplicitApprovals.FALSE);
+ } catch (IllegalArgumentException e) {
+ logger.atWarning().withCause(e).log(
+ "Ignoring invalid value %s for enabling implicit approvals in '%s.config' of project"
+ + " %s. Falling back to global config or default value.",
+ enableImplicitApprovalsString, pluginName, project.get());
+ }
+ }
+
+ try {
+ return pluginConfigFromGerritConfig.getEnum(
+ KEY_ENABLE_IMPLICIT_APPROVALS, EnableImplicitApprovals.FALSE);
+ } catch (IllegalArgumentException e) {
+ logger.atWarning().withCause(e).log(
+ "Ignoring invalid value %s for enabling implict approvals in gerrit.config (parameter"
+ + " plugin.%s.%s). Falling back to default value %s.",
+ pluginConfigFromGerritConfig.getString(KEY_ENABLE_IMPLICIT_APPROVALS),
+ pluginName,
+ KEY_ENABLE_IMPLICIT_APPROVALS,
+ EnableImplicitApprovals.FALSE);
+ return EnableImplicitApprovals.FALSE;
+ }
}
/**
diff --git a/javatests/com/google/gerrit/plugins/codeowners/backend/CodeOwnerApprovalCheckWithSelfApprovalsIgnoredTest.java b/javatests/com/google/gerrit/plugins/codeowners/backend/CodeOwnerApprovalCheckWithSelfApprovalsIgnoredTest.java
index 3195389..3112b07 100644
--- a/javatests/com/google/gerrit/plugins/codeowners/backend/CodeOwnerApprovalCheckWithSelfApprovalsIgnoredTest.java
+++ b/javatests/com/google/gerrit/plugins/codeowners/backend/CodeOwnerApprovalCheckWithSelfApprovalsIgnoredTest.java
@@ -273,6 +273,65 @@
}
@Test
+ @GerritConfig(name = "plugin.code-owners.enableImplicitApprovals", value = "forced")
+ public void implicitlyApprovedByUploaderWhoIsChangeOwner() 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(codeOwner, "Change Adding A File", JgitPath.of(path).get(), "file content")
+ .getChangeId();
+
+ // Verify that the file is approved.
+ Stream<FileCodeOwnerStatus> fileCodeOwnerStatuses =
+ codeOwnerApprovalCheck.getFileStatuses(getChangeNotes(changeId));
+ FileCodeOwnerStatusSubject fileCodeOwnerStatusSubject =
+ assertThatStream(fileCodeOwnerStatuses).onlyElement();
+ fileCodeOwnerStatusSubject.hasNewPathStatus().value().hasPathThat().isEqualTo(path);
+ fileCodeOwnerStatusSubject
+ .hasNewPathStatus()
+ .value()
+ .hasStatusThat()
+ .isEqualTo(CodeOwnerStatus.APPROVED);
+ }
+
+ @Test
+ @GerritConfig(name = "plugin.code-owners.enableImplicitApprovals", value = "forced")
+ public void implicitlyApprovedByUploader() throws Exception {
+ TestAccount changeOwner =
+ accountCreator.create(
+ "changeOwner", "changeOwner@example.com", "ChangeOwner", /* displayName= */ null);
+
+ TestAccount codeOwner =
+ accountCreator.create(
+ "codeOwner", "codeOwner@example.com", "CodeOwner", /* displayName= */ null);
+ setAsRootCodeOwners(codeOwner);
+
+ Path path = Paths.get("/foo/bar.baz");
+ String changeId =
+ createChange(changeOwner, "Change Adding A File", JgitPath.of(path).get(), "file content")
+ .getChangeId();
+
+ // Upload another patch set by a code owner.
+ amendChange(codeOwner, changeId);
+
+ // Verify that the file is approved.
+ Stream<FileCodeOwnerStatus> fileCodeOwnerStatuses =
+ codeOwnerApprovalCheck.getFileStatuses(getChangeNotes(changeId));
+ FileCodeOwnerStatusSubject fileCodeOwnerStatusSubject =
+ assertThatStream(fileCodeOwnerStatuses).onlyElement();
+ fileCodeOwnerStatusSubject.hasNewPathStatus().value().hasPathThat().isEqualTo(path);
+ fileCodeOwnerStatusSubject
+ .hasNewPathStatus()
+ .value()
+ .hasStatusThat()
+ .isEqualTo(CodeOwnerStatus.APPROVED);
+ }
+
+ @Test
public void notOverriddenByUploaderWhoIsChangeOwner() throws Exception {
TestAccount changeOwner =
accountCreator.create(
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 5a78e50..159ce96 100644
--- a/javatests/com/google/gerrit/plugins/codeowners/backend/config/GeneralConfigTest.java
+++ b/javatests/com/google/gerrit/plugins/codeowners/backend/config/GeneralConfigTest.java
@@ -37,6 +37,7 @@
import com.google.gerrit.acceptance.config.GerritConfig;
import com.google.gerrit.plugins.codeowners.acceptance.AbstractCodeOwnersTest;
import com.google.gerrit.plugins.codeowners.backend.CodeOwnerReference;
+import com.google.gerrit.plugins.codeowners.backend.EnableImplicitApprovals;
import com.google.gerrit.plugins.codeowners.backend.FallbackCodeOwners;
import com.google.gerrit.plugins.codeowners.common.CodeOwnerConfigValidationPolicy;
import com.google.gerrit.plugins.codeowners.common.MergeCommitStrategy;
@@ -449,16 +450,27 @@
}
@Test
+ public void cannotGetEnableImplicitApprovalsForNullProject() throws Exception {
+ NullPointerException npe =
+ assertThrows(
+ NullPointerException.class,
+ () -> generalConfig.getEnableImplicitApprovals(/* project= */ null, new Config()));
+ assertThat(npe).hasMessageThat().isEqualTo("project");
+ }
+
+ @Test
public void cannotGetEnableImplicitApprovalsForNullPluginConfig() throws Exception {
NullPointerException npe =
assertThrows(
- NullPointerException.class, () -> generalConfig.getEnableImplicitApprovals(null));
+ NullPointerException.class,
+ () -> generalConfig.getEnableImplicitApprovals(project, /* pluginConfig= */ null));
assertThat(npe).hasMessageThat().isEqualTo("pluginConfig");
}
@Test
public void noEnableImplicitApprovalsConfiguration() throws Exception {
- assertThat(generalConfig.getEnableImplicitApprovals(new Config())).isFalse();
+ assertThat(generalConfig.getEnableImplicitApprovals(project, new Config()))
+ .isEqualTo(EnableImplicitApprovals.FALSE);
}
@Test
@@ -466,7 +478,8 @@
public void
enableImplicitApprovalsConfigurationIsRetrievedFromGerritConfigIfNotSpecifiedOnProjectLevel()
throws Exception {
- assertThat(generalConfig.getEnableImplicitApprovals(new Config())).isTrue();
+ assertThat(generalConfig.getEnableImplicitApprovals(project, new Config()))
+ .isEqualTo(EnableImplicitApprovals.TRUE);
}
@Test
@@ -476,7 +489,26 @@
throws Exception {
Config cfg = new Config();
cfg.setString(SECTION_CODE_OWNERS, null, KEY_ENABLE_IMPLICIT_APPROVALS, "false");
- assertThat(generalConfig.getEnableImplicitApprovals(cfg)).isFalse();
+ assertThat(generalConfig.getEnableImplicitApprovals(project, cfg))
+ .isEqualTo(EnableImplicitApprovals.FALSE);
+ }
+
+ @Test
+ @GerritConfig(name = "plugin.code-owners.enableImplicitApprovals", value = "true")
+ public void invalidEnableImplicitApprovalsConfigurationInPluginConfigIsIgnored()
+ throws Exception {
+ Config cfg = new Config();
+ cfg.setString(SECTION_CODE_OWNERS, null, KEY_ENABLE_IMPLICIT_APPROVALS, "INVALID");
+ assertThat(generalConfig.getEnableImplicitApprovals(project, cfg))
+ .isEqualTo(EnableImplicitApprovals.TRUE);
+ }
+
+ @Test
+ @GerritConfig(name = "plugin.code-owners.enableImplicitApprovals", value = "INVALID")
+ public void invalidEnableImplicitApprovalsConfigurationInGerritConfigIsIgnored()
+ throws Exception {
+ assertThat(generalConfig.getEnableImplicitApprovals(project, cfg))
+ .isEqualTo(EnableImplicitApprovals.FALSE);
}
@Test
diff --git a/resources/Documentation/config.md b/resources/Documentation/config.md
index 8c036fb..b40e939 100644
--- a/resources/Documentation/config.md
+++ b/resources/Documentation/config.md
@@ -91,22 +91,37 @@
<a id="pluginCodeOwnersEnableImplicitApprovals">plugin.@PLUGIN@.enableImplictApprovals</a>
: Whether an implicit code owner approval from the last uploader is
assumed.\
- This setting has no effect if self approvals from the last uploader are
- ignored because the [required label](#pluginCodeOwnersRequiredApproval)
- is configured to [ignore self
- approvals](../../../Documentation/config-labels.html#label_ignoreSelfApproval)
+ \
+ Can be `FALSE`, `TRUE` or `FORCED`.\
+ \
+ `FALSE`:\
+ Implicit code-owner approvals of the the patch set uploader are
+ disabled.\
+ \
+ `TRUE`:\
+ Implicit code-owner approvals of the patch set uploader are enabled, but
+ only if the configured [required
+ label](#pluginCodeOwnersRequiredApproval) is not configured to [ignore
+ self approvals](../../../Documentation/config-labels.html#label_ignoreSelfApproval)
from the uploader.\
- If enabled, code owners need to be aware of their implicit approval when
- they upload new patch sets for other users (e.g. if a contributor pushes
- a change to a wrong branch and a code owner helps them to get it rebased
- onto the correct branch, the rebased change has implicit approvals from
- the code owner, since the code owner is the uploader).\
+ \
+ `FORCED`:\
+ Implicit code-owner approvals of the patch set uploader are enabled,
+ even if the configured [required
+ label](#pluginCodeOwnersRequiredApproval) disallows self approvals.\
+ \
+ If enabled/enforced, code owners need to be aware of their implicit
+ approval when they upload new patch sets for other users (e.g. if a
+ contributor pushes a change to a wrong branch and a code owner helps
+ them to get it rebased onto the correct branch, the rebased change has
+ implicit approvals from the code owner, since the code owner is the
+ uploader).\
If implicit code owner approvals are disabled, code owners can still
self-approve their own changes by voting on the change.\
Can be overridden per project by setting
[codeOwners.enableImplictApprovals](#codeOwnersEnableImplicitApprovals)
in `@PLUGIN@.config`.\
- By default `false`.
+ By default `FALSE`.
<a id="pluginCodeOwnersGlobalCodeOwner">plugin.@PLUGIN@.globalCodeOwner</a>
: The email of a user that should be a code owner globally across all
@@ -454,16 +469,15 @@
<a id="codeOwnersEnableImplicitApprovals">codeOwners.enableImplicitApprovals</a>
: Whether an implicit code owner approval from the last uploader is
assumed.\
- This setting has no effect if self approvals from the last uploader are
- ignored because the [required label](#codeOwnersRequiredApproval)
- is configured to [ignore self
- approvals](../../../Documentation/config-labels.html#label_ignoreSelfApproval)
- from the uploader.\
- If enabled, code owners need to be aware of their implicit approval when
- they upload new patch sets for other users (e.g. if a contributor pushes
- a change to a wrong branch and a code owner helps them to get it rebased
- onto the correct branch, the rebased change has implicit approvals from
- the code owner, since the code owner is the uploader).\
+ Can be `FALSE`, `TRUE` or `FORCED` (see
+ [plugin.@PLUGIN@.enableImplicitApprovals](#pluginCodeOwnersEnableImplicitApprovals)
+ for an explanation of these values).\
+ If enabled/enforced, code owners need to be aware of their implicit
+ approval when they upload new patch sets for other users (e.g. if a
+ contributor pushes a change to a wrong branch and a code owner helps
+ them to get it rebased onto the correct branch, the rebased change has
+ implicit approvals from the code owner, since the code owner is the
+ uploader).\
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
