Do not reject commit upon connectivity problems
When the issue existence could not be checked against the issue tracker
because of a connectivity problem, the push will be accepted and the
reason of the failure logged and displayed to the user.
Bug: Issue 12372
Change-Id: Ib5cfa2fafa81f3adf6d50aed44e2f523c86fea5b
diff --git a/src/main/java/com/googlesource/gerrit/plugins/its/base/validation/ItsValidateComment.java b/src/main/java/com/googlesource/gerrit/plugins/its/base/validation/ItsValidateComment.java
index f02050b..b0c9e39 100644
--- a/src/main/java/com/googlesource/gerrit/plugins/its/base/validation/ItsValidateComment.java
+++ b/src/main/java/com/googlesource/gerrit/plugins/its/base/validation/ItsValidateComment.java
@@ -47,6 +47,12 @@
@Inject private IssueExtractor issueExtractor;
+ private enum ItsExistenceCheckResult {
+ EXISTS,
+ DOESNT_EXIST,
+ CONNECTIVITY_FAILURE
+ }
+
private List<CommitValidationMessage> validCommit(Project.NameKey project, RevCommit commit)
throws CommitValidationException {
List<CommitValidationMessage> ret = Lists.newArrayList();
@@ -63,16 +69,23 @@
List<String> nonExistingIssueIds = Lists.newArrayList();
client = itsFacadeFactory.getFacade(project);
for (String issueId : issueIds) {
- boolean exists = false;
+ ItsExistenceCheckResult existenceCheckResult;
try {
- exists = client.exists(issueId);
+ existenceCheckResult =
+ client.exists(issueId)
+ ? ItsExistenceCheckResult.EXISTS
+ : ItsExistenceCheckResult.DOESNT_EXIST;
} catch (IOException e) {
- synopsis = "Failed to check whether or not issue " + issueId + " exists";
+ synopsis =
+ "Failed to check whether or not issue "
+ + issueId
+ + " exists, due to connectivity issue. Commit will be accepted.";
log.warn(synopsis, e);
details = e.toString();
- ret.add(commitValidationFailure(synopsis, details));
+ existenceCheckResult = ItsExistenceCheckResult.CONNECTIVITY_FAILURE;
+ ret.add(commitValidationFailure(synopsis, details, existenceCheckResult));
}
- if (!exists) {
+ if (existenceCheckResult == ItsExistenceCheckResult.DOESNT_EXIST) {
nonExistingIssueIds.add(issueId);
}
}
@@ -95,7 +108,7 @@
sb.append(" Issue-Tracker");
details = sb.toString();
- ret.add(commitValidationFailure(synopsis, details));
+ ret.add(commitValidationFailure(synopsis, details, ItsExistenceCheckResult.DOESNT_EXIST));
}
} else if (!itsConfig
.getDummyIssuePattern()
@@ -118,7 +131,7 @@
sb.append(" Issue-Tracker");
details = sb.toString();
- ret.add(commitValidationFailure(synopsis, details));
+ ret.add(commitValidationFailure(synopsis, details, ItsExistenceCheckResult.DOESNT_EXIST));
}
break;
case OPTIONAL:
@@ -128,10 +141,12 @@
return ret;
}
- private CommitValidationMessage commitValidationFailure(String synopsis, String details)
+ private CommitValidationMessage commitValidationFailure(
+ String synopsis, String details, ItsExistenceCheckResult existenceCheck)
throws CommitValidationException {
CommitValidationMessage ret = new CommitValidationMessage(synopsis + "\n" + details, false);
- if (itsConfig.getItsAssociationPolicy() == ItsAssociationPolicy.MANDATORY) {
+ if (itsConfig.getItsAssociationPolicy() == ItsAssociationPolicy.MANDATORY
+ && existenceCheck != ItsExistenceCheckResult.CONNECTIVITY_FAILURE) {
throw new CommitValidationException(synopsis, Collections.singletonList(ret));
}
return ret;
diff --git a/src/main/resources/Documentation/config-common.md b/src/main/resources/Documentation/config-common.md
index 666481f..109aba4 100644
--- a/src/main/resources/Documentation/config-common.md
+++ b/src/main/resources/Documentation/config-common.md
@@ -43,7 +43,9 @@
MANDATORY
: One or more issue-ids are required in the git commit message. The git push will
- be rejected otherwise.
+ be rejected otherwise. Note that in case of connectivity issues with ITS,
+ the commit will be accepted. The client will be notified about the
+ connectivity issue and the result.
SUGGESTED
: Whenever the git commit message does not contain one or more issue-ids,
diff --git a/src/test/java/com/googlesource/gerrit/plugins/its/base/validation/ItsValidateCommentTest.java b/src/test/java/com/googlesource/gerrit/plugins/its/base/validation/ItsValidateCommentTest.java
index d247a7c..666d463 100644
--- a/src/test/java/com/googlesource/gerrit/plugins/its/base/validation/ItsValidateCommentTest.java
+++ b/src/test/java/com/googlesource/gerrit/plugins/its/base/validation/ItsValidateCommentTest.java
@@ -468,20 +468,21 @@
assertEquals("Size of returned CommitValidationMessages does not match", 2, ret.size());
assertTrue(
- "First CommitValidationMessages does not contain " + "'Failed'",
- ret.get(0).getMessage().contains("Failed"));
+ "First CommitValidationMessages does not contain " + "'Failed to check'",
+ ret.get(0).getMessage().contains("Failed to check"));
assertTrue(
"First CommitValidationMessages does not contain '4711'",
ret.get(0).getMessage().contains("4711"));
+ assertTrue(
+ "First CommitValidationMessages contains reason of failure",
+ ret.get(0).getMessage().contains("InjectedEx1"));
assertFalse(
"First CommitValidationMessages contains '42', although " + "that bug exists",
ret.get(0).getMessage().contains("42"));
assertTrue(
"Second CommitValidationMessages does not contain " + "'Non-existing'",
ret.get(1).getMessage().contains("Non-existing"));
- assertTrue(
- "Second CommitValidationMessages does not contain '4711'",
- ret.get(1).getMessage().contains("4711"));
+
assertTrue(
"Second CommitValidationMessages does not contain '42'",
ret.get(1).getMessage().contains("42"));
@@ -489,6 +490,114 @@
assertLogMessageContains("4711");
}
+ public void testMandatoryMatchingSingleIOException()
+ throws CommitValidationException, IOException {
+ List<CommitValidationMessage> ret;
+ ItsValidateComment ivc = injector.getInstance(ItsValidateComment.class);
+ ReceiveCommand command = createMock(ReceiveCommand.class);
+ RevCommit commit = createMock(RevCommit.class);
+ CommitReceivedEvent event = newCommitReceivedEvent(command, project, null, commit, null);
+
+ expect(itsConfig.getItsAssociationPolicy())
+ .andReturn(ItsAssociationPolicy.MANDATORY)
+ .atLeastOnce();
+ expect(commit.getFullMessage()).andReturn("bug#4711").atLeastOnce();
+ expect(commit.getId()).andReturn(commit).anyTimes();
+ expect(commit.getName()).andReturn("TestCommit").anyTimes();
+ expect(issueExtractor.getIssueIds("bug#4711")).andReturn(new String[] {"4711"}).atLeastOnce();
+ expect(itsFacadeFactory.getFacade(project.getNameKey())).andReturn(itsFacade).anyTimes();
+ expect(itsFacade.exists("4711")).andThrow(new IOException("InjectedEx1")).atLeastOnce();
+
+ replayMocks();
+
+ ret = ivc.onCommitReceived(event);
+
+ assertEquals("Size of returned CommitValidationMessages does not match", 1, ret.size());
+ assertTrue(
+ "First CommitValidationMessages does not contain " + "'Failed to check'",
+ ret.get(0).getMessage().contains("Failed to check"));
+ assertTrue(
+ "First CommitValidationMessages does not contain '4711'",
+ ret.get(0).getMessage().contains("4711"));
+ assertTrue(
+ "First CommitValidationMessages contains reason of failure",
+ ret.get(0).getMessage().contains("InjectedEx1"));
+
+ assertLogMessageContains("4711");
+ }
+
+ public void testMandatoryMatchingMultipleIOExceptionIsNonExisting() throws IOException {
+ List<CommitValidationMessage> ret;
+ ItsValidateComment ivc = injector.getInstance(ItsValidateComment.class);
+ ReceiveCommand command = createMock(ReceiveCommand.class);
+ RevCommit commit = createMock(RevCommit.class);
+ CommitReceivedEvent event = newCommitReceivedEvent(command, project, null, commit, null);
+
+ expect(itsConfig.getItsAssociationPolicy())
+ .andReturn(ItsAssociationPolicy.MANDATORY)
+ .atLeastOnce();
+ expect(commit.getFullMessage()).andReturn("bug#4711, bug#42").atLeastOnce();
+ expect(commit.getId()).andReturn(commit).anyTimes();
+ expect(commit.getName()).andReturn("TestCommit").anyTimes();
+ expect(issueExtractor.getIssueIds("bug#4711, bug#42"))
+ .andReturn(new String[] {"4711", "42"})
+ .atLeastOnce();
+ expect(itsFacadeFactory.getFacade(project.getNameKey())).andReturn(itsFacade).anyTimes();
+ expect(itsFacade.exists("4711")).andThrow(new IOException("InjectedEx1")).atLeastOnce();
+ expect(itsFacade.exists("42")).andReturn(false).atLeastOnce();
+
+ replayMocks();
+
+ try {
+ ivc.onCommitReceived(event);
+ fail("onCommitReceived did not throw any exception");
+ } catch (CommitValidationException e) {
+ assertLogMessageContains("Failed to check whether or not issue 4711 exists");
+ assertTrue(
+ "Message of thrown CommitValidationException does not " + "contain 'Non-existing'",
+ e.getMessage().contains("Non-existing"));
+ }
+ }
+
+ public void testMandatoryMatchingMultipleIOExceptionExisting()
+ throws CommitValidationException, IOException {
+ List<CommitValidationMessage> ret;
+ ItsValidateComment ivc = injector.getInstance(ItsValidateComment.class);
+ ReceiveCommand command = createMock(ReceiveCommand.class);
+ RevCommit commit = createMock(RevCommit.class);
+ CommitReceivedEvent event = newCommitReceivedEvent(command, project, null, commit, null);
+
+ expect(itsConfig.getItsAssociationPolicy())
+ .andReturn(ItsAssociationPolicy.MANDATORY)
+ .atLeastOnce();
+ expect(commit.getFullMessage()).andReturn("bug#4711, bug#42").atLeastOnce();
+ expect(commit.getId()).andReturn(commit).anyTimes();
+ expect(commit.getName()).andReturn("TestCommit").anyTimes();
+ expect(issueExtractor.getIssueIds("bug#4711, bug#42"))
+ .andReturn(new String[] {"4711", "42"})
+ .atLeastOnce();
+ expect(itsFacadeFactory.getFacade(project.getNameKey())).andReturn(itsFacade).anyTimes();
+ expect(itsFacade.exists("4711")).andThrow(new IOException("InjectedEx1")).atLeastOnce();
+ expect(itsFacade.exists("42")).andReturn(true).atLeastOnce();
+
+ replayMocks();
+
+ ret = ivc.onCommitReceived(event);
+
+ assertEquals("Size of returned CommitValidationMessages does not match", 1, ret.size());
+ assertTrue(
+ "First CommitValidationMessages does not contain " + "'Failed to check'",
+ ret.get(0).getMessage().contains("Failed to check"));
+ assertTrue(
+ "First CommitValidationMessages does not contain '4711'",
+ ret.get(0).getMessage().contains("4711"));
+ assertTrue(
+ "First CommitValidationMessages contains reason of failure",
+ ret.get(0).getMessage().contains("InjectedEx1"));
+
+ assertLogMessageContains("4711");
+ }
+
public void assertEmptyList(List<CommitValidationMessage> list) {
if (!list.isEmpty()) {
StringBuffer sb = new StringBuffer();
