Merge changes I74098f90,I3216fe91,I0bf9974b,I7311efb4,If53eb1d1
* changes:
GeneralConfig: Include cause when logging warning for IllegalArgumentException
Make toString methods in AutoValue classes final
Fix typos
Include plugin name into validation messages
Add config options to not reject non-resolvable code owners / imports
diff --git a/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerResolverResult.java b/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerResolverResult.java
index 4a9e372..0744e5b 100644
--- a/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerResolverResult.java
+++ b/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerResolverResult.java
@@ -66,7 +66,7 @@
}
@Override
- public String toString() {
+ public final String toString() {
return MoreObjects.toStringHelper(this)
.add("codeOwners", codeOwners())
.add("ownedByAllUsers", ownedByAllUsers())
diff --git a/java/com/google/gerrit/plugins/codeowners/backend/PathCodeOwnersResult.java b/java/com/google/gerrit/plugins/codeowners/backend/PathCodeOwnersResult.java
index 934d730..bc45ded 100644
--- a/java/com/google/gerrit/plugins/codeowners/backend/PathCodeOwnersResult.java
+++ b/java/com/google/gerrit/plugins/codeowners/backend/PathCodeOwnersResult.java
@@ -71,7 +71,7 @@
}
@Override
- public String toString() {
+ public final String toString() {
return MoreObjects.toStringHelper(this)
.add("path", path())
.add("codeOwnerConfig", codeOwnerConfig())
diff --git a/java/com/google/gerrit/plugins/codeowners/backend/UnresolvedImport.java b/java/com/google/gerrit/plugins/codeowners/backend/UnresolvedImport.java
index 4d1b975..865c469 100644
--- a/java/com/google/gerrit/plugins/codeowners/backend/UnresolvedImport.java
+++ b/java/com/google/gerrit/plugins/codeowners/backend/UnresolvedImport.java
@@ -51,7 +51,7 @@
}
@Override
- public String toString() {
+ public final String toString() {
return MoreObjects.toStringHelper(this)
.add("keyOfImportingCodeOwnerConfig", keyOfImportingCodeOwnerConfig())
.add("keyOfImportedCodeOwnerConfig", keyOfImportedCodeOwnerConfig())
diff --git a/java/com/google/gerrit/plugins/codeowners/backend/config/CodeOwnersPluginConfiguration.java b/java/com/google/gerrit/plugins/codeowners/backend/config/CodeOwnersPluginConfiguration.java
index a440235..4b70846 100644
--- a/java/com/google/gerrit/plugins/codeowners/backend/config/CodeOwnersPluginConfiguration.java
+++ b/java/com/google/gerrit/plugins/codeowners/backend/config/CodeOwnersPluginConfiguration.java
@@ -107,7 +107,8 @@
/**
* Checks whether code owner configs in the given project are read-only.
*
- * @param project the project for it should be checked whether code owner configs are read-only
+ * @param project the project for which it should be checked whether code owner configs are
+ * read-only
* @return whether code owner configs in the given project are read-only
*/
public boolean areCodeOwnerConfigsReadOnly(Project.NameKey project) {
@@ -116,6 +117,34 @@
}
/**
+ * Checks whether newly added non-resolvable code owners should be rejected on commit received and
+ * submit.
+ *
+ * @param project the project for which it should be checked whether non-resolvable code owners
+ * should be rejected
+ * @return whether newly added non-resolvable code owners should be rejected on commit received
+ * and submit
+ */
+ public boolean rejectNonResolvableCodeOwners(Project.NameKey project) {
+ requireNonNull(project, "project");
+ return generalConfig.getRejectNonResolvableCodeOwners(getPluginConfig(project));
+ }
+
+ /**
+ * Checks whether newly added non-resolvable imports should be rejected on commit received and
+ * submit.
+ *
+ * @param project the project for which it should be checked whether non-resolvable imports should
+ * be rejected
+ * @return whether newly added non-resolvable imports should be rejected on commit received and
+ * submit
+ */
+ public boolean rejectNonResolvableImports(Project.NameKey project) {
+ requireNonNull(project, "project");
+ return generalConfig.getRejectNonResolvableImports(getPluginConfig(project));
+ }
+
+ /**
* Whether code owner configs should be validated when a commit is received.
*
* @param project the project for it should be checked whether code owner configs should be
@@ -179,7 +208,7 @@
/**
* Checks whether an implicit code owner approval from the last uploader is assumed.
*
- * @param project the project for it should be checked whether implict approvals are enabled
+ * @param project the project for it should be checked whether implicit approvals are enabled
* @return whether an implicit code owner approval from the last uploader is assumed
*/
public boolean areImplicitApprovalsEnabled(Project.NameKey project) {
diff --git a/java/com/google/gerrit/plugins/codeowners/backend/config/GeneralConfig.java b/java/com/google/gerrit/plugins/codeowners/backend/config/GeneralConfig.java
index 5d7062b..e14ece9 100644
--- a/java/com/google/gerrit/plugins/codeowners/backend/config/GeneralConfig.java
+++ b/java/com/google/gerrit/plugins/codeowners/backend/config/GeneralConfig.java
@@ -82,6 +82,13 @@
@VisibleForTesting static final int DEFAULT_MAX_PATHS_IN_CHANGE_MESSAGES = 100;
+ @VisibleForTesting
+ public static final String KEY_REJECT_NON_RESOLVABLE_CODE_OWNERS =
+ "rejectNonResolvableCodeOwners";
+
+ @VisibleForTesting
+ public static final String KEY_REJECT_NON_RESOLVABLE_IMPORTS = "rejectNonResolvableImports";
+
private final String pluginName;
private final PluginConfig pluginConfigFromGerritConfig;
@@ -208,20 +215,58 @@
* Gets the read-only configuration from the given plugin config with fallback to {@code
* gerrit.config}.
*
- * <p>The read-only controls whether code owner config files are read-only and all modifications
- * of code owner config files should be rejected.
+ * <p>The read-only configuration controls whether code owner config files are read-only and all
+ * modifications of code owner config files should be rejected.
*
* @param pluginConfig the plugin config from which the read-only configuration should be read.
* @return whether code owner config files are read-only
*/
boolean getReadOnly(Config pluginConfig) {
- requireNonNull(pluginConfig, "pluginConfig");
+ return getBooleanConfig(pluginConfig, KEY_READ_ONLY, false);
+ }
- if (pluginConfig.getString(SECTION_CODE_OWNERS, null, KEY_READ_ONLY) != null) {
- return pluginConfig.getBoolean(SECTION_CODE_OWNERS, null, KEY_READ_ONLY, false);
+ /**
+ * Gets the reject-non-resolvable-code-owners configuration from the given plugin config with
+ * fallback to {@code gerrit.config}.
+ *
+ * <p>The reject-non-resolvable-code-owners configuration controls whether code owner config files
+ * with newly added non-resolvable code owners should be rejected on commit received and on
+ * submit.
+ *
+ * @param pluginConfig the plugin config from which the reject-non-resolvable-code-owners
+ * configuration should be read.
+ * @return whether code owner config files with newly added non-resolvable code owners should be
+ * rejected on commit received and on submit
+ */
+ boolean getRejectNonResolvableCodeOwners(Config pluginConfig) {
+ return getBooleanConfig(pluginConfig, KEY_REJECT_NON_RESOLVABLE_CODE_OWNERS, true);
+ }
+
+ /**
+ * Gets the reject-non-resolvable-imports configuration from the given plugin config with fallback
+ * to {@code gerrit.config}.
+ *
+ * <p>The reject-non-resolvable-imports configuration controls whether code owner config files
+ * with newly added non-resolvable imports should be rejected on commit received and on submit.
+ *
+ * @param pluginConfig the plugin config from which the reject-non-resolvable-imports
+ * configuration should be read.
+ * @return whether code owner config files with newly added non-resolvable imports should be
+ * rejected on commit received and on submit
+ */
+ boolean getRejectNonResolvableImports(Config pluginConfig) {
+ return getBooleanConfig(pluginConfig, KEY_REJECT_NON_RESOLVABLE_IMPORTS, true);
+ }
+
+ private boolean getBooleanConfig(Config pluginConfig, String key, boolean defaultValue) {
+ requireNonNull(pluginConfig, "pluginConfig");
+ requireNonNull(key, "key");
+
+ if (pluginConfig.getString(SECTION_CODE_OWNERS, null, key) != null) {
+ return pluginConfig.getBoolean(SECTION_CODE_OWNERS, null, key, defaultValue);
}
- return pluginConfigFromGerritConfig.getBoolean(KEY_READ_ONLY, false);
+ return pluginConfigFromGerritConfig.getBoolean(key, defaultValue);
}
/**
@@ -242,7 +287,7 @@
return pluginConfig.getEnum(
SECTION_CODE_OWNERS, null, KEY_FALLBACK_CODE_OWNERS, FallbackCodeOwners.NONE);
} catch (IllegalArgumentException e) {
- logger.atWarning().log(
+ logger.atWarning().withCause(e).log(
"Ignoring invalid value %s for fallback code owners in '%s.config' of project %s."
+ " Falling back to global config.",
fallbackCodeOwnersString, pluginName, project.get());
@@ -253,7 +298,7 @@
return pluginConfigFromGerritConfig.getEnum(
KEY_FALLBACK_CODE_OWNERS, FallbackCodeOwners.NONE);
} catch (IllegalArgumentException e) {
- logger.atWarning().log(
+ logger.atWarning().withCause(e).log(
"Ignoring invalid value %s for fallback code owners in gerrit.config (parameter"
+ " plugin.%s.%s). Falling back to default value %s.",
pluginConfigFromGerritConfig.getString(KEY_FALLBACK_CODE_OWNERS),
@@ -287,7 +332,7 @@
KEY_MAX_PATHS_IN_CHANGE_MESSAGES,
DEFAULT_MAX_PATHS_IN_CHANGE_MESSAGES);
} catch (IllegalArgumentException e) {
- logger.atWarning().log(
+ logger.atWarning().withCause(e).log(
"Ignoring invalid value %s for max paths in change messages in '%s.config' of"
+ " project %s. Falling back to global config.",
maxPathInChangeMessagesString, pluginName, project.get());
@@ -298,7 +343,7 @@
return pluginConfigFromGerritConfig.getInt(
KEY_MAX_PATHS_IN_CHANGE_MESSAGES, DEFAULT_MAX_PATHS_IN_CHANGE_MESSAGES);
} catch (IllegalArgumentException e) {
- logger.atWarning().log(
+ logger.atWarning().withCause(e).log(
"Ignoring invalid value %s for max paths in change messages in gerrit.config (parameter"
+ " plugin.%s.%s). Falling back to default value %s.",
pluginConfigFromGerritConfig.getString(KEY_MAX_PATHS_IN_CHANGE_MESSAGES),
@@ -356,7 +401,7 @@
return pluginConfig.getEnum(
SECTION_CODE_OWNERS, null, key, CodeOwnerConfigValidationPolicy.TRUE);
} catch (IllegalArgumentException e) {
- logger.atWarning().log(
+ logger.atWarning().withCause(e).log(
"Ignoring invalid value %s for the code owner config validation policy in '%s.config'"
+ " of project %s. Falling back to global config.",
codeOwnerConfigValidationPolicyString, pluginName, project.get());
@@ -366,7 +411,7 @@
try {
return pluginConfigFromGerritConfig.getEnum(key, CodeOwnerConfigValidationPolicy.TRUE);
} catch (IllegalArgumentException e) {
- logger.atWarning().log(
+ logger.atWarning().withCause(e).log(
"Ignoring invalid value %s for the code owner config validation policy in gerrit.config"
+ " (parameter plugin.%s.%s). Falling back to default value %s.",
pluginConfigFromGerritConfig.getString(key),
@@ -402,7 +447,7 @@
KEY_MERGE_COMMIT_STRATEGY,
MergeCommitStrategy.ALL_CHANGED_FILES);
} catch (IllegalArgumentException e) {
- logger.atWarning().log(
+ logger.atWarning().withCause(e).log(
"Ignoring invalid value %s for merge commit stategy in '%s.config' of project %s."
+ " Falling back to global config or default value.",
mergeCommitStrategyString, pluginName, project.get());
@@ -413,7 +458,7 @@
return pluginConfigFromGerritConfig.getEnum(
KEY_MERGE_COMMIT_STRATEGY, MergeCommitStrategy.ALL_CHANGED_FILES);
} catch (IllegalArgumentException e) {
- logger.atWarning().log(
+ logger.atWarning().withCause(e).log(
"Ignoring invalid value %s for merge commit stategy in gerrit.config (parameter plugin.%s.%s)."
+ " Falling back to default value %s.",
pluginConfigFromGerritConfig.getString(KEY_MERGE_COMMIT_STRATEGY),
diff --git a/java/com/google/gerrit/plugins/codeowners/common/MergeCommitStrategy.java b/java/com/google/gerrit/plugins/codeowners/common/MergeCommitStrategy.java
index da9c76d..ca08938 100644
--- a/java/com/google/gerrit/plugins/codeowners/common/MergeCommitStrategy.java
+++ b/java/com/google/gerrit/plugins/codeowners/common/MergeCommitStrategy.java
@@ -17,7 +17,7 @@
/** Strategy that defines for merge commits which files require code owner approvals. */
public enum MergeCommitStrategy {
/**
- * All files which differ between the merge commmit that is being reviewed and its first parent
+ * All files which differ between the merge commit that is being reviewed and its first parent
* commit (which is the HEAD of the destination branch) require code owner approvals.
*
* <p>Using this strategy is the safest option, but requires code owners to also approve files
@@ -30,7 +30,7 @@
ALL_CHANGED_FILES,
/**
- * Only files which differ between the merge commmit that is being reviewed and the auto merge
+ * Only files which differ between the merge commit that is being reviewed and the auto merge
* commit (the result of automatically merging the 2 parent commits, may contain Git conflict
* markers) require code owner approvals.
*
diff --git a/java/com/google/gerrit/plugins/codeowners/restapi/CheckCodeOwnerConfigFiles.java b/java/com/google/gerrit/plugins/codeowners/restapi/CheckCodeOwnerConfigFiles.java
index 73ed96d..b0da9f7 100644
--- a/java/com/google/gerrit/plugins/codeowners/restapi/CheckCodeOwnerConfigFiles.java
+++ b/java/com/google/gerrit/plugins/codeowners/restapi/CheckCodeOwnerConfigFiles.java
@@ -161,7 +161,8 @@
codeOwnerConfig -> {
problemsByPath.putAll(
codeOwnerBackend.getFilePath(codeOwnerConfig.key()).toString(),
- checkCodeOwnerConfig(codeOwnerBackend, codeOwnerConfig, verbosity));
+ checkCodeOwnerConfig(
+ branchNameKey.project(), codeOwnerBackend, codeOwnerConfig, verbosity));
return true;
},
(codeOwnerConfigFilePath, configInvalidException) -> {
@@ -176,12 +177,13 @@
}
private ImmutableList<ConsistencyProblemInfo> checkCodeOwnerConfig(
+ Project.NameKey project,
CodeOwnerBackend codeOwnerBackend,
CodeOwnerConfig codeOwnerConfig,
@Nullable ConsistencyProblemInfo.Status verbosity) {
return codeOwnerConfigValidator
.validateCodeOwnerConfig(
- currentUser.get().asIdentifiedUser(), codeOwnerBackend, codeOwnerConfig)
+ project, currentUser.get().asIdentifiedUser(), codeOwnerBackend, codeOwnerConfig)
.map(
commitValidationMessage ->
createConsistencyProblemInfo(commitValidationMessage, verbosity))
diff --git a/java/com/google/gerrit/plugins/codeowners/validation/CodeOwnerConfigValidator.java b/java/com/google/gerrit/plugins/codeowners/validation/CodeOwnerConfigValidator.java
index 9458289..e4714bd 100644
--- a/java/com/google/gerrit/plugins/codeowners/validation/CodeOwnerConfigValidator.java
+++ b/java/com/google/gerrit/plugins/codeowners/validation/CodeOwnerConfigValidator.java
@@ -29,6 +29,7 @@
import com.google.gerrit.entities.PatchSet;
import com.google.gerrit.entities.Project;
import com.google.gerrit.exceptions.StorageException;
+import com.google.gerrit.extensions.annotations.PluginName;
import com.google.gerrit.plugins.codeowners.backend.ChangedFiles;
import com.google.gerrit.plugins.codeowners.backend.CodeOwnerBackend;
import com.google.gerrit.plugins.codeowners.backend.CodeOwnerConfig;
@@ -125,6 +126,7 @@
public class CodeOwnerConfigValidator implements CommitValidationListener, MergeValidationListener {
private static final FluentLogger logger = FluentLogger.forEnclosingClass();
+ private final String pluginName;
private final CodeOwnersPluginConfiguration codeOwnersPluginConfiguration;
private final GitRepositoryManager repoManager;
private final ChangedFiles changedFiles;
@@ -137,6 +139,7 @@
@Inject
CodeOwnerConfigValidator(
+ @PluginName String pluginName,
CodeOwnersPluginConfiguration codeOwnersPluginConfiguration,
GitRepositoryManager repoManager,
ChangedFiles changedFiles,
@@ -146,6 +149,7 @@
ChangeNotes.Factory changeNotesFactory,
PatchSetUtil patchSetUtil,
IdentifiedUser.GenericFactory userFactory) {
+ this.pluginName = pluginName;
this.codeOwnersPluginConfiguration = codeOwnersPluginConfiguration;
this.repoManager = repoManager;
this.changedFiles = changedFiles;
@@ -178,6 +182,7 @@
validationResult =
Optional.of(
ValidationResult.create(
+ pluginName,
"skipping validation of code owner config files",
new CommitValidationMessage(
"code owners config validation is disabled", ValidationMessage.Type.HINT)));
@@ -246,6 +251,7 @@
validationResult =
Optional.of(
ValidationResult.create(
+ pluginName,
"skipping validation of code owner config files",
new CommitValidationMessage(
"code owners config validation is disabled", ValidationMessage.Type.HINT)));
@@ -300,6 +306,7 @@
if (codeOwnersPluginConfiguration.isDisabled(branchNameKey)) {
return Optional.of(
ValidationResult.create(
+ pluginName,
"skipping validation of code owner config files",
new CommitValidationMessage(
"code-owners functionality is disabled", ValidationMessage.Type.HINT)));
@@ -307,6 +314,7 @@
if (codeOwnersPluginConfiguration.areCodeOwnerConfigsReadOnly(branchNameKey.project())) {
return Optional.of(
ValidationResult.create(
+ pluginName,
"modifying code owner config files not allowed",
new CommitValidationMessage(
"code owner config files are configured to be read-only",
@@ -352,6 +360,7 @@
// validate the code owner config files
return Optional.of(
ValidationResult.create(
+ pluginName,
modifiedCodeOwnerConfigFiles.stream()
.flatMap(
changedFile ->
@@ -375,6 +384,7 @@
e.getMessage()));
return Optional.of(
ValidationResult.create(
+ pluginName,
"skipping validation of code owner config files",
new CommitValidationMessage(
"code-owners plugin configuration is invalid,"
@@ -477,7 +487,8 @@
// 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
// warnings so that the update is not blocked.
- return validateCodeOwnerConfig(user, codeOwnerBackend, codeOwnerConfig)
+ return validateCodeOwnerConfig(
+ branchNameKey.project(), user, codeOwnerBackend, codeOwnerConfig)
.map(CodeOwnerConfigValidator::downgradeErrorToWarning);
}
@@ -488,9 +499,14 @@
// Validate the parsed code owner config.
if (baseCodeOwnerConfig.isPresent()) {
return validateCodeOwnerConfig(
- user, codeOwnerBackend, codeOwnerConfig, baseCodeOwnerConfig.get());
+ branchNameKey.project(),
+ user,
+ codeOwnerBackend,
+ codeOwnerConfig,
+ baseCodeOwnerConfig.get());
}
- return validateCodeOwnerConfig(user, codeOwnerBackend, codeOwnerConfig);
+ return validateCodeOwnerConfig(
+ branchNameKey.project(), user, codeOwnerBackend, codeOwnerConfig);
}
/**
@@ -655,6 +671,7 @@
* <p>Validation errors that exist in both code owner configs are returned as warning (because
* they are not newly introduced by the given code owner config).
*
+ * @param project the name of the project
* @param user user for which the code owner visibility checks should be performed
* @param codeOwnerBackend the code owner backend from which the code owner configs were loaded
* @param codeOwnerConfig the code owner config that should be validated
@@ -663,6 +680,7 @@
* empty stream if there are no issues
*/
private Stream<CommitValidationMessage> validateCodeOwnerConfig(
+ Project.NameKey project,
IdentifiedUser user,
CodeOwnerBackend codeOwnerBackend,
CodeOwnerConfig codeOwnerConfig,
@@ -671,9 +689,9 @@
requireNonNull(baseCodeOwnerConfig, "baseCodeOwnerConfig");
ImmutableSet<CommitValidationMessage> issuesInBaseVersion =
- validateCodeOwnerConfig(user, codeOwnerBackend, baseCodeOwnerConfig)
+ validateCodeOwnerConfig(project, user, codeOwnerBackend, baseCodeOwnerConfig)
.collect(toImmutableSet());
- return validateCodeOwnerConfig(user, codeOwnerBackend, codeOwnerConfig)
+ return validateCodeOwnerConfig(project, user, codeOwnerBackend, codeOwnerConfig)
.map(
commitValidationMessage ->
issuesInBaseVersion.contains(commitValidationMessage)
@@ -684,6 +702,7 @@
/**
* Validates the given code owner config and returns validation issues as stream.
*
+ * @param project the name of the project
* @param user user for which the code owner visibility checks should be performed
* @param codeOwnerBackend the code owner backend from which the code owner config was loaded
* @param codeOwnerConfig the code owner config that should be validated
@@ -691,17 +710,22 @@
* empty stream if there are no issues
*/
public Stream<CommitValidationMessage> validateCodeOwnerConfig(
- IdentifiedUser user, CodeOwnerBackend codeOwnerBackend, CodeOwnerConfig codeOwnerConfig) {
+ Project.NameKey project,
+ IdentifiedUser user,
+ CodeOwnerBackend codeOwnerBackend,
+ CodeOwnerConfig codeOwnerConfig) {
requireNonNull(codeOwnerConfig, "codeOwnerConfig");
return Streams.concat(
validateCodeOwnerReferences(
- user, codeOwnerBackend.getFilePath(codeOwnerConfig.key()), codeOwnerConfig),
- validateImports(codeOwnerBackend.getFilePath(codeOwnerConfig.key()), codeOwnerConfig));
+ project, user, codeOwnerBackend.getFilePath(codeOwnerConfig.key()), codeOwnerConfig),
+ validateImports(
+ project, codeOwnerBackend.getFilePath(codeOwnerConfig.key()), codeOwnerConfig));
}
/**
* Validates the code owner references of the given code owner config.
*
+ * @param project the name of the project
* @param user user for which the code owner visibility checks should be performed
* @param codeOwnerConfigFilePath the path of the code owner config file which contains the code
* owner references
@@ -711,12 +735,16 @@
* empty stream if there are no issues
*/
private Stream<CommitValidationMessage> validateCodeOwnerReferences(
- IdentifiedUser user, Path codeOwnerConfigFilePath, CodeOwnerConfig codeOwnerConfig) {
+ Project.NameKey project,
+ IdentifiedUser user,
+ Path codeOwnerConfigFilePath,
+ CodeOwnerConfig codeOwnerConfig) {
return codeOwnerConfig.codeOwnerSets().stream()
.flatMap(codeOwnerSet -> codeOwnerSet.codeOwners().stream())
.map(
codeOwnerReference ->
- validateCodeOwnerReference(user, codeOwnerConfigFilePath, codeOwnerReference))
+ validateCodeOwnerReference(
+ project, user, codeOwnerConfigFilePath, codeOwnerReference))
.filter(Optional::isPresent)
.map(Optional::get);
}
@@ -724,6 +752,7 @@
/**
* Validates a code owner reference.
*
+ * @param project the name of the project
* @param user user for which the code owner visibility checks should be performed
* @param codeOwnerConfigFilePath the path of the code owner config file which contains the code
* owner reference
@@ -732,10 +761,14 @@
* Optional#empty()} if there is no issue
*/
private Optional<CommitValidationMessage> validateCodeOwnerReference(
- IdentifiedUser user, Path codeOwnerConfigFilePath, CodeOwnerReference codeOwnerReference) {
+ Project.NameKey project,
+ IdentifiedUser user,
+ Path codeOwnerConfigFilePath,
+ CodeOwnerReference codeOwnerReference) {
CodeOwnerResolver codeOwnerResolver = codeOwnerResolverProvider.get().forUser(user);
if (!codeOwnerResolver.isEmailDomainAllowed(codeOwnerReference.email()).get()) {
- return error(
+ return nonResolvableCodeOwner(
+ project,
String.format(
"the domain of the code owner email '%s' in '%s' is not allowed for code owners",
codeOwnerReference.email(), codeOwnerConfigFilePath));
@@ -752,7 +785,8 @@
// CodeOwerResolver for details). We intentionally return the same generic message in all these
// cases so that uploaders cannot probe emails for existence (e.g. they cannot add an email and
// conclude from the error message whether the email exists).
- return error(
+ return nonResolvableCodeOwner(
+ project,
String.format(
"code owner email '%s' in '%s' cannot be resolved for %s",
codeOwnerReference.email(), codeOwnerConfigFilePath, user.getLoggableName()));
@@ -761,6 +795,7 @@
/**
* Validates the imports of the given code owner config.
*
+ * @param project the name of the project
* @param codeOwnerConfigFilePath the path of the code owner config file which contains the code
* owner config
* @param codeOwnerConfig the code owner config for which the imports should be validated
@@ -768,12 +803,13 @@
* if there are no issues
*/
private Stream<CommitValidationMessage> validateImports(
- Path codeOwnerConfigFilePath, CodeOwnerConfig codeOwnerConfig) {
+ Project.NameKey project, Path codeOwnerConfigFilePath, CodeOwnerConfig codeOwnerConfig) {
return Streams.concat(
codeOwnerConfig.imports().stream()
.map(
codeOwnerConfigReference ->
validateCodeOwnerConfigReference(
+ project,
codeOwnerConfigFilePath,
codeOwnerConfig.key(),
codeOwnerConfig.revision(),
@@ -784,6 +820,7 @@
.map(
codeOwnerConfigReference ->
validateCodeOwnerConfigReference(
+ project,
codeOwnerConfigFilePath,
codeOwnerConfig.key(),
codeOwnerConfig.revision(),
@@ -796,6 +833,7 @@
/**
* Validates a code owner config reference.
*
+ * @param project the name of the project
* @param codeOwnerConfigFilePath the path of the code owner config file which contains the code
* owner config reference
* @param keyOfImportingCodeOwnerConfig key of the importing code owner config
@@ -807,6 +845,7 @@
* Optional#empty()} if there is no issue
*/
private Optional<CommitValidationMessage> validateCodeOwnerConfigReference(
+ Project.NameKey project,
Path codeOwnerConfigFilePath,
CodeOwnerConfig.Key keyOfImportingCodeOwnerConfig,
ObjectId codeOwnerConfigRevision,
@@ -821,14 +860,16 @@
// we intentionally use the same error message for non-existing and non-readable projects so
// that uploaders cannot probe for the existence of projects (e.g. deduce from the error
// message whether a project exists)
- return invalidImport(
+ return nonResolvableImport(
+ project,
importType,
codeOwnerConfigFilePath,
String.format("project '%s' not found", keyOfImportedCodeOwnerConfig.project().get()));
}
if (!projectState.get().statePermitsRead()) {
- return invalidImport(
+ return nonResolvableImport(
+ project,
importType,
codeOwnerConfigFilePath,
String.format(
@@ -844,7 +885,8 @@
// we intentionally use the same error message for non-existing and non-readable branches so
// that uploaders cannot probe for the existence of branches (e.g. deduce from the error
// message whether a branch exists)
- return invalidImport(
+ return nonResolvableImport(
+ project,
importType,
codeOwnerConfigFilePath,
String.format(
@@ -857,7 +899,8 @@
codeOwnersPluginConfiguration.getBackend(keyOfImportedCodeOwnerConfig.branchNameKey());
if (!codeOwnerBackend.isCodeOwnerConfigFile(
keyOfImportedCodeOwnerConfig.project(), codeOwnerConfigReference.fileName())) {
- return invalidImport(
+ return nonResolvableImport(
+ project,
importType,
codeOwnerConfigFilePath,
String.format(
@@ -868,7 +911,8 @@
if (!codeOwnerBackend
.getCodeOwnerConfig(keyOfImportedCodeOwnerConfig, revision.get())
.isPresent()) {
- return invalidImport(
+ return nonResolvableImport(
+ project,
importType,
codeOwnerConfigFilePath,
String.format(
@@ -881,7 +925,8 @@
} catch (StorageException storageException) {
if (getInvalidConfigCause(storageException).isPresent()) {
// The imported code owner config is non-parseable.
- return invalidImport(
+ return nonResolvableImport(
+ project,
importType,
codeOwnerConfigFilePath,
String.format(
@@ -944,16 +989,29 @@
}
}
- private Optional<CommitValidationMessage> invalidImport(
- CodeOwnerConfigImportType importType, Path codeOwnerConfigFilePath, String message) {
- return error(
- String.format(
- "invalid %s import in '%s': %s",
- importType.getType(), codeOwnerConfigFilePath, message));
+ private Optional<CommitValidationMessage> nonResolvableImport(
+ Project.NameKey project,
+ CodeOwnerConfigImportType importType,
+ Path codeOwnerConfigFilePath,
+ String message) {
+ return Optional.of(
+ new CommitValidationMessage(
+ String.format(
+ "invalid %s import in '%s': %s",
+ importType.getType(), codeOwnerConfigFilePath, message),
+ codeOwnersPluginConfiguration.rejectNonResolvableImports(project)
+ ? ValidationMessage.Type.ERROR
+ : ValidationMessage.Type.WARNING));
}
- private Optional<CommitValidationMessage> error(String message) {
- return Optional.of(new CommitValidationMessage(message, ValidationMessage.Type.ERROR));
+ private Optional<CommitValidationMessage> nonResolvableCodeOwner(
+ Project.NameKey project, String message) {
+ return Optional.of(
+ new CommitValidationMessage(
+ message,
+ codeOwnersPluginConfiguration.rejectNonResolvableCodeOwners(project)
+ ? ValidationMessage.Type.ERROR
+ : ValidationMessage.Type.WARNING));
}
/** The result of validating code owner config files. */
@@ -963,21 +1021,26 @@
"code owner config files validated, no issues found";
private static final String INVALID_MSG = "invalid code owner config files";
+ abstract String pluginName();
+
abstract String summaryMessage();
abstract ImmutableList<CommitValidationMessage> validationMessages();
static ValidationResult create(
- String summaryMessage, CommitValidationMessage commitValidationMessage) {
+ String pluginName, String summaryMessage, CommitValidationMessage commitValidationMessage) {
return new AutoValue_CodeOwnerConfigValidator_ValidationResult(
- summaryMessage, ImmutableList.of(commitValidationMessage));
+ pluginName, summaryMessage, ImmutableList.of(commitValidationMessage));
}
- static ValidationResult create(Stream<CommitValidationMessage> validationMessagesStream) {
+ static ValidationResult create(
+ String pluginName, Stream<CommitValidationMessage> validationMessagesStream) {
ImmutableList<CommitValidationMessage> validationMessages =
validationMessagesStream.collect(toImmutableList());
return new AutoValue_CodeOwnerConfigValidator_ValidationResult(
- validationMessages.isEmpty() ? NO_ISSUES_MSG : INVALID_MSG, validationMessages);
+ pluginName,
+ validationMessages.isEmpty() ? NO_ISSUES_MSG : INVALID_MSG,
+ validationMessages);
}
/**
@@ -992,7 +1055,8 @@
List<CommitValidationMessage> processForOnCommitReceived(boolean dryRun)
throws CommitValidationException {
if (!dryRun && hasError()) {
- throw new CommitValidationException(summaryMessage(), validationMessages());
+ throw new CommitValidationException(
+ withPluginName(summaryMessage()), withPluginName(validationMessages()));
}
return validationMessagesWithIncludedSummaryMessage();
@@ -1033,8 +1097,8 @@
return ImmutableList.<CommitValidationMessage>builder()
.add(
new CommitValidationMessage(
- summaryMessage(), getValidationMessageTypeForSummaryMessage()))
- .addAll(validationMessages())
+ withPluginName(summaryMessage()), getValidationMessageTypeForSummaryMessage()))
+ .addAll(withPluginName(validationMessages()))
.build();
}
@@ -1082,7 +1146,7 @@
*/
private String getMessage(List<CommitValidationMessage> validationMessages) {
checkState(!validationMessages.isEmpty(), "expected at least 1 validation message");
- StringBuilder msgBuilder = new StringBuilder(summaryMessage()).append(":");
+ StringBuilder msgBuilder = new StringBuilder(withPluginName(summaryMessage())).append(":");
for (CommitValidationMessage msg : validationMessages) {
msgBuilder
.append("\n ")
@@ -1092,5 +1156,16 @@
}
return msgBuilder.toString();
}
+
+ private String withPluginName(String message) {
+ return "[" + pluginName() + "] " + message;
+ }
+
+ private ImmutableList<CommitValidationMessage> withPluginName(
+ ImmutableList<CommitValidationMessage> validationMessages) {
+ return validationMessages.stream()
+ .map(msg -> new CommitValidationMessage(withPluginName(msg.getMessage()), msg.getType()))
+ .collect(toImmutableList());
+ }
}
}
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 55dee9b..758d0ef 100644
--- a/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/CheckCodeOwnerConfigFilesIT.java
+++ b/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/CheckCodeOwnerConfigFilesIT.java
@@ -184,6 +184,18 @@
@Test
public void issuesInCodeOwnerConfigFile() throws Exception {
+ testIssuesInCodeOwnerConfigFile(ConsistencyProblemInfo.Status.ERROR);
+ }
+
+ @Test
+ @GerritConfig(name = "plugin.code-owners.rejectNonResolvableCodeOwners", value = "false")
+ @GerritConfig(name = "plugin.code-owners.rejectNonResolvableImports", value = "false")
+ public void issuesInCodeOwnerConfigFileReportedAsWarnings() throws Exception {
+ testIssuesInCodeOwnerConfigFile(ConsistencyProblemInfo.Status.WARNING);
+ }
+
+ private void testIssuesInCodeOwnerConfigFile(ConsistencyProblemInfo.Status expectedStatus)
+ throws Exception {
// imports are not supported for the proto backend
assume().that(backendConfig.getDefaultBackend()).isNotInstanceOf(ProtoBackend.class);
@@ -228,19 +240,22 @@
ImmutableMap.of(
pathOfInvalidConfig1,
ImmutableList.of(
- error(
+ problem(
+ expectedStatus,
String.format(
"invalid global import in '%s': '/not-a-code-owner-config' is"
+ " not a code owner config file",
pathOfInvalidConfig1))),
pathOfInvalidConfig2,
ImmutableList.of(
- error(
+ problem(
+ expectedStatus,
String.format(
"code owner email 'unknown1@example.com' in '%s' cannot be"
+ " resolved for admin",
pathOfInvalidConfig2)),
- error(
+ problem(
+ expectedStatus,
String.format(
"code owner email 'unknown2@example.com' in '%s' cannot be"
+ " resolved for admin",
@@ -515,27 +530,69 @@
@Test
public void allIssuesAreReturnedIfNoLevelIsSpecified() throws Exception {
- testIssuesAreFilteredByVerbosity(
- /** verbosity */
- null);
+ testIssuesAreFilteredByVerbosity(/* verbosity= */ null, ConsistencyProblemInfo.Status.ERROR);
}
@Test
public void allIssuesAreReturnedIfLevelIsSetToWarning() throws Exception {
- testIssuesAreFilteredByVerbosity(ConsistencyProblemInfo.Status.WARNING);
+ testIssuesAreFilteredByVerbosity(
+ ConsistencyProblemInfo.Status.WARNING, ConsistencyProblemInfo.Status.ERROR);
}
@Test
public void onlyFatalAndErrorIssuesAreReturnedIfLevelIsSetToError() throws Exception {
- testIssuesAreFilteredByVerbosity(ConsistencyProblemInfo.Status.ERROR);
+ testIssuesAreFilteredByVerbosity(
+ ConsistencyProblemInfo.Status.ERROR, ConsistencyProblemInfo.Status.ERROR);
}
@Test
public void onlyFatalIssuesAreReturnedIfLevelIsSetToFatal() throws Exception {
- testIssuesAreFilteredByVerbosity(ConsistencyProblemInfo.Status.FATAL);
+ testIssuesAreFilteredByVerbosity(
+ ConsistencyProblemInfo.Status.FATAL, ConsistencyProblemInfo.Status.ERROR);
}
- private void testIssuesAreFilteredByVerbosity(@Nullable ConsistencyProblemInfo.Status verbosity)
+ @Test
+ @GerritConfig(name = "plugin.code-owners.rejectNonResolvableCodeOwners", value = "false")
+ @GerritConfig(name = "plugin.code-owners.rejectNonResolvableImports", value = "false")
+ public void
+ allIssuesAreReturnedIfNoLevelIsSpecified_issuesInCodeOwnerConfigFileReportedAsWarnings()
+ throws Exception {
+ testIssuesAreFilteredByVerbosity(/* verbosity= */ null, ConsistencyProblemInfo.Status.WARNING);
+ }
+
+ @Test
+ @GerritConfig(name = "plugin.code-owners.rejectNonResolvableCodeOwners", value = "false")
+ @GerritConfig(name = "plugin.code-owners.rejectNonResolvableImports", value = "false")
+ public void
+ allIssuesAreReturnedIfLevelIsSetToWarning_issuesInCodeOwnerConfigFileReportedAsWarnings()
+ throws Exception {
+ testIssuesAreFilteredByVerbosity(
+ ConsistencyProblemInfo.Status.WARNING, ConsistencyProblemInfo.Status.WARNING);
+ }
+
+ @Test
+ @GerritConfig(name = "plugin.code-owners.rejectNonResolvableCodeOwners", value = "false")
+ @GerritConfig(name = "plugin.code-owners.rejectNonResolvableImports", value = "false")
+ public void
+ onlyFatalAndErrorIssuesAreReturnedIfLevelIsSetToError_issuesInCodeOwnerConfigFileReportedAsWarnings()
+ throws Exception {
+ testIssuesAreFilteredByVerbosity(
+ ConsistencyProblemInfo.Status.ERROR, ConsistencyProblemInfo.Status.WARNING);
+ }
+
+ @Test
+ @GerritConfig(name = "plugin.code-owners.rejectNonResolvableCodeOwners", value = "false")
+ @GerritConfig(name = "plugin.code-owners.rejectNonResolvableImports", value = "false")
+ public void
+ onlyFatalIssuesAreReturnedIfLevelIsSetToFatal_issuesInCodeOwnerConfigFileReportedAsWarnings()
+ throws Exception {
+ testIssuesAreFilteredByVerbosity(
+ ConsistencyProblemInfo.Status.FATAL, ConsistencyProblemInfo.Status.WARNING);
+ }
+
+ private void testIssuesAreFilteredByVerbosity(
+ @Nullable ConsistencyProblemInfo.Status verbosity,
+ ConsistencyProblemInfo.Status expectedStatus)
throws Exception {
// create a non-parseable code owner config, that will be reported as fatal
String pathOfNonParseableCodeOwnerConfig = "/" + getCodeOwnerConfigFileName();
@@ -572,12 +629,15 @@
ProtoBackend.class,
"1:8: Expected \"{\"."))))));
if (verbosity == null
- || ConsistencyProblemInfo.Status.ERROR.equals(verbosity)
- || ConsistencyProblemInfo.Status.WARNING.equals(verbosity)) {
+ || (ConsistencyProblemInfo.Status.ERROR.equals(verbosity)
+ && (expectedStatus.equals(ConsistencyProblemInfo.Status.FATAL)
+ || expectedStatus.equals(ConsistencyProblemInfo.Status.ERROR)))
+ || (ConsistencyProblemInfo.Status.WARNING.equals(verbosity))) {
expectedMasterIssues.put(
pathOfInvalidConfig,
ImmutableList.of(
- error(
+ problem(
+ expectedStatus,
String.format(
"code owner email 'unknown@example.com' in '%s' cannot be"
+ " resolved for admin",
@@ -603,6 +663,10 @@
return new ConsistencyProblemInfo(ConsistencyProblemInfo.Status.ERROR, message);
}
+ private ConsistencyProblemInfo problem(ConsistencyProblemInfo.Status status, String message) {
+ return new ConsistencyProblemInfo(status, message);
+ }
+
private Map<String, Map<String, List<ConsistencyProblemInfo>>> checkCodeOwnerConfigFilesIn(
Project.NameKey projectName) throws RestApiException {
return projectCodeOwnersApiFactory.project(projectName).checkCodeOwnerConfigFiles().check();
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 82c6127..e65efbd 100644
--- a/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/CodeOwnerConfigValidatorIT.java
+++ b/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/CodeOwnerConfigValidatorIT.java
@@ -469,7 +469,7 @@
.isEqualTo(
String.format(
"Failed to submit 1 change due to the following problems:\n"
- + "Change %d: invalid code owner config files:\n"
+ + "Change %d: [code-owners] invalid code owner config files:\n"
+ " ERROR: code owner email '%s' in '%s' cannot be resolved for %s",
r.getChange().getId().get(),
unknownEmail,
@@ -807,10 +807,11 @@
String abbreviatedCommit = abbreviateName(r.getCommit());
r.assertErrorStatus(
- String.format("commit %s: %s", abbreviatedCommit, "invalid code owner config files"));
+ String.format(
+ "commit %s: [code-owners] %s", abbreviatedCommit, "invalid code owner config files"));
r.assertMessage(
String.format(
- "error: commit %s: %s",
+ "error: commit %s: [code-owners] %s",
abbreviatedCommit,
String.format(
"code owner email '%s' in '%s' cannot be resolved for %s",
@@ -821,7 +822,7 @@
// the pre-existing issue is returned as warning
r.assertMessage(
String.format(
- "warning: commit %s: code owner email '%s' in '%s' cannot be resolved for %s",
+ "warning: commit %s: [code-owners] code owner email '%s' in '%s' cannot be resolved for %s",
abbreviatedCommit,
unknownEmail1,
codeOwnerConfigOperations.codeOwnerConfig(codeOwnerConfigKey).getFilePath(),
@@ -866,7 +867,7 @@
.isEqualTo(
String.format(
"Failed to submit 1 change due to the following problems:\n"
- + "Change %d: invalid code owner config files:\n"
+ + "Change %d: [code-owners] invalid code owner config files:\n"
+ " ERROR: code owner email '%s' in '%s' cannot be resolved for %s",
r.getChange().getId().get(),
unknownEmail,
@@ -915,7 +916,7 @@
r.assertOkStatus();
r.assertMessage(
String.format(
- "error: commit %s: %s",
+ "error: commit %s: [code-owners] %s",
abbreviatedCommit,
String.format(
"code owner email '%s' in '%s' cannot be resolved for %s",
@@ -926,7 +927,8 @@
// the pre-existing issue is returned as warning
r.assertMessage(
String.format(
- "warning: commit %s: code owner email '%s' in '%s' cannot be resolved for %s",
+ "warning: commit %s: [code-owners] code owner email '%s' in '%s' cannot be resolved"
+ + " for %s",
abbreviatedCommit,
unknownEmail1,
codeOwnerConfigOperations.codeOwnerConfig(codeOwnerConfigKey).getFilePath(),
@@ -979,7 +981,7 @@
.isEqualTo(
String.format(
"Failed to submit 1 change due to the following problems:\n"
- + "Change %d: invalid code owner config files:\n"
+ + "Change %d: [code-owners] invalid code owner config files:\n"
+ " ERROR: code owner email '%s' in '%s' cannot be resolved for %s",
r.getChange().getId().get(),
admin.email(),
@@ -1594,6 +1596,139 @@
assertThat(gApi.changes().id(r.getChangeId()).get().status).isEqualTo(ChangeStatus.MERGED);
}
+ @Test
+ @GerritConfig(name = "plugin.code-owners.rejectNonResolvableCodeOwners", value = "false")
+ public void canUploadAndSubmitConfigWithUnresolvableCodeOwners() throws Exception {
+ CodeOwnerConfig.Key codeOwnerConfigKey = createCodeOwnerConfigKey("/");
+
+ // upload a code owner config that has issues (non-resolvable code owners)
+ String unknownEmail = "non-existing-email@example.com";
+ PushOneCommit.Result r =
+ createChange(
+ "Add code owners",
+ codeOwnerConfigOperations.codeOwnerConfig(codeOwnerConfigKey).getJGitFilePath(),
+ format(
+ CodeOwnerConfig.builder(codeOwnerConfigKey, TEST_REVISION)
+ .addCodeOwnerSet(CodeOwnerSet.createWithoutPathExpressions(unknownEmail))
+ .build()));
+ assertOkWithWarnings(
+ r,
+ "invalid code owner config files",
+ String.format(
+ "code owner email '%s' in '%s' cannot be resolved for %s",
+ unknownEmail,
+ codeOwnerConfigOperations.codeOwnerConfig(codeOwnerConfigKey).getFilePath(),
+ identifiedUserFactory.create(admin.id()).getLoggableName()));
+
+ // submit the change
+ approve(r.getChangeId());
+ gApi.changes().id(r.getChangeId()).current().submit();
+ assertThat(gApi.changes().id(r.getChangeId()).get().status).isEqualTo(ChangeStatus.MERGED);
+ }
+
+ @Test
+ @GerritConfig(name = "plugin.code-owners.rejectNonResolvableImports", value = "false")
+ public void canUploadAndSubmitConfigWithUnresolvableImports() throws Exception {
+ // imports are not supported for the proto backend
+ assume().that(backendConfig.getDefaultBackend()).isNotInstanceOf(ProtoBackend.class);
+
+ CodeOwnerConfig.Key keyOfImportingCodeOwnerConfig = createCodeOwnerConfigKey("/");
+
+ // upload a code owner config that has issues (non-resolvable imports)
+ Project.NameKey nonExistingProject = Project.nameKey("non-existing");
+ CodeOwnerConfigReference codeOwnerConfigReference =
+ CodeOwnerConfigReference.builder(
+ CodeOwnerConfigImportMode.GLOBAL_CODE_OWNER_SETS_ONLY,
+ codeOwnerConfigOperations
+ .codeOwnerConfig(CodeOwnerConfig.Key.create(nonExistingProject, "master", "/"))
+ .getFilePath())
+ .setProject(nonExistingProject)
+ .build();
+ PushOneCommit.Result r =
+ createChange(
+ "Add code owners",
+ codeOwnerConfigOperations
+ .codeOwnerConfig(keyOfImportingCodeOwnerConfig)
+ .getJGitFilePath(),
+ format(
+ CodeOwnerConfig.builder(keyOfImportingCodeOwnerConfig, TEST_REVISION)
+ .addImport(codeOwnerConfigReference)
+ .addCodeOwnerSet(
+ CodeOwnerSet.builder()
+ .addPathExpression("foo")
+ .addImport(codeOwnerConfigReference)
+ .build())
+ .build()));
+ assertOkWithWarnings(
+ r,
+ "invalid code owner config files",
+ String.format(
+ "invalid %s import in '%s': project '%s' not found",
+ CodeOwnerConfigImportType.GLOBAL.getType(),
+ codeOwnerConfigOperations.codeOwnerConfig(keyOfImportingCodeOwnerConfig).getFilePath(),
+ nonExistingProject.get()),
+ String.format(
+ "invalid %s import in '%s': project '%s' not found",
+ CodeOwnerConfigImportType.PER_FILE.getType(),
+ codeOwnerConfigOperations.codeOwnerConfig(keyOfImportingCodeOwnerConfig).getFilePath(),
+ nonExistingProject.get()));
+
+ // submit the change
+ approve(r.getChangeId());
+ gApi.changes().id(r.getChangeId()).current().submit();
+ assertThat(gApi.changes().id(r.getChangeId()).get().status).isEqualTo(ChangeStatus.MERGED);
+ }
+
+ @Test
+ @GerritConfig(name = "plugin.code-owners.rejectNonResolvableCodeOwners", value = "true")
+ @GerritConfig(name = "plugin.code-owners.rejectNonResolvableImports", value = "true")
+ @GerritConfig(name = "plugin.code-owners.enableValidationOnCommitReceived", value = "false")
+ @GerritConfig(name = "plugin.code-owners.enableValidationOnSubmit", value = "false")
+ public void rejectConfigOptionsAreIgnoredIfValidationIsDisabled() throws Exception {
+ // imports are not supported for the proto backend
+ assume().that(backendConfig.getDefaultBackend()).isNotInstanceOf(ProtoBackend.class);
+
+ CodeOwnerConfig.Key keyOfImportingCodeOwnerConfig = createCodeOwnerConfigKey("/");
+
+ // upload a code owner config that has issues (non-resolvable code owners and non-resolvable
+ // imports)
+ String unknownEmail = "non-existing-email@example.com";
+ Project.NameKey nonExistingProject = Project.nameKey("non-existing");
+ CodeOwnerConfigReference codeOwnerConfigReference =
+ CodeOwnerConfigReference.builder(
+ CodeOwnerConfigImportMode.GLOBAL_CODE_OWNER_SETS_ONLY,
+ codeOwnerConfigOperations
+ .codeOwnerConfig(CodeOwnerConfig.Key.create(nonExistingProject, "master", "/"))
+ .getFilePath())
+ .setProject(nonExistingProject)
+ .build();
+ PushOneCommit.Result r =
+ createChange(
+ "Add code owners",
+ codeOwnerConfigOperations
+ .codeOwnerConfig(keyOfImportingCodeOwnerConfig)
+ .getJGitFilePath(),
+ format(
+ CodeOwnerConfig.builder(keyOfImportingCodeOwnerConfig, TEST_REVISION)
+ .addImport(codeOwnerConfigReference)
+ .addCodeOwnerSet(CodeOwnerSet.createWithoutPathExpressions(unknownEmail))
+ .addCodeOwnerSet(
+ CodeOwnerSet.builder()
+ .addPathExpression("foo")
+ .addImport(codeOwnerConfigReference)
+ .build())
+ .build()));
+ assertOkWithHints(
+ r,
+ "skipping validation of code owner config files",
+ "code owners config validation is disabled");
+
+ // submit the change
+ approve(r.getChangeId());
+ gApi.changes().id(r.getChangeId()).current().submit();
+ assertThat(gApi.changes().id(r.getChangeId()).get().status).isEqualTo(ChangeStatus.MERGED);
+ }
+
private CodeOwnerConfig createCodeOwnerConfigWithImport(
CodeOwnerConfig.Key keyOfImportingCodeOwnerConfig,
CodeOwnerConfigImportType importType,
@@ -1658,7 +1793,8 @@
pushResult.assertOkStatus();
for (String hint : hints) {
pushResult.assertMessage(
- String.format("hint: commit %s: %s", abbreviateName(pushResult.getCommit()), hint));
+ String.format(
+ "hint: commit %s: [code-owners] %s", abbreviateName(pushResult.getCommit()), hint));
}
pushResult.assertNotMessage("fatal");
pushResult.assertNotMessage("error");
@@ -1670,7 +1806,8 @@
pushResult.assertOkStatus();
for (String error : errors) {
pushResult.assertMessage(
- String.format("fatal: commit %s: %s", abbreviateName(pushResult.getCommit()), error));
+ String.format(
+ "fatal: commit %s: [code-owners] %s", abbreviateName(pushResult.getCommit()), error));
}
pushResult.assertNotMessage("error");
pushResult.assertNotMessage("warning");
@@ -1682,7 +1819,8 @@
pushResult.assertOkStatus();
for (String error : errors) {
pushResult.assertMessage(
- String.format("error: commit %s: %s", abbreviateName(pushResult.getCommit()), error));
+ String.format(
+ "error: commit %s: [code-owners] %s", abbreviateName(pushResult.getCommit()), error));
}
pushResult.assertNotMessage("fatal");
pushResult.assertNotMessage("warning");
@@ -1694,7 +1832,9 @@
pushResult.assertOkStatus();
for (String warning : warnings) {
pushResult.assertMessage(
- String.format("warning: commit %s: %s", abbreviateName(pushResult.getCommit()), warning));
+ String.format(
+ "warning: commit %s: [code-owners] %s",
+ abbreviateName(pushResult.getCommit()), warning));
}
pushResult.assertNotMessage("fatal");
pushResult.assertNotMessage("error");
@@ -1704,9 +1844,11 @@
private void assertErrorWithMessages(
PushOneCommit.Result pushResult, String summaryMessage, String... errors) throws Exception {
String abbreviatedCommit = abbreviateName(pushResult.getCommit());
- pushResult.assertErrorStatus(String.format("commit %s: %s", abbreviatedCommit, summaryMessage));
+ pushResult.assertErrorStatus(
+ String.format("commit %s: [code-owners] %s", abbreviatedCommit, summaryMessage));
for (String error : errors) {
- pushResult.assertMessage(String.format("error: commit %s: %s", abbreviatedCommit, error));
+ pushResult.assertMessage(
+ String.format("error: commit %s: [code-owners] %s", abbreviatedCommit, error));
}
pushResult.assertNotMessage("fatal");
pushResult.assertNotMessage("warning");
@@ -1716,9 +1858,11 @@
private void assertFatalWithMessages(
PushOneCommit.Result pushResult, String summaryMessage, String... errors) throws Exception {
String abbreviatedCommit = abbreviateName(pushResult.getCommit());
- pushResult.assertErrorStatus(String.format("commit %s: %s", abbreviatedCommit, summaryMessage));
+ pushResult.assertErrorStatus(
+ String.format("commit %s: [code-owners] %s", abbreviatedCommit, summaryMessage));
for (String error : errors) {
- pushResult.assertMessage(String.format("fatal: commit %s: %s", abbreviatedCommit, error));
+ pushResult.assertMessage(
+ String.format("fatal: commit %s: [code-owners] %s", abbreviatedCommit, error));
}
pushResult.assertNotMessage("error");
pushResult.assertNotMessage("warning");
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 43771de..ff2785c 100644
--- a/javatests/com/google/gerrit/plugins/codeowners/backend/config/GeneralConfigTest.java
+++ b/javatests/com/google/gerrit/plugins/codeowners/backend/config/GeneralConfigTest.java
@@ -27,6 +27,8 @@
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;
import static com.google.gerrit.plugins.codeowners.backend.config.GeneralConfig.KEY_READ_ONLY;
+import static com.google.gerrit.plugins.codeowners.backend.config.GeneralConfig.KEY_REJECT_NON_RESOLVABLE_CODE_OWNERS;
+import static com.google.gerrit.plugins.codeowners.backend.config.GeneralConfig.KEY_REJECT_NON_RESOLVABLE_IMPORTS;
import static com.google.gerrit.testing.GerritJUnit.assertThrows;
import static com.google.gerrit.truth.OptionalSubject.assertThat;
@@ -104,7 +106,8 @@
@Test
public void cannotGetReadOnlyForNullPluginConfig() throws Exception {
NullPointerException npe =
- assertThrows(NullPointerException.class, () -> generalConfig.getReadOnly(null));
+ assertThrows(
+ NullPointerException.class, () -> generalConfig.getReadOnly(/* pluginConfig= */ null));
assertThat(npe).hasMessageThat().isEqualTo("pluginConfig");
}
@@ -130,6 +133,70 @@
}
@Test
+ public void cannotGetRejectNonResolvableCodeOwnersForNullPluginConfig() throws Exception {
+ NullPointerException npe =
+ assertThrows(
+ NullPointerException.class,
+ () -> generalConfig.getRejectNonResolvableCodeOwners(/* pluginConfig= */ null));
+ assertThat(npe).hasMessageThat().isEqualTo("pluginConfig");
+ }
+
+ @Test
+ public void noRejectNonResolvableCodeOwnersConfiguration() throws Exception {
+ assertThat(generalConfig.getRejectNonResolvableCodeOwners(new Config())).isTrue();
+ }
+
+ @Test
+ @GerritConfig(name = "plugin.code-owners.rejectNonResolvableCodeOwners", value = "false")
+ public void
+ rejectNonResolvableCodeOwnersConfigurationIsRetrievedFromGerritConfigIfNotSpecifiedOnProjectLevel()
+ throws Exception {
+ assertThat(generalConfig.getRejectNonResolvableCodeOwners(new Config())).isFalse();
+ }
+
+ @Test
+ @GerritConfig(name = "plugin.code-owners.rejectNonResolvableCodeOwners", value = "false")
+ public void
+ rejectNonResolvableCodeOwnersConfigurationInPluginConfigOverridesRejectNonResolvableCodeOwnersConfigurationInGerritConfig()
+ throws Exception {
+ Config cfg = new Config();
+ cfg.setString(SECTION_CODE_OWNERS, null, KEY_REJECT_NON_RESOLVABLE_CODE_OWNERS, "true");
+ assertThat(generalConfig.getRejectNonResolvableCodeOwners(cfg)).isTrue();
+ }
+
+ @Test
+ public void cannotGetRejectNonResolvableImportsForNullPluginConfig() throws Exception {
+ NullPointerException npe =
+ assertThrows(
+ NullPointerException.class,
+ () -> generalConfig.getRejectNonResolvableImports(/* pluginConfig= */ null));
+ assertThat(npe).hasMessageThat().isEqualTo("pluginConfig");
+ }
+
+ @Test
+ public void noRejectNonResolvableImportsConfiguration() throws Exception {
+ assertThat(generalConfig.getRejectNonResolvableImports(new Config())).isTrue();
+ }
+
+ @Test
+ @GerritConfig(name = "plugin.code-owners.rejectNonResolvableImports", value = "false")
+ public void
+ rejectNonResolvableImportsConfigurationIsRetrievedFromGerritConfigIfNotSpecifiedOnProjectLevel()
+ throws Exception {
+ assertThat(generalConfig.getRejectNonResolvableImports(new Config())).isFalse();
+ }
+
+ @Test
+ @GerritConfig(name = "plugin.code-owners.rejectNonResolvableImports", value = "false")
+ public void
+ rejectNonResolvableImportsConfigurationInPluginConfigOverridesRejectNonResolvableImportsConfigurationInGerritConfig()
+ throws Exception {
+ Config cfg = new Config();
+ cfg.setString(SECTION_CODE_OWNERS, null, KEY_REJECT_NON_RESOLVABLE_IMPORTS, "true");
+ assertThat(generalConfig.getRejectNonResolvableImports(cfg)).isTrue();
+ }
+
+ @Test
public void cannotGetEnableValidationOnCommitReceivedForNullProject() throws Exception {
NullPointerException npe =
assertThrows(
diff --git a/resources/Documentation/config.md b/resources/Documentation/config.md
index d546374..90c7b3c 100644
--- a/resources/Documentation/config.md
+++ b/resources/Documentation/config.md
@@ -147,6 +147,37 @@
in `@PLUGIN@.config`.\
By default `true`.
+<a id="pluginCodeOwnersRejectNonResolvableCodeOwners">plugin.@PLUGIN@.rejectNonResolvableCodeOwners</a>
+: Whether modifications of code owner config files that newly add
+ non-resolvable code owners should be rejected on commit received and
+ submit.\
+ if `true` newly added non-resolveable code owners are reported as errors
+ and the commit is rejected.\
+ If `false` newly added non-resolvable code owners are only reported as
+ warnings and the commit is not rejected.\
+ This setting has no effect if the validation is disabled via
+ [enableValidationOnCommitReceived](#pluginCodeOwnersEnableValidationOnCommitReceived)
+ or [enableValidationOnSubmit](#pluginCodeOwnersEnableValidationOnSubmit).
+ Can be overridden per project by setting
+ [codeOwners.rejectNonResolvableCodeOwners](#codeOwnersRejectNonResolvableCodeOwners)
+ in `@PLUGIN@.config`.\
+ By default `true`.
+
+<a id="pluginCodeOwnersRejectNonResolvableImports">plugin.@PLUGIN@.rejectNonResolvableImports</a>
+: Whether modifications of code owner config files that newly add
+ non-resolvable imports should be rejected on commit received an submit.\
+ if `true` newly added non-resolveable imports are reported as errors and
+ the commit is rejected.\
+ If `false` newly added non-resolvable imports are only reported as
+ warnings and the commit is not rejected.\
+ This setting has no effect if the validation is disabled via
+ [enableValidationOnCommitReceived](#pluginCodeOwnersEnableValidationOnCommitReceived)
+ or [enableValidationOnSubmit](#pluginCodeOwnersEnableValidationOnSubmit).
+ Can be overridden per project by setting
+ [codeOwners.rejectNonResolvableImports](#codeOwnersRejectNonResolvableImports)
+ in `@PLUGIN@.config`.\
+ By default `true`.
+
<a id="pluginCodeOwnersAllowedEmailDomain">plugin.@PLUGIN@.allowedEmailDomain</a>
: Email domain that allows to assign code ownerships to emails with this
domain.\
@@ -465,6 +496,41 @@
[plugin.@PLUGIN@.enableValidationOnSubmit](#pluginCodeOwnersEnableValidationOnSubmit)
in `gerrit.config` is used.
+<a id="codeOwnersRejectNonResolvableCodeOwners">codeOwners.rejectNonResolvableCodeOwners</a>
+: Whether modifications of code owner config files that newly add
+ non-resolvable code owners should be rejected on commit received and
+ submit.\
+ if `true` newly added non-resolveable code owners are reported as errors
+ and the commit is rejected.\
+ If `false` newly added non-resolvable code owners are only reported as
+ warnings and the commit is not rejected.\
+ This setting has no effect if the validation is disabled via
+ [enableValidationOnCommitReceived](#codeOwnersEnableValidationOnCommitReceived)
+ or [enableValidationOnSubmit](#codeOwnersEnableValidationOnSubmit).
+ Overrides the global setting
+ [plugin.@PLUGIN@.rejectNonResolvableCodeOwners](#pluginCodeOwnersRejectNonResolvableCodeOwners)
+ in `gerrit.config`.\
+ If not set, the global setting
+ [plugin.@PLUGIN@.rejectNonResolvableCodeOwners](#pluginCodeOwnersRejectNonResolvableCodeOwners)
+ in `gerrit.config` is used.
+
+<a id="codeOwnersRejectNonResolvableImports">codeOwners.rejectNonResolvableImports</a>
+: Whether modifications of code owner config files that newly add
+ non-resolvable imports should be rejected on commit received an submit.\
+ if `true` newly added non-resolveable imports are reported as errors and
+ the commit is rejected.\
+ If `false` newly added non-resolvable imports are only reported as
+ warnings and the commit is not rejected.\
+ This setting has no effect if the validation is disabled via
+ [enableValidationOnCommitReceived](#codeOwnersEnableValidationOnCommitReceived)
+ or [enableValidationOnSubmit](#codeOwnersEnableValidationOnSubmit).
+ Overrides the global setting
+ [plugin.@PLUGIN@.rejectNonResolvableImports](#pluginCodeOwnersRejectNonResolvableImports)
+ in `gerrit.config`.\
+ If not set, the global setting
+ [plugin.@PLUGIN@.rejectNonResolvableImports](#pluginCodeOwnersRejectNonResolvableImports)
+ in `gerrit.config` is used.
+
<a id="codeOwnersRequiredApproval">codeOwners.requiredApproval</a>
: Approval that is required from code owners to approve the files in a
change.\
diff --git a/resources/Documentation/validation.md b/resources/Documentation/validation.md
index 746221e..47c3ef5 100644
--- a/resources/Documentation/validation.md
+++ b/resources/Documentation/validation.md
@@ -116,6 +116,13 @@
* the project from which the file should be imported doesn't permit reads
(e.g. has the state `HIDDEN`)
+**NOTE:** Whether commits that newly add non-resolvable code owners and
+non-resolvable imports are rejected on commit received and on submit is
+controlled by the
+[rejectNonResolvableCodeOwners](config.html#pluginCodeOwnersRejectNonResolvableCodeOwners)
+and [rejectNonResolvableImports](config.html#pluginCodeOwnersRejectNonResolvableImports)
+config settings.
+
The following things are **not** checked (not an exhaustive list):
* Cycles in imports of owner config files:\
