Merge changes Ie5b99c2a,I0746546c,I40349324,Idf584990,If008a747, ...
* changes:
config-faqs: Explain how to define default code owners
Add metric to count submit rule errors
config.md: Fix bad link
CodeOwnerSubmitRule: Report metric for invalid OWNERS files
Improve error message if CodeOwnerSubmitRule fails due invalid OWNERS file
Move getParsingErrorMessage method up to test base class
Test that CodeOwnerSubmitRule returns rule error if OWNERS file is invalid
Fix names of code owner metrics
Fix counting submit rule runs as metric
Move method to get code owner config file name up to base test class
Add metric that counts how often invalid OWNERS files cause failures
Allow to configure an info URL that is shown if an OWNERS file is invalid
GeneralConfig: Factor out a method to read a string value
GeneralConfig: Fix variable name
Add specific exception to signal invalid code owner config files
CheckCodeOwner: Handle files owned by all users
Config doc: Clarify that this is not the config that defines code owners
Document that keeping the global config in All-Projects may be dangerous
User guide: Make clear that label functions are orthogonal to code owners
User guide: Explain when code owner submit records in the index get stale
User guide: Add section about code owners submit rule
Disallow projects to override inherited list configuration parameters
diff --git a/java/com/google/gerrit/plugins/codeowners/acceptance/AbstractCodeOwnersTest.java b/java/com/google/gerrit/plugins/codeowners/acceptance/AbstractCodeOwnersTest.java
index c85c506..13c3b3e 100644
--- a/java/com/google/gerrit/plugins/codeowners/acceptance/AbstractCodeOwnersTest.java
+++ b/java/com/google/gerrit/plugins/codeowners/acceptance/AbstractCodeOwnersTest.java
@@ -14,6 +14,7 @@
package com.google.gerrit.plugins.codeowners.acceptance;
+import static com.google.common.truth.Truth.assertThat;
import static com.google.gerrit.server.group.SystemGroupBackend.REGISTERED_USERS;
import com.google.common.collect.ImmutableList;
@@ -36,8 +37,12 @@
import com.google.gerrit.extensions.restapi.RestApiException;
import com.google.gerrit.plugins.codeowners.acceptance.testsuite.CodeOwnerConfigOperations;
import com.google.gerrit.plugins.codeowners.acceptance.testsuite.TestCodeOwnerConfigCreation.Builder;
+import com.google.gerrit.plugins.codeowners.backend.CodeOwnerBackend;
+import com.google.gerrit.plugins.codeowners.backend.config.BackendConfig;
import com.google.gerrit.plugins.codeowners.backend.config.CodeOwnersPluginConfiguration;
import com.google.gerrit.plugins.codeowners.backend.config.StatusConfig;
+import com.google.gerrit.plugins.codeowners.backend.findowners.FindOwnersBackend;
+import com.google.gerrit.plugins.codeowners.backend.proto.ProtoBackend;
import com.google.gerrit.plugins.codeowners.util.JgitPath;
import com.google.inject.Inject;
import java.nio.file.Path;
@@ -67,11 +72,13 @@
@Inject private ProjectOperations projectOperations;
private CodeOwnerConfigOperations codeOwnerConfigOperations;
+ private BackendConfig backendConfig;
@Before
public void testSetup() throws Exception {
codeOwnerConfigOperations =
plugin.getSysInjector().getInstance(CodeOwnerConfigOperations.class);
+ backendConfig = plugin.getSysInjector().getInstance(BackendConfig.class);
}
protected String createChangeWithFileDeletion(Path filePath) throws Exception {
@@ -225,6 +232,26 @@
}
/**
+ * Returns the parsing error message for the non-parseable code owner config that was created by
+ * {@link #createNonParseableCodeOwnerConfig(String)}.
+ */
+ protected String getParsingErrorMessageForNonParseableCodeOwnerConfig() {
+ return getParsingErrorMessage(
+ ImmutableMap.of(
+ FindOwnersBackend.class,
+ "invalid line: INVALID",
+ ProtoBackend.class,
+ "1:8: Expected \"{\"."));
+ }
+
+ protected String getParsingErrorMessage(
+ ImmutableMap<Class<? extends CodeOwnerBackend>, String> messagesByBackend) {
+ CodeOwnerBackend codeOwnerBackend = backendConfig.getDefaultBackend();
+ assertThat(messagesByBackend).containsKey(codeOwnerBackend.getClass());
+ return messagesByBackend.get(codeOwnerBackend.getClass());
+ }
+
+ /**
* Creates a default code owner config with the given test accounts as code owners.
*
* @param testAccounts the accounts of the users that should be code owners
@@ -303,4 +330,14 @@
PushOneCommit push = pushFactory.create(admin.newIdent(), testRepo, subject, files);
return push.to("refs/for/master");
}
+
+ protected String getCodeOwnerConfigFileName() {
+ CodeOwnerBackend backend = backendConfig.getDefaultBackend();
+ if (backend instanceof FindOwnersBackend) {
+ return FindOwnersBackend.CODE_OWNER_CONFIG_FILE_NAME;
+ } else if (backend instanceof ProtoBackend) {
+ return ProtoBackend.CODE_OWNER_CONFIG_FILE_NAME;
+ }
+ throw new IllegalStateException("unknown code owner backend: " + backend.getClass().getName());
+ }
}
diff --git a/java/com/google/gerrit/plugins/codeowners/api/CodeOwnerCheckInfo.java b/java/com/google/gerrit/plugins/codeowners/api/CodeOwnerCheckInfo.java
index ec3cb4f..c22449d 100644
--- a/java/com/google/gerrit/plugins/codeowners/api/CodeOwnerCheckInfo.java
+++ b/java/com/google/gerrit/plugins/codeowners/api/CodeOwnerCheckInfo.java
@@ -83,6 +83,9 @@
*/
public boolean isGlobalCodeOwner;
+ /** Whether the the specified path in the branch is owned by all users (aka {@code *}). */
+ public boolean isOwnedByAllUsers;
+
/** Debug logs that may help to understand why the user is or isn't a code owner. */
public List<String> debugLogs;
}
diff --git a/java/com/google/gerrit/plugins/codeowners/api/CodeOwnerProjectConfigInput.java b/java/com/google/gerrit/plugins/codeowners/api/CodeOwnerProjectConfigInput.java
index 0cd47fa..8bc182e 100644
--- a/java/com/google/gerrit/plugins/codeowners/api/CodeOwnerProjectConfigInput.java
+++ b/java/com/google/gerrit/plugins/codeowners/api/CodeOwnerProjectConfigInput.java
@@ -86,6 +86,12 @@
*/
public String overrideInfoUrl;
+ /**
+ * URL for a page that provides project/host-specific information about how to deal with invalid
+ * code owner config files.
+ */
+ public String invalidCodeOwnerConfigInfoUrl;
+
/** Whether code owner config files are read-only. */
public Boolean readOnly;
diff --git a/java/com/google/gerrit/plugins/codeowners/api/GeneralInfo.java b/java/com/google/gerrit/plugins/codeowners/api/GeneralInfo.java
index 2f4a133..d0c123f 100644
--- a/java/com/google/gerrit/plugins/codeowners/api/GeneralInfo.java
+++ b/java/com/google/gerrit/plugins/codeowners/api/GeneralInfo.java
@@ -47,6 +47,12 @@
*/
public String overrideInfoUrl;
+ /**
+ * Optional URL for a page that provides project/host-specific information about how to deal with
+ * invalid code owner config files.
+ */
+ public String invalidCodeOwnerConfigInfoUrl;
+
/** Policy that controls who should own paths that have no code owners defined. */
public FallbackCodeOwners fallbackCodeOwners;
}
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/CodeOwnerConfigFile.java b/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerConfigFile.java
index b704f2b..90fd878 100644
--- a/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerConfigFile.java
+++ b/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerConfigFile.java
@@ -213,8 +213,9 @@
@Override
protected void onLoad() throws IOException, ConfigInvalidException {
if (revision != null) {
- Optional<String> codeOwnerConfigFileContent =
- getFileIfItExists(JgitPath.of(codeOwnerConfigKey.filePath(defaultFileName)).get());
+ String codeOwnerConfigFilePath =
+ JgitPath.of(codeOwnerConfigKey.filePath(defaultFileName)).get();
+ Optional<String> codeOwnerConfigFileContent = getFileIfItExists(codeOwnerConfigFilePath);
if (codeOwnerConfigFileContent.isPresent()) {
try (Timer1.Context<String> ctx =
codeOwnerMetrics.parseCodeOwnerConfig.start(
@@ -224,7 +225,12 @@
codeOwnerConfigParser.parse(
revision, codeOwnerConfigKey, codeOwnerConfigFileContent.get()));
} catch (CodeOwnerConfigParseException e) {
- throw new ConfigInvalidException(e.getFullMessage(defaultFileName), e);
+ throw new InvalidCodeOwnerConfigException(
+ e.getFullMessage(defaultFileName),
+ projectName,
+ getRefName(),
+ codeOwnerConfigFilePath,
+ e);
}
}
}
diff --git a/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerConfigScanner.java b/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerConfigScanner.java
index 4df1696..20d7a4c 100644
--- a/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerConfigScanner.java
+++ b/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerConfigScanner.java
@@ -14,7 +14,7 @@
package com.google.gerrit.plugins.codeowners.backend;
-import static com.google.gerrit.plugins.codeowners.backend.CodeOwners.getInvalidConfigCause;
+import static com.google.gerrit.plugins.codeowners.backend.CodeOwners.getInvalidCodeOwnerConfigCause;
import static java.util.Objects.requireNonNull;
import com.google.common.flogger.FluentLogger;
@@ -26,7 +26,6 @@
import com.google.inject.Inject;
import java.io.IOException;
import java.util.Optional;
-import org.eclipse.jgit.errors.ConfigInvalidException;
import org.eclipse.jgit.lib.Repository;
import org.eclipse.jgit.revwalk.RevWalk;
@@ -144,16 +143,16 @@
try {
codeOwnerConfig = treeWalk.getCodeOwnerConfig();
} catch (CodeOwnersInternalServerErrorException codeOwnersInternalServerErrorException) {
- Optional<ConfigInvalidException> configInvalidException =
- getInvalidConfigCause(codeOwnersInternalServerErrorException);
- if (!configInvalidException.isPresent()) {
+ Optional<InvalidCodeOwnerConfigException> invalidCodeOwnerConfigException =
+ getInvalidCodeOwnerConfigCause(codeOwnersInternalServerErrorException);
+ if (!invalidCodeOwnerConfigException.isPresent()) {
// Propagate any failure that is not related to the contents of the code owner config.
throw codeOwnersInternalServerErrorException;
}
// The code owner config is invalid and cannot be parsed.
invalidCodeOwnerConfigCallback.onInvalidCodeOwnerConfig(
- treeWalk.getFilePath(), configInvalidException.get());
+ treeWalk.getFilePath(), invalidCodeOwnerConfigException.get());
continue;
}
diff --git a/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerSubmitRule.java b/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerSubmitRule.java
index 1785a1d..c35d1b0 100644
--- a/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerSubmitRule.java
+++ b/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerSubmitRule.java
@@ -78,7 +78,7 @@
}
try (Timer0.Context ctx = codeOwnerMetrics.runCodeOwnerSubmitRule.start()) {
- codeOwnerMetrics.countCodeOwnerConfigReads.increment();
+ codeOwnerMetrics.countCodeOwnerSubmitRuleRuns.increment();
logger.atFine().log(
"run code owner submit rule (project = %s, change = %d)",
changeData.project().get(), changeData.getId().get());
@@ -100,6 +100,7 @@
changeData.currentPatchSet().id().get(), changeData.change().getId().get()));
return Optional.of(notReady());
} catch (Throwable t) {
+ String cause = t.getClass().getSimpleName();
String errorMessage = "Failed to evaluate code owner statuses";
if (changeData != null) {
errorMessage +=
@@ -109,11 +110,33 @@
}
Optional<InvalidPathException> invalidPathException =
CodeOwnersExceptionHook.getInvalidPathException(t);
+ Optional<InvalidCodeOwnerConfigException> invalidCodeOwnerConfigException =
+ CodeOwners.getInvalidCodeOwnerConfigCause(t);
if (invalidPathException.isPresent()) {
+ cause = "invalid_path";
errorMessage += String.format(" (cause: %s)", invalidPathException.get().getMessage());
+ } else if (invalidCodeOwnerConfigException.isPresent()) {
+ codeOwnerMetrics.countInvalidCodeOwnerConfigFiles.increment(
+ invalidCodeOwnerConfigException.get().getProjectName().get(),
+ invalidCodeOwnerConfigException.get().getRef(),
+ invalidCodeOwnerConfigException.get().getCodeOwnerConfigFilePath());
+
+ cause = "invalid_code_owner_config_file";
+ errorMessage +=
+ String.format(" (cause: %s)", invalidCodeOwnerConfigException.get().getMessage());
+
+ Optional<String> invalidCodeOwnerConfigInfoUrl =
+ codeOwnersPluginConfiguration
+ .getProjectConfig(invalidCodeOwnerConfigException.get().getProjectName())
+ .getInvalidCodeOwnerConfigInfoUrl();
+ if (invalidCodeOwnerConfigInfoUrl.isPresent()) {
+ errorMessage +=
+ String.format(".\nFor help check %s", invalidCodeOwnerConfigInfoUrl.get());
+ }
}
errorMessage += ".";
logger.atSevere().withCause(t).log(errorMessage);
+ codeOwnerMetrics.countCodeOwnerSubmitRuleErrors.increment(cause);
return Optional.of(ruleError(errorMessage));
}
}
diff --git a/java/com/google/gerrit/plugins/codeowners/backend/CodeOwners.java b/java/com/google/gerrit/plugins/codeowners/backend/CodeOwners.java
index 32ad7b5..b9058d4 100644
--- a/java/com/google/gerrit/plugins/codeowners/backend/CodeOwners.java
+++ b/java/com/google/gerrit/plugins/codeowners/backend/CodeOwners.java
@@ -24,7 +24,6 @@
import com.google.inject.Singleton;
import java.nio.file.Path;
import java.util.Optional;
-import org.eclipse.jgit.errors.ConfigInvalidException;
import org.eclipse.jgit.lib.ObjectId;
/**
@@ -101,13 +100,14 @@
/**
* Checks whether the given exception was caused by a non-parseable code owner config ({@link
- * ConfigInvalidException}). If yes, the {@link ConfigInvalidException} is returned. If no, {@link
- * Optional#empty()} is returned.
+ * InvalidCodeOwnerConfigException}). If yes, the {@link InvalidCodeOwnerConfigException} is
+ * returned. If no, {@link Optional#empty()} is returned.
*/
- public static Optional<ConfigInvalidException> getInvalidConfigCause(Throwable e) {
+ public static Optional<InvalidCodeOwnerConfigException> getInvalidCodeOwnerConfigCause(
+ Throwable e) {
return Throwables.getCausalChain(e).stream()
- .filter(t -> t instanceof ConfigInvalidException)
- .map(t -> (ConfigInvalidException) t)
+ .filter(t -> t instanceof InvalidCodeOwnerConfigException)
+ .map(t -> (InvalidCodeOwnerConfigException) t)
.findFirst();
}
}
diff --git a/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnersExceptionHook.java b/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnersExceptionHook.java
index c8b3043..468f5bb 100644
--- a/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnersExceptionHook.java
+++ b/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnersExceptionHook.java
@@ -17,11 +17,13 @@
import com.google.common.base.Throwables;
import com.google.common.collect.ImmutableList;
import com.google.gerrit.common.Nullable;
+import com.google.gerrit.plugins.codeowners.backend.config.CodeOwnersPluginConfiguration;
import com.google.gerrit.plugins.codeowners.backend.config.InvalidPluginConfigurationException;
+import com.google.gerrit.plugins.codeowners.metrics.CodeOwnerMetrics;
import com.google.gerrit.server.ExceptionHook;
+import com.google.inject.Inject;
import java.nio.file.InvalidPathException;
import java.util.Optional;
-import org.eclipse.jgit.errors.ConfigInvalidException;
/**
* Class to define the HTTP response status code and message for exceptions that can occur for all
@@ -37,6 +39,17 @@
* </ul>
*/
public class CodeOwnersExceptionHook implements ExceptionHook {
+ private final CodeOwnersPluginConfiguration codeOwnersPluginConfiguration;
+ private final CodeOwnerMetrics codeOwnerMetrics;
+
+ @Inject
+ CodeOwnersExceptionHook(
+ CodeOwnersPluginConfiguration codeOwnersPluginConfiguration,
+ CodeOwnerMetrics codeOwnerMetric) {
+ this.codeOwnersPluginConfiguration = codeOwnersPluginConfiguration;
+ this.codeOwnerMetrics = codeOwnerMetric;
+ }
+
@Override
public boolean skipRetryWithTrace(String actionType, String actionName, Throwable throwable) {
return isInvalidPluginConfigurationException(throwable)
@@ -52,10 +65,23 @@
return ImmutableList.of(invalidPluginConfigurationException.get().getMessage());
}
- Optional<ConfigInvalidException> configInvalidException =
- CodeOwners.getInvalidConfigCause(throwable);
- if (configInvalidException.isPresent()) {
- return ImmutableList.of(configInvalidException.get().getMessage());
+ Optional<InvalidCodeOwnerConfigException> invalidCodeOwnerConfigException =
+ CodeOwners.getInvalidCodeOwnerConfigCause(throwable);
+ if (invalidCodeOwnerConfigException.isPresent()) {
+ codeOwnerMetrics.countInvalidCodeOwnerConfigFiles.increment(
+ invalidCodeOwnerConfigException.get().getProjectName().get(),
+ invalidCodeOwnerConfigException.get().getRef(),
+ invalidCodeOwnerConfigException.get().getCodeOwnerConfigFilePath());
+
+ ImmutableList.Builder<String> messages = ImmutableList.builder();
+ messages.add(invalidCodeOwnerConfigException.get().getMessage());
+ codeOwnersPluginConfiguration
+ .getProjectConfig(invalidCodeOwnerConfigException.get().getProjectName())
+ .getInvalidCodeOwnerConfigInfoUrl()
+ .ifPresent(
+ invalidCodeOwnerConfigInfoUrl ->
+ messages.add(String.format("For help check %s", invalidCodeOwnerConfigInfoUrl)));
+ return messages.build();
}
Optional<InvalidPathException> invalidPathException = getInvalidPathException(throwable);
@@ -115,6 +141,6 @@
}
private static boolean isInvalidCodeOwnerConfigException(Throwable throwable) {
- return CodeOwners.getInvalidConfigCause(throwable).isPresent();
+ return CodeOwners.getInvalidCodeOwnerConfigCause(throwable).isPresent();
}
}
diff --git a/java/com/google/gerrit/plugins/codeowners/backend/InvalidCodeOwnerConfigCallback.java b/java/com/google/gerrit/plugins/codeowners/backend/InvalidCodeOwnerConfigCallback.java
index fed838a..400432e 100644
--- a/java/com/google/gerrit/plugins/codeowners/backend/InvalidCodeOwnerConfigCallback.java
+++ b/java/com/google/gerrit/plugins/codeowners/backend/InvalidCodeOwnerConfigCallback.java
@@ -15,7 +15,6 @@
package com.google.gerrit.plugins.codeowners.backend;
import java.nio.file.Path;
-import org.eclipse.jgit.errors.ConfigInvalidException;
/** Callback interface to let callers handle invalid code owner config files. */
public interface InvalidCodeOwnerConfigCallback {
@@ -23,8 +22,9 @@
* Invoked when an invalid code owner config file is found.
*
* @param codeOwnerConfigFilePath the path of the invalid code owner config file
- * @param configInvalidException the parsing exception
+ * @param invalidCodeOwnerConfigException the parsing exception
*/
void onInvalidCodeOwnerConfig(
- Path codeOwnerConfigFilePath, ConfigInvalidException configInvalidException);
+ Path codeOwnerConfigFilePath,
+ InvalidCodeOwnerConfigException invalidCodeOwnerConfigException);
}
diff --git a/java/com/google/gerrit/plugins/codeowners/backend/InvalidCodeOwnerConfigException.java b/java/com/google/gerrit/plugins/codeowners/backend/InvalidCodeOwnerConfigException.java
new file mode 100644
index 0000000..7806c59
--- /dev/null
+++ b/java/com/google/gerrit/plugins/codeowners/backend/InvalidCodeOwnerConfigException.java
@@ -0,0 +1,61 @@
+// 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;
+
+import static java.util.Objects.requireNonNull;
+
+import com.google.gerrit.common.Nullable;
+import com.google.gerrit.entities.Project;
+import org.eclipse.jgit.errors.ConfigInvalidException;
+
+/** Exception that is thrown if there is an invalid code owner config file. */
+public class InvalidCodeOwnerConfigException extends ConfigInvalidException {
+ private static final long serialVersionUID = 1L;
+
+ private final Project.NameKey projectName;
+ private final String ref;
+ private final String codeOwnerConfigFilePath;
+
+ public InvalidCodeOwnerConfigException(
+ String message, Project.NameKey projectName, String ref, String codeOwnerConfigFilePath) {
+ this(message, projectName, ref, codeOwnerConfigFilePath, /* cause= */ null);
+ }
+
+ public InvalidCodeOwnerConfigException(
+ String message,
+ Project.NameKey projectName,
+ String ref,
+ String codeOwnerConfigFilePath,
+ @Nullable Throwable cause) {
+ super(message, cause);
+
+ this.projectName = requireNonNull(projectName, "projectName");
+ this.ref = requireNonNull(ref, "ref");
+ this.codeOwnerConfigFilePath =
+ requireNonNull(codeOwnerConfigFilePath, "codeOwnerConfigFilePath");
+ }
+
+ public Project.NameKey getProjectName() {
+ return projectName;
+ }
+
+ public String getRef() {
+ return ref;
+ }
+
+ public String getCodeOwnerConfigFilePath() {
+ return codeOwnerConfigFilePath;
+ }
+}
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..04046ac 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. */
@@ -250,6 +242,11 @@
return generalConfig.getOverrideInfoUrl(pluginConfig);
}
+ /** Gets the invalid code owner config info URL that is configured. */
+ public Optional<String> getInvalidCodeOwnerConfigInfoUrl() {
+ return generalConfig.getInvalidCodeOwnerConfigInfoUrl(pluginConfig);
+ }
+
/**
* Whether the code owners functionality is disabled for the given branch.
*
@@ -493,21 +490,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..72e2022 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;
/**
@@ -74,6 +77,8 @@
public static final String KEY_EXEMPTED_USER = "exemptedUser";
public static final String KEY_ENABLE_IMPLICIT_APPROVALS = "enableImplicitApprovals";
public static final String KEY_OVERRIDE_INFO_URL = "overrideInfoUrl";
+ public static final String KEY_INVALID_CODE_OWNER_CONFIG_INFO_URL =
+ "invalidCodeOwnerConfigInfoUrl";
public static final String KEY_REJECT_NON_RESOLVABLE_CODE_OWNERS =
"rejectNonResolvableCodeOwners";
public static final String KEY_REJECT_NON_RESOLVABLE_IMPORTS = "rejectNonResolvableImports";
@@ -177,15 +182,7 @@
* project, {@link Optional#empty()} if no file extension should be used
*/
Optional<String> getFileExtension(Config pluginConfig) {
- requireNonNull(pluginConfig, "pluginConfig");
-
- String fileExtension =
- pluginConfig.getString(SECTION_CODE_OWNERS, /* subsection= */ null, KEY_FILE_EXTENSION);
- if (fileExtension != null) {
- return Optional.of(fileExtension);
- }
-
- return Optional.ofNullable(pluginConfigFromGerritConfig.getString(KEY_FILE_EXTENSION));
+ return getStringValue(pluginConfig, KEY_FILE_EXTENSION);
}
/**
@@ -796,19 +793,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 +809,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());
}
/**
@@ -850,14 +823,53 @@
* such URL is configured
*/
Optional<String> getOverrideInfoUrl(Config pluginConfig) {
+ return getStringValue(pluginConfig, KEY_OVERRIDE_INFO_URL);
+ }
+
+ /**
+ * Gets an URL that leads to an information page about invalid code owner config files.
+ *
+ * <p>The URL is retrieved from the given plugin config, with fallback to the {@code
+ * gerrit.config}.
+ *
+ * @param pluginConfig the plugin config from which the invalid code owner config info URL should
+ * be read.
+ * @return URL that leads to an information page about invalid code owner config files, {@link
+ * Optional#empty()} if no such URL is configured
+ */
+ Optional<String> getInvalidCodeOwnerConfigInfoUrl(Config pluginConfig) {
+ return getStringValue(pluginConfig, KEY_INVALID_CODE_OWNER_CONFIG_INFO_URL);
+ }
+
+ private Optional<String> getStringValue(Config pluginConfig, String key) {
requireNonNull(pluginConfig, "pluginConfig");
- String fileExtension =
- pluginConfig.getString(SECTION_CODE_OWNERS, /* subsection= */ null, KEY_OVERRIDE_INFO_URL);
- if (fileExtension != null) {
- return Optional.of(fileExtension);
+ String value = pluginConfig.getString(SECTION_CODE_OWNERS, /* subsection= */ null, key);
+ if (value != null) {
+ return Optional.of(value);
}
- return Optional.ofNullable(pluginConfigFromGerritConfig.getString(KEY_OVERRIDE_INFO_URL));
+ return Optional.ofNullable(pluginConfigFromGerritConfig.getString(key));
+ }
+
+ /**
+ * 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/java/com/google/gerrit/plugins/codeowners/metrics/CodeOwnerMetrics.java b/java/com/google/gerrit/plugins/codeowners/metrics/CodeOwnerMetrics.java
index f3cc516..cf847df 100644
--- a/java/com/google/gerrit/plugins/codeowners/metrics/CodeOwnerMetrics.java
+++ b/java/com/google/gerrit/plugins/codeowners/metrics/CodeOwnerMetrics.java
@@ -15,6 +15,8 @@
package com.google.gerrit.plugins.codeowners.metrics;
import com.google.gerrit.metrics.Counter0;
+import com.google.gerrit.metrics.Counter1;
+import com.google.gerrit.metrics.Counter3;
import com.google.gerrit.metrics.Description;
import com.google.gerrit.metrics.Description.Units;
import com.google.gerrit.metrics.Field;
@@ -22,6 +24,7 @@
import com.google.gerrit.metrics.MetricMaker;
import com.google.gerrit.metrics.Timer0;
import com.google.gerrit.metrics.Timer1;
+import com.google.gerrit.server.logging.Metadata;
import com.google.inject.Inject;
import com.google.inject.Singleton;
@@ -55,7 +58,9 @@
// counter metrics
public final Counter0 countCodeOwnerConfigReads;
public final Counter0 countCodeOwnerConfigCacheReads;
+ public final Counter1<String> countCodeOwnerSubmitRuleErrors;
public final Counter0 countCodeOwnerSubmitRuleRuns;
+ public final Counter3<String, String, String> countInvalidCodeOwnerConfigFiles;
private final MetricMaker metricMaker;
@@ -149,15 +154,37 @@
createCounter(
"count_code_owner_config_cache_reads",
"Total number of code owner config reads from cache");
+ this.countCodeOwnerSubmitRuleErrors =
+ createCounter1(
+ "count_code_owner_submit_rule_errors",
+ "Total number of code owner submit rule errors",
+ Field.ofString("cause", Metadata.Builder::cause)
+ .description("The cause of the submit rule error.")
+ .build());
this.countCodeOwnerSubmitRuleRuns =
createCounter(
"count_code_owner_submit_rule_runs", "Total number of code owner submit rule runs");
+ this.countInvalidCodeOwnerConfigFiles =
+ createCounter3(
+ "count_invalid_code_owner_config_files",
+ "Total number of failed requests caused by an invalid / non-parsable code owner config"
+ + " file",
+ Field.ofString("project", Metadata.Builder::projectName)
+ .description(
+ "The name of the project that contains the invalid code owner config file.")
+ .build(),
+ Field.ofString("branch", Metadata.Builder::branchName)
+ .description(
+ "The name of the branch that contains the invalid code owner config file.")
+ .build(),
+ Field.ofString("path", Metadata.Builder::filePath)
+ .description("The path of the invalid code owner config file.")
+ .build());
}
private Timer0 createLatencyTimer(String name, String description) {
return metricMaker.newTimer(
- "code_owners/" + name,
- new Description(description).setCumulative().setUnit(Units.MILLISECONDS));
+ name, new Description(description).setCumulative().setUnit(Units.MILLISECONDS));
}
private Timer1<String> createTimerWithClassField(
@@ -168,17 +195,26 @@
.build();
return metricMaker.newTimer(
- "code_owners/" + name,
+ name,
new Description(description).setCumulative().setUnit(Description.Units.MILLISECONDS),
CODE_OWNER_BACKEND_FIELD);
}
private Counter0 createCounter(String name, String description) {
- return metricMaker.newCounter("code_owners/" + name, new Description(description).setRate());
+ return metricMaker.newCounter(name, new Description(description).setRate());
+ }
+
+ private <F1> Counter1<F1> createCounter1(String name, String description, Field<F1> field1) {
+ return metricMaker.newCounter(name, new Description(description).setRate(), field1);
+ }
+
+ private <F1, F2, F3> Counter3<F1, F2, F3> createCounter3(
+ String name, String description, Field<F1> field1, Field<F2> field2, Field<F3> field3) {
+ return metricMaker.newCounter(
+ name, new Description(description).setRate(), field1, field2, field3);
}
private Histogram0 createHistogram(String name, String description) {
- return metricMaker.newHistogram(
- "code_owners/" + name, new Description(description).setCumulative());
+ return metricMaker.newHistogram(name, new Description(description).setCumulative());
}
}
diff --git a/java/com/google/gerrit/plugins/codeowners/restapi/CheckCodeOwner.java b/java/com/google/gerrit/plugins/codeowners/restapi/CheckCodeOwner.java
index 9ab1ed1..64aa0dd 100644
--- a/java/com/google/gerrit/plugins/codeowners/restapi/CheckCodeOwner.java
+++ b/java/com/google/gerrit/plugins/codeowners/restapi/CheckCodeOwner.java
@@ -133,6 +133,7 @@
List<String> messages = new ArrayList<>();
List<Path> codeOwnerConfigFilePaths = new ArrayList<>();
AtomicBoolean isCodeOwnershipAssignedToEmail = new AtomicBoolean(false);
+ AtomicBoolean isCodeOwnershipAssignedToAllUsers = new AtomicBoolean(false);
AtomicBoolean isDefaultCodeOwner = new AtomicBoolean(false);
AtomicBoolean hasRevelantCodeOwnerDefinitions = new AtomicBoolean(false);
AtomicBoolean parentCodeOwnersAreIgnored = new AtomicBoolean(false);
@@ -159,7 +160,8 @@
pathCodeOwnersResult.get().getPathCodeOwners().stream()
.filter(cor -> cor.email().equals(email))
.findAny();
- if (codeOwnerReference.isPresent()) {
+ if (codeOwnerReference.isPresent()
+ && !CodeOwnerResolver.ALL_USERS_WILDCARD.equals(email)) {
isCodeOwnershipAssignedToEmail.set(true);
if (RefNames.isConfigRef(codeOwnerConfig.key().ref())) {
@@ -174,7 +176,31 @@
"found email %s as code owner in %s", email, codeOwnerConfigFilePath));
codeOwnerConfigFilePaths.add(codeOwnerConfigFilePath);
}
- } else if (codeOwnerResolverProvider
+ }
+
+ if (pathCodeOwnersResult.get().getPathCodeOwners().stream()
+ .anyMatch(cor -> cor.email().equals(CodeOwnerResolver.ALL_USERS_WILDCARD))) {
+ isCodeOwnershipAssignedToAllUsers.set(true);
+
+ if (RefNames.isConfigRef(codeOwnerConfig.key().ref())) {
+ messages.add(
+ String.format(
+ "found email %s as code owner in default code owner config",
+ CodeOwnerResolver.ALL_USERS_WILDCARD));
+ isDefaultCodeOwner.set(true);
+ } else {
+ Path codeOwnerConfigFilePath = codeOwners.getFilePath(codeOwnerConfig.key());
+ messages.add(
+ String.format(
+ "found email %s as code owner in %s",
+ CodeOwnerResolver.ALL_USERS_WILDCARD, codeOwnerConfigFilePath));
+ if (!codeOwnerConfigFilePaths.contains(codeOwnerConfigFilePath)) {
+ codeOwnerConfigFilePaths.add(codeOwnerConfigFilePath);
+ }
+ }
+ }
+
+ if (codeOwnerResolverProvider
.get()
.resolvePathCodeOwners(codeOwnerConfig, absolutePath)
.hasRevelantCodeOwnerDefinitions()) {
@@ -189,31 +215,46 @@
return !pathCodeOwnersResult.get().ignoreParentCodeOwners();
});
- boolean isGlobalCodeOwner = isGlobalCodeOwner(branchResource.getNameKey());
- if (isGlobalCodeOwner) {
+ boolean isGlobalCodeOwner = false;
+
+ if (isGlobalCodeOwner(branchResource.getNameKey(), email)) {
+ isGlobalCodeOwner = true;
messages.add(String.format("found email %s as global code owner", email));
isCodeOwnershipAssignedToEmail.set(true);
}
+ if (isGlobalCodeOwner(branchResource.getNameKey(), CodeOwnerResolver.ALL_USERS_WILDCARD)) {
+ isGlobalCodeOwner = true;
+ messages.add(
+ String.format(
+ "found email %s as global code owner", CodeOwnerResolver.ALL_USERS_WILDCARD));
+ isCodeOwnershipAssignedToAllUsers.set(true);
+ }
+
OptionalResultWithMessages<Boolean> isResolvableResult = isResolvable();
boolean isResolvable = isResolvableResult.get();
messages.addAll(isResolvableResult.messages());
boolean isFallbackCodeOwner =
!isCodeOwnershipAssignedToEmail.get()
+ && !isCodeOwnershipAssignedToAllUsers.get()
&& !hasRevelantCodeOwnerDefinitions.get()
&& !parentCodeOwnersAreIgnored.get()
&& isFallbackCodeOwner(branchResource.getNameKey());
CodeOwnerCheckInfo codeOwnerCheckInfo = new CodeOwnerCheckInfo();
codeOwnerCheckInfo.isCodeOwner =
- (isCodeOwnershipAssignedToEmail.get() || isFallbackCodeOwner) && isResolvable;
+ (isCodeOwnershipAssignedToEmail.get()
+ || isCodeOwnershipAssignedToAllUsers.get()
+ || isFallbackCodeOwner)
+ && isResolvable;
codeOwnerCheckInfo.isResolvable = isResolvable;
codeOwnerCheckInfo.codeOwnerConfigFilePaths =
codeOwnerConfigFilePaths.stream().map(Path::toString).collect(toList());
codeOwnerCheckInfo.isFallbackCodeOwner = isFallbackCodeOwner && isResolvable;
codeOwnerCheckInfo.isDefaultCodeOwner = isDefaultCodeOwner.get();
codeOwnerCheckInfo.isGlobalCodeOwner = isGlobalCodeOwner;
+ codeOwnerCheckInfo.isOwnedByAllUsers = isCodeOwnershipAssignedToAllUsers.get();
codeOwnerCheckInfo.debugLogs = messages;
return Response.ok(codeOwnerCheckInfo);
}
@@ -238,7 +279,7 @@
}
}
- private boolean isGlobalCodeOwner(Project.NameKey projectName) {
+ private boolean isGlobalCodeOwner(Project.NameKey projectName, String email) {
return codeOwnersPluginConfiguration.getProjectConfig(projectName).getGlobalCodeOwners()
.stream()
.filter(cor -> cor.email().equals(email))
diff --git a/java/com/google/gerrit/plugins/codeowners/restapi/CodeOwnerProjectConfigJson.java b/java/com/google/gerrit/plugins/codeowners/restapi/CodeOwnerProjectConfigJson.java
index 8140fac..49053ec 100644
--- a/java/com/google/gerrit/plugins/codeowners/restapi/CodeOwnerProjectConfigJson.java
+++ b/java/com/google/gerrit/plugins/codeowners/restapi/CodeOwnerProjectConfigJson.java
@@ -110,6 +110,8 @@
generalInfo.mergeCommitStrategy = codeOwnersConfig.getMergeCommitStrategy();
generalInfo.implicitApprovals = codeOwnersConfig.areImplicitApprovalsEnabled() ? true : null;
generalInfo.overrideInfoUrl = codeOwnersConfig.getOverrideInfoUrl().orElse(null);
+ generalInfo.invalidCodeOwnerConfigInfoUrl =
+ codeOwnersConfig.getInvalidCodeOwnerConfigInfoUrl().orElse(null);
generalInfo.fallbackCodeOwners = codeOwnersConfig.getFallbackCodeOwners();
return generalInfo;
}
diff --git a/java/com/google/gerrit/plugins/codeowners/restapi/PutCodeOwnerProjectConfig.java b/java/com/google/gerrit/plugins/codeowners/restapi/PutCodeOwnerProjectConfig.java
index ed130c8..96b467e 100644
--- a/java/com/google/gerrit/plugins/codeowners/restapi/PutCodeOwnerProjectConfig.java
+++ b/java/com/google/gerrit/plugins/codeowners/restapi/PutCodeOwnerProjectConfig.java
@@ -23,6 +23,7 @@
import static com.google.gerrit.plugins.codeowners.backend.config.GeneralConfig.KEY_FALLBACK_CODE_OWNERS;
import static com.google.gerrit.plugins.codeowners.backend.config.GeneralConfig.KEY_FILE_EXTENSION;
import static com.google.gerrit.plugins.codeowners.backend.config.GeneralConfig.KEY_GLOBAL_CODE_OWNER;
+import static com.google.gerrit.plugins.codeowners.backend.config.GeneralConfig.KEY_INVALID_CODE_OWNER_CONFIG_INFO_URL;
import static com.google.gerrit.plugins.codeowners.backend.config.GeneralConfig.KEY_MAX_PATHS_IN_CHANGE_MESSAGES;
import static com.google.gerrit.plugins.codeowners.backend.config.GeneralConfig.KEY_MERGE_COMMIT_STRATEGY;
import static com.google.gerrit.plugins.codeowners.backend.config.GeneralConfig.KEY_OVERRIDE_INFO_URL;
@@ -199,6 +200,14 @@
input.overrideInfoUrl);
}
+ if (input.invalidCodeOwnerConfigInfoUrl != null) {
+ codeOwnersConfig.setString(
+ SECTION_CODE_OWNERS,
+ /* subsection= */ null,
+ KEY_INVALID_CODE_OWNER_CONFIG_INFO_URL,
+ input.invalidCodeOwnerConfigInfoUrl);
+ }
+
if (input.readOnly != null) {
codeOwnersConfig.setBoolean(
SECTION_CODE_OWNERS, /* subsection= */ null, KEY_READ_ONLY, input.readOnly);
diff --git a/java/com/google/gerrit/plugins/codeowners/testing/CodeOwnerCheckInfoSubject.java b/java/com/google/gerrit/plugins/codeowners/testing/CodeOwnerCheckInfoSubject.java
index 8d6524b..2d33006 100644
--- a/java/com/google/gerrit/plugins/codeowners/testing/CodeOwnerCheckInfoSubject.java
+++ b/java/com/google/gerrit/plugins/codeowners/testing/CodeOwnerCheckInfoSubject.java
@@ -89,6 +89,14 @@
check("isGlobalCodeOwner").that(codeOwnerCheckInfo().isGlobalCodeOwner).isFalse();
}
+ public void isOwnedByAllUsers() {
+ check("isOwnedByAllUsers").that(codeOwnerCheckInfo().isOwnedByAllUsers).isTrue();
+ }
+
+ public void isNotOwnedByAllUsers() {
+ check("isOwnedByAllUsers").that(codeOwnerCheckInfo().isOwnedByAllUsers).isFalse();
+ }
+
public void hasDebugLogsThatContainAllOf(String... expectedMessages) {
for (String expectedMessage : expectedMessages) {
check("debugLogs").that(codeOwnerCheckInfo().debugLogs).contains(expectedMessage);
diff --git a/java/com/google/gerrit/plugins/codeowners/validation/CodeOwnerConfigValidator.java b/java/com/google/gerrit/plugins/codeowners/validation/CodeOwnerConfigValidator.java
index 6e59621..8bb6fd7 100644
--- a/java/com/google/gerrit/plugins/codeowners/validation/CodeOwnerConfigValidator.java
+++ b/java/com/google/gerrit/plugins/codeowners/validation/CodeOwnerConfigValidator.java
@@ -17,7 +17,7 @@
import static com.google.common.base.Preconditions.checkState;
import static com.google.common.collect.ImmutableList.toImmutableList;
import static com.google.common.collect.ImmutableSet.toImmutableSet;
-import static com.google.gerrit.plugins.codeowners.backend.CodeOwners.getInvalidConfigCause;
+import static com.google.gerrit.plugins.codeowners.backend.CodeOwners.getInvalidCodeOwnerConfigCause;
import static java.util.Objects.requireNonNull;
import com.google.auto.value.AutoValue;
@@ -37,6 +37,7 @@
import com.google.gerrit.plugins.codeowners.backend.CodeOwnerReference;
import com.google.gerrit.plugins.codeowners.backend.CodeOwnerResolver;
import com.google.gerrit.plugins.codeowners.backend.CodeOwnersInternalServerErrorException;
+import com.google.gerrit.plugins.codeowners.backend.InvalidCodeOwnerConfigException;
import com.google.gerrit.plugins.codeowners.backend.PathCodeOwners;
import com.google.gerrit.plugins.codeowners.backend.config.CodeOwnersPluginConfigSnapshot;
import com.google.gerrit.plugins.codeowners.backend.config.CodeOwnersPluginConfiguration;
@@ -467,9 +468,9 @@
codeOwnerConfigKey, revCommit.name())));
} catch (CodeOwnersInternalServerErrorException codeOwnersInternalServerErrorException) {
// Loading the code owner config has failed.
- Optional<ConfigInvalidException> configInvalidException =
- getInvalidConfigCause(codeOwnersInternalServerErrorException);
- if (!configInvalidException.isPresent()) {
+ Optional<InvalidCodeOwnerConfigException> invalidCodeOwnerConfigException =
+ getInvalidCodeOwnerConfigCause(codeOwnersInternalServerErrorException);
+ if (!invalidCodeOwnerConfigException.isPresent()) {
// Propagate any failure that is not related to the contents of the code owner config.
throw codeOwnersInternalServerErrorException;
}
@@ -481,7 +482,7 @@
// it.
return Stream.of(
new CommitValidationMessage(
- configInvalidException.get().getMessage(),
+ invalidCodeOwnerConfigException.get().getMessage(),
getValidationMessageTypeForParsingError(
codeOwnerBackend, branchNameKey, changedFile, revWalk, revCommit)));
}
@@ -496,7 +497,7 @@
baseCodeOwnerConfig =
getBaseCodeOwnerConfig(codeOwnerBackend, branchNameKey, changedFile, revWalk, revCommit);
} catch (CodeOwnersInternalServerErrorException codeOwnersInternalServerErrorException) {
- if (getInvalidConfigCause(codeOwnersInternalServerErrorException).isPresent()) {
+ if (getInvalidCodeOwnerConfigCause(codeOwnersInternalServerErrorException).isPresent()) {
// The base code owner config is non-parseable. Since the update makes the code owner
// config parseable, it is a good update even if the code owner config still contains
// issues. Hence in this case we downgrade all validation errors in the new version to
@@ -630,7 +631,7 @@
return ValidationMessage.Type.FATAL;
} catch (CodeOwnersInternalServerErrorException codeOwnersInternalServerErrorException) {
// Loading the base code owner config has failed.
- if (getInvalidConfigCause(codeOwnersInternalServerErrorException).isPresent()) {
+ if (getInvalidCodeOwnerConfigCause(codeOwnersInternalServerErrorException).isPresent()) {
// The code owner config was already non-parseable before, hence we do not need to
// block the upload if the code owner config is still non-parseable.
// Using warning as type means that uploads are not blocked.
@@ -988,7 +989,7 @@
revision.get().name()));
}
} catch (CodeOwnersInternalServerErrorException codeOwnersInternalServerErrorException) {
- if (getInvalidConfigCause(codeOwnersInternalServerErrorException).isPresent()) {
+ if (getInvalidCodeOwnerConfigCause(codeOwnersInternalServerErrorException).isPresent()) {
// The imported code owner config is non-parseable.
return nonResolvableImport(
codeOwnerConfigRevision,
diff --git a/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/CheckCodeOwnerConfigFilesIT.java b/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/CheckCodeOwnerConfigFilesIT.java
index a2e37ef..a275287 100644
--- a/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/CheckCodeOwnerConfigFilesIT.java
+++ b/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/CheckCodeOwnerConfigFilesIT.java
@@ -35,18 +35,13 @@
import com.google.gerrit.extensions.restapi.RestApiException;
import com.google.gerrit.extensions.restapi.UnprocessableEntityException;
import com.google.gerrit.plugins.codeowners.acceptance.AbstractCodeOwnersIT;
-import com.google.gerrit.plugins.codeowners.backend.CodeOwnerBackend;
import com.google.gerrit.plugins.codeowners.backend.CodeOwnerConfig;
import com.google.gerrit.plugins.codeowners.backend.CodeOwnerConfigImportMode;
import com.google.gerrit.plugins.codeowners.backend.CodeOwnerConfigReference;
-import com.google.gerrit.plugins.codeowners.backend.config.BackendConfig;
-import com.google.gerrit.plugins.codeowners.backend.findowners.FindOwnersBackend;
-import com.google.gerrit.plugins.codeowners.backend.proto.ProtoBackend;
import com.google.inject.Inject;
import java.util.HashMap;
import java.util.List;
import java.util.Map;
-import org.junit.Before;
import org.junit.Test;
/**
@@ -62,13 +57,6 @@
@Inject private RequestScopeOperations requestScopeOperations;
@Inject private ProjectOperations projectOperations;
- private BackendConfig backendConfig;
-
- @Before
- public void setUpCodeOwnersPlugin() throws Exception {
- backendConfig = plugin.getSysInjector().getInstance(BackendConfig.class);
- }
-
@Test
public void requiresAuthenticatedUser() throws Exception {
requestScopeOperations.setApiUserAnonymous();
@@ -172,12 +160,7 @@
+ " branch = master):\n %s",
codeOwnerConfigPath,
project,
- getParsingErrorMessage(
- ImmutableMap.of(
- FindOwnersBackend.class,
- "invalid line: INVALID",
- ProtoBackend.class,
- "1:8: Expected \"{\".")))))),
+ getParsingErrorMessageForNonParseableCodeOwnerConfig())))),
"refs/meta/config", ImmutableMap.of());
}
@@ -618,12 +601,7 @@
"invalid code owner config file '%s' (project = %s, branch = master):\n %s",
pathOfNonParseableCodeOwnerConfig,
project,
- getParsingErrorMessage(
- ImmutableMap.of(
- FindOwnersBackend.class,
- "invalid line: INVALID",
- ProtoBackend.class,
- "1:8: Expected \"{\"."))))));
+ getParsingErrorMessageForNonParseableCodeOwnerConfig()))));
if (verbosity == null
|| (ConsistencyProblemInfo.Status.ERROR.equals(verbosity)
&& (expectedStatus.equals(ConsistencyProblemInfo.Status.FATAL)
@@ -667,21 +645,4 @@
Project.NameKey projectName) throws RestApiException {
return projectCodeOwnersApiFactory.project(projectName).checkCodeOwnerConfigFiles().check();
}
-
- private String getCodeOwnerConfigFileName() {
- CodeOwnerBackend backend = backendConfig.getDefaultBackend();
- if (backend instanceof FindOwnersBackend) {
- return FindOwnersBackend.CODE_OWNER_CONFIG_FILE_NAME;
- } else if (backend instanceof ProtoBackend) {
- return ProtoBackend.CODE_OWNER_CONFIG_FILE_NAME;
- }
- throw new IllegalStateException("unknown code owner backend: " + backend.getClass().getName());
- }
-
- private String getParsingErrorMessage(
- ImmutableMap<Class<? extends CodeOwnerBackend>, String> messagesByBackend) {
- CodeOwnerBackend codeOwnerBackend = backendConfig.getDefaultBackend();
- assertThat(messagesByBackend).containsKey(codeOwnerBackend.getClass());
- return messagesByBackend.get(codeOwnerBackend.getClass());
- }
}
diff --git a/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/CheckCodeOwnerConfigFilesInRevisionIT.java b/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/CheckCodeOwnerConfigFilesInRevisionIT.java
index 0a6c5ad..b003707 100644
--- a/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/CheckCodeOwnerConfigFilesInRevisionIT.java
+++ b/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/CheckCodeOwnerConfigFilesInRevisionIT.java
@@ -24,7 +24,6 @@
import com.google.gerrit.extensions.api.config.ConsistencyCheckInfo.ConsistencyProblemInfo;
import com.google.gerrit.extensions.restapi.RestApiException;
import com.google.gerrit.plugins.codeowners.acceptance.AbstractCodeOwnersIT;
-import com.google.gerrit.plugins.codeowners.backend.CodeOwnerBackend;
import com.google.gerrit.plugins.codeowners.backend.CodeOwnerConfig;
import com.google.gerrit.plugins.codeowners.backend.CodeOwnerSet;
import com.google.gerrit.plugins.codeowners.backend.config.BackendConfig;
@@ -525,11 +524,4 @@
"unknown code owner backend: %s",
backendConfig.getDefaultBackend().getClass().getName()));
}
-
- private String getParsingErrorMessage(
- ImmutableMap<Class<? extends CodeOwnerBackend>, String> messagesByBackend) {
- CodeOwnerBackend codeOwnerBackend = backendConfig.getDefaultBackend();
- assertThat(messagesByBackend).containsKey(codeOwnerBackend.getClass());
- return messagesByBackend.get(codeOwnerBackend.getClass());
- }
}
diff --git a/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/CheckCodeOwnerIT.java b/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/CheckCodeOwnerIT.java
index 8b56cd5..238dc9f 100644
--- a/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/CheckCodeOwnerIT.java
+++ b/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/CheckCodeOwnerIT.java
@@ -28,6 +28,7 @@
import com.google.gerrit.common.Nullable;
import com.google.gerrit.entities.Account;
import com.google.gerrit.entities.Project;
+import com.google.gerrit.entities.RefNames;
import com.google.gerrit.extensions.api.projects.ConfigInput;
import com.google.gerrit.extensions.client.ProjectState;
import com.google.gerrit.extensions.restapi.AuthException;
@@ -35,17 +36,14 @@
import com.google.gerrit.extensions.restapi.ResourceNotFoundException;
import com.google.gerrit.extensions.restapi.RestApiException;
import com.google.gerrit.plugins.codeowners.acceptance.AbstractCodeOwnersIT;
+import com.google.gerrit.plugins.codeowners.acceptance.testsuite.TestCodeOwnerConfigCreation;
import com.google.gerrit.plugins.codeowners.acceptance.testsuite.TestPathExpressions;
import com.google.gerrit.plugins.codeowners.api.CodeOwnerCheckInfo;
-import com.google.gerrit.plugins.codeowners.backend.CodeOwnerBackend;
import com.google.gerrit.plugins.codeowners.backend.CodeOwnerConfig;
import com.google.gerrit.plugins.codeowners.backend.CodeOwnerConfigImportMode;
import com.google.gerrit.plugins.codeowners.backend.CodeOwnerConfigReference;
import com.google.gerrit.plugins.codeowners.backend.CodeOwnerResolver;
import com.google.gerrit.plugins.codeowners.backend.CodeOwnerSet;
-import com.google.gerrit.plugins.codeowners.backend.config.BackendConfig;
-import com.google.gerrit.plugins.codeowners.backend.findowners.FindOwnersBackend;
-import com.google.gerrit.plugins.codeowners.backend.proto.ProtoBackend;
import com.google.gerrit.plugins.codeowners.restapi.CheckCodeOwnerCapability;
import com.google.gerrit.plugins.codeowners.util.JgitPath;
import com.google.gerrit.server.ServerInitiated;
@@ -55,6 +53,7 @@
import com.google.gerrit.server.git.meta.MetaDataUpdate;
import com.google.inject.Inject;
import com.google.inject.Provider;
+import java.util.Arrays;
import org.eclipse.jgit.lib.Repository;
import org.junit.Before;
import org.junit.Test;
@@ -72,12 +71,10 @@
@Inject @ServerInitiated private Provider<AccountsUpdate> accountsUpdate;
@Inject private ExternalIdNotes.Factory externalIdNotesFactory;
- private BackendConfig backendConfig;
private TestPathExpressions testPathExpressions;
@Before
public void setUpCodeOwnersPlugin() throws Exception {
- backendConfig = plugin.getSysInjector().getInstance(BackendConfig.class);
testPathExpressions = plugin.getSysInjector().getInstance(TestPathExpressions.class);
}
@@ -138,6 +135,7 @@
.containsExactly(getCodeOwnerConfigFilePath("/foo/"));
assertThat(checkCodeOwnerInfo).isNotDefaultCodeOwner();
assertThat(checkCodeOwnerInfo).isNotGlobalCodeOwner();
+ assertThat(checkCodeOwnerInfo).isNotOwnedByAllUsers();
assertThat(checkCodeOwnerInfo)
.hasDebugLogsThatContainAllOf(
String.format(
@@ -168,6 +166,7 @@
.inOrder();
assertThat(checkCodeOwnerInfo).isNotDefaultCodeOwner();
assertThat(checkCodeOwnerInfo).isNotGlobalCodeOwner();
+ assertThat(checkCodeOwnerInfo).isNotOwnedByAllUsers();
assertThat(checkCodeOwnerInfo)
.hasDebugLogsThatContainAllOf(
String.format(
@@ -211,6 +210,7 @@
.inOrder();
assertThat(checkCodeOwnerInfo).isNotDefaultCodeOwner();
assertThat(checkCodeOwnerInfo).isNotGlobalCodeOwner();
+ assertThat(checkCodeOwnerInfo).isNotOwnedByAllUsers();
assertThat(checkCodeOwnerInfo)
.hasDebugLogsThatContainAllOf(
String.format(
@@ -235,7 +235,7 @@
.addSecondaryEmail(secondaryEmail)
.update();
- setAsRootCodeOwner(secondaryEmail);
+ setAsRootCodeOwners(secondaryEmail);
CodeOwnerCheckInfo checkCodeOwnerInfo = checkCodeOwner(ROOT_PATH, secondaryEmail);
assertThat(checkCodeOwnerInfo).isCodeOwner();
@@ -245,6 +245,7 @@
.containsExactly(getCodeOwnerConfigFilePath(ROOT_PATH));
assertThat(checkCodeOwnerInfo).isNotDefaultCodeOwner();
assertThat(checkCodeOwnerInfo).isNotGlobalCodeOwner();
+ assertThat(checkCodeOwnerInfo).isNotOwnedByAllUsers();
assertThat(checkCodeOwnerInfo)
.hasDebugLogsThatContainAllOf(
String.format(
@@ -254,6 +255,57 @@
}
@Test
+ public void checkCodeOwner_ownedByAllUsers() throws Exception {
+ TestAccount codeOwner =
+ accountCreator.create(
+ "codeOwner", "codeOwner@example.com", "Code Owner", /* displayName= */ null);
+
+ setAsRootCodeOwners(CodeOwnerResolver.ALL_USERS_WILDCARD);
+
+ CodeOwnerCheckInfo checkCodeOwnerInfo = checkCodeOwner(ROOT_PATH, codeOwner.email());
+ assertThat(checkCodeOwnerInfo).isCodeOwner();
+ assertThat(checkCodeOwnerInfo).isResolvable();
+ assertThat(checkCodeOwnerInfo)
+ .hasCodeOwnerConfigFilePathsThat()
+ .containsExactly(getCodeOwnerConfigFilePath(ROOT_PATH));
+ assertThat(checkCodeOwnerInfo).isNotDefaultCodeOwner();
+ assertThat(checkCodeOwnerInfo).isNotGlobalCodeOwner();
+ assertThat(checkCodeOwnerInfo).isOwnedByAllUsers();
+ assertThat(checkCodeOwnerInfo)
+ .hasDebugLogsThatContainAllOf(
+ String.format(
+ "found email %s as code owner in %s",
+ CodeOwnerResolver.ALL_USERS_WILDCARD, getCodeOwnerConfigFilePath(ROOT_PATH)));
+ }
+
+ @Test
+ public void checkCodeOwner_ownedByEmailAndOwnedByAllUsers() throws Exception {
+ TestAccount codeOwner =
+ accountCreator.create(
+ "codeOwner", "codeOwner@example.com", "Code Owner", /* displayName= */ null);
+
+ setAsRootCodeOwners(codeOwner.email(), CodeOwnerResolver.ALL_USERS_WILDCARD);
+
+ CodeOwnerCheckInfo checkCodeOwnerInfo = checkCodeOwner(ROOT_PATH, codeOwner.email());
+ assertThat(checkCodeOwnerInfo).isCodeOwner();
+ assertThat(checkCodeOwnerInfo).isResolvable();
+ assertThat(checkCodeOwnerInfo)
+ .hasCodeOwnerConfigFilePathsThat()
+ .containsExactly(getCodeOwnerConfigFilePath(ROOT_PATH));
+ assertThat(checkCodeOwnerInfo).isNotDefaultCodeOwner();
+ assertThat(checkCodeOwnerInfo).isNotGlobalCodeOwner();
+ assertThat(checkCodeOwnerInfo).isOwnedByAllUsers();
+ assertThat(checkCodeOwnerInfo)
+ .hasDebugLogsThatContainAllOf(
+ String.format(
+ "found email %s as code owner in %s",
+ codeOwner.email(), getCodeOwnerConfigFilePath(ROOT_PATH)),
+ String.format(
+ "found email %s as code owner in %s",
+ CodeOwnerResolver.ALL_USERS_WILDCARD, getCodeOwnerConfigFilePath(ROOT_PATH)));
+ }
+
+ @Test
public void checkNonCodeOwner() throws Exception {
CodeOwnerCheckInfo checkCodeOwnerInfo = checkCodeOwner(ROOT_PATH, user.email());
assertThat(checkCodeOwnerInfo).isNotCodeOwner();
@@ -261,6 +313,7 @@
assertThat(checkCodeOwnerInfo).hasCodeOwnerConfigFilePathsThat().isEmpty();
assertThat(checkCodeOwnerInfo).isNotDefaultCodeOwner();
assertThat(checkCodeOwnerInfo).isNotGlobalCodeOwner();
+ assertThat(checkCodeOwnerInfo).isNotOwnedByAllUsers();
assertThat(checkCodeOwnerInfo)
.hasDebugLogsThatContainAllOf(String.format("resolved to account %s", user.id()));
}
@@ -269,7 +322,7 @@
public void checkNonExistingEmail() throws Exception {
String nonExistingEmail = "non-exiting@example.com";
- setAsRootCodeOwner(nonExistingEmail);
+ setAsRootCodeOwners(nonExistingEmail);
CodeOwnerCheckInfo checkCodeOwnerInfo = checkCodeOwner(ROOT_PATH, nonExistingEmail);
assertThat(checkCodeOwnerInfo).isNotCodeOwner();
@@ -279,6 +332,7 @@
.containsExactly(getCodeOwnerConfigFilePath(ROOT_PATH));
assertThat(checkCodeOwnerInfo).isNotDefaultCodeOwner();
assertThat(checkCodeOwnerInfo).isNotGlobalCodeOwner();
+ assertThat(checkCodeOwnerInfo).isNotOwnedByAllUsers();
assertThat(checkCodeOwnerInfo)
.hasDebugLogsThatContainAllOf(
String.format(
@@ -293,7 +347,7 @@
public void checkAmbiguousExistingEmail() throws Exception {
String ambiguousEmail = "ambiguous@example.com";
- setAsRootCodeOwner(ambiguousEmail);
+ setAsRootCodeOwners(ambiguousEmail);
// Add the email to 2 accounts to make it ambiguous.
addEmail(user.id(), ambiguousEmail);
@@ -307,6 +361,7 @@
.containsExactly(getCodeOwnerConfigFilePath(ROOT_PATH));
assertThat(checkCodeOwnerInfo).isNotDefaultCodeOwner();
assertThat(checkCodeOwnerInfo).isNotGlobalCodeOwner();
+ assertThat(checkCodeOwnerInfo).isNotOwnedByAllUsers();
assertThat(checkCodeOwnerInfo)
.hasDebugLogsThatContainAllOf(
String.format(
@@ -328,7 +383,7 @@
extIdNotes.commit(md);
}
- setAsRootCodeOwner(orphanedEmail);
+ setAsRootCodeOwners(orphanedEmail);
CodeOwnerCheckInfo checkCodeOwnerInfo = checkCodeOwner(ROOT_PATH, orphanedEmail);
assertThat(checkCodeOwnerInfo).isNotCodeOwner();
@@ -338,6 +393,7 @@
.containsExactly(getCodeOwnerConfigFilePath(ROOT_PATH));
assertThat(checkCodeOwnerInfo).isNotDefaultCodeOwner();
assertThat(checkCodeOwnerInfo).isNotGlobalCodeOwner();
+ assertThat(checkCodeOwnerInfo).isNotOwnedByAllUsers();
assertThat(checkCodeOwnerInfo)
.hasDebugLogsThatContainAllOf(
String.format(
@@ -368,6 +424,7 @@
.containsExactly(getCodeOwnerConfigFilePath(ROOT_PATH));
assertThat(checkCodeOwnerInfo).isNotDefaultCodeOwner();
assertThat(checkCodeOwnerInfo).isNotGlobalCodeOwner();
+ assertThat(checkCodeOwnerInfo).isNotOwnedByAllUsers();
assertThat(checkCodeOwnerInfo)
.hasDebugLogsThatContainAllOf(
String.format(
@@ -398,6 +455,7 @@
.containsExactly(getCodeOwnerConfigFilePath(ROOT_PATH));
assertThat(checkCodeOwnerInfo).isNotDefaultCodeOwner();
assertThat(checkCodeOwnerInfo).isNotGlobalCodeOwner();
+ assertThat(checkCodeOwnerInfo).isNotOwnedByAllUsers();
assertThat(checkCodeOwnerInfo)
.hasDebugLogsThatContainAllOf(
String.format(
@@ -431,6 +489,7 @@
.containsExactly(getCodeOwnerConfigFilePath(ROOT_PATH));
assertThat(checkCodeOwnerInfo).isNotDefaultCodeOwner();
assertThat(checkCodeOwnerInfo).isNotGlobalCodeOwner();
+ assertThat(checkCodeOwnerInfo).isNotOwnedByAllUsers();
assertThat(checkCodeOwnerInfo)
.hasDebugLogsThatContainAllOf(
String.format(
@@ -452,11 +511,12 @@
assertThat(checkCodeOwnerInfo).hasCodeOwnerConfigFilePathsThat().isEmpty();
assertThat(checkCodeOwnerInfo).isNotDefaultCodeOwner();
assertThat(checkCodeOwnerInfo).isNotGlobalCodeOwner();
+ assertThat(checkCodeOwnerInfo).isNotOwnedByAllUsers();
}
@Test
public void checkAllUsersWildcard_ownedByAllUsers() throws Exception {
- setAsRootCodeOwner(CodeOwnerResolver.ALL_USERS_WILDCARD);
+ setAsRootCodeOwners(CodeOwnerResolver.ALL_USERS_WILDCARD);
CodeOwnerCheckInfo checkCodeOwnerInfo =
checkCodeOwner(ROOT_PATH, CodeOwnerResolver.ALL_USERS_WILDCARD);
@@ -467,6 +527,7 @@
.containsExactly(getCodeOwnerConfigFilePath(ROOT_PATH));
assertThat(checkCodeOwnerInfo).isNotDefaultCodeOwner();
assertThat(checkCodeOwnerInfo).isNotGlobalCodeOwner();
+ assertThat(checkCodeOwnerInfo).isOwnedByAllUsers();
assertThat(checkCodeOwnerInfo)
.hasDebugLogsThatContainAllOf(
String.format(
@@ -491,6 +552,7 @@
assertThat(checkCodeOwnerInfo).hasCodeOwnerConfigFilePathsThat().isEmpty();
assertThat(checkCodeOwnerInfo).isDefaultCodeOwner();
assertThat(checkCodeOwnerInfo).isNotGlobalCodeOwner();
+ assertThat(checkCodeOwnerInfo).isNotOwnedByAllUsers();
assertThat(checkCodeOwnerInfo)
.hasDebugLogsThatContainAllOf(
String.format(
@@ -500,6 +562,32 @@
}
@Test
+ public void checkDefaultCodeOwner_ownedByAllUsers() throws Exception {
+ TestAccount defaultCodeOwner =
+ accountCreator.create(
+ "defaultCodeOwner",
+ "defaultCodeOwner@example.com",
+ "Default Code Owner",
+ /* displayName= */ null);
+ setAsDefaultCodeOwner(CodeOwnerResolver.ALL_USERS_WILDCARD);
+
+ String path = "/foo/bar/baz.md";
+ CodeOwnerCheckInfo checkCodeOwnerInfo = checkCodeOwner(path, defaultCodeOwner.email());
+ assertThat(checkCodeOwnerInfo).isCodeOwner();
+ assertThat(checkCodeOwnerInfo).isResolvable();
+ assertThat(checkCodeOwnerInfo).hasCodeOwnerConfigFilePathsThat().isEmpty();
+ assertThat(checkCodeOwnerInfo).isDefaultCodeOwner();
+ assertThat(checkCodeOwnerInfo).isNotGlobalCodeOwner();
+ assertThat(checkCodeOwnerInfo).isOwnedByAllUsers();
+ assertThat(checkCodeOwnerInfo)
+ .hasDebugLogsThatContainAllOf(
+ String.format(
+ "found email %s as code owner in default code owner config",
+ CodeOwnerResolver.ALL_USERS_WILDCARD),
+ String.format("resolved to account %s", defaultCodeOwner.id()));
+ }
+
+ @Test
@GerritConfig(name = "plugin.code-owners.globalCodeOwner", value = "globalCodeOwner@example.com")
public void checkGlobalCodeOwner() throws Exception {
TestAccount globalCodeOwner =
@@ -516,6 +604,7 @@
assertThat(checkCodeOwnerInfo).hasCodeOwnerConfigFilePathsThat().isEmpty();
assertThat(checkCodeOwnerInfo).isNotDefaultCodeOwner();
assertThat(checkCodeOwnerInfo).isGlobalCodeOwner();
+ assertThat(checkCodeOwnerInfo).isNotOwnedByAllUsers();
assertThat(checkCodeOwnerInfo)
.hasDebugLogsThatContainAllOf(
String.format("found email %s as global code owner", globalCodeOwner.email()),
@@ -523,6 +612,33 @@
}
@Test
+ @GerritConfig(
+ name = "plugin.code-owners.globalCodeOwner",
+ value = CodeOwnerResolver.ALL_USERS_WILDCARD)
+ public void checkGlobalCodeOwner_ownedByAllUsers() throws Exception {
+ TestAccount globalCodeOwner =
+ accountCreator.create(
+ "globalCodeOwner",
+ "globalCodeOwner@example.com",
+ "Global Code Owner",
+ /* displayName= */ null);
+
+ String path = "/foo/bar/baz.md";
+ CodeOwnerCheckInfo checkCodeOwnerInfo = checkCodeOwner(path, globalCodeOwner.email());
+ assertThat(checkCodeOwnerInfo).isCodeOwner();
+ assertThat(checkCodeOwnerInfo).isResolvable();
+ assertThat(checkCodeOwnerInfo).hasCodeOwnerConfigFilePathsThat().isEmpty();
+ assertThat(checkCodeOwnerInfo).isNotDefaultCodeOwner();
+ assertThat(checkCodeOwnerInfo).isGlobalCodeOwner();
+ assertThat(checkCodeOwnerInfo).isOwnedByAllUsers();
+ assertThat(checkCodeOwnerInfo)
+ .hasDebugLogsThatContainAllOf(
+ String.format(
+ "found email %s as global code owner", CodeOwnerResolver.ALL_USERS_WILDCARD),
+ String.format("resolved to account %s", globalCodeOwner.id()));
+ }
+
+ @Test
public void checkCodeOwnerForOtherUser() throws Exception {
TestAccount codeOwner =
accountCreator.create(
@@ -538,6 +654,7 @@
.containsExactly(getCodeOwnerConfigFilePath("/foo/"));
assertThat(checkCodeOwnerInfo).isNotDefaultCodeOwner();
assertThat(checkCodeOwnerInfo).isNotGlobalCodeOwner();
+ assertThat(checkCodeOwnerInfo).isNotOwnedByAllUsers();
assertThat(checkCodeOwnerInfo)
.hasDebugLogsThatContainAllOf(
String.format(
@@ -576,6 +693,7 @@
.containsExactly(getCodeOwnerConfigFilePath(ROOT_PATH));
assertThat(checkCodeOwnerInfo).isNotDefaultCodeOwner();
assertThat(checkCodeOwnerInfo).isNotGlobalCodeOwner();
+ assertThat(checkCodeOwnerInfo).isNotOwnedByAllUsers();
assertThat(checkCodeOwnerInfo)
.hasDebugLogsThatContainAllOf(
String.format(
@@ -598,7 +716,7 @@
.addSecondaryEmail(secondaryEmail)
.update();
- setAsRootCodeOwner(secondaryEmail);
+ setAsRootCodeOwners(secondaryEmail);
CodeOwnerCheckInfo checkCodeOwnerInfo = checkCodeOwner(ROOT_PATH, secondaryEmail, user.email());
assertThat(checkCodeOwnerInfo).isNotCodeOwner();
@@ -608,6 +726,7 @@
.containsExactly(getCodeOwnerConfigFilePath(ROOT_PATH));
assertThat(checkCodeOwnerInfo).isNotDefaultCodeOwner();
assertThat(checkCodeOwnerInfo).isNotGlobalCodeOwner();
+ assertThat(checkCodeOwnerInfo).isNotOwnedByAllUsers();
assertThat(checkCodeOwnerInfo)
.hasDebugLogsThatContainAllOf(
String.format(
@@ -1193,26 +1312,27 @@
return folderPath + getCodeOwnerConfigFileName();
}
- private void setAsRootCodeOwner(String email) {
+ private void setAsRootCodeOwners(String... emails) {
+ TestCodeOwnerConfigCreation.Builder builder =
+ codeOwnerConfigOperations
+ .newCodeOwnerConfig()
+ .project(project)
+ .branch("master")
+ .folderPath(ROOT_PATH);
+ Arrays.stream(emails).forEach(builder::addCodeOwnerEmail);
+ builder.create();
+ }
+
+ private void setAsDefaultCodeOwner(String email) {
codeOwnerConfigOperations
.newCodeOwnerConfig()
.project(project)
- .branch("master")
+ .branch(RefNames.REFS_CONFIG)
.folderPath(ROOT_PATH)
.addCodeOwnerEmail(email)
.create();
}
- private String getCodeOwnerConfigFileName() {
- CodeOwnerBackend backend = backendConfig.getDefaultBackend();
- if (backend instanceof FindOwnersBackend) {
- return FindOwnersBackend.CODE_OWNER_CONFIG_FILE_NAME;
- } else if (backend instanceof ProtoBackend) {
- return ProtoBackend.CODE_OWNER_CONFIG_FILE_NAME;
- }
- throw new IllegalStateException("unknown code owner backend: " + backend.getClass().getName());
- }
-
private void addEmail(Account.Id accountId, String email) throws Exception {
accountsUpdate
.get()
diff --git a/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/CodeOwnerConfigValidatorIT.java b/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/CodeOwnerConfigValidatorIT.java
index 555bfcc..c364abc 100644
--- a/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/CodeOwnerConfigValidatorIT.java
+++ b/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/CodeOwnerConfigValidatorIT.java
@@ -2465,27 +2465,10 @@
backendConfig.getDefaultBackend().getClass().getName()));
}
- private String getParsingErrorMessage(
- ImmutableMap<Class<? extends CodeOwnerBackend>, String> messagesByBackend) {
- CodeOwnerBackend codeOwnerBackend = backendConfig.getDefaultBackend();
- assertThat(messagesByBackend).containsKey(codeOwnerBackend.getClass());
- return messagesByBackend.get(codeOwnerBackend.getClass());
- }
-
private String abbreviateName(AnyObjectId id) throws Exception {
return ObjectIds.abbreviateName(id, testRepo.getRevWalk().getObjectReader());
}
- private String getCodeOwnerConfigFileName() {
- CodeOwnerBackend backend = backendConfig.getDefaultBackend();
- if (backend instanceof FindOwnersBackend) {
- return FindOwnersBackend.CODE_OWNER_CONFIG_FILE_NAME;
- } else if (backend instanceof ProtoBackend) {
- return ProtoBackend.CODE_OWNER_CONFIG_FILE_NAME;
- }
- throw new IllegalStateException("unknown code owner backend: " + backend.getClass().getName());
- }
-
private static void assertOkWithoutMessages(PushOneCommit.Result pushResult) {
pushResult.assertOkStatus();
pushResult.assertNotMessage("fatal");
diff --git a/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/CodeOwnerSubmitRuleIT.java b/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/CodeOwnerSubmitRuleIT.java
index 4e94762..00577c7 100644
--- a/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/CodeOwnerSubmitRuleIT.java
+++ b/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/CodeOwnerSubmitRuleIT.java
@@ -25,6 +25,7 @@
import com.google.gerrit.acceptance.TestAccount;
import com.google.gerrit.acceptance.config.GerritConfig;
import com.google.gerrit.acceptance.testsuite.project.ProjectOperations;
+import com.google.gerrit.common.Nullable;
import com.google.gerrit.entities.BranchNameKey;
import com.google.gerrit.entities.RefNames;
import com.google.gerrit.extensions.api.changes.ReviewInput;
@@ -38,6 +39,7 @@
import com.google.gerrit.plugins.codeowners.api.CodeOwnerStatusInfo;
import com.google.gerrit.plugins.codeowners.common.CodeOwnerStatus;
import com.google.gerrit.plugins.codeowners.testing.SubmitRequirementInfoSubject;
+import com.google.gerrit.plugins.codeowners.util.JgitPath;
import com.google.inject.Inject;
import org.eclipse.jgit.lib.ObjectId;
import org.eclipse.jgit.lib.RefUpdate;
@@ -350,6 +352,67 @@
}
@Test
+ public void changeIsNotSubmittableIfOwnersFileIsNonParsable() throws Exception {
+ testChangeIsNotSubmittableIfOwnersFileIsNonParsable(/* invalidCodeOwnerConfigInfoUrl= */ null);
+ }
+
+ @Test
+ @GerritConfig(name = "plugin.code-owners.invalidCodeOwnerConfigInfoUrl", value = "http://foo.bar")
+ public void changeIsNotSubmittableIfOwnersFileIsNonParsable_withInvalidCodeOwnerConfigInfoUrl()
+ throws Exception {
+ testChangeIsNotSubmittableIfOwnersFileIsNonParsable("http://foo.bar");
+ }
+
+ private void testChangeIsNotSubmittableIfOwnersFileIsNonParsable(
+ @Nullable String invalidCodeOwnerConfigInfoUrl) throws Exception {
+ // Add a non-parsable code owner config.
+ String nameOfInvalidCodeOwnerConfigFile = getCodeOwnerConfigFileName();
+ createNonParseableCodeOwnerConfig(nameOfInvalidCodeOwnerConfigFile);
+
+ PushOneCommit.Result r = createChange("Some Change", "foo.txt", "some content");
+ String changeId = r.getChangeId();
+
+ // Apply Code-Review+2 to satisfy the MaxWithBlock function of the Code-Review label.
+ approve(changeId);
+
+ ChangeInfo changeInfo =
+ gApi.changes()
+ .id(changeId)
+ .get(
+ ListChangesOption.SUBMITTABLE,
+ ListChangesOption.ALL_REVISIONS,
+ ListChangesOption.CURRENT_ACTIONS);
+ assertThat(changeInfo.submittable).isFalse();
+
+ // Check that the submit button is not visible.
+ assertThat(changeInfo.revisions.get(r.getCommit().getName()).actions.get("submit")).isNull();
+
+ // Check the submit requirement.
+ assertThatCollection(changeInfo.requirements).isEmpty();
+
+ // Try to submit the change.
+ ResourceConflictException exception =
+ assertThrows(
+ ResourceConflictException.class, () -> gApi.changes().id(changeId).current().submit());
+ assertThat(exception)
+ .hasMessageThat()
+ .isEqualTo(
+ String.format(
+ "Failed to submit 1 change due to the following problems:\n"
+ + "Change %s: submit rule error: Failed to evaluate code owner statuses for"
+ + " patch set 1 of change %s (cause: invalid code owner config file '%s'"
+ + " (project = %s, branch = master):\n %s).%s",
+ changeInfo._number,
+ changeInfo._number,
+ JgitPath.of(nameOfInvalidCodeOwnerConfigFile).getAsAbsolutePath(),
+ project,
+ getParsingErrorMessageForNonParseableCodeOwnerConfig(),
+ invalidCodeOwnerConfigInfoUrl != null
+ ? String.format("\nFor help check %s.", invalidCodeOwnerConfigInfoUrl)
+ : ""));
+ }
+
+ @Test
@GerritConfig(
name = "plugin.code-owners.mergeCommitStrategy",
value = "FILES_WITH_CONFLICT_RESOLUTION")
diff --git a/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/GetCodeOwnerConfigFilesIT.java b/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/GetCodeOwnerConfigFilesIT.java
index da9c694..f958012 100644
--- a/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/GetCodeOwnerConfigFilesIT.java
+++ b/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/GetCodeOwnerConfigFilesIT.java
@@ -21,14 +21,9 @@
import com.google.gerrit.entities.RefNames;
import com.google.gerrit.extensions.restapi.BadRequestException;
import com.google.gerrit.plugins.codeowners.acceptance.AbstractCodeOwnersIT;
-import com.google.gerrit.plugins.codeowners.backend.CodeOwnerBackend;
import com.google.gerrit.plugins.codeowners.backend.CodeOwnerConfig;
import com.google.gerrit.plugins.codeowners.backend.CodeOwnerSet;
-import com.google.gerrit.plugins.codeowners.backend.config.BackendConfig;
-import com.google.gerrit.plugins.codeowners.backend.findowners.FindOwnersBackend;
-import com.google.gerrit.plugins.codeowners.backend.proto.ProtoBackend;
import com.google.gerrit.plugins.codeowners.util.JgitPath;
-import org.junit.Before;
import org.junit.Test;
/**
@@ -36,13 +31,6 @@
* com.google.gerrit.plugins.codeowners.restapi.GetCodeOwnerConfigFiles} REST endpoint.
*/
public class GetCodeOwnerConfigFilesIT extends AbstractCodeOwnersIT {
- private BackendConfig backendConfig;
-
- @Before
- public void setUpCodeOwnersPlugin() throws Exception {
- backendConfig = plugin.getSysInjector().getInstance(BackendConfig.class);
- }
-
@Test
public void noCodeOwnerConfigFiles() throws Exception {
assertThat(
@@ -434,14 +422,4 @@
codeOwnerConfigOperations.codeOwnerConfig(codeOwnerConfigKey3).getFilePath())
.inOrder();
}
-
- private String getCodeOwnerConfigFileName() {
- CodeOwnerBackend backend = backendConfig.getDefaultBackend();
- if (backend instanceof FindOwnersBackend) {
- return FindOwnersBackend.CODE_OWNER_CONFIG_FILE_NAME;
- } else if (backend instanceof ProtoBackend) {
- return ProtoBackend.CODE_OWNER_CONFIG_FILE_NAME;
- }
- throw new IllegalStateException("unknown code owner backend: " + backend.getClass().getName());
- }
}
diff --git a/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/PutCodeOwnerProjectConfigIT.java b/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/PutCodeOwnerProjectConfigIT.java
index f92d566..5834499 100644
--- a/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/PutCodeOwnerProjectConfigIT.java
+++ b/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/PutCodeOwnerProjectConfigIT.java
@@ -426,6 +426,36 @@
}
@Test
+ public void setInvalidCodeOwnerConfigInfoUrl() throws Exception {
+ assertThat(
+ codeOwnersPluginConfiguration
+ .getProjectConfig(project)
+ .getInvalidCodeOwnerConfigInfoUrl())
+ .isEmpty();
+
+ CodeOwnerProjectConfigInput input = new CodeOwnerProjectConfigInput();
+ input.invalidCodeOwnerConfigInfoUrl = "http://foo.bar";
+ CodeOwnerProjectConfigInfo updatedConfig =
+ projectCodeOwnersApiFactory.project(project).updateConfig(input);
+ assertThat(updatedConfig.general.invalidCodeOwnerConfigInfoUrl).isEqualTo("http://foo.bar");
+ assertThat(
+ codeOwnersPluginConfiguration
+ .getProjectConfig(project)
+ .getInvalidCodeOwnerConfigInfoUrl())
+ .value()
+ .isEqualTo("http://foo.bar");
+
+ input.invalidCodeOwnerConfigInfoUrl = "";
+ updatedConfig = projectCodeOwnersApiFactory.project(project).updateConfig(input);
+ assertThat(updatedConfig.general.invalidCodeOwnerConfigInfoUrl).isNull();
+ assertThat(
+ codeOwnersPluginConfiguration
+ .getProjectConfig(project)
+ .getInvalidCodeOwnerConfigInfoUrl())
+ .isEmpty();
+ }
+
+ @Test
public void setReadOnly() throws Exception {
assertThat(
codeOwnersPluginConfiguration.getProjectConfig(project).areCodeOwnerConfigsReadOnly())
diff --git a/javatests/com/google/gerrit/plugins/codeowners/backend/CodeOwnerConfigScannerTest.java b/javatests/com/google/gerrit/plugins/codeowners/backend/CodeOwnerConfigScannerTest.java
index ee086a1..18fff7f 100644
--- a/javatests/com/google/gerrit/plugins/codeowners/backend/CodeOwnerConfigScannerTest.java
+++ b/javatests/com/google/gerrit/plugins/codeowners/backend/CodeOwnerConfigScannerTest.java
@@ -28,7 +28,6 @@
import com.google.gerrit.plugins.codeowners.acceptance.AbstractCodeOwnersTest;
import com.google.gerrit.plugins.codeowners.acceptance.testsuite.CodeOwnerConfigOperations;
import java.nio.file.Paths;
-import org.eclipse.jgit.errors.ConfigInvalidException;
import org.eclipse.jgit.junit.TestRepository;
import org.eclipse.jgit.lib.Ref;
import org.eclipse.jgit.lib.Repository;
@@ -159,7 +158,8 @@
// Verify that we received the expected callbacks for the invalid code onwer config.
Mockito.verify(invalidCodeOwnerConfigCallback)
- .onInvalidCodeOwnerConfig(eq(Paths.get("/OWNERS")), any(ConfigInvalidException.class));
+ .onInvalidCodeOwnerConfig(
+ eq(Paths.get("/OWNERS")), any(InvalidCodeOwnerConfigException.class));
verifyNoMoreInteractions(invalidCodeOwnerConfigCallback);
}
@@ -189,7 +189,8 @@
// Verify that we received the expected callbacks for the invalid code onwer config.
Mockito.verify(invalidCodeOwnerConfigCallback)
- .onInvalidCodeOwnerConfig(eq(Paths.get("/OWNERS")), any(ConfigInvalidException.class));
+ .onInvalidCodeOwnerConfig(
+ eq(Paths.get("/OWNERS")), any(InvalidCodeOwnerConfigException.class));
verifyNoMoreInteractions(invalidCodeOwnerConfigCallback);
}
diff --git a/javatests/com/google/gerrit/plugins/codeowners/backend/CodeOwnerSubmitRuleTest.java b/javatests/com/google/gerrit/plugins/codeowners/backend/CodeOwnerSubmitRuleTest.java
index 1772809..dc709aa 100644
--- a/javatests/com/google/gerrit/plugins/codeowners/backend/CodeOwnerSubmitRuleTest.java
+++ b/javatests/com/google/gerrit/plugins/codeowners/backend/CodeOwnerSubmitRuleTest.java
@@ -19,12 +19,15 @@
import static org.mockito.Mockito.mock;
import static org.mockito.Mockito.when;
+import com.google.gerrit.acceptance.config.GerritConfig;
import com.google.gerrit.acceptance.testsuite.request.RequestScopeOperations;
+import com.google.gerrit.common.Nullable;
import com.google.gerrit.entities.Change;
import com.google.gerrit.plugins.codeowners.acceptance.AbstractCodeOwnersTest;
import com.google.gerrit.plugins.codeowners.acceptance.testsuite.CodeOwnerConfigOperations;
import com.google.gerrit.plugins.codeowners.testing.SubmitRecordSubject;
import com.google.gerrit.plugins.codeowners.testing.SubmitRequirementSubject;
+import com.google.gerrit.plugins.codeowners.util.JgitPath;
import com.google.gerrit.server.query.change.ChangeData;
import com.google.inject.Inject;
import org.junit.Before;
@@ -138,10 +141,49 @@
}
@Test
- public void ruleErrorWhenChangeDataIsNull() throws Exception {
+ public void ruleError_changeDataIsNull() throws Exception {
SubmitRecordSubject submitRecordSubject =
assertThatOptional(codeOwnerSubmitRule.evaluate(/* changeData= */ null)).value();
submitRecordSubject.hasStatusThat().isRuleError();
submitRecordSubject.hasErrorMessageThat().isEqualTo("Failed to evaluate code owner statuses.");
}
+
+ @Test
+ public void ruleError_nonParsableCodeOwnerConfig() throws Exception {
+ testRuleErrorForNonParsableCodeOwnerConfigl(/* invalidCodeOwnerConfigInfoUrl= */ null);
+ }
+
+ @Test
+ @GerritConfig(name = "plugin.code-owners.invalidCodeOwnerConfigInfoUrl", value = "http://foo.bar")
+ public void ruleError_nonParsableCodeOwnerConfig_withInvalidCodeOwnerConfigInfoUrl()
+ throws Exception {
+ testRuleErrorForNonParsableCodeOwnerConfigl("http://foo.bar");
+ }
+
+ public void testRuleErrorForNonParsableCodeOwnerConfigl(
+ @Nullable String invalidCodeOwnerConfigInfoUrl) throws Exception {
+ String nameOfInvalidCodeOwnerConfigFile = getCodeOwnerConfigFileName();
+ createNonParseableCodeOwnerConfig(nameOfInvalidCodeOwnerConfigFile);
+
+ ChangeData changeData = createChange().getChange();
+
+ SubmitRecordSubject submitRecordSubject =
+ assertThatOptional(codeOwnerSubmitRule.evaluate(changeData)).value();
+ submitRecordSubject.hasStatusThat().isRuleError();
+ submitRecordSubject
+ .hasErrorMessageThat()
+ .isEqualTo(
+ String.format(
+ "Failed to evaluate code owner statuses for patch set %d of change %d"
+ + " (cause: invalid code owner config file '%s' (project = %s, branch = master):\n"
+ + " %s).%s",
+ changeData.change().currentPatchSetId().get(),
+ changeData.change().getId().get(),
+ JgitPath.of(nameOfInvalidCodeOwnerConfigFile).getAsAbsolutePath(),
+ project,
+ getParsingErrorMessageForNonParseableCodeOwnerConfig(),
+ invalidCodeOwnerConfigInfoUrl != null
+ ? String.format("\nFor help check %s.", invalidCodeOwnerConfigInfoUrl)
+ : ""));
+ }
}
diff --git a/javatests/com/google/gerrit/plugins/codeowners/backend/CodeOwnersExceptionHookTest.java b/javatests/com/google/gerrit/plugins/codeowners/backend/CodeOwnersExceptionHookTest.java
index 7b73c83..1c0f9c1 100644
--- a/javatests/com/google/gerrit/plugins/codeowners/backend/CodeOwnersExceptionHookTest.java
+++ b/javatests/com/google/gerrit/plugins/codeowners/backend/CodeOwnersExceptionHookTest.java
@@ -18,12 +18,12 @@
import static com.google.gerrit.truth.OptionalSubject.assertThat;
import com.google.common.collect.ImmutableList;
+import com.google.gerrit.acceptance.config.GerritConfig;
import com.google.gerrit.plugins.codeowners.acceptance.AbstractCodeOwnersTest;
import com.google.gerrit.plugins.codeowners.backend.config.InvalidPluginConfigurationException;
import com.google.gerrit.server.ExceptionHook.Status;
import java.nio.file.InvalidPathException;
import java.util.Optional;
-import org.eclipse.jgit.errors.ConfigInvalidException;
import org.junit.Before;
import org.junit.Test;
@@ -42,8 +42,9 @@
assertThat(skipRetryWithTrace(newExceptionWithCause(newInvalidPluginConfigurationException())))
.isTrue();
- assertThat(skipRetryWithTrace(newConfigInvalidException())).isTrue();
- assertThat(skipRetryWithTrace(newExceptionWithCause(newConfigInvalidException()))).isTrue();
+ assertThat(skipRetryWithTrace(newInvalidCodeOwnerConfigException())).isTrue();
+ assertThat(skipRetryWithTrace(newExceptionWithCause(newInvalidCodeOwnerConfigException())))
+ .isTrue();
assertThat(skipRetryWithTrace(newInvalidPathException())).isTrue();
assertThat(skipRetryWithTrace(newExceptionWithCause(newInvalidPathException()))).isTrue();
@@ -67,11 +68,12 @@
assertThat(getUserMessages(newExceptionWithCause(invalidPluginConfigurationException)))
.containsExactly(invalidPluginConfigurationException.getMessage());
- ConfigInvalidException configInvalidException = newConfigInvalidException();
- assertThat(getUserMessages(configInvalidException))
- .containsExactly(configInvalidException.getMessage());
- assertThat(getUserMessages(newExceptionWithCause(configInvalidException)))
- .containsExactly(configInvalidException.getMessage());
+ InvalidCodeOwnerConfigException invalidCodeOwnerConfigException =
+ newInvalidCodeOwnerConfigException();
+ assertThat(getUserMessages(invalidCodeOwnerConfigException))
+ .containsExactly(invalidCodeOwnerConfigException.getMessage());
+ assertThat(getUserMessages(newExceptionWithCause(invalidCodeOwnerConfigException)))
+ .containsExactly(invalidCodeOwnerConfigException.getMessage());
InvalidPathException invalidPathException = newInvalidPathException();
assertThat(getUserMessages(invalidPathException))
@@ -91,6 +93,21 @@
}
@Test
+ @GerritConfig(name = "plugin.code-owners.invalidCodeOwnerConfigInfoUrl", value = "http://foo.bar")
+ public void getUserMessages_withInvalidCodeOwnerConfigInfoUrl() throws Exception {
+ InvalidCodeOwnerConfigException invalidCodeOwnerConfigException =
+ newInvalidCodeOwnerConfigException();
+ assertThat(getUserMessages(invalidCodeOwnerConfigException))
+ .containsExactly(
+ invalidCodeOwnerConfigException.getMessage(), "For help check http://foo.bar")
+ .inOrder();
+ assertThat(getUserMessages(newExceptionWithCause(invalidCodeOwnerConfigException)))
+ .containsExactly(
+ invalidCodeOwnerConfigException.getMessage(), "For help check http://foo.bar")
+ .inOrder();
+ }
+
+ @Test
public void getStatus() throws Exception {
Status conflictStatus = Status.create(409, "Conflict");
assertThat(getStatus(newInvalidPluginConfigurationException()))
@@ -100,8 +117,8 @@
.value()
.isEqualTo(conflictStatus);
- assertThat(getStatus(newConfigInvalidException())).value().isEqualTo(conflictStatus);
- assertThat(getStatus(newExceptionWithCause(newConfigInvalidException())))
+ assertThat(getStatus(newInvalidCodeOwnerConfigException())).value().isEqualTo(conflictStatus);
+ assertThat(getStatus(newExceptionWithCause(newInvalidCodeOwnerConfigException())))
.value()
.isEqualTo(conflictStatus);
@@ -138,8 +155,8 @@
return new InvalidPluginConfigurationException("code-owners", "message");
}
- private ConfigInvalidException newConfigInvalidException() {
- return new ConfigInvalidException("message");
+ private InvalidCodeOwnerConfigException newInvalidCodeOwnerConfigException() {
+ return new InvalidCodeOwnerConfigException("message", project, "refs/heads/master", "/OWNERS");
}
private InvalidPathException newInvalidPathException() {
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..9220d02 100644
--- a/javatests/com/google/gerrit/plugins/codeowners/backend/config/GeneralConfigTest.java
+++ b/javatests/com/google/gerrit/plugins/codeowners/backend/config/GeneralConfigTest.java
@@ -25,6 +25,7 @@
import static com.google.gerrit.plugins.codeowners.backend.config.GeneralConfig.KEY_FALLBACK_CODE_OWNERS;
import static com.google.gerrit.plugins.codeowners.backend.config.GeneralConfig.KEY_FILE_EXTENSION;
import static com.google.gerrit.plugins.codeowners.backend.config.GeneralConfig.KEY_GLOBAL_CODE_OWNER;
+import static com.google.gerrit.plugins.codeowners.backend.config.GeneralConfig.KEY_INVALID_CODE_OWNER_CONFIG_INFO_URL;
import static com.google.gerrit.plugins.codeowners.backend.config.GeneralConfig.KEY_MAX_PATHS_IN_CHANGE_MESSAGES;
import static com.google.gerrit.plugins.codeowners.backend.config.GeneralConfig.KEY_MERGE_COMMIT_STRATEGY;
import static com.google.gerrit.plugins.codeowners.backend.config.GeneralConfig.KEY_OVERRIDE_INFO_URL;
@@ -1355,23 +1356,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 +1429,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
@@ -1456,6 +1501,48 @@
}
@Test
+ public void cannotGetInvalidCodeOwnerConfigInfoUrlForNullPluginConfig() throws Exception {
+ NullPointerException npe =
+ assertThrows(
+ NullPointerException.class,
+ () -> generalConfig.getInvalidCodeOwnerConfigInfoUrl(/* pluginConfig= */ null));
+ assertThat(npe).hasMessageThat().isEqualTo("pluginConfig");
+ }
+
+ @Test
+ public void noInvalidCodeOwnerConfigInfoUrlConfigured() throws Exception {
+ assertThat(generalConfig.getInvalidCodeOwnerConfigInfoUrl(new Config())).isEmpty();
+ }
+
+ @Test
+ @GerritConfig(
+ name = "plugin.code-owners.invalidCodeOwnerConfigInfoUrl",
+ value = "http://foo.example.com")
+ public void invalidCodeOwnerConfigInfoIsRetrievedFromGerritConfigIfNotSpecifiedOnProjectLevel()
+ throws Exception {
+ assertThat(generalConfig.getInvalidCodeOwnerConfigInfoUrl(new Config()))
+ .value()
+ .isEqualTo("http://foo.example.com");
+ }
+
+ @Test
+ @GerritConfig(
+ name = "plugin.code-owners.invalidCodeOwnerConfigInfoUrl",
+ value = "http://foo.example.com")
+ public void invalidCodeOwnerConfigInfoUrlInPluginConfigOverridesOverrideInfoUrlInGerritConfig()
+ throws Exception {
+ Config cfg = new Config();
+ cfg.setString(
+ SECTION_CODE_OWNERS,
+ /* subsection= */ null,
+ KEY_INVALID_CODE_OWNER_CONFIG_INFO_URL,
+ "http://bar.example.com");
+ assertThat(generalConfig.getInvalidCodeOwnerConfigInfoUrl(cfg))
+ .value()
+ .isEqualTo("http://bar.example.com");
+ }
+
+ @Test
public void cannotGetFallbackCodeOwnersForNullProject() throws Exception {
NullPointerException npe =
assertThrows(
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/javatests/com/google/gerrit/plugins/codeowners/restapi/CodeOwnerProjectConfigJsonTest.java b/javatests/com/google/gerrit/plugins/codeowners/restapi/CodeOwnerProjectConfigJsonTest.java
index ffecde7..bec1973 100644
--- a/javatests/com/google/gerrit/plugins/codeowners/restapi/CodeOwnerProjectConfigJsonTest.java
+++ b/javatests/com/google/gerrit/plugins/codeowners/restapi/CodeOwnerProjectConfigJsonTest.java
@@ -157,6 +157,8 @@
.thenReturn(FallbackCodeOwners.ALL_USERS);
when(codeOwnersPluginConfigSnapshot.getOverrideInfoUrl())
.thenReturn(Optional.of("http://foo.example.com"));
+ when(codeOwnersPluginConfigSnapshot.getInvalidCodeOwnerConfigInfoUrl())
+ .thenReturn(Optional.of("http://bar.example.com"));
when(codeOwnersPluginConfigSnapshot.isDisabled()).thenReturn(false);
when(codeOwnersPluginConfigSnapshot.isDisabled(any(String.class))).thenReturn(false);
when(codeOwnersPluginConfigSnapshot.getBackend()).thenReturn(findOwnersBackend);
@@ -184,6 +186,8 @@
assertThat(codeOwnerProjectConfigInfo.general.fileExtension).isEqualTo("foo");
assertThat(codeOwnerProjectConfigInfo.general.overrideInfoUrl)
.isEqualTo("http://foo.example.com");
+ assertThat(codeOwnerProjectConfigInfo.general.invalidCodeOwnerConfigInfoUrl)
+ .isEqualTo("http://bar.example.com");
assertThat(codeOwnerProjectConfigInfo.general.mergeCommitStrategy)
.isEqualTo(MergeCommitStrategy.FILES_WITH_CONFLICT_RESOLUTION);
assertThat(codeOwnerProjectConfigInfo.general.fallbackCodeOwners)
@@ -278,6 +282,8 @@
.thenReturn(FallbackCodeOwners.ALL_USERS);
when(codeOwnersPluginConfigSnapshot.getOverrideInfoUrl())
.thenReturn(Optional.of("http://foo.example.com"));
+ when(codeOwnersPluginConfigSnapshot.getInvalidCodeOwnerConfigInfoUrl())
+ .thenReturn(Optional.of("http://bar.example.com"));
when(codeOwnersPluginConfigSnapshot.isDisabled(any(String.class))).thenReturn(false);
when(codeOwnersPluginConfigSnapshot.getBackend("refs/heads/master"))
.thenReturn(findOwnersBackend);
@@ -298,6 +304,8 @@
assertThat(codeOwnerBranchConfigInfo.general.fileExtension).isEqualTo("foo");
assertThat(codeOwnerBranchConfigInfo.general.overrideInfoUrl)
.isEqualTo("http://foo.example.com");
+ assertThat(codeOwnerBranchConfigInfo.general.invalidCodeOwnerConfigInfoUrl)
+ .isEqualTo("http://bar.example.com");
assertThat(codeOwnerBranchConfigInfo.general.mergeCommitStrategy)
.isEqualTo(MergeCommitStrategy.FILES_WITH_CONFLICT_RESOLUTION);
assertThat(codeOwnerBranchConfigInfo.general.fallbackCodeOwners)
diff --git a/resources/Documentation/config-faqs.md b/resources/Documentation/config-faqs.md
index 8e0ba49..7396934 100644
--- a/resources/Documentation/config-faqs.md
+++ b/resources/Documentation/config-faqs.md
@@ -4,6 +4,7 @@
* [How to check if the code owners functionality is enabled for a project or branch](#checkIfEnabled)
* [How to avoid issues with code owner config files](#avoidIssuesWithCodeOwnerConfigs)
* [How to investigate issues with code owner config files](#investigateIssuesWithCodeOwnerConfigs)
+* [How to define default code owners](#defineDefaultCodeOwners)
* [How to setup code owner overrides](#setupOverrides)
* [What's the best place to keep the global plugin
configuration](#globalPluginConfiguration)
@@ -100,6 +101,23 @@
Also see [above](#avoidIssuesWithCodeOwnerConfigs) how to avoid issues with code
owner config files in the first place.
+## <a id="defineDefaultCodeOwners">How to define default code owners
+
+[Default code owners](backend-find-owners.html#defaultCodeOwnerConfiguration)
+that apply to all branches can be defined in an `OWNERS` file in the root
+directory of the `refs/meta/config` branch.
+
+To add an `OWNERS` file in the `refs/meta/config` branch do (requires to be a
+project owner):
+
+* clone the repository
+* fetch and checkout the `refs/meta/config` branch (e.g. `git fetch origin
+ refs/meta/config && git checkout FETCH_HEAD`)
+* create or edit the `OWNERS` file
+* commit the changes
+* push the newly created commit back to the `refs/meta/config` branch (e.g. `git
+ push origin HEAD:refs/meta/config`)
+
## <a id="setupOverrides">How to setup code owner overrides
To setup code owner overrides do:
@@ -145,7 +163,9 @@
preferred can depend on the system setup, e.g. changes to `gerrit.config` may be
harder to do and require a multi-day rollout, whereas changes of the
`All-Projects` configuration can be done through the [REST
-API](rest-api.html#update-code-owner-project-config) and are always instant.
+API](rest-api.html#update-code-owner-project-config) and are always instant
+(this can also be a disadvantage as it means that also bad config changes are
+effective immediately).
**NOTE:** Any configuration that is done in `All-Projects` overrides the
corresponding configuration that is inherited from `gerrit.config`.
diff --git a/resources/Documentation/config.md b/resources/Documentation/config.md
index dd7cd35..21a22bd 100644
--- a/resources/Documentation/config.md
+++ b/resources/Documentation/config.md
@@ -11,20 +11,82 @@
This page describes all available configuration parameters. For configuration
recommendations please consult the [config guide](config-guide.html).
+**NOTE**: This is the configuration that controls the behavior of the @PLUGIN@
+plugin. Code owners (except global code owners) are not defined here, but in
+[code owner config files](user-guide.html#codeOwnerConfigFiles) (e.g. `OWNERS`
+files) that are stored in the source tree of the repository.
+
## <a id="inheritance">Inheritance</a>
Projects inherit the configuration of their parent projects, following the chain
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 +127,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.
@@ -104,10 +166,20 @@
The frontend displays a link to this page on the change screen so that
users can discover the override instructions easily.\
Can be overridden per project by setting
- [codeOwners.overrideInfoUrl](#codeOwnersFileExtension) in
+ [codeOwners.overrideInfoUrl](#codeOwnersOverrideInfoUrl) in
`@PLUGIN@.config`.\
By default unset (no override info URL).
+<a id="pluginCodeOwnersInvalidCodeOwnerConfigInfoUrl">plugin.@PLUGIN@.invalidCodeOwnerConfigInfoUrl</a>
+: A URL for a page that provides host-specific information about how to
+ deal with invalid code owner config files.\
+ This URL is included into error messages that indicate invalid code
+ owner config files.\
+ Can be overridden per project by setting
+ [codeOwners.invalidCodeOwnerConfigInfoUrl](#codeOwnersInvalidCodeOwnerConfigInfoUrl)
+ in `@PLUGIN@.config`.\
+ By default unset (no invalid code owner config info URL).
+
<a id="pluginCodeOwnersEnableImplicitApprovals">plugin.@PLUGIN@.enableImplicitApprovals</a>
: Whether an implicit code owner approval from the last uploader is
assumed.\
@@ -151,7 +223,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 +234,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 +422,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 +568,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.\
@@ -570,6 +642,19 @@
[plugin.@PLUGIN@.overrideInfoUrl](#pluginCodeOwnersOverrideInfoUrl) in
`gerrit.config` is used.
+<a id="codeOwnersInvalidCodeOwnerConfigInfoUrl">codeOwners.invalidCodeOwnerConfigInfoUrl</a>
+: A URL for a page that provides project-specific information about how
+ to deal with invalid code owner config files.\
+ This URL is included into error messages that indicate invalid code
+ owner config files.\
+ Overrides the global setting
+ [plugin.@PLUGIN@.invalidCodeOwnerConfigInfoUrl](#pluginCodeOwnersInvalidCodeOwnerConfigInfoUrl)
+ in `gerrit.config` and the `codeOwners.invalidCodeOwnerConfigInfoUrl`
+ setting from parent projects.\
+ If not set, the global setting
+ [plugin.@PLUGIN@.invalidCodeOwnerConfigInfoUrl](#pluginCodeOwnersInvalidCodeOwnerConfigInfoUrl)
+ in `gerrit.config` is used.
+
<a id="codeOwnersEnableImplicitApprovals">codeOwners.enableImplicitApprovals</a>
: Whether an implicit code owner approval from the last uploader is
assumed.\
@@ -600,7 +685,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 +699,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 +912,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.\
diff --git a/resources/Documentation/metrics.md b/resources/Documentation/metrics.md
index 565ca67..5cff902 100644
--- a/resources/Documentation/metrics.md
+++ b/resources/Documentation/metrics.md
@@ -57,8 +57,21 @@
Total number of code owner config reads from backend.
* `count_code_owner_config_cache_reads`:
Total number of code owner config reads from cache.
+* `count_code_owner_submit_rule_errors`:
+ Total number of code owner submit rule errors.
+ * `cause`:
+ The cause of the submit rule error.
* `count_code_owner_submit_rule_runs`:
Total number of code owner submit rule runs.
+* `count_invalid_code_owner_config_files`:
+ Total number of failed requests caused by an invalid / non-parsable code owner
+ config file.
+ * `project`:
+ The name of the project that contains the invalid code owner config file.
+ * `branch`:
+ The name of the branch that contains the invalid code owner config file.
+ * `path`:
+ The path of the invalid code owner config file.
---
diff --git a/resources/Documentation/rest-api.md b/resources/Documentation/rest-api.md
index 29d5fd8..40fb545 100644
--- a/resources/Documentation/rest-api.md
+++ b/resources/Documentation/rest-api.md
@@ -807,6 +807,7 @@
| `is_default_code_owner` | Whether the given email is configured as a default code owner in the code owner config file in `refs/meta/config`. Note that if the email is configured as default code owner, but the email is not resolvable (see `is_resolvable` field), the user is not a code owner.
| `is_global_code_owner` | Whether the given email is configured as a global
code owner. Note that if the email is configured as global code owner, but the email is not resolvable (see `is_resolvable` field), the user is not a code owner.
+| `is_owned_by_all_users` | Whether the the specified path in the branch is owned by all users (aka `*`).
| `debug_logs` | List of debug logs that may help to understand why the user is or isn't a code owner.
---
@@ -879,6 +880,7 @@
| `merge_commit_strategy` | optional | Strategy that defines for merge commits which files require code owner approvals. Can be `ALL_CHANGED_FILES` or `FILES_WITH_CONFLICT_RESOLUTION` (see [mergeCommitStrategy](config.html#pluginCodeOwnersMergeCommitStrategy) for an explanation of these values).
| `implicit_approvals` | optional | Whether an implicit code owner approval from the last uploader is assumed.
| `override_info_url` | optional | URL for a page that provides project/host-specific information about how to request a code owner override.
+| `invalid_code_owner_config_info_url` | optional | URL for a page that provides project/host-specific information about how to deal with invalid code owner config files.
| `read_only` | optional | Whether code owner config files are read-only.
| `exempt_pure_reverts` | optional | Whether pure revert changes are exempted from needing code owner approvals for submit.
| `enable_validation_on_commit_received` | optional | Policy for validating code owner config files when a commit is received. Allowed values are `true` (the code owner config file validation is enabled and the upload of invalid code owner config files is rejected), `false` (the code owner config file validation is disabled, invalid code owner config files are not rejected) and `dry_run` (code owner config files are validated, but invalid code owner config files are not rejected).
@@ -951,6 +953,7 @@
| `merge_commit_strategy` || Strategy that defines for merge commits which files require code owner approvals. Can be `ALL_CHANGED_FILES` or `FILES_WITH_CONFLICT_RESOLUTION` (see [mergeCommitStrategy](config.html#pluginCodeOwnersMergeCommitStrategy) for an explanation of these values).
| `implicit_approvals` | optional | Whether an implicit code owner approval from the last uploader is assumed (see [enableImplicitApprovals](config.html#pluginCodeOwnersEnableImplicitApprovals) for details). When unset, `false`.
| `override_info_url` | optional | Optional URL for a page that provides project/host-specific information about how to request a code owner override.
+| `invalid_code_owner_config_info_url` | optional | Optional URL for a page that provides project/host-specific information about how to deal with invalid code owner config files.
|`fallback_code_owners` || Policy that controls who should own paths that have no code owners defined. Possible values are: `NONE`: Paths for which no code owners are defined are owned by no one. `PROJECT_OWNERS`: Paths for which no code owners are defined are owned by the project owners. `ALL_USERS`: Paths for which no code owners are defined are owned by all users.
### <a id="owned-paths-info"> OwnedPathsInfo
diff --git a/resources/Documentation/user-guide.md b/resources/Documentation/user-guide.md
index 7f06bb1..f7318ee 100644
--- a/resources/Documentation/user-guide.md
+++ b/resources/Documentation/user-guide.md
@@ -219,6 +219,39 @@
are trusted, as it prevents that code owners need to approve the same changes
multiple times, but for different branches.
+## <a id="codeOwnersSubmitRule">Code Owners Submit Rule
+
+The logic that checks whether a change has sufficient [code owner
+approvals](#codeOwnerApproval) to be submitted is implemented in the code owners
+submit rule. If the code owners submit rule finds that code owner approvals are
+missing the submission of the change is blocked. In this case it's possible to
+use a [code owner override](#codeOwnerOverride) to unblock the change
+submission.
+
+**NOTE:** Besides the code owners submit rule there may be further submit rules
+that block the change submission for other reasons that are not related to code
+owners. E.g. configured [label
+functions](../../../Documentation/config-labels.html#label_function) are
+completely orthogonal to code owner approvals. If, for example, `Code-Review+1`
+votes are required as code owner approval, but the `Code-Review` label has the
+function `MaxWithBlock` the change submission is still blocked if a max approval
+(aka `Code-Review+2`) is missing or if a veto vote (aka `Code-Review-2`) is
+present.
+
+**NOTE:** Gerrit submit rules are executed on submit and when change details are
+loaded, e.g. when loading the change screen (to know whether the submit button
+should be enabled). In addition, submit rules are executed on every change
+update because the result of running submit rules is stored as submit records in
+the change index. This makes the submit records available when querying changes
+(without needing to run the submit rules for every change in the result which
+would be too expensive). For code owners the submit records that are stored in
+the index can become stale for 2 reasons: 1. [code owner config
+files](#codeOwnerConfigFiles) are changed after the change has been indexed
+(e.g. new code owners are added), 2. [if the code owners plugin configuration
+was changed in a way that affected the result of the code owners submit
+rule](config.html#staleIndexOnConfigChanges). Callers of change queries should
+be aware of this.
+
---
Back to [@PLUGIN@ documentation index](index.html)
