Make log less verbose if change message extention fails due to config error
CodeOwnersOnAddReviewer extends the change message when a code owner is
added as a reviewer. It lists the owned paths in the message. If any
relevant OWNERS file is invalid and cannot be parsed, we cannot compute
the owned files, and hence the message cannot be extended in this case.
Adding the reviewer succeeds but only the change message that is posted
for it doesn't contain any owned files. This works as intended, but we
log a verbose error (with stacktrace) in this case that is not needed.
Fix the exeption handling so that we log a warning only if the exception
is caused by a configuration error. Otherwise still log the verbose
error so that it can be investigated when it happens.
OnCodeOwnerApproval extends the change message when a code owner
approval is applied. Similarly to CodeOwnersOnAddReviewer we cannot
extend the message if relevant OWNERS files are invalid. Here too the
exception was ignored and logged with stacktrace. Catching and logging
this exception was done generically by PluginContext. Now we are
handling the exception on our own, so that also here we can log a
warning only when the exception is caused by a configuration error.
Signed-off-by: Edwin Kempin <ekempin@google.com>
Change-Id: I0d1b3cf747643fd7bef512e6dce0fbd8bb940da9
diff --git a/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnersExceptionHook.java b/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnersExceptionHook.java
index c7da676..5eb316c 100644
--- a/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnersExceptionHook.java
+++ b/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnersExceptionHook.java
@@ -52,9 +52,7 @@
@Override
public boolean skipRetryWithTrace(String actionType, String actionName, Throwable throwable) {
- return isInvalidPluginConfigurationException(throwable)
- || isInvalidCodeOwnerConfigException(throwable)
- || isInvalidPathException(throwable);
+ return isCausedByConfigurationError(throwable);
}
@Override
@@ -102,9 +100,7 @@
@Override
public Optional<Status> getStatus(Throwable throwable) {
- if (isInvalidPluginConfigurationException(throwable)
- || isInvalidCodeOwnerConfigException(throwable)
- || isInvalidPathException(throwable)) {
+ if (isCausedByConfigurationError(throwable)) {
return Optional.of(Status.create(409, "Conflict"));
}
return Optional.empty();
@@ -115,6 +111,28 @@
return getCause(CodeOwnersInternalServerErrorException.class, throwable);
}
+ public static boolean isCausedByConfigurationError(Throwable throwable) {
+ return isInvalidPluginConfigurationException(throwable)
+ || isInvalidCodeOwnerConfigException(throwable)
+ || isInvalidPathException(throwable);
+ }
+
+ public static Optional<? extends Exception> getCauseOfConfigurationError(Throwable throwable) {
+ Optional<InvalidPathException> invalidPathException =
+ CodeOwnersExceptionHook.getInvalidPathException(throwable);
+ if (invalidPathException.isPresent()) {
+ return invalidPathException;
+ }
+
+ Optional<InvalidPluginConfigurationException> invalidPluginConfigurationException =
+ CodeOwnersExceptionHook.getInvalidPluginConfigurationCause(throwable);
+ if (invalidPluginConfigurationException.isPresent()) {
+ return invalidPluginConfigurationException;
+ }
+
+ return CodeOwners.getInvalidCodeOwnerConfigCause(throwable);
+ }
+
private static boolean isInvalidPluginConfigurationException(Throwable throwable) {
return getInvalidPluginConfigurationCause(throwable).isPresent();
}
diff --git a/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnersOnAddReviewer.java b/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnersOnAddReviewer.java
index 4e81f46..ae1f477 100644
--- a/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnersOnAddReviewer.java
+++ b/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnersOnAddReviewer.java
@@ -165,9 +165,17 @@
})
.call();
} catch (Exception e) {
- logger.atSevere().withCause(e).log(
- "Failed to post code-owners change message for reviewer on change %s in project %s.",
- changeId, projectName);
+ Optional<? extends Exception> configurationError =
+ CodeOwnersExceptionHook.getCauseOfConfigurationError(e);
+ if (configurationError.isPresent()) {
+ logger.atWarning().log(
+ "Failed to post code-owners change message for reviewer on change %s in project %s: %s",
+ changeId, projectName, configurationError.get().getMessage());
+ } else {
+ logger.atSevere().withCause(e).log(
+ "Failed to post code-owners change message for reviewer on change %s in project %s.",
+ changeId, projectName);
+ }
}
}
diff --git a/java/com/google/gerrit/plugins/codeowners/backend/OnCodeOwnerApproval.java b/java/com/google/gerrit/plugins/codeowners/backend/OnCodeOwnerApproval.java
index b538ebd..907f065 100644
--- a/java/com/google/gerrit/plugins/codeowners/backend/OnCodeOwnerApproval.java
+++ b/java/com/google/gerrit/plugins/codeowners/backend/OnCodeOwnerApproval.java
@@ -17,6 +17,7 @@
import static com.google.common.base.Preconditions.checkState;
import com.google.common.collect.ImmutableList;
+import com.google.common.flogger.FluentLogger;
import com.google.gerrit.entities.PatchSet;
import com.google.gerrit.metrics.Timer0;
import com.google.gerrit.plugins.codeowners.backend.config.CodeOwnersPluginConfiguration;
@@ -51,6 +52,8 @@
*/
@Singleton
class OnCodeOwnerApproval implements OnPostReview {
+ private static final FluentLogger logger = FluentLogger.forEnclosingClass();
+
private final CodeOwnersPluginConfiguration codeOwnersPluginConfiguration;
private final CodeOwnerApprovalCheck codeOwnerApprovalCheck;
private final CodeOwnerMetrics codeOwnerMetrics;
@@ -103,6 +106,23 @@
approvals,
requiredApproval,
maxPathsInChangeMessage);
+ } catch (Exception e) {
+ Optional<? extends Exception> configurationError =
+ CodeOwnersExceptionHook.getCauseOfConfigurationError(e);
+ if (configurationError.isPresent()) {
+ logger.atWarning().log(
+ "Failed to post code-owners change message for code owner approval on change %s"
+ + " in project %s: %s",
+ changeNotes.getChangeId(),
+ changeNotes.getProjectName(),
+ configurationError.get().getMessage());
+ } else {
+ logger.atSevere().withCause(e).log(
+ "Failed to post code-owners change message for code owner approval on change %s"
+ + " in project %s.",
+ changeNotes.getChangeId(), changeNotes.getProjectName());
+ }
+ return Optional.empty();
}
}
diff --git a/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/CodeOwnersOnAddReviewerIT.java b/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/CodeOwnersOnAddReviewerIT.java
index bfc0283..70fb164 100644
--- a/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/CodeOwnersOnAddReviewerIT.java
+++ b/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/CodeOwnersOnAddReviewerIT.java
@@ -83,6 +83,18 @@
}
@Test
+ public void noChangeMessageAddedIfInvalidCodeOwnerConfigFilesExist() throws Exception {
+ createNonParseableCodeOwnerConfig(getCodeOwnerConfigFileName());
+
+ String changeId = createChange("Test Change", "foo/bar.baz", "file content").getChangeId();
+
+ gApi.changes().id(changeId).addReviewer(user.email());
+
+ Collection<ChangeMessageInfo> messages = gApi.changes().id(changeId).get().messages;
+ assertThat(Iterables.getLast(messages).message).isEqualTo("Uploaded patch set 1.");
+ }
+
+ @Test
public void changeMessageListsOwnedPaths() throws Exception {
codeOwnerConfigOperations
.newCodeOwnerConfig()
diff --git a/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/OnCodeOwnerApprovalIT.java b/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/OnCodeOwnerApprovalIT.java
index efd2166..d3fe19a 100644
--- a/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/OnCodeOwnerApprovalIT.java
+++ b/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/OnCodeOwnerApprovalIT.java
@@ -65,6 +65,19 @@
}
@Test
+ public void changeMessageNotExtendedIfInvalidCodeOwnerConfigFilesExist() throws Exception {
+ createNonParseableCodeOwnerConfig(getCodeOwnerConfigFileName());
+
+ String path = "foo/bar.baz";
+ String changeId = createChange("Test Change", path, "file content").getChangeId();
+
+ recommend(changeId);
+
+ Collection<ChangeMessageInfo> messages = gApi.changes().id(changeId).get().messages;
+ assertThat(Iterables.getLast(messages).message).isEqualTo("Patch Set 1: Code-Review+1");
+ }
+
+ @Test
public void changeMessageListsNewlyApprovedPaths() throws Exception {
codeOwnerConfigOperations
.newCodeOwnerConfig()
