Merge "Add class that updates all code owner configs in a branch atomically"
diff --git a/java/com/google/gerrit/plugins/codeowners/restapi/CheckCodeOwnerConfigFiles.java b/java/com/google/gerrit/plugins/codeowners/restapi/CheckCodeOwnerConfigFiles.java
index e18ddb3..07788ca 100644
--- a/java/com/google/gerrit/plugins/codeowners/restapi/CheckCodeOwnerConfigFiles.java
+++ b/java/com/google/gerrit/plugins/codeowners/restapi/CheckCodeOwnerConfigFiles.java
@@ -154,7 +154,7 @@
problemsByPath.put(
codeOwnerConfigFilePath.toString(),
new ConsistencyProblemInfo(
- ConsistencyProblemInfo.Status.ERROR, configInvalidException.getMessage()));
+ ConsistencyProblemInfo.Status.FATAL, configInvalidException.getMessage()));
},
pathGlob);
diff --git a/java/com/google/gerrit/plugins/codeowners/validation/CodeOwnerConfigValidator.java b/java/com/google/gerrit/plugins/codeowners/validation/CodeOwnerConfigValidator.java
index 7fbda44..85cfca4 100644
--- a/java/com/google/gerrit/plugins/codeowners/validation/CodeOwnerConfigValidator.java
+++ b/java/com/google/gerrit/plugins/codeowners/validation/CodeOwnerConfigValidator.java
@@ -517,11 +517,11 @@
}
/**
- * Returns the {@link com.google.gerrit.server.git.validators.ValidationMessage.Type} (ERROR or
+ * Returns the {@link com.google.gerrit.server.git.validators.ValidationMessage.Type} (FATAL or
* WARNING) that should be used for a parsing error of the code owner config file (specified as
* {@code changedFile}).
*
- * <p>If {@link com.google.gerrit.server.git.validators.ValidationMessage.Type#ERROR} is returned
+ * <p>If {@link com.google.gerrit.server.git.validators.ValidationMessage.Type#FATAL} is returned
* the upload will be blocked, if {@link
* com.google.gerrit.server.git.validators.ValidationMessage.Type#WARNING} is returned the upload
* can succeed and the parsing error will only be shown as warning.
@@ -531,7 +531,7 @@
* it is not making anything worse. Hence in this case the parsing error should be returned as
* {@link com.google.gerrit.server.git.validators.ValidationMessage.Type#WARNING}, whereas a new
* parsing error should be returned as {@link
- * com.google.gerrit.server.git.validators.ValidationMessage.Type#ERROR}.
+ * com.google.gerrit.server.git.validators.ValidationMessage.Type#FATAL}.
*
* @param codeOwnerBackend the code owner backend from which the code owner config can be loaded
* @param branchNameKey the project and branch of the code owner config
@@ -567,8 +567,8 @@
codeOwnerBackend.getCodeOwnerConfig(baseCodeOwnerConfigKey, revWalk, parentRevision);
// The code owner config at the parent revision is parseable. This means the parsing error
// is introduced by the new commit and we should block uploading it, which we achieve by
- // setting the validation message type to error.
- return ValidationMessage.Type.ERROR;
+ // setting the validation message type to fatal.
+ return ValidationMessage.Type.FATAL;
} catch (StorageException storageException) {
// Loading the base code owner config has failed.
if (getInvalidConfigCause(storageException).isPresent()) {
@@ -584,8 +584,8 @@
// The code owner config is newly created. Hence the parsing error comes from the commit
// that is being pushed and we want to block it from uploading. To do this we set the
- // validation message type to error.
- return ValidationMessage.Type.ERROR;
+ // validation message type to fatal.
+ return ValidationMessage.Type.FATAL;
}
/**
@@ -608,12 +608,13 @@
/**
* Returns a copy of the given commit validation message with type warning if the type the given
- * commit validation message is error. Otherwise it returns the given commit validation message
- * unchanged.
+ * commit validation message is fatal or error. Otherwise it returns the given commit validation
+ * message unchanged.
*/
private static CommitValidationMessage downgradeErrorToWarning(
CommitValidationMessage commitValidationMessage) {
- if (CommitValidationMessage.Type.ERROR.equals(commitValidationMessage.getType())) {
+ if (CommitValidationMessage.Type.FATAL.equals(commitValidationMessage.getType())
+ || CommitValidationMessage.Type.ERROR.equals(commitValidationMessage.getType())) {
return new CommitValidationMessage(
commitValidationMessage.getMessage(), ValidationMessage.Type.WARNING);
}
@@ -977,7 +978,8 @@
return validationMessages().stream()
.anyMatch(
validationMessage ->
- ValidationMessage.Type.ERROR.equals(validationMessage.getType()));
+ ValidationMessage.Type.FATAL.equals(validationMessage.getType())
+ || ValidationMessage.Type.ERROR.equals(validationMessage.getType()));
}
private ImmutableList<CommitValidationMessage> validationMessagesWithIncludedSummaryMessage() {
@@ -995,6 +997,7 @@
* <p>The following validation message type will be returned:
*
* <ul>
+ * <li>FATAL: if any of the validation message has type fatal
* <li>ERROR: if any of the validation message has type error
* <li>WARNING: if any of the validation message has type warning and none has type error
* <li>HINT: otherwise
@@ -1005,6 +1008,10 @@
if (!validationMessages().isEmpty()) {
for (CommitValidationMessage validationMessage : validationMessages()) {
+ if (ValidationMessage.Type.FATAL.equals(validationMessage.getType())) {
+ validationMessageType = ValidationMessage.Type.FATAL;
+ break;
+ }
if (ValidationMessage.Type.ERROR.equals(validationMessage.getType())) {
validationMessageType = ValidationMessage.Type.ERROR;
break;
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 c1d7f0b..2c7b15d 100644
--- a/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/CheckCodeOwnerConfigFilesIT.java
+++ b/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/CheckCodeOwnerConfigFilesIT.java
@@ -159,7 +159,7 @@
ImmutableMap.of(
codeOwnerConfigPath,
ImmutableList.of(
- error(
+ fatal(
String.format(
"invalid code owner config file '%s':\n %s",
codeOwnerConfigPath,
@@ -475,6 +475,10 @@
pathOfInvalidConfig3)))));
}
+ private ConsistencyProblemInfo fatal(String message) {
+ return new ConsistencyProblemInfo(ConsistencyProblemInfo.Status.FATAL, message);
+ }
+
private ConsistencyProblemInfo error(String message) {
return new ConsistencyProblemInfo(ConsistencyProblemInfo.Status.ERROR, message);
}
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 9f4d64d..57b9506 100644
--- a/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/CheckCodeOwnerConfigFilesInRevisionIT.java
+++ b/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/CheckCodeOwnerConfigFilesInRevisionIT.java
@@ -126,7 +126,7 @@
.containsExactly(
codeOwnerConfigPath,
ImmutableList.of(
- error(
+ fatal(
String.format(
"invalid code owner config file '%s':\n %s",
codeOwnerConfigPath,
@@ -398,6 +398,10 @@
unknownEmail3, codeOwnerConfigPath3))));
}
+ private ConsistencyProblemInfo fatal(String message) {
+ return new ConsistencyProblemInfo(ConsistencyProblemInfo.Status.FATAL, message);
+ }
+
private ConsistencyProblemInfo error(String message) {
return new ConsistencyProblemInfo(ConsistencyProblemInfo.Status.ERROR, message);
}
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 357c5d8..bc42987 100644
--- a/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/CodeOwnerConfigValidatorIT.java
+++ b/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/CodeOwnerConfigValidatorIT.java
@@ -319,7 +319,7 @@
"Add code owners",
codeOwnerConfigOperations.codeOwnerConfig(codeOwnerConfigKey).getJGitFilePath(),
"INVALID");
- assertOkWithErrors(
+ assertOkWithFatals(
r,
"invalid code owner config files",
String.format(
@@ -552,7 +552,7 @@
"Add code owners",
codeOwnerConfigOperations.codeOwnerConfig(codeOwnerConfigKey).getJGitFilePath(),
"INVALID");
- assertErrorWithMessages(
+ assertFatalWithMessages(
r,
"invalid code owner config files",
String.format(
@@ -582,7 +582,7 @@
codeOwnerConfigOperations.codeOwnerConfig(codeOwnerConfigKey2).getJGitFilePath(),
"ALSO-INVALID"));
PushOneCommit.Result r = push.to("refs/for/master");
- assertErrorWithMessages(
+ assertFatalWithMessages(
r,
"invalid code owner config files",
String.format(
@@ -1481,6 +1481,7 @@
private static void assertOkWithoutMessages(PushOneCommit.Result pushResult) {
pushResult.assertOkStatus();
+ pushResult.assertNotMessage("fatal");
pushResult.assertNotMessage("error");
pushResult.assertNotMessage("warning");
pushResult.assertNotMessage("hint");
@@ -1493,10 +1494,23 @@
pushResult.assertMessage(
String.format("hint: commit %s: %s", abbreviateName(pushResult.getCommit()), hint));
}
+ pushResult.assertNotMessage("fatal");
pushResult.assertNotMessage("error");
pushResult.assertNotMessage("warning");
}
+ private void assertOkWithFatals(PushOneCommit.Result pushResult, String... errors)
+ throws Exception {
+ pushResult.assertOkStatus();
+ for (String error : errors) {
+ pushResult.assertMessage(
+ String.format("fatal: commit %s: %s", abbreviateName(pushResult.getCommit()), error));
+ }
+ pushResult.assertNotMessage("error");
+ pushResult.assertNotMessage("warning");
+ pushResult.assertNotMessage("hint");
+ }
+
private void assertOkWithErrors(PushOneCommit.Result pushResult, String... errors)
throws Exception {
pushResult.assertOkStatus();
@@ -1504,6 +1518,7 @@
pushResult.assertMessage(
String.format("error: commit %s: %s", abbreviateName(pushResult.getCommit()), error));
}
+ pushResult.assertNotMessage("fatal");
pushResult.assertNotMessage("warning");
pushResult.assertNotMessage("hint");
}
@@ -1515,6 +1530,7 @@
pushResult.assertMessage(
String.format("warning: commit %s: %s", abbreviateName(pushResult.getCommit()), warning));
}
+ pushResult.assertNotMessage("fatal");
pushResult.assertNotMessage("error");
pushResult.assertNotMessage("hint");
}
@@ -1526,6 +1542,19 @@
for (String error : errors) {
pushResult.assertMessage(String.format("error: commit %s: %s", abbreviatedCommit, error));
}
+ pushResult.assertNotMessage("fatal");
+ pushResult.assertNotMessage("warning");
+ pushResult.assertNotMessage("hint");
+ }
+
+ 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));
+ for (String error : errors) {
+ pushResult.assertMessage(String.format("fatal: commit %s: %s", abbreviatedCommit, error));
+ }
+ pushResult.assertNotMessage("error");
pushResult.assertNotMessage("warning");
pushResult.assertNotMessage("hint");
}
