Disallow projects to override inherited list configuration parameters
Some configuration parameters have a list of values and can be specified
multiple times. If such a value was set on project level the complete
inherited list was overridden. It was not possible to just add a value
to the inherited list, but if this was wanted the complete list with the
additional value had to be set on project level.
Example:
If in the parent project 'codeOwners.globalCodeOwner = [A, B]' was set
and a child project set 'codeOwners.globalCodeOwner = [C]', the
inherited global code owners A and B were overridden, and the effective
global code owner was only C.
For most users this behavior was non-obvious, mainly due to the way how
list values are represented in config files. E.g.
'codeOwners.globalCodeOwner = [A, B]' is written as:
[codeOwners]
globalCodeOwner = A
globalCodeOwner = B
and the wrong expectation was that adding in a child project
[codeOweners]
globalCodeOwner = C
would result in the global code owners being [A,B,C].
This behavior was not only non-intuitive but also made it hard to
maintain global code owners globally. E.g. if A and B should be global
code owners for all projects, with the above example we would have
needed to repeat them in the child project configuration:
[codeOweners]
globalCodeOwner = A
globalCodeOwner = B
globalCodeOwner = C
This meant that configuring a new global code owner required adding
it in all config files that set any global code owner, which was
impractical.
It also meant that global code owners couldn't be enforced for all
projects, which can be dangerous if globally operating bots depend on
this configuration. E.g. a project owner removing inherited global code
owners (intended or not) breaks all bots that rely on being global code
owner.
Due to these issues we are changing the way how list configuration
parameters are inherited, so that child projects can only extend the
inherited list, but not override or unset it.
Example:
If a parent project defines
[codeOwners]
globalCodeOwner = A
globalCodeOwner = B
and a child project adds
[codeOweners]
globalCodeOwner = C
the effective global code owners are [A,B,C] now.
This behavior matches JGits default behavior for inheriting config
values from a base config (see JGits Config class):
For multi-value / list parameters the the returned list is the list of
values set in the base config (aka parent config) + the list of values
in the current config (aka child config). Note that the values from the
base config (aka parent config) config are always returned first. This
way values from the base config (aka parent config) are overridden when
reading single-value parameters, since for the single-value parameters
always only the last value counts.
To implement this kind of inheritance between projects we simply use the
base config support in JGit (see CodeOwnersPluginConfig). In addition we
must apply the same logic when taking values from gerrit.config into
account.
This change affects all multi-value / list configuration parameters,
which are:
* globalCodeOwner
* exemptedUser
* overrideApproval
* disabledBranch
This means these parameters can no longer be overridden/unset by child
projects.
The behavior for single-value parameters (single string, boolean, enum,
int, long) is not changed. Setting them for a project still overrides
any inherited value for the same parameter.
Signed-off-by: Edwin Kempin <ekempin@google.com>
Change-Id: I1d2576767155c43f49f7b5fdfe582331e743e090
diff --git a/java/com/google/gerrit/plugins/codeowners/backend/BackendModule.java b/java/com/google/gerrit/plugins/codeowners/backend/BackendModule.java
index c22dafe..0ba6d41 100644
--- a/java/com/google/gerrit/plugins/codeowners/backend/BackendModule.java
+++ b/java/com/google/gerrit/plugins/codeowners/backend/BackendModule.java
@@ -19,6 +19,7 @@
import com.google.gerrit.extensions.events.ReviewerAddedListener;
import com.google.gerrit.extensions.registration.DynamicMap;
import com.google.gerrit.extensions.registration.DynamicSet;
+import com.google.gerrit.plugins.codeowners.backend.config.CodeOwnersPluginConfig;
import com.google.gerrit.plugins.codeowners.backend.config.CodeOwnersPluginConfigSnapshot;
import com.google.gerrit.server.ExceptionHook;
import com.google.gerrit.server.IdentifiedUser;
@@ -34,6 +35,7 @@
factory(CodeOwnersUpdate.Factory.class);
factory(CodeOwnerConfigScanner.Factory.class);
factory(CodeOwnersPluginConfigSnapshot.Factory.class);
+ factory(CodeOwnersPluginConfig.Factory.class);
DynamicMap.mapOf(binder(), CodeOwnerBackend.class);
diff --git a/java/com/google/gerrit/plugins/codeowners/backend/config/AbstractRequiredApprovalConfig.java b/java/com/google/gerrit/plugins/codeowners/backend/config/AbstractRequiredApprovalConfig.java
index b72c1f0..59cce10 100644
--- a/java/com/google/gerrit/plugins/codeowners/backend/config/AbstractRequiredApprovalConfig.java
+++ b/java/com/google/gerrit/plugins/codeowners/backend/config/AbstractRequiredApprovalConfig.java
@@ -53,6 +53,12 @@
* Reads the required approvals for the specified project from the given plugin config with
* fallback to {@code gerrit.config}.
*
+ * <p>Inherited required approvals are included into the returned list at the first position (see
+ * {@link Config#getStringList(String, String, String)}).
+ *
+ * <p>The returned list contains duplicates if the exact same require approval is set for
+ * different projects in the line of parent projects.
+ *
* @param projectState state of the project for which the required approvals should be read
* @param pluginConfig the plugin config from which the required approvals should be read
* @return the required approvals, an empty list if none was configured
@@ -62,47 +68,33 @@
requireNonNull(pluginConfig, "pluginConfig");
ImmutableList.Builder<RequiredApproval> requiredApprovalList = ImmutableList.builder();
- String[] requiredApprovals =
- pluginConfig.getStringList(SECTION_CODE_OWNERS, /* subsection= */ null, getConfigKey());
- if (requiredApprovals.length > 0) {
- for (String requiredApproval : requiredApprovals) {
- try {
- requiredApprovalList.add(RequiredApproval.parse(projectState, requiredApproval));
- } catch (IllegalStateException | IllegalArgumentException e) {
- throw new InvalidPluginConfigurationException(
- pluginName,
- String.format(
- "Required approval '%s' that is configured in %s.config"
- + " (parameter %s.%s) is invalid: %s",
- requiredApproval,
- pluginName,
- SECTION_CODE_OWNERS,
- getConfigKey(),
- e.getMessage()));
- }
+ for (String requiredApproval :
+ pluginConfigFactory.getFromGerritConfig(pluginName).getStringList(getConfigKey())) {
+ try {
+ requiredApprovalList.add(RequiredApproval.parse(projectState, requiredApproval));
+ } catch (IllegalStateException | IllegalArgumentException e) {
+ throw new InvalidPluginConfigurationException(
+ pluginName,
+ String.format(
+ "Required approval '%s' that is configured in gerrit.config"
+ + " (parameter plugin.%s.%s) is invalid: %s",
+ requiredApproval, pluginName, getConfigKey(), e.getMessage()));
}
- return requiredApprovalList.build();
}
-
- requiredApprovals =
- pluginConfigFactory.getFromGerritConfig(pluginName).getStringList(getConfigKey());
- if (requiredApprovals.length > 0) {
- for (String requiredApproval : requiredApprovals) {
- try {
- requiredApprovalList.add(RequiredApproval.parse(projectState, requiredApproval));
- } catch (IllegalStateException | IllegalArgumentException e) {
- throw new InvalidPluginConfigurationException(
- pluginName,
- String.format(
- "Required approval '%s' that is configured in gerrit.config"
- + " (parameter plugin.%s.%s) is invalid: %s",
- requiredApproval, pluginName, getConfigKey(), e.getMessage()));
- }
+ for (String requiredApproval :
+ pluginConfig.getStringList(SECTION_CODE_OWNERS, /* subsection= */ null, getConfigKey())) {
+ try {
+ requiredApprovalList.add(RequiredApproval.parse(projectState, requiredApproval));
+ } catch (IllegalStateException | IllegalArgumentException e) {
+ throw new InvalidPluginConfigurationException(
+ pluginName,
+ String.format(
+ "Required approval '%s' that is configured in %s.config"
+ + " (parameter %s.%s) is invalid: %s",
+ requiredApproval, pluginName, SECTION_CODE_OWNERS, getConfigKey(), e.getMessage()));
}
- return requiredApprovalList.build();
}
-
- return ImmutableList.of();
+ return requiredApprovalList.build();
}
/**
diff --git a/java/com/google/gerrit/plugins/codeowners/backend/config/CodeOwnersPluginConfig.java b/java/com/google/gerrit/plugins/codeowners/backend/config/CodeOwnersPluginConfig.java
new file mode 100644
index 0000000..7bd0971
--- /dev/null
+++ b/java/com/google/gerrit/plugins/codeowners/backend/config/CodeOwnersPluginConfig.java
@@ -0,0 +1,179 @@
+// 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.config;
+
+import static com.google.gerrit.server.project.ProjectCache.noSuchProject;
+
+import com.google.gerrit.entities.Project;
+import com.google.gerrit.extensions.annotations.PluginName;
+import com.google.gerrit.server.project.NoSuchProjectException;
+import com.google.gerrit.server.project.ProjectCache;
+import com.google.gerrit.server.project.ProjectState;
+import com.google.inject.Inject;
+import com.google.inject.assistedinject.Assisted;
+import java.util.Arrays;
+import java.util.Set;
+import org.eclipse.jgit.lib.Config;
+
+/**
+ * Class to read the {@code code-owners.config} file in the {@code refs/meta/config} branch of a
+ * project with taking inherited config parameters from parent projects into account.
+ *
+ * <p>For inheriting config parameters from parent projects we rely on base config support in JGit's
+ * {@link Config} class.
+ *
+ * <p>For single-value parameters (string, boolean, enum, int, long) this means:
+ *
+ * <ul>
+ * <li>If a parameter is not set, it is read from the parent project.
+ * <li>If a parameter is set, it overrides any value that is set in the parent project.
+ * </ul>
+ *
+ * <p>For multi-value parameters (string list) this means:
+ *
+ * <ul>
+ * <li>If a parameter is not set, the values are read from the parent projects.
+ * <li>If any value for the parameter is set, it is added to the inherited value list (the
+ * inherited value list is extended).
+ * <li>If the exact same value is set for different projects in the line of parent projects this
+ * value appears multiple times in the value list (list may contain duplicates).
+ * <li>The inherited value list cannot be overridden (this means the inherited values cannot be
+ * unset/overridden).
+ * </ul>
+ *
+ * <p>Please note that this inheritance behavior is different from what {@link
+ * com.google.gerrit.server.config.PluginConfigFactory} does. {@code PluginConfigFactory} has 2
+ * modes:
+ *
+ * <ul>
+ * <li>merge = false: Inherited list values are overridden.
+ * <li>merge = true: Inherited list values are extended the same way as in this class, but for
+ * single-value parameters the inherited value from the parent project takes precedence.
+ * </ul>
+ *
+ * <p>For the {@code code-owners.config} we want that:
+ *
+ * <ul>
+ * <li>Single-value parameters override inherited settings so that they can be controlled per
+ * project (e.g. whether validation of OWNERS files should be done).
+ * <li>Multi-value parameters cannot be overridden, but only extended (e.g. this allows to enforce
+ * global code owners or exempted users globally).
+ * </ul>
+ */
+public class CodeOwnersPluginConfig {
+ public interface Factory {
+ CodeOwnersPluginConfig create(Project.NameKey projectName);
+ }
+
+ private static final String CONFIG_EXTENSION = ".config";
+
+ private final String pluginName;
+ private final ProjectCache projectCache;
+ private final Project.NameKey projectName;
+ private Config config;
+
+ @Inject
+ CodeOwnersPluginConfig(
+ @PluginName String pluginName,
+ ProjectCache projectCache,
+ @Assisted Project.NameKey projectName) {
+ this.pluginName = pluginName;
+ this.projectCache = projectCache;
+ this.projectName = projectName;
+ }
+
+ public Config get() {
+ if (config == null) {
+ config = load();
+ }
+ return config;
+ }
+
+ /**
+ * Load the {@code code-owners.config} file of the project and sets all parent {@code
+ * code-owners.config}s as base configs.
+ *
+ * @throws IllegalStateException if the project doesn't exist
+ */
+ private Config load() {
+ try {
+ ProjectState projectState =
+ projectCache.get(projectName).orElseThrow(noSuchProject(projectName));
+ String fileName = pluginName + CONFIG_EXTENSION;
+
+ Config mergedConfig = null;
+
+ // Iterate in-order from All-Projects through the project hierarchy to this project. For each
+ // project read the code-owners.config and set the parent code-owners.config as base config.
+ for (ProjectState p : projectState.treeInOrder()) {
+ Config currentConfig = p.getConfig(fileName).get();
+ if (mergedConfig == null) {
+ mergedConfig = currentConfig;
+ } else {
+ mergedConfig = createConfigWithBase(currentConfig, mergedConfig);
+ }
+ }
+ return mergedConfig;
+ } catch (NoSuchProjectException e) {
+ throw new IllegalStateException(
+ String.format(
+ "cannot get %s plugin config for non-existing project %s", pluginName, projectName),
+ e);
+ }
+ }
+
+ /**
+ * Creates a copy of the given {@code config} with the given {@code baseConfig} as base config.
+ *
+ * <p>JGit doesn't allow to set a base config on an existing {@link Config}. Hence create a new
+ * (empty) config with the base config and then copy over all sections and subsection.
+ *
+ * @param config config that should be copied
+ * @param baseConfig config that should be set as base config
+ */
+ private Config createConfigWithBase(Config config, Config baseConfig) {
+ // Create a new Config with the parent Config as base config.
+ Config configWithBase = new Config(baseConfig);
+
+ // Copy all sections and subsections from the given config.
+ for (String section : config.getSections()) {
+ for (String name : config.getNames(section)) {
+ configWithBase.setStringList(
+ section,
+ /* subsection = */ null,
+ name,
+ Arrays.asList(config.getStringList(section, /* subsection = */ null, name)));
+ }
+
+ for (String subsection : config.getSubsections(section)) {
+ Set<String> allNames = config.getNames(section, subsection);
+ if (allNames.isEmpty()) {
+ // Set empty subsection.
+ configWithBase.setString(section, subsection, /* name= */ null, /* value= */ null);
+ } else {
+ for (String name : allNames) {
+ configWithBase.setStringList(
+ section,
+ subsection,
+ name,
+ Arrays.asList(config.getStringList(section, subsection, name)));
+ }
+ }
+ }
+ }
+
+ return configWithBase;
+ }
+}
diff --git a/java/com/google/gerrit/plugins/codeowners/backend/config/CodeOwnersPluginConfigSnapshot.java b/java/com/google/gerrit/plugins/codeowners/backend/config/CodeOwnersPluginConfigSnapshot.java
index 669b92f..52eeba4 100644
--- a/java/com/google/gerrit/plugins/codeowners/backend/config/CodeOwnersPluginConfigSnapshot.java
+++ b/java/com/google/gerrit/plugins/codeowners/backend/config/CodeOwnersPluginConfigSnapshot.java
@@ -27,7 +27,6 @@
import com.google.gerrit.entities.BranchNameKey;
import com.google.gerrit.entities.LabelType;
import com.google.gerrit.entities.Project;
-import com.google.gerrit.extensions.annotations.PluginName;
import com.google.gerrit.plugins.codeowners.backend.CodeOwnerBackend;
import com.google.gerrit.plugins.codeowners.backend.CodeOwnerReference;
import com.google.gerrit.plugins.codeowners.backend.CodeOwnersInternalServerErrorException;
@@ -36,8 +35,6 @@
import com.google.gerrit.plugins.codeowners.common.CodeOwnerConfigValidationPolicy;
import com.google.gerrit.plugins.codeowners.common.MergeCommitStrategy;
import com.google.gerrit.server.account.Emails;
-import com.google.gerrit.server.config.PluginConfigFactory;
-import com.google.gerrit.server.project.NoSuchProjectException;
import com.google.gerrit.server.project.ProjectCache;
import com.google.gerrit.server.project.ProjectState;
import com.google.inject.Inject;
@@ -56,8 +53,6 @@
CodeOwnersPluginConfigSnapshot create(Project.NameKey projectName);
}
- private final String pluginName;
- private final PluginConfigFactory pluginConfigFactory;
private final ProjectCache projectCache;
private final Emails emails;
private final BackendConfig backendConfig;
@@ -72,8 +67,7 @@
@Inject
CodeOwnersPluginConfigSnapshot(
- @PluginName String pluginName,
- PluginConfigFactory pluginConfigFactory,
+ CodeOwnersPluginConfig.Factory codeOwnersPluginConfigFactory,
ProjectCache projectCache,
Emails emails,
BackendConfig backendConfig,
@@ -82,8 +76,6 @@
RequiredApprovalConfig requiredApprovalConfig,
StatusConfig statusConfig,
@Assisted Project.NameKey projectName) {
- this.pluginName = pluginName;
- this.pluginConfigFactory = pluginConfigFactory;
this.projectCache = projectCache;
this.emails = emails;
this.backendConfig = backendConfig;
@@ -92,7 +84,7 @@
this.requiredApprovalConfig = requiredApprovalConfig;
this.statusConfig = statusConfig;
this.projectName = projectName;
- this.pluginConfig = loadPluginConfig();
+ this.pluginConfig = codeOwnersPluginConfigFactory.create(projectName).get();
}
/** Gets the file extension of code owner config files, if any configured. */
@@ -493,21 +485,4 @@
projectCache.get(projectName).orElseThrow(illegalState(projectName));
return requiredApprovalConfig.get(projectState, pluginConfig);
}
-
- /**
- * Reads and returns the config from the {@code code-owners.config} file in {@code
- * refs/meta/config} branch.
- *
- * @return the code owners configurations
- */
- private Config loadPluginConfig() {
- try {
- return pluginConfigFactory.getProjectPluginConfigWithInheritance(projectName, pluginName);
- } catch (NoSuchProjectException e) {
- throw new IllegalStateException(
- String.format(
- "cannot get %s plugin config for non-existing project %s", pluginName, projectName),
- e);
- }
- }
}
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 ac474cb..6c6a749 100644
--- a/java/com/google/gerrit/plugins/codeowners/backend/config/GeneralConfig.java
+++ b/java/com/google/gerrit/plugins/codeowners/backend/config/GeneralConfig.java
@@ -21,6 +21,7 @@
import com.google.common.base.Strings;
import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableSet;
+import com.google.common.collect.Streams;
import com.google.common.flogger.FluentLogger;
import com.google.gerrit.entities.BranchNameKey;
import com.google.gerrit.entities.Project;
@@ -40,8 +41,10 @@
import java.util.ArrayList;
import java.util.Arrays;
import java.util.List;
+import java.util.Objects;
import java.util.Optional;
import java.util.regex.PatternSyntaxException;
+import java.util.stream.Stream;
import org.eclipse.jgit.lib.Config;
/**
@@ -796,19 +799,7 @@
*/
ImmutableSet<CodeOwnerReference> getGlobalCodeOwners(Config pluginConfig) {
requireNonNull(pluginConfig, "pluginConfig");
-
- if (pluginConfig.getString(SECTION_CODE_OWNERS, /* subsection= */ null, KEY_GLOBAL_CODE_OWNER)
- != null) {
- return Arrays.stream(
- pluginConfig.getStringList(
- SECTION_CODE_OWNERS, /* subsection= */ null, KEY_GLOBAL_CODE_OWNER))
- .filter(value -> !value.trim().isEmpty())
- .map(CodeOwnerReference::create)
- .collect(toImmutableSet());
- }
-
- return Arrays.stream(pluginConfigFromGerritConfig.getStringList(KEY_GLOBAL_CODE_OWNER))
- .filter(value -> !value.trim().isEmpty())
+ return getMultiValue(pluginConfig, KEY_GLOBAL_CODE_OWNER)
.map(CodeOwnerReference::create)
.collect(toImmutableSet());
}
@@ -824,19 +815,7 @@
*/
ImmutableSet<String> getExemptedUsers(Config pluginConfig) {
requireNonNull(pluginConfig, "pluginConfig");
-
- if (pluginConfig.getString(SECTION_CODE_OWNERS, /* subsection= */ null, KEY_EXEMPTED_USER)
- != null) {
- return Arrays.stream(
- pluginConfig.getStringList(
- SECTION_CODE_OWNERS, /* subsection= */ null, KEY_EXEMPTED_USER))
- .filter(value -> !value.trim().isEmpty())
- .collect(toImmutableSet());
- }
-
- return Arrays.stream(pluginConfigFromGerritConfig.getStringList(KEY_EXEMPTED_USER))
- .filter(value -> !value.trim().isEmpty())
- .collect(toImmutableSet());
+ return getMultiValue(pluginConfig, KEY_EXEMPTED_USER).collect(toImmutableSet());
}
/**
@@ -860,4 +839,25 @@
return Optional.ofNullable(pluginConfigFromGerritConfig.getString(KEY_OVERRIDE_INFO_URL));
}
+
+ /**
+ * Gets the values for a parameter that can be set multiple times with taking inherited values
+ * from {@code gerrit.config} into account.
+ *
+ * <p>The inherited values from {@code gerrit.config} are included into the returned list at the
+ * first position. This matches the behavior in {@link Config#getStringList(String, String,
+ * String)} that includes inherited values from the base config into the result list at the first
+ * position too.
+ *
+ * <p>The returned stream contains duplicates if the exact same value is set for different
+ * projects in the line of parent projects.
+ */
+ private Stream<String> getMultiValue(Config pluginConfig, String key) {
+ return Streams.concat(
+ Arrays.stream(pluginConfigFromGerritConfig.getStringList(key)),
+ Arrays.stream(
+ pluginConfig.getStringList(SECTION_CODE_OWNERS, /* subsection= */ null, key)))
+ .filter(Objects::nonNull)
+ .filter(value -> !value.trim().isEmpty());
+ }
}
diff --git a/java/com/google/gerrit/plugins/codeowners/backend/config/StatusConfig.java b/java/com/google/gerrit/plugins/codeowners/backend/config/StatusConfig.java
index 62ae72f..c3d4dee 100644
--- a/java/com/google/gerrit/plugins/codeowners/backend/config/StatusConfig.java
+++ b/java/com/google/gerrit/plugins/codeowners/backend/config/StatusConfig.java
@@ -180,31 +180,30 @@
requireNonNull(pluginConfig, "pluginConfig");
requireNonNull(branch, "branch");
- String disabledBranches =
- pluginConfig.getString(SECTION_CODE_OWNERS, null, KEY_DISABLED_BRANCH);
- if (disabledBranches != null) {
- // a value for KEY_DISABLED_BRANCH is set on project-level
- return isDisabledForBranch(
- pluginConfig.getStringList(SECTION_CODE_OWNERS, null, KEY_DISABLED_BRANCH),
- branch.branch(),
- "Disabled branch '%s' that is configured for project "
- + branch.project()
- + " in "
- + pluginName
- + ".config (parameter "
- + SECTION_CODE_OWNERS
- + "."
- + KEY_DISABLED_BRANCH
- + ") is invalid.");
+ // check if the branch is disabled in gerrit.config
+ boolean isDisabled =
+ isDisabledForBranch(
+ pluginConfigFromGerritConfig.getStringList(KEY_DISABLED_BRANCH),
+ branch.branch(),
+ "Disabled branch '%s' that is configured for in gerrit.config (parameter plugin."
+ + pluginName
+ + "."
+ + KEY_DISABLED_BRANCH
+ + ") is invalid.");
+ if (isDisabled) {
+ return true;
}
- // there is no project-level configuration for KEY_DISABLED_BRANCH, check if it's set in
- // gerrit.config
+ // check if the branch is disabled on project level
return isDisabledForBranch(
- pluginConfigFromGerritConfig.getStringList(KEY_DISABLED_BRANCH),
+ pluginConfig.getStringList(SECTION_CODE_OWNERS, null, KEY_DISABLED_BRANCH),
branch.branch(),
- "Disabled branch '%s' that is configured for in gerrit.config (parameter plugin."
+ "Disabled branch '%s' that is configured for project "
+ + branch.project()
+ + " in "
+ pluginName
+ + ".config (parameter "
+ + SECTION_CODE_OWNERS
+ "."
+ KEY_DISABLED_BRANCH
+ ") is invalid.");
diff --git a/javatests/com/google/gerrit/plugins/codeowners/backend/config/CodeOwnersPluginConfigSnapshotTest.java b/javatests/com/google/gerrit/plugins/codeowners/backend/config/CodeOwnersPluginConfigSnapshotTest.java
index d1a4adb..5cc4cf2 100644
--- a/javatests/com/google/gerrit/plugins/codeowners/backend/config/CodeOwnersPluginConfigSnapshotTest.java
+++ b/javatests/com/google/gerrit/plugins/codeowners/backend/config/CodeOwnersPluginConfigSnapshotTest.java
@@ -204,18 +204,20 @@
@GerritConfig(
name = "plugin.code-owners.globalCodeOwner",
values = {"global-code-owner-1@example.com", "global-code-owner-2@example.com"})
- public void globalCodeOwnersOnProjectLevelOverrideGloballyConfiguredGlobalCodeOwners()
+ public void globalCodeOwnersOnProjectLevelExtendsGloballyConfiguredGlobalCodeOwners()
throws Exception {
- accountCreator.create(
- "globalCodeOwner1",
- "global-code-owner-1@example.com",
- "Global Code Owner 1",
- /* displayName= */ null);
- accountCreator.create(
- "globalCodeOwner2",
- "global-code-owner-2@example.com",
- "Global Code Owner 2",
- /* displayName= */ null);
+ TestAccount globalCodeOwner1 =
+ accountCreator.create(
+ "globalCodeOwner1",
+ "global-code-owner-1@example.com",
+ "Global Code Owner 1",
+ /* displayName= */ null);
+ TestAccount globalCodeOwner2 =
+ accountCreator.create(
+ "globalCodeOwner2",
+ "global-code-owner-2@example.com",
+ "Global Code Owner 2",
+ /* displayName= */ null);
TestAccount globalCodeOwner3 =
accountCreator.create(
"globalCodeOwner3",
@@ -231,7 +233,43 @@
configureGlobalCodeOwners(allProjects, globalCodeOwner3.email(), globalCodeOwner4.email());
assertThat(cfgSnapshot().getGlobalCodeOwners())
.comparingElementsUsing(hasEmail())
- .containsExactly(globalCodeOwner3.email(), globalCodeOwner4.email());
+ .containsExactly(
+ globalCodeOwner1.email(),
+ globalCodeOwner2.email(),
+ globalCodeOwner3.email(),
+ globalCodeOwner4.email());
+ }
+
+ @Test
+ @GerritConfig(
+ name = "plugin.code-owners.globalCodeOwner",
+ values = {"global-code-owner-1@example.com", "global-code-owner-2@example.com"})
+ public void
+ globalCodeOwnersOnProjectLevelExtendsGloballyConfiguredGlobalCodeOwners_duplicatesAreFilteredOut()
+ throws Exception {
+ TestAccount globalCodeOwner1 =
+ accountCreator.create(
+ "globalCodeOwner1",
+ "global-code-owner-1@example.com",
+ "Global Code Owner 1",
+ /* displayName= */ null);
+ TestAccount globalCodeOwner2 =
+ accountCreator.create(
+ "globalCodeOwner2",
+ "global-code-owner-2@example.com",
+ "Global Code Owner 2",
+ /* displayName= */ null);
+ TestAccount globalCodeOwner3 =
+ accountCreator.create(
+ "globalCodeOwner3",
+ "global-code-owner-3@example.com",
+ "Global Code Owner 3",
+ /* displayName= */ null);
+ configureGlobalCodeOwners(allProjects, globalCodeOwner1.email(), globalCodeOwner3.email());
+ assertThat(cfgSnapshot().getGlobalCodeOwners())
+ .comparingElementsUsing(hasEmail())
+ .containsExactly(
+ globalCodeOwner1.email(), globalCodeOwner2.email(), globalCodeOwner3.email());
}
@Test
@@ -239,16 +277,18 @@
name = "plugin.code-owners.globalCodeOwner",
values = {"global-code-owner-1@example.com", "global-code-owner-2@example.com"})
public void globalCodeOwnersAreInheritedFromParentProject() throws Exception {
- accountCreator.create(
- "globalCodeOwner1",
- "global-code-owner-1@example.com",
- "Global Code Owner 1",
- /* displayName= */ null);
- accountCreator.create(
- "globalCodeOwner2",
- "global-code-owner-2@example.com",
- "Global Code Owner 2",
- /* displayName= */ null);
+ TestAccount globalCodeOwner1 =
+ accountCreator.create(
+ "globalCodeOwner1",
+ "global-code-owner-1@example.com",
+ "Global Code Owner 1",
+ /* displayName= */ null);
+ TestAccount globalCodeOwner2 =
+ accountCreator.create(
+ "globalCodeOwner2",
+ "global-code-owner-2@example.com",
+ "Global Code Owner 2",
+ /* displayName= */ null);
TestAccount globalCodeOwner3 =
accountCreator.create(
"globalCodeOwner3",
@@ -264,11 +304,46 @@
configureGlobalCodeOwners(allProjects, globalCodeOwner3.email(), globalCodeOwner4.email());
assertThat(cfgSnapshot().getGlobalCodeOwners())
.comparingElementsUsing(hasEmail())
- .containsExactly(globalCodeOwner3.email(), globalCodeOwner4.email());
+ .containsExactly(
+ globalCodeOwner1.email(),
+ globalCodeOwner2.email(),
+ globalCodeOwner3.email(),
+ globalCodeOwner4.email());
}
@Test
- public void inheritedGlobalCodeOwnersCanBeOverridden() throws Exception {
+ @GerritConfig(
+ name = "plugin.code-owners.globalCodeOwner",
+ values = {"global-code-owner-1@example.com", "global-code-owner-2@example.com"})
+ public void globalCodeOwnersAreInheritedFromParentProject_duplicatesAreFilteredOut()
+ throws Exception {
+ TestAccount globalCodeOwner1 =
+ accountCreator.create(
+ "globalCodeOwner1",
+ "global-code-owner-1@example.com",
+ "Global Code Owner 1",
+ /* displayName= */ null);
+ TestAccount globalCodeOwner2 =
+ accountCreator.create(
+ "globalCodeOwner2",
+ "global-code-owner-2@example.com",
+ "Global Code Owner 2",
+ /* displayName= */ null);
+ TestAccount globalCodeOwner3 =
+ accountCreator.create(
+ "globalCodeOwner3",
+ "global-code-owner-3@example.com",
+ "Global Code Owner 3",
+ /* displayName= */ null);
+ configureGlobalCodeOwners(allProjects, globalCodeOwner1.email(), globalCodeOwner3.email());
+ assertThat(cfgSnapshot().getGlobalCodeOwners())
+ .comparingElementsUsing(hasEmail())
+ .containsExactly(
+ globalCodeOwner1.email(), globalCodeOwner2.email(), globalCodeOwner3.email());
+ }
+
+ @Test
+ public void inheritedGlobalCodeOwnersCanBeExtended() throws Exception {
TestAccount globalCodeOwner1 =
accountCreator.create(
"globalCodeOwner1",
@@ -297,11 +372,43 @@
configureGlobalCodeOwners(project, globalCodeOwner3.email(), globalCodeOwner4.email());
assertThat(cfgSnapshot().getGlobalCodeOwners())
.comparingElementsUsing(hasEmail())
- .containsExactly(globalCodeOwner3.email(), globalCodeOwner4.email());
+ .containsExactly(
+ globalCodeOwner1.email(),
+ globalCodeOwner2.email(),
+ globalCodeOwner3.email(),
+ globalCodeOwner4.email());
}
@Test
- public void inheritedGlobalCodeOwnersCanBeRemoved() throws Exception {
+ public void inheritedGlobalCodeOwnersCanBeExtended_duplicatesAreFilteredOut() throws Exception {
+ TestAccount globalCodeOwner1 =
+ accountCreator.create(
+ "globalCodeOwner1",
+ "global-code-owner-1@example.com",
+ "Global Code Owner 1",
+ /* displayName= */ null);
+ TestAccount globalCodeOwner2 =
+ accountCreator.create(
+ "globalCodeOwner2",
+ "global-code-owner-2@example.com",
+ "Global Code Owner 2",
+ /* displayName= */ null);
+ TestAccount globalCodeOwner3 =
+ accountCreator.create(
+ "globalCodeOwner3",
+ "global-code-owner-3@example.com",
+ "Global Code Owner 3",
+ /* displayName= */ null);
+ configureGlobalCodeOwners(allProjects, globalCodeOwner1.email(), globalCodeOwner2.email());
+ configureGlobalCodeOwners(project, globalCodeOwner1.email(), globalCodeOwner3.email());
+ assertThat(cfgSnapshot().getGlobalCodeOwners())
+ .comparingElementsUsing(hasEmail())
+ .containsExactly(
+ globalCodeOwner1.email(), globalCodeOwner2.email(), globalCodeOwner3.email());
+ }
+
+ @Test
+ public void inheritedGlobalCodeOwnersCannotBeRemoved() throws Exception {
TestAccount globalCodeOwner1 =
accountCreator.create(
"globalCodeOwner1",
@@ -316,7 +423,9 @@
/* displayName= */ null);
configureGlobalCodeOwners(allProjects, globalCodeOwner1.email(), globalCodeOwner2.email());
configureGlobalCodeOwners(project, "");
- assertThat(cfgSnapshot().getGlobalCodeOwners()).isEmpty();
+ assertThat(cfgSnapshot().getGlobalCodeOwners())
+ .comparingElementsUsing(hasEmail())
+ .containsExactly(globalCodeOwner1.email(), globalCodeOwner2.email());
}
@Test
@@ -349,12 +458,20 @@
@GerritConfig(
name = "plugin.code-owners.exemptedUser",
values = {"exempted-user-1@example.com", "exempted-user-2@example.com"})
- public void exemptedAccountsOnProjectLevelOverrideGloballyConfiguredExemptedAcounts()
+ public void exemptedAccountsOnProjectLevelExtendsGloballyConfiguredExemptedAcounts()
throws Exception {
- accountCreator.create(
- "exemptedUser1", "exempted-user-1@example.com", "Exempted User 1", /* displayName= */ null);
- accountCreator.create(
- "exemptedUser2", "exempted-user-2@example.com", "Exempted User 2", /* displayName= */ null);
+ TestAccount exemptedUser1 =
+ accountCreator.create(
+ "exemptedUser1",
+ "exempted-user-1@example.com",
+ "Exempted User 1",
+ /* displayName= */ null);
+ TestAccount exemptedUser2 =
+ accountCreator.create(
+ "exemptedUser2",
+ "exempted-user-2@example.com",
+ "Exempted User 2",
+ /* displayName= */ null);
TestAccount exemptedUser3 =
accountCreator.create(
"exemptedUser3",
@@ -369,7 +486,38 @@
/* displayName= */ null);
configureExemptedUsers(allProjects, exemptedUser3.email(), exemptedUser4.email());
assertThat(cfgSnapshot().getExemptedAccounts())
- .containsExactly(exemptedUser3.id(), exemptedUser4.id());
+ .containsExactly(
+ exemptedUser1.id(), exemptedUser2.id(), exemptedUser3.id(), exemptedUser4.id());
+ }
+
+ @Test
+ @GerritConfig(
+ name = "plugin.code-owners.exemptedUser",
+ values = {"exempted-user-1@example.com", "exempted-user-2@example.com"})
+ public void
+ exemptedAccountsOnProjectLevelExtendsGloballyConfiguredExemptedAcounts_duplicatesAreFilteredOut()
+ throws Exception {
+ TestAccount exemptedUser1 =
+ accountCreator.create(
+ "exemptedUser1",
+ "exempted-user-1@example.com",
+ "Exempted User 1",
+ /* displayName= */ null);
+ TestAccount exemptedUser2 =
+ accountCreator.create(
+ "exemptedUser2",
+ "exempted-user-2@example.com",
+ "Exempted User 2",
+ /* displayName= */ null);
+ TestAccount exemptedUser3 =
+ accountCreator.create(
+ "exemptedUser3",
+ "exempted-user-3@example.com",
+ "Exempted User 3",
+ /* displayName= */ null);
+ configureExemptedUsers(allProjects, exemptedUser1.email(), exemptedUser3.email());
+ assertThat(cfgSnapshot().getExemptedAccounts())
+ .containsExactly(exemptedUser1.id(), exemptedUser2.id(), exemptedUser3.id());
}
@Test
@@ -377,10 +525,18 @@
name = "plugin.code-owners.exemptedUser",
values = {"exempted-user-1@example.com", "exempted-user-2@example.com"})
public void exemptedAccountsAreInheritedFromParentProject() throws Exception {
- accountCreator.create(
- "exemptedUser1", "exempted-user-1@example.com", "Exempted User 1", /* displayName= */ null);
- accountCreator.create(
- "exemptedUser2", "exempted-user-2@example.com", "Exempted User 2", /* displayName= */ null);
+ TestAccount exemptedUser1 =
+ accountCreator.create(
+ "exemptedUser1",
+ "exempted-user-1@example.com",
+ "Exempted User 1",
+ /* displayName= */ null);
+ TestAccount exemptedUser2 =
+ accountCreator.create(
+ "exemptedUser2",
+ "exempted-user-2@example.com",
+ "Exempted User 2",
+ /* displayName= */ null);
TestAccount exemptedUser3 =
accountCreator.create(
"exemptedUser3",
@@ -395,11 +551,41 @@
/* displayName= */ null);
configureExemptedUsers(allProjects, exemptedUser3.email(), exemptedUser4.email());
assertThat(cfgSnapshot().getExemptedAccounts())
- .containsExactly(exemptedUser3.id(), exemptedUser4.id());
+ .containsExactly(
+ exemptedUser1.id(), exemptedUser2.id(), exemptedUser3.id(), exemptedUser4.id());
}
@Test
- public void inheritedExemptedAccountsCanBeOverridden() throws Exception {
+ @GerritConfig(
+ name = "plugin.code-owners.exemptedUser",
+ values = {"exempted-user-1@example.com", "exempted-user-2@example.com"})
+ public void exemptedAccountsAreInheritedFromParentProject_duplicatesAreFilteredOut()
+ throws Exception {
+ TestAccount exemptedUser1 =
+ accountCreator.create(
+ "exemptedUser1",
+ "exempted-user-1@example.com",
+ "Exempted User 1",
+ /* displayName= */ null);
+ TestAccount exemptedUser2 =
+ accountCreator.create(
+ "exemptedUser2",
+ "exempted-user-2@example.com",
+ "Exempted User 2",
+ /* displayName= */ null);
+ TestAccount exemptedUser3 =
+ accountCreator.create(
+ "exemptedUser3",
+ "exempted-user-3@example.com",
+ "Exempted User 3",
+ /* displayName= */ null);
+ configureExemptedUsers(allProjects, exemptedUser1.email(), exemptedUser3.email());
+ assertThat(cfgSnapshot().getExemptedAccounts())
+ .containsExactly(exemptedUser1.id(), exemptedUser2.id(), exemptedUser3.id());
+ }
+
+ @Test
+ public void inheritedExemptedAccountsCanBeExtended() throws Exception {
TestAccount exemptedUser1 =
accountCreator.create(
"exemptedUser1",
@@ -427,11 +613,38 @@
configureExemptedUsers(allProjects, exemptedUser1.email(), exemptedUser2.email());
configureExemptedUsers(project, exemptedUser3.email(), exemptedUser4.email());
assertThat(cfgSnapshot().getExemptedAccounts())
- .containsExactly(exemptedUser3.id(), exemptedUser4.id());
+ .containsExactly(
+ exemptedUser1.id(), exemptedUser2.id(), exemptedUser3.id(), exemptedUser4.id());
}
@Test
- public void inheritedExemptedAccountsCanBeRemoved() throws Exception {
+ public void inheritedExemptedAccountsCanBeExtended_duplicatesAreFilteredOut() throws Exception {
+ TestAccount exemptedUser1 =
+ accountCreator.create(
+ "exemptedUser1",
+ "exempted-user-1@example.com",
+ "Exempted User 1",
+ /* displayName= */ null);
+ TestAccount exemptedUser2 =
+ accountCreator.create(
+ "exemptedUser2",
+ "exempted-user-2@example.com",
+ "Exempted User 2",
+ /* displayName= */ null);
+ TestAccount exemptedUser3 =
+ accountCreator.create(
+ "exemptedUser3",
+ "exempted-user-3@example.com",
+ "Exempted User 3",
+ /* displayName= */ null);
+ configureExemptedUsers(allProjects, exemptedUser1.email(), exemptedUser2.email());
+ configureExemptedUsers(project, exemptedUser1.email(), exemptedUser3.email());
+ assertThat(cfgSnapshot().getExemptedAccounts())
+ .containsExactly(exemptedUser1.id(), exemptedUser2.id(), exemptedUser3.id());
+ }
+
+ @Test
+ public void inheritedExemptedAccountsCannotBeRemoved() throws Exception {
TestAccount exemptedUser1 =
accountCreator.create(
"exemptedUser1",
@@ -446,7 +659,8 @@
/* displayName= */ null);
configureExemptedUsers(allProjects, exemptedUser1.email(), exemptedUser2.email());
configureExemptedUsers(project, "");
- assertThat(cfgSnapshot().getExemptedAccounts()).isEmpty();
+ assertThat(cfgSnapshot().getExemptedAccounts())
+ .containsExactly(exemptedUser1.id(), exemptedUser2.id());
}
@Test
@@ -629,21 +843,23 @@
}
@Test
- public void inheritedDisabledBranchCanBeOverridden() throws Exception {
+ public void inheritedDisabledBranchCanBeExtended() throws Exception {
configureDisabledBranch(allProjects, "refs/heads/master");
configureDisabledBranch(project, "refs/heads/test");
- assertThat(cfgSnapshot().isDisabled("master")).isFalse();
+ assertThat(cfgSnapshot().isDisabled("master")).isTrue();
assertThat(cfgSnapshot().isDisabled("test")).isTrue();
}
@Test
- public void inheritedDisabledBranchCanBeRemoved() throws Exception {
+ public void inheritedDisabledBranchCannotBeRemoved() throws Exception {
configureDisabledBranch(allProjects, "refs/heads/master");
- // override the inherited config with an empty value to enable code owners for all branches
+ // trying to override the inherited config with an empty value to enable code owners for all
+ // branches doesn't work because the empty string is added to the inherited value list so that
+ // disabledBranch is ["refs/heads/master", ""] now
configureDisabledBranch(project, "");
- assertThat(cfgSnapshot().isDisabled("master")).isFalse();
+ assertThat(cfgSnapshot().isDisabled("master")).isTrue();
}
@Test
@@ -1057,15 +1273,31 @@
@Test
@GerritConfig(name = "plugin.code-owners.overrideApproval", value = "Owners-Override+1")
- public void overrideApprovalConfiguredOnProjectLevelOverridesGloballyConfiguredOverrideApproval()
+ public void overrideApprovalConfiguredOnProjectLevelExtendsGloballyConfiguredOverrideApproval()
throws Exception {
createOwnersOverrideLabel();
createOwnersOverrideLabel("Other-Override");
configureOverrideApproval(project, "Other-Override+1");
ImmutableSet<RequiredApproval> requiredApprovals = cfgSnapshot().getOverrideApprovals();
+ assertThat(requiredApprovals).hasSize(2);
+ assertThat(requiredApprovals).element(0).hasLabelNameThat().isEqualTo("Owners-Override");
+ assertThat(requiredApprovals).element(0).hasValueThat().isEqualTo(1);
+ assertThat(requiredApprovals).element(1).hasLabelNameThat().isEqualTo("Other-Override");
+ assertThat(requiredApprovals).element(1).hasValueThat().isEqualTo(1);
+ }
+
+ @Test
+ @GerritConfig(name = "plugin.code-owners.overrideApproval", value = "Owners-Override+1")
+ public void
+ overrideApprovalConfiguredOnProjectLevelExtendsGloballyConfiguredOverrideApproval_duplicatesAreFilteredOut()
+ throws Exception {
+ createOwnersOverrideLabel();
+
+ configureOverrideApproval(project, "Owners-Override+1");
+ ImmutableSet<RequiredApproval> requiredApprovals = cfgSnapshot().getOverrideApprovals();
assertThat(requiredApprovals).hasSize(1);
- assertThat(requiredApprovals).element(0).hasLabelNameThat().isEqualTo("Other-Override");
+ assertThat(requiredApprovals).element(0).hasLabelNameThat().isEqualTo("Owners-Override");
assertThat(requiredApprovals).element(0).hasValueThat().isEqualTo(1);
}
@@ -1082,45 +1314,80 @@
@Test
@GerritConfig(name = "plugin.code-owners.overrideApproval", value = "Owners-Override+1")
- public void inheritedOverrideApprovalOverridesGloballyConfiguredOverrideApproval()
+ public void inheritedOverrideApprovalExtendsGloballyConfiguredOverrideApproval()
throws Exception {
createOwnersOverrideLabel();
createOwnersOverrideLabel("Other-Override");
configureOverrideApproval(allProjects, "Other-Override+1");
ImmutableSet<RequiredApproval> requiredApprovals = cfgSnapshot().getOverrideApprovals();
+ assertThat(requiredApprovals).hasSize(2);
+ assertThat(requiredApprovals).element(0).hasLabelNameThat().isEqualTo("Owners-Override");
+ assertThat(requiredApprovals).element(0).hasValueThat().isEqualTo(1);
+ assertThat(requiredApprovals).element(1).hasLabelNameThat().isEqualTo("Other-Override");
+ assertThat(requiredApprovals).element(1).hasValueThat().isEqualTo(1);
+ }
+
+ @Test
+ @GerritConfig(name = "plugin.code-owners.overrideApproval", value = "Owners-Override+1")
+ public void
+ inheritedOverrideApprovalExtendsGloballyConfiguredOverrideApproval_duplicatesAreFilteredOut()
+ throws Exception {
+ createOwnersOverrideLabel();
+
+ configureOverrideApproval(allProjects, "Owners-Override+1");
+ ImmutableSet<RequiredApproval> requiredApprovals = cfgSnapshot().getOverrideApprovals();
assertThat(requiredApprovals).hasSize(1);
- assertThat(requiredApprovals).element(0).hasLabelNameThat().isEqualTo("Other-Override");
+ assertThat(requiredApprovals).element(0).hasLabelNameThat().isEqualTo("Owners-Override");
assertThat(requiredApprovals).element(0).hasValueThat().isEqualTo(1);
}
@Test
- public void projectLevelOverrideApprovalOverridesInheritedOverrideApproval() throws Exception {
+ public void projectLevelOverrideApprovalExtendsInheritedOverrideApproval() throws Exception {
createOwnersOverrideLabel();
createOwnersOverrideLabel("Other-Override");
configureOverrideApproval(allProjects, "Owners-Override+1");
configureOverrideApproval(project, "Other-Override+1");
ImmutableSet<RequiredApproval> requiredApprovals = cfgSnapshot().getOverrideApprovals();
- assertThat(requiredApprovals).hasSize(1);
- assertThat(requiredApprovals).element(0).hasLabelNameThat().isEqualTo("Other-Override");
+ assertThat(requiredApprovals).hasSize(2);
+ assertThat(requiredApprovals).element(0).hasLabelNameThat().isEqualTo("Owners-Override");
assertThat(requiredApprovals).element(0).hasValueThat().isEqualTo(1);
+ assertThat(requiredApprovals).element(1).hasLabelNameThat().isEqualTo("Other-Override");
+ assertThat(requiredApprovals).element(1).hasValueThat().isEqualTo(1);
}
@Test
public void
- projectLevelOverrideApprovalOverridesInheritedOverrideApprovalWithDifferentLabelValue()
+ projectLevelOverrideApprovalExtendsInheritedOverrideApproval_duplicatesAreFilteredOut()
throws Exception {
+ createOwnersOverrideLabel();
+
+ configureOverrideApproval(allProjects, "Owners-Override+1");
+ configureOverrideApproval(project, "Owners-Override+1");
+ ImmutableSet<RequiredApproval> requiredApprovals = cfgSnapshot().getOverrideApprovals();
+ assertThat(requiredApprovals).hasSize(1);
+ assertThat(requiredApprovals).element(0).hasLabelNameThat().isEqualTo("Owners-Override");
+ assertThat(requiredApprovals).element(0).hasValueThat().isEqualTo(1);
+ }
+
+ @Test
+ public void projectLevelOverrideApprovalExtendsInheritedOverrideApprovalWithDifferentLabelValue()
+ throws Exception {
LabelDefinitionInput input = new LabelDefinitionInput();
input.values = ImmutableMap.of("+2", "Super-Override", "+1", "Override", " 0", "No Override");
gApi.projects().name(project.get()).label("Owners-Override").create(input).get();
configureOverrideApproval(allProjects, "Owners-Override+1");
configureOverrideApproval(project, "Owners-Override+2");
+
+ // if the same label is configured multiple times as override approval, only the definition with
+ // the lowest value is returned (since all higher values are implicitly considered as overrides
+ // as well)
ImmutableSet<RequiredApproval> requiredApprovals = cfgSnapshot().getOverrideApprovals();
assertThat(requiredApprovals).hasSize(1);
assertThat(requiredApprovals).element(0).hasLabelNameThat().isEqualTo("Owners-Override");
- assertThat(requiredApprovals).element(0).hasValueThat().isEqualTo(2);
+ assertThat(requiredApprovals).element(0).hasValueThat().isEqualTo(1);
}
@Test
diff --git a/javatests/com/google/gerrit/plugins/codeowners/backend/config/CodeOwnersPluginConfigTest.java b/javatests/com/google/gerrit/plugins/codeowners/backend/config/CodeOwnersPluginConfigTest.java
new file mode 100644
index 0000000..9ca0339
--- /dev/null
+++ b/javatests/com/google/gerrit/plugins/codeowners/backend/config/CodeOwnersPluginConfigTest.java
@@ -0,0 +1,476 @@
+// 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.config;
+
+import static com.google.common.truth.Truth.assertThat;
+
+import com.google.common.collect.ImmutableList;
+import com.google.gerrit.acceptance.testsuite.project.ProjectOperations;
+import com.google.gerrit.common.Nullable;
+import com.google.gerrit.entities.Project;
+import com.google.gerrit.entities.RefNames;
+import com.google.gerrit.plugins.codeowners.acceptance.AbstractCodeOwnersTest;
+import com.google.inject.Inject;
+import org.eclipse.jgit.junit.TestRepository;
+import org.eclipse.jgit.lib.Config;
+import org.eclipse.jgit.lib.Ref;
+import org.eclipse.jgit.lib.Repository;
+import org.eclipse.jgit.revwalk.RevCommit;
+import org.junit.Before;
+import org.junit.Test;
+
+/** Tests for {@link CodeOwnersPluginConfig}. */
+public class CodeOwnersPluginConfigTest extends AbstractCodeOwnersTest {
+ private static final String SECTION = CodeOwnersPluginConfiguration.SECTION_CODE_OWNERS;
+ private static final String SUBSECTION = "subsection";
+ private static final String SUBSECTION_2 = "subsection2";
+ private static final String SUBSECTION_3 = "subsection3";
+ private static final String KEY = "key";
+ private static final String VALUE = "foo";
+ private static final String VALUE_2 = "bar";
+ private static final String VALUE_3 = "baz";
+ private static final String VALUE_4 = "foo_bar";
+ private static final String VALUE_5 = "foo_baz";
+ private static final String VALUE_6 = "bar_foo";
+
+ @Inject private ProjectOperations projectOperations;
+
+ private CodeOwnersPluginConfig.Factory codeOwnersPluginConfigFactory;
+ private Project.NameKey parent;
+
+ @Before
+ public void setUpCodeOwnersPlugin() throws Exception {
+ codeOwnersPluginConfigFactory =
+ plugin.getSysInjector().getInstance(CodeOwnersPluginConfig.Factory.class);
+ }
+
+ @Before
+ public void setUpProject() throws Exception {
+ parent = project;
+ project = projectOperations.newProject().parent(parent).create();
+ }
+
+ @Test
+ public void getSingleValue_noValueSet() throws Exception {
+ assertThat(cfg().getString(SECTION, /* subsection= */ null, KEY)).isNull();
+ }
+
+ @Test
+ public void getSingleValue_singleValueSet() throws Exception {
+ setSingleValue(project, VALUE);
+ assertThat(cfg().getString(SECTION, /* subsection= */ null, KEY)).isEqualTo(VALUE);
+ }
+
+ @Test
+ public void getSingleValue_multiValueSet() throws Exception {
+ setMultiValue(project, VALUE, VALUE_2, VALUE_3);
+
+ // last value takes precedence
+ assertThat(cfg().getString(SECTION, /* subsection= */ null, KEY)).isEqualTo(VALUE_3);
+ }
+
+ @Test
+ public void getSingleValue_singleValueSetForParent() throws Exception {
+ setSingleValue(parent, VALUE);
+ assertThat(cfg().getString(SECTION, /* subsection= */ null, KEY)).isEqualTo(VALUE);
+ }
+
+ @Test
+ public void getSingleValue_multiValueSetForParent() throws Exception {
+ setMultiValue(parent, VALUE, VALUE_2, VALUE_3);
+
+ // last value takes precedence
+ assertThat(cfg().getString(SECTION, /* subsection= */ null, KEY)).isEqualTo(VALUE_3);
+ }
+
+ @Test
+ public void getSingleValue_valueOverridesSingleParentValues() throws Exception {
+ setSingleValue(allProjects, VALUE);
+ setSingleValue(parent, VALUE_2);
+ setSingleValue(project, VALUE_3);
+ assertThat(cfg().getString(SECTION, /* subsection= */ null, KEY)).isEqualTo(VALUE_3);
+ }
+
+ @Test
+ public void getSingleValue_valueOverridesMultiParentValues() throws Exception {
+ setMultiValue(allProjects, VALUE, VALUE_2);
+ setMultiValue(parent, VALUE_3, VALUE_4);
+ setSingleValue(project, VALUE_5);
+ assertThat(cfg().getString(SECTION, /* subsection= */ null, KEY)).isEqualTo(VALUE_5);
+ }
+
+ @Test
+ public void getSingleValue_unsetSingleParentValues() throws Exception {
+ setSingleValue(allProjects, VALUE);
+ setSingleValue(parent, VALUE_2);
+ setSingleValue(project, "");
+ assertThat(cfg().getString(SECTION, /* subsection= */ null, KEY)).isNull();
+ }
+
+ @Test
+ public void getSingleValue_unsetMultiParentValues() throws Exception {
+ setMultiValue(allProjects, VALUE, VALUE_2);
+ setMultiValue(parent, VALUE_3, VALUE_4);
+ setSingleValue(project, "");
+ assertThat(cfg().getString(SECTION, /* subsection= */ null, KEY)).isNull();
+ }
+
+ @Test
+ public void getSingleValueFromSubsection_noValueSet() throws Exception {
+ assertThat(cfg().getString(SECTION, SUBSECTION, KEY)).isNull();
+ }
+
+ @Test
+ public void getSingleValueFromSubsection_singleValueSet() throws Exception {
+ setSingleValueForSubsection(project, SUBSECTION, VALUE);
+ assertThat(cfg().getString(SECTION, SUBSECTION, KEY)).isEqualTo(VALUE);
+ }
+
+ @Test
+ public void getSingleValueFromSubsection_multiValueSet() throws Exception {
+ setMultiValueForSubsection(project, SUBSECTION, VALUE, VALUE_2, VALUE_3);
+
+ // last value takes precedence
+ assertThat(cfg().getString(SECTION, SUBSECTION, KEY)).isEqualTo(VALUE_3);
+ }
+
+ @Test
+ public void getSingleValueFromSubsection_singleValueSetForParent() throws Exception {
+ setSingleValueForSubsection(parent, SUBSECTION, VALUE);
+ assertThat(cfg().getString(SECTION, SUBSECTION, KEY)).isEqualTo(VALUE);
+ }
+
+ @Test
+ public void getSingleValueFromSubsection_multiValueSetForParent() throws Exception {
+ setMultiValueForSubsection(parent, SUBSECTION, VALUE, VALUE_2, VALUE_3);
+
+ // last value takes precedence
+ assertThat(cfg().getString(SECTION, SUBSECTION, KEY)).isEqualTo(VALUE_3);
+ }
+
+ @Test
+ public void getSingleValueFromSubsection_valueOverridesSingleParentValues() throws Exception {
+ setSingleValueForSubsection(allProjects, SUBSECTION, VALUE);
+ setSingleValueForSubsection(parent, SUBSECTION, VALUE_2);
+ setSingleValueForSubsection(project, SUBSECTION, VALUE_3);
+ assertThat(cfg().getString(SECTION, SUBSECTION, KEY)).isEqualTo(VALUE_3);
+ }
+
+ @Test
+ public void getSingleValueFromSubsection_valueOverridesMultiParentValues() throws Exception {
+ setMultiValueForSubsection(allProjects, SUBSECTION, VALUE, VALUE_2);
+ setMultiValueForSubsection(parent, SUBSECTION, VALUE_3, VALUE_4);
+ setSingleValueForSubsection(project, SUBSECTION, VALUE_5);
+ assertThat(cfg().getString(SECTION, SUBSECTION, KEY)).isEqualTo(VALUE_5);
+ }
+
+ @Test
+ public void getSingleValueFromSubsection_unsetSingleParentValues() throws Exception {
+ setSingleValueForSubsection(allProjects, SUBSECTION, VALUE);
+ setSingleValueForSubsection(parent, SUBSECTION, VALUE_2);
+ setSingleValueForSubsection(project, SUBSECTION, "");
+ assertThat(cfg().getString(SECTION, SUBSECTION, KEY)).isNull();
+ }
+
+ @Test
+ public void getSingleValueFromSubsection_unsetMultiParentValues() throws Exception {
+ setMultiValueForSubsection(allProjects, SUBSECTION, VALUE, VALUE_2);
+ setMultiValueForSubsection(parent, SUBSECTION, VALUE_3, VALUE_4);
+ setSingleValueForSubsection(project, SUBSECTION, "");
+ assertThat(cfg().getString(SECTION, SUBSECTION, KEY)).isNull();
+ }
+
+ @Test
+ public void getMultiValue_noValueSet() throws Exception {
+ assertThat(cfg().getStringList(SECTION, /* subsection= */ null, KEY)).isEmpty();
+ }
+
+ @Test
+ public void getMultiValue_singleValueSet() throws Exception {
+ setSingleValue(project, VALUE);
+ assertThat(cfg().getStringList(SECTION, /* subsection= */ null, KEY))
+ .asList()
+ .containsExactly(VALUE);
+ }
+
+ @Test
+ public void getMultiValue_multiValueSet() throws Exception {
+ setMultiValue(project, VALUE, VALUE_2, VALUE_3);
+ assertThat(cfg().getStringList(SECTION, /* subsection= */ null, KEY))
+ .asList()
+ .containsExactly(VALUE, VALUE_2, VALUE_3)
+ .inOrder();
+ }
+
+ @Test
+ public void getMultiValue_singleValueSetForParent() throws Exception {
+ setSingleValue(parent, VALUE);
+ assertThat(cfg().getStringList(SECTION, /* subsection= */ null, KEY))
+ .asList()
+ .containsExactly(VALUE);
+ }
+
+ @Test
+ public void getMultiValue_multiValueSetForParent() throws Exception {
+ setMultiValue(parent, VALUE, VALUE_2, VALUE_3);
+ assertThat(cfg().getStringList(SECTION, /* subsection= */ null, KEY))
+ .asList()
+ .containsExactly(VALUE, VALUE_2, VALUE_3)
+ .inOrder();
+ }
+
+ @Test
+ public void getMultiValue_singleValueExtendsSingleParentValues() throws Exception {
+ setSingleValue(allProjects, VALUE);
+ setSingleValue(parent, VALUE_2);
+ setSingleValue(project, VALUE_3);
+ assertThat(cfg().getStringList(SECTION, /* subsection= */ null, KEY))
+ .asList()
+ .containsExactly(VALUE, VALUE_2, VALUE_3)
+ .inOrder();
+ }
+
+ @Test
+ public void getMultiValue_singleValueOverridesMultiParentValues() throws Exception {
+ setMultiValue(allProjects, VALUE, VALUE_2);
+ setMultiValue(parent, VALUE_3, VALUE_4);
+ setSingleValue(project, VALUE_5);
+ assertThat(cfg().getStringList(SECTION, /* subsection= */ null, KEY))
+ .asList()
+ .containsExactly(VALUE, VALUE_2, VALUE_3, VALUE_4, VALUE_5)
+ .inOrder();
+ }
+
+ @Test
+ public void getMultiValue_multiValueExtendsSingleParentValues() throws Exception {
+ setSingleValue(allProjects, VALUE);
+ setSingleValue(parent, VALUE_2);
+ setMultiValue(project, VALUE_3, VALUE_4);
+ assertThat(cfg().getStringList(SECTION, /* subsection= */ null, KEY))
+ .asList()
+ .containsExactly(VALUE, VALUE_2, VALUE_3, VALUE_4)
+ .inOrder();
+ }
+
+ @Test
+ public void getMultiValue_multiValueExtendsMultiParentValues() throws Exception {
+ setMultiValue(allProjects, VALUE, VALUE_2);
+ setMultiValue(parent, VALUE_3, VALUE_4);
+ setMultiValue(project, VALUE_5, VALUE_6);
+ assertThat(cfg().getStringList(SECTION, /* subsection= */ null, KEY))
+ .asList()
+ .containsExactly(VALUE, VALUE_2, VALUE_3, VALUE_4, VALUE_5, VALUE_6)
+ .inOrder();
+ }
+
+ @Test
+ public void getMultiValue_multiValueExtendsMultiParentValues_withDuplicates() throws Exception {
+ setMultiValue(allProjects, VALUE, VALUE_2);
+ setMultiValue(parent, VALUE_3);
+ setMultiValue(project, VALUE, VALUE_2);
+ assertThat(cfg().getStringList(SECTION, /* subsection= */ null, KEY))
+ .asList()
+ .containsExactly(VALUE, VALUE_2, VALUE_3, VALUE, VALUE_2)
+ .inOrder();
+ }
+
+ @Test
+ public void getMultiValue_cannotUnsetSingleParentValues() throws Exception {
+ setSingleValue(allProjects, VALUE);
+ setSingleValue(parent, VALUE_2);
+ setSingleValue(project, "");
+
+ // the empty string is returned as null value in the list
+ assertThat(cfg().getStringList(SECTION, /* subsection= */ null, KEY))
+ .asList()
+ .containsExactly(VALUE, VALUE_2, null)
+ .inOrder();
+ }
+
+ @Test
+ public void getMultiValue_cannotUnsetMultiParentValues() throws Exception {
+ setMultiValue(allProjects, VALUE, VALUE_2);
+ setMultiValue(parent, VALUE_3, VALUE_4);
+ setSingleValue(project, "");
+
+ // the empty string is returned as null value in the list
+ assertThat(cfg().getStringList(SECTION, /* subsection= */ null, KEY))
+ .asList()
+ .containsExactly(VALUE, VALUE_2, VALUE_3, VALUE_4, null)
+ .inOrder();
+ }
+
+ @Test
+ public void getMultiValueFromSubsection_noValueSet() throws Exception {
+ assertThat(cfg().getStringList(SECTION, SUBSECTION, KEY)).isEmpty();
+ }
+
+ @Test
+ public void getMultiValueFromSubsection_singleValueSet() throws Exception {
+ setSingleValueForSubsection(project, SUBSECTION, VALUE);
+ assertThat(cfg().getStringList(SECTION, SUBSECTION, KEY)).asList().containsExactly(VALUE);
+ }
+
+ @Test
+ public void getMultiValueFromSubsection_multiValueSet() throws Exception {
+ setMultiValueForSubsection(project, SUBSECTION, VALUE, VALUE_2, VALUE_3);
+ assertThat(cfg().getStringList(SECTION, SUBSECTION, KEY))
+ .asList()
+ .containsExactly(VALUE, VALUE_2, VALUE_3)
+ .inOrder();
+ }
+
+ @Test
+ public void getMultiValueFromSubsection_singleValueSetForParent() throws Exception {
+ setSingleValueForSubsection(parent, SUBSECTION, VALUE);
+ assertThat(cfg().getStringList(SECTION, SUBSECTION, KEY)).asList().containsExactly(VALUE);
+ }
+
+ @Test
+ public void getMultiValueFromSubsection_multiValueSetForParent() throws Exception {
+ setMultiValueForSubsection(parent, SUBSECTION, VALUE, VALUE_2, VALUE_3);
+ assertThat(cfg().getStringList(SECTION, SUBSECTION, KEY))
+ .asList()
+ .containsExactly(VALUE, VALUE_2, VALUE_3)
+ .inOrder();
+ }
+
+ @Test
+ public void getMultiValueFromSubsection_singleValueExtendsSingleParentValues() throws Exception {
+ setSingleValueForSubsection(allProjects, SUBSECTION, VALUE);
+ setSingleValueForSubsection(parent, SUBSECTION, VALUE_2);
+ setSingleValueForSubsection(project, SUBSECTION, VALUE_3);
+ assertThat(cfg().getStringList(SECTION, SUBSECTION, KEY))
+ .asList()
+ .containsExactly(VALUE, VALUE_2, VALUE_3)
+ .inOrder();
+ }
+
+ @Test
+ public void getMultiValueFromSubsection_singleValueOverridesMultiParentValues() throws Exception {
+ setMultiValueForSubsection(allProjects, SUBSECTION, VALUE, VALUE_2);
+ setMultiValueForSubsection(parent, SUBSECTION, VALUE_3, VALUE_4);
+ setSingleValueForSubsection(project, SUBSECTION, VALUE_5);
+ assertThat(cfg().getStringList(SECTION, SUBSECTION, KEY))
+ .asList()
+ .containsExactly(VALUE, VALUE_2, VALUE_3, VALUE_4, VALUE_5)
+ .inOrder();
+ }
+
+ @Test
+ public void getMultiValueFromSubsection_multiValueExtendsSingleParentValues() throws Exception {
+ setSingleValueForSubsection(allProjects, SUBSECTION, VALUE);
+ setSingleValueForSubsection(parent, SUBSECTION, VALUE_2);
+ setMultiValueForSubsection(project, SUBSECTION, VALUE_3, VALUE_4);
+ assertThat(cfg().getStringList(SECTION, SUBSECTION, KEY))
+ .asList()
+ .containsExactly(VALUE, VALUE_2, VALUE_3, VALUE_4)
+ .inOrder();
+ }
+
+ @Test
+ public void getMultiValueFromSubsection_multiValueExtendsMultiParentValues() throws Exception {
+ setMultiValueForSubsection(allProjects, SUBSECTION, VALUE, VALUE_2);
+ setMultiValueForSubsection(parent, SUBSECTION, VALUE_3, VALUE_4);
+ setMultiValueForSubsection(project, SUBSECTION, VALUE_5, VALUE_6);
+ assertThat(cfg().getStringList(SECTION, SUBSECTION, KEY))
+ .asList()
+ .containsExactly(VALUE, VALUE_2, VALUE_3, VALUE_4, VALUE_5, VALUE_6)
+ .inOrder();
+ }
+
+ @Test
+ public void getMultiValueFromSubsection_cannotUnsetSingleParentValues() throws Exception {
+ setSingleValueForSubsection(allProjects, SUBSECTION, VALUE);
+ setSingleValueForSubsection(parent, SUBSECTION, VALUE_2);
+ setSingleValueForSubsection(project, SUBSECTION, "");
+
+ // the empty string is returned as null value in the list
+ assertThat(cfg().getStringList(SECTION, SUBSECTION, KEY))
+ .asList()
+ .containsExactly(VALUE, VALUE_2, null)
+ .inOrder();
+ }
+
+ @Test
+ public void getMultiValueFromSubsection_cannotUnsetMultiParentValues() throws Exception {
+ setMultiValueForSubsection(allProjects, SUBSECTION, VALUE, VALUE_2);
+ setMultiValueForSubsection(parent, SUBSECTION, VALUE_3, VALUE_4);
+ setSingleValueForSubsection(project, SUBSECTION, "");
+
+ // the empty string is returned as null value in the list
+ assertThat(cfg().getStringList(SECTION, SUBSECTION, KEY))
+ .asList()
+ .containsExactly(VALUE, VALUE_2, VALUE_3, VALUE_4, null)
+ .inOrder();
+ }
+
+ @Test
+ public void getSubsections() throws Exception {
+ setSingleValueForSubsection(allProjects, SUBSECTION, VALUE);
+ setSingleValueForSubsection(parent, SUBSECTION_2, VALUE_2);
+ setSingleValueForSubsection(project, SUBSECTION_3, VALUE_3);
+ assertThat(cfg().getSubsections(SECTION))
+ .containsExactly(SUBSECTION, SUBSECTION_2, SUBSECTION_3);
+ }
+
+ @Test
+ public void getEmptySubsections() throws Exception {
+ createConfigWithEmptySubsection(allProjects, SUBSECTION);
+ createConfigWithEmptySubsection(parent, SUBSECTION_2);
+ createConfigWithEmptySubsection(project, SUBSECTION_3);
+ assertThat(cfg().getSubsections(SECTION))
+ .containsExactly(SUBSECTION, SUBSECTION_2, SUBSECTION_3);
+ }
+
+ private Config cfg() {
+ return codeOwnersPluginConfigFactory.create(project).get();
+ }
+
+ private void setSingleValue(Project.NameKey project, String value) throws Exception {
+ setSingleValueForSubsection(project, /* subsection= */ null, value);
+ }
+
+ private void setSingleValueForSubsection(
+ Project.NameKey project, @Nullable String subsection, String value) throws Exception {
+ setCodeOwnersConfig(project, subsection, KEY, value);
+ }
+
+ private void setMultiValue(Project.NameKey project, String... values) throws Exception {
+ setMultiValueForSubsection(project, /* subsection= */ null, values);
+ }
+
+ private void setMultiValueForSubsection(
+ Project.NameKey project, @Nullable String subsection, String... values) throws Exception {
+ setCodeOwnersConfig(project, subsection, KEY, ImmutableList.copyOf(values));
+ }
+
+ private void createConfigWithEmptySubsection(Project.NameKey project, String subsection)
+ throws Exception {
+ try (TestRepository<Repository> testRepo =
+ new TestRepository<>(repoManager.openRepository(project))) {
+ Ref ref = testRepo.getRepository().exactRef(RefNames.REFS_CONFIG);
+ RevCommit head = testRepo.getRevWalk().parseCommit(ref.getObjectId());
+ testRepo.update(
+ RefNames.REFS_CONFIG,
+ testRepo
+ .commit()
+ .parent(head)
+ .message("Configure code owner backend")
+ .add("code-owners.config", String.format("[%s \"%s\"]", SECTION, subsection)));
+ }
+ projectCache.evict(project);
+ }
+}
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 aed1651..dc6692f 100644
--- a/javatests/com/google/gerrit/plugins/codeowners/backend/config/GeneralConfigTest.java
+++ b/javatests/com/google/gerrit/plugins/codeowners/backend/config/GeneralConfigTest.java
@@ -1355,23 +1355,49 @@
@GerritConfig(
name = "plugin.code-owners.globalCodeOwner",
values = {"bot1@example.com", "bot2@example.com"})
- public void globalCodeOnwersInPluginConfigOverrideGlobalCodeOwnersInGerritConfig()
+ public void globalCodeOwnersInPluginConfigExtendGlobalCodeOwnersInGerritConfig()
throws Exception {
Config cfg = new Config();
cfg.setString(
SECTION_CODE_OWNERS, /* subsection= */ null, KEY_GLOBAL_CODE_OWNER, "bot3@example.com");
assertThat(generalConfig.getGlobalCodeOwners(cfg))
- .containsExactly(CodeOwnerReference.create("bot3@example.com"));
+ .containsExactly(
+ CodeOwnerReference.create("bot1@example.com"),
+ CodeOwnerReference.create("bot2@example.com"),
+ CodeOwnerReference.create("bot3@example.com"));
}
@Test
@GerritConfig(
name = "plugin.code-owners.globalCodeOwner",
values = {"bot1@example.com", "bot2@example.com"})
- public void inheritedGlobalOwnersCanBeRemovedOnProjectLevel() throws Exception {
+ public void
+ globalCodeOwnersInPluginConfigExtendGlobalCodeOwnersInGerritConfig_duplicatesFilteredOut()
+ throws Exception {
+ Config cfg = new Config();
+ cfg.setStringList(
+ SECTION_CODE_OWNERS,
+ /* subsection= */ null,
+ KEY_GLOBAL_CODE_OWNER,
+ ImmutableList.of("bot1@example.com", "bot3@example.com"));
+ assertThat(generalConfig.getGlobalCodeOwners(cfg))
+ .containsExactly(
+ CodeOwnerReference.create("bot1@example.com"),
+ CodeOwnerReference.create("bot2@example.com"),
+ CodeOwnerReference.create("bot3@example.com"));
+ }
+
+ @Test
+ @GerritConfig(
+ name = "plugin.code-owners.globalCodeOwner",
+ values = {"bot1@example.com", "bot2@example.com"})
+ public void inheritedGlobalOwnersCannotBeRemovedOnProjectLevel() throws Exception {
Config cfg = new Config();
cfg.setString(SECTION_CODE_OWNERS, /* subsection= */ null, KEY_GLOBAL_CODE_OWNER, "");
- assertThat(generalConfig.getGlobalCodeOwners(cfg)).isEmpty();
+ assertThat(generalConfig.getGlobalCodeOwners(cfg))
+ .containsExactly(
+ CodeOwnerReference.create("bot1@example.com"),
+ CodeOwnerReference.create("bot2@example.com"));
}
@Test
@@ -1402,21 +1428,39 @@
@GerritConfig(
name = "plugin.code-owners.exemptedUser",
values = {"bot1@example.com", "bot2@example.com"})
- public void exemptedUsersInPluginConfigOverrideExemptedUsersInGerritConfig() throws Exception {
+ public void exemptedUsersInPluginConfigExtendExemptedUsersInGerritConfig() throws Exception {
Config cfg = new Config();
cfg.setString(
SECTION_CODE_OWNERS, /* subsection= */ null, KEY_EXEMPTED_USER, "bot3@example.com");
- assertThat(generalConfig.getExemptedUsers(cfg)).containsExactly("bot3@example.com");
+ assertThat(generalConfig.getExemptedUsers(cfg))
+ .containsExactly("bot1@example.com", "bot2@example.com", "bot3@example.com");
}
@Test
@GerritConfig(
name = "plugin.code-owners.exemptedUser",
values = {"bot1@example.com", "bot2@example.com"})
- public void inheritedExemptedUsersCanBeRemovedOnProjectLevel() throws Exception {
+ public void exemptedUsersInPluginConfigExtendExemptedUsersInGerritConfig_duplicatesFilteredOut()
+ throws Exception {
+ Config cfg = new Config();
+ cfg.setStringList(
+ SECTION_CODE_OWNERS,
+ /* subsection= */ null,
+ KEY_EXEMPTED_USER,
+ ImmutableList.of("bot1@example.com", "bot3@example.com"));
+ assertThat(generalConfig.getExemptedUsers(cfg))
+ .containsExactly("bot1@example.com", "bot2@example.com", "bot3@example.com");
+ }
+
+ @Test
+ @GerritConfig(
+ name = "plugin.code-owners.exemptedUser",
+ values = {"bot1@example.com", "bot2@example.com"})
+ public void inheritedExemptedUsersCannotBeRemovedOnProjectLevel() throws Exception {
Config cfg = new Config();
cfg.setString(SECTION_CODE_OWNERS, /* subsection= */ null, KEY_EXEMPTED_USER, "");
- assertThat(generalConfig.getExemptedUsers(cfg)).isEmpty();
+ assertThat(generalConfig.getExemptedUsers(cfg))
+ .containsExactly("bot1@example.com", "bot2@example.com");
}
@Test
diff --git a/javatests/com/google/gerrit/plugins/codeowners/backend/config/StatusConfigTest.java b/javatests/com/google/gerrit/plugins/codeowners/backend/config/StatusConfigTest.java
index 4e82aeb..1537c55 100644
--- a/javatests/com/google/gerrit/plugins/codeowners/backend/config/StatusConfigTest.java
+++ b/javatests/com/google/gerrit/plugins/codeowners/backend/config/StatusConfigTest.java
@@ -235,12 +235,12 @@
@Test
@GerritConfig(name = "plugin.code-owners.disabledBranch", value = "refs/heads/master")
public void
- disabledBranchConfigurationInPluginConfigOverridesDisabledBranchConfigurationInGerritConfig()
+ disabledBranchConfigurationInPluginConfigExtendsDisabledBranchConfigurationInGerritConfig()
throws Exception {
Config cfg = new Config();
cfg.setString(SECTION_CODE_OWNERS, null, KEY_DISABLED_BRANCH, "refs/heads/test");
assertThat(statusConfig.isDisabledForBranch(cfg, BranchNameKey.create(project, "master")))
- .isFalse();
+ .isTrue();
assertThat(statusConfig.isDisabledForBranch(cfg, BranchNameKey.create(project, "test")))
.isTrue();
}
@@ -248,12 +248,12 @@
@Test
@GerritConfig(name = "plugin.code-owners.disabledBranch", value = "refs/heads/master")
public void
- disabledBranchConfigurationInPluginConfigCanRemoveDisabledBranchConfigurationInGerritConfig()
+ disabledBranchConfigurationInPluginConfigCannotRemoveDisabledBranchConfigurationInGerritConfig()
throws Exception {
Config cfg = new Config();
cfg.setString(SECTION_CODE_OWNERS, null, KEY_DISABLED_BRANCH, "");
assertThat(statusConfig.isDisabledForBranch(cfg, BranchNameKey.create(project, "master")))
- .isFalse();
+ .isTrue();
}
@Test
diff --git a/resources/Documentation/config.md b/resources/Documentation/config.md
index dd7cd35..76c6c91 100644
--- a/resources/Documentation/config.md
+++ b/resources/Documentation/config.md
@@ -17,14 +17,71 @@
of parent projects until the `All-Projects` root project is reached which
inherits the configuration from `gerrit.config`.
-Setting a configuration parameter for a project overrides any inherited value
-for this configuration parameter.
+Setting a single-value configuration parameter (single string, boolean, enum,
+int, long) for a project overrides any inherited value for this configuration
+parameter.
-**NOTE:** Some configuration parameters have a list of values and can be
-specified multiple times (e.g. `disabledBranch`). If such a value is set on
-project level it means that the complete inherited list is overridden. It's
-*not* possible to just add a value to the inherited list, but if this is wanted
-the complete list with the additional value has to be set on project level.
+Example for single-value configuration parameters:
+
+parent `code-owners.config`:
+```
+ [codeOwners]
+ readOnly = true
+ overrideInfoUrl = https://owners-overrides.example.com
+ exemptPureReverts = true
+```
+\
+project `code-owners.config`:
+```
+ [codeOwners]
+ readOnly = false
+ overrideInfoUrl = https://foo.example.com/owners-overrides
+ fileExtension = fork
+```
+\
+effective configuration:
+```
+ [code-owners]
+ readOnly = false
+ overrideInfoUrl = https://foo.example.com/owners-overrides
+ fileExtension = fork
+ exemptPureReverts = true
+```
+\
+In contrast to this, if a value for a multi-value / list configuration parameter
+is set, the value is added to the inherited value list (the inherited value list
+is extended, not overridden). Overriding/unsetting an inherited value list is
+not possible.
+
+Example for multi-value / list configuration parameters:
+
+parent `code-owners.config`:
+```
+ [codeOwners]
+ globalCodeOwner = bot-foo@example.com
+ globalCodeOwner = bot-bar@example.com
+ exemptedUser = bot-abc@example.com
+ exemptedUser = bot-xyz@example.com
+ disabledBranch = refs/meta/config
+```
+\
+project `code-owners.config`:
+```
+ [codeOwners]
+ globalCodeOwner = bot-baz@example.com
+ disabledBranch =
+```
+\
+effective configuration:
+```
+ [code-owners]
+ globalCodeOwner = bot-foo@example.com
+ globalCodeOwner = bot-bar@example.com
+ globalCodeOwner = bot-baz@example.com
+ exemptedUser = bot-abc@example.com
+ exemptedUser = bot-xyz@example.com
+ disabledBranch = refs/meta/config
+```
## <a id="staleIndexOnConfigChanges">
**NOTE:** Some configuration changes can lead to changes becoming stale in the
@@ -65,7 +122,7 @@
approvals.\
This allows branches to opt-out of the code owners functionality.\
Can be set multiple times.\
- Can be overridden per project by setting
+ The configured value list can be extended on project-level by setting
[codeOwners.disabledBranch](#codeOwnersDisabledBranch) in
`@PLUGIN@.config`.\
By default unset.
@@ -151,7 +208,7 @@
can be added to the `Service Users` group (since members of this group
are not suggested as code owners).\
Can be specified multiple times to set multiple global code owners.\
- Can be overridden per project by setting
+ The configured value list can be extended on project-level by setting
[codeOwners.globalCodeOwner](#codeOwnersGlobalCodeOwner) in
`@PLUGIN@.config`.\
By default unset (no global code owners).
@@ -162,7 +219,7 @@
If a user is exempted from requiring code owner approvals changes that
are uploaded by this user are automatically code-owner approved.\
Can be specified multiple times to exempt multiple users.\
- Can be overridden per project by setting
+ The configured value list can be extended on project-level by setting
[codeOwners.exemptedUser](#codeOwnersExemptedUser) in
`@PLUGIN@.config`.\
By default unset (no exempted users).
@@ -350,7 +407,7 @@
approvals](../../../Documentation/config-labels.html#label_ignoreSelfApproval)
from the uploader, any override vote from the uploader on that label is
ignored for the code owners check.\
- Can be overridden per project by setting
+ The configured value list can be extended on project-level by setting
[codeOwners.overrideApproval](#codeOwnersOverrideApproval) in
`@PLUGIN@.config`.\
By default unset which means that the override functionality is
@@ -496,7 +553,7 @@
approvals.\
This allows branches to opt-out of the code owners functionality.\
Can be set multiple times.\
- Overrides the global setting
+ Extends the global setting
[plugin.@PLUGIN@.disabledBranch](#pluginCodeOwnersDisabledBranch) in
`gerrit.config` and the `codeOwners.disabledBranch` setting from parent
projects.\
@@ -600,7 +657,7 @@
can be added to the `Service Users` group (since members of this group
are not suggested as code owners).\
Can be specified multiple times to set multiple global code owners.\
- Overrides the global setting
+ Extends the global setting
[plugin.@PLUGIN@.globalCodeOwner](#pluginCodeOwnersGlobalCodeOwner) in
`gerrit.config` and the `codeOwners.globalCodeOwner` setting from parent
projects.\
@@ -614,7 +671,7 @@
If a user is exempted from requiring code owner approvals changes that
are uploaded by this user are automatically code-owner approved.\
Can be specified multiple times to exempt multiple users.\
- Overrides the global setting
+ Extends the global setting
[plugin.@PLUGIN@.exemptedUser](#pluginCodeOwnersExemptedUser) in
`gerrit.config` and the `codeOwners.exemptedUser` setting from parent
projects.\
@@ -827,7 +884,7 @@
approvals](../../../Documentation/config-labels.html#label_ignoreSelfApproval)
from the uploader, any override vote from the uploader on that label is
ignored for the code owners check.\
- Overrides the global setting
+ Extends the global setting
[plugin.@PLUGIN@.overrideApproval](#pluginCodeOwnersOverrideApproval) in
`gerrit.config` and the `codeOwners.overrideApproval` setting from
parent projects.\
