Add a message text field to checks for further details
Checkers should be able to show short messages directly on a check (e.g.
short failure reason; further infos).
For now this is just a bare string. In the long-term, we might consider
to add markdown support but we would have to provide the general
infrastructure for it in Gerrit first (e.g. have markdown rendering for
our hosts) and we would need to do this for other parts of Gerrit as
well.
Signed-off-by: Edwin Kempin <ekempin@google.com>
Change-Id: If51d31b79ff5859aa4fd6dceda248ea2397e96f0
diff --git a/java/com/google/gerrit/plugins/checks/Check.java b/java/com/google/gerrit/plugins/checks/Check.java
index bd46191..30bfa41 100644
--- a/java/com/google/gerrit/plugins/checks/Check.java
+++ b/java/com/google/gerrit/plugins/checks/Check.java
@@ -37,6 +37,9 @@
/** State that this check is in. */
public abstract CheckState state();
+ /** Short message explaining the check state. */
+ public abstract Optional<String> message();
+
/** Fully qualified URL to detailed result on the Checker's service. */
public abstract Optional<String> url();
@@ -64,6 +67,8 @@
public abstract Builder setState(CheckState state);
+ public abstract Builder setMessage(String message);
+
public abstract Builder setUrl(String url);
public abstract Builder setStarted(Timestamp started);
diff --git a/java/com/google/gerrit/plugins/checks/CheckJson.java b/java/com/google/gerrit/plugins/checks/CheckJson.java
index 4b5e9f3..3ef8937 100644
--- a/java/com/google/gerrit/plugins/checks/CheckJson.java
+++ b/java/com/google/gerrit/plugins/checks/CheckJson.java
@@ -66,6 +66,7 @@
info.patchSetId = check.key().patchSet().patchSetId;
info.state = check.state();
+ info.message = check.message().orElse(null);
info.url = check.url().orElse(null);
info.started = check.started().orElse(null);
info.finished = check.finished().orElse(null);
diff --git a/java/com/google/gerrit/plugins/checks/CheckUpdate.java b/java/com/google/gerrit/plugins/checks/CheckUpdate.java
index fd10148..315160d 100644
--- a/java/com/google/gerrit/plugins/checks/CheckUpdate.java
+++ b/java/com/google/gerrit/plugins/checks/CheckUpdate.java
@@ -23,6 +23,8 @@
public abstract class CheckUpdate {
public abstract Optional<CheckState> state();
+ public abstract Optional<String> message();
+
public abstract Optional<String> url();
public abstract Optional<Timestamp> started();
@@ -39,6 +41,8 @@
public abstract static class Builder {
public abstract Builder setState(CheckState state);
+ public abstract Builder setMessage(String message);
+
public abstract Builder setUrl(String url);
public abstract Builder setStarted(Timestamp started);
diff --git a/java/com/google/gerrit/plugins/checks/acceptance/testsuite/CheckOperationsImpl.java b/java/com/google/gerrit/plugins/checks/acceptance/testsuite/CheckOperationsImpl.java
index a348777..a6a1c02 100644
--- a/java/com/google/gerrit/plugins/checks/acceptance/testsuite/CheckOperationsImpl.java
+++ b/java/com/google/gerrit/plugins/checks/acceptance/testsuite/CheckOperationsImpl.java
@@ -125,6 +125,7 @@
private static CheckUpdate toCheckUpdate(TestCheckUpdate testUpdate) {
CheckUpdate.Builder update = CheckUpdate.builder();
+ testUpdate.message().ifPresent(update::setMessage);
testUpdate.state().ifPresent(update::setState);
testUpdate.started().ifPresent(update::setStarted);
testUpdate.finished().ifPresent(update::setFinished);
diff --git a/java/com/google/gerrit/plugins/checks/acceptance/testsuite/TestCheckUpdate.java b/java/com/google/gerrit/plugins/checks/acceptance/testsuite/TestCheckUpdate.java
index e3230af..90864a6 100644
--- a/java/com/google/gerrit/plugins/checks/acceptance/testsuite/TestCheckUpdate.java
+++ b/java/com/google/gerrit/plugins/checks/acceptance/testsuite/TestCheckUpdate.java
@@ -27,6 +27,8 @@
public abstract Optional<CheckState> state();
+ public abstract Optional<String> message();
+
public abstract Optional<String> url();
public abstract Optional<Timestamp> started();
@@ -47,6 +49,12 @@
public abstract Builder state(CheckState state);
+ public abstract Builder message(String message);
+
+ public Builder clearMessage() {
+ return message("");
+ }
+
public abstract Builder url(String url);
public Builder clearUrl() {
diff --git a/java/com/google/gerrit/plugins/checks/api/CheckInfo.java b/java/com/google/gerrit/plugins/checks/api/CheckInfo.java
index 3096fec..77cbdef 100644
--- a/java/com/google/gerrit/plugins/checks/api/CheckInfo.java
+++ b/java/com/google/gerrit/plugins/checks/api/CheckInfo.java
@@ -33,6 +33,8 @@
/** State that this check exited. */
public CheckState state;
+ /** Short message explaining the check state. */
+ @Nullable public String message;
/** Fully qualified URL to detailed result on the Checker's service. */
@Nullable public String url;
/** Timestamp of when this check was created. */
@@ -65,6 +67,7 @@
&& Objects.equals(other.patchSetId, patchSetId)
&& Objects.equals(other.checkerUuid, checkerUuid)
&& Objects.equals(other.state, state)
+ && Objects.equals(other.message, message)
&& Objects.equals(other.url, url)
&& Objects.equals(other.started, started)
&& Objects.equals(other.finished, finished)
@@ -83,6 +86,7 @@
patchSetId,
checkerUuid,
state,
+ message,
url,
started,
finished,
@@ -101,6 +105,7 @@
.add("patchSetId", patchSetId)
.add("checkerUuid", checkerUuid)
.add("state", state)
+ .add("message", message)
.add("url", url)
.add("started", started)
.add("finished", finished)
diff --git a/java/com/google/gerrit/plugins/checks/api/CheckInput.java b/java/com/google/gerrit/plugins/checks/api/CheckInput.java
index 633d2fb..2162ce3 100644
--- a/java/com/google/gerrit/plugins/checks/api/CheckInput.java
+++ b/java/com/google/gerrit/plugins/checks/api/CheckInput.java
@@ -25,6 +25,8 @@
@Nullable public String checkerUuid;
/** State of the check. */
@Nullable public CheckState state;
+ /** Short message explaining the check state. */
+ @Nullable public String message;
/** Fully qualified URL to detailed result on the Checker's service. */
@Nullable public String url;
/** Date/Time at which the checker started processing this check. */
@@ -40,6 +42,7 @@
CheckInput other = (CheckInput) o;
return Objects.equals(other.checkerUuid, checkerUuid)
&& Objects.equals(other.state, state)
+ && Objects.equals(other.message, message)
&& Objects.equals(other.url, url)
&& Objects.equals(other.started, started)
&& Objects.equals(other.finished, finished);
@@ -47,7 +50,7 @@
@Override
public int hashCode() {
- return Objects.hash(checkerUuid, state, url, started, finished);
+ return Objects.hash(checkerUuid, state, message, url, started, finished);
}
@Override
@@ -55,6 +58,7 @@
return MoreObjects.toStringHelper(this)
.add("checkerUuid", checkerUuid)
.add("state", state)
+ .add("message", message)
.add("url", url)
.add("started", started)
.add("finished", finished)
diff --git a/java/com/google/gerrit/plugins/checks/api/PostCheck.java b/java/com/google/gerrit/plugins/checks/api/PostCheck.java
index ad011d7..d6934e7 100644
--- a/java/com/google/gerrit/plugins/checks/api/PostCheck.java
+++ b/java/com/google/gerrit/plugins/checks/api/PostCheck.java
@@ -117,6 +117,10 @@
checkUpdateBuilder.setState(input.state);
}
+ if (input.message != null) {
+ checkUpdateBuilder.setMessage(input.message.trim());
+ }
+
if (input.url != null) {
checkUpdateBuilder.setUrl(UrlValidator.clean(input.url));
}
diff --git a/java/com/google/gerrit/plugins/checks/db/NoteDbCheck.java b/java/com/google/gerrit/plugins/checks/db/NoteDbCheck.java
index 2b52000..cb60d2c 100644
--- a/java/com/google/gerrit/plugins/checks/db/NoteDbCheck.java
+++ b/java/com/google/gerrit/plugins/checks/db/NoteDbCheck.java
@@ -17,6 +17,7 @@
private NoteDbCheck() {}
public CheckState state = CheckState.NOT_STARTED;
+ @Nullable public String message;
@Nullable public String url;
@Nullable public Timestamp started;
@Nullable public Timestamp finished;
@@ -27,6 +28,9 @@
Check toCheck(CheckKey key) {
Check.Builder newCheck =
Check.builder(key).setState(state).setCreated(created).setUpdated(updated);
+ if (message != null) {
+ newCheck.setMessage(message);
+ }
if (url != null) {
newCheck.setUrl(url);
}
@@ -60,6 +64,11 @@
state = update.state().get();
modified = true;
}
+ if (update.message().isPresent()
+ && !update.message().get().equals(Strings.nullToEmpty(message))) {
+ message = Strings.emptyToNull(update.message().get());
+ modified = true;
+ }
if (update.url().isPresent() && !update.url().get().equals(Strings.nullToEmpty(url))) {
url = Strings.emptyToNull(update.url().get());
modified = true;
diff --git a/javatests/com/google/gerrit/plugins/checks/acceptance/api/CreateCheckIT.java b/javatests/com/google/gerrit/plugins/checks/acceptance/api/CreateCheckIT.java
index 1fc7c8a..85c176b 100644
--- a/javatests/com/google/gerrit/plugins/checks/acceptance/api/CreateCheckIT.java
+++ b/javatests/com/google/gerrit/plugins/checks/acceptance/api/CreateCheckIT.java
@@ -79,6 +79,7 @@
CheckInfo info = checksApiFactory.revision(patchSetId).create(input).get();
assertThat(info.checkerUuid).isEqualTo(checkerUuid.get());
assertThat(info.state).isEqualTo(CheckState.RUNNING);
+ assertThat(info.message).isNull();
assertThat(info.url).isNull();
assertThat(info.started).isNull();
assertThat(info.finished).isNull();
@@ -130,6 +131,19 @@
}
@Test
+ public void createCheckWithMessage() throws Exception {
+ CheckerUuid checkerUuid = checkerOperations.newChecker().repository(project).create();
+
+ CheckInput input = new CheckInput();
+ input.checkerUuid = checkerUuid.get();
+ input.message = "some message";
+
+ CheckInfo info = checksApiFactory.revision(patchSetId).create(input).get();
+ assertThat(info.message).isEqualTo(input.message);
+ assertThat(getCheck(project, patchSetId, checkerUuid).message()).hasValue(input.message);
+ }
+
+ @Test
public void createCheckWithUrl() throws Exception {
CheckerUuid checkerUuid = checkerOperations.newChecker().repository(project).create();
diff --git a/javatests/com/google/gerrit/plugins/checks/acceptance/api/UpdateCheckIT.java b/javatests/com/google/gerrit/plugins/checks/acceptance/api/UpdateCheckIT.java
index 3c37987..665b3eb 100644
--- a/javatests/com/google/gerrit/plugins/checks/acceptance/api/UpdateCheckIT.java
+++ b/javatests/com/google/gerrit/plugins/checks/acceptance/api/UpdateCheckIT.java
@@ -91,6 +91,26 @@
}
@Test
+ public void updateMessage() throws Exception {
+ CheckInput input = new CheckInput();
+ input.message = "some message";
+
+ CheckInfo info = checksApiFactory.revision(patchSetId).id(checkKey.checkerUuid()).update(input);
+ assertThat(info.message).isEqualTo(input.message);
+ }
+
+ @Test
+ public void unsetMessage() throws Exception {
+ checkOperations.check(checkKey).forUpdate().message("some message").upsert();
+
+ CheckInput input = new CheckInput();
+ input.message = "";
+
+ CheckInfo info = checksApiFactory.revision(patchSetId).id(checkKey.checkerUuid()).update(input);
+ assertThat(info.message).isNull();
+ }
+
+ @Test
public void updateUrl() throws Exception {
CheckInput input = new CheckInput();
input.url = "http://example.com/my-check";
@@ -311,6 +331,7 @@
.check(checkKey)
.forUpdate()
.state(CheckState.FAILED)
+ .message("some message")
.url("https://www.example.com")
.upsert();
@@ -318,6 +339,7 @@
input.state = CheckState.NOT_STARTED;
CheckInfo info = checksApiFactory.revision(patchSetId).id(checkKey.checkerUuid()).update(input);
+ assertThat(info.message).isEqualTo("some message");
assertThat(info.url).isEqualTo("https://www.example.com");
}
}
diff --git a/javatests/com/google/gerrit/plugins/checks/acceptance/testsuite/CheckOperationsImplTest.java b/javatests/com/google/gerrit/plugins/checks/acceptance/testsuite/CheckOperationsImplTest.java
index 0b53666..b3166b6 100644
--- a/javatests/com/google/gerrit/plugins/checks/acceptance/testsuite/CheckOperationsImplTest.java
+++ b/javatests/com/google/gerrit/plugins/checks/acceptance/testsuite/CheckOperationsImplTest.java
@@ -72,6 +72,7 @@
assertThat(foundCheck.patchSetId).isEqualTo(checkKey.patchSet().get());
assertThat(foundCheck.checkerUuid).isEqualTo(checkerUuid.get());
assertThat(foundCheck.state).isNotNull();
+ assertThat(foundCheck.message).isNull();
assertThat(foundCheck.url).isNull();
assertThat(foundCheck.started).isNull();
assertThat(foundCheck.finished).isNull();
@@ -130,6 +131,26 @@
}
@Test
+ public void specifiedMessageIsRespectedForCheckCreation() throws Exception {
+ CheckerUuid checkerUuid = checkerOperations.newChecker().repository(project).create();
+ CheckKey checkKey = CheckKey.create(project, createChange().getPatchSetId(), checkerUuid);
+ checkOperations.newCheck(checkKey).message("some message").upsert();
+
+ CheckInfo check = getCheckFromServer(checkKey);
+ assertThat(check.message).isEqualTo("some message");
+ }
+
+ @Test
+ public void requestingNoMessageIsPossibleForCheckCreation() throws Exception {
+ CheckerUuid checkerUuid = checkerOperations.newChecker().create();
+ CheckKey checkKey = CheckKey.create(project, createChange().getPatchSetId(), checkerUuid);
+ checkOperations.newCheck(checkKey).clearMessage().upsert();
+
+ CheckInfo check = getCheckFromServer(checkKey);
+ assertThat(check.message).isNull();
+ }
+
+ @Test
public void specifiedUrlIsRespectedForCheckCreation() throws Exception {
CheckerUuid checkerUuid = checkerOperations.newChecker().repository(project).create();
CheckKey checkKey = CheckKey.create(project, createChange().getPatchSetId(), checkerUuid);
@@ -259,6 +280,28 @@
}
@Test
+ public void messageOfExistingCheckCanBeRetrieved() throws Exception {
+ CheckerUuid checkerUuid = checkerOperations.newChecker().repository(project).create();
+ CheckKey checkKey = CheckKey.create(project, createChange().getPatchSetId(), checkerUuid);
+ checkOperations.newCheck(checkKey).message("some message").upsert();
+
+ Optional<String> message = checkOperations.check(checkKey).get().message();
+
+ assertThat(message).hasValue("some message");
+ }
+
+ @Test
+ public void emptyMessageOfExistingCheckCanBeRetrieved() throws Exception {
+ CheckerUuid checkerUuid = checkerOperations.newChecker().repository(project).create();
+ CheckKey checkKey = CheckKey.create(project, createChange().getPatchSetId(), checkerUuid);
+ checkOperations.newCheck(checkKey).clearMessage().upsert();
+
+ Optional<String> message = checkOperations.check(checkKey).get().message();
+
+ assertThat(message).isEmpty();
+ }
+
+ @Test
public void urlOfExistingCheckCanBeRetrieved() throws Exception {
CheckerUuid checkerUuid = checkerOperations.newChecker().repository(project).create();
CheckKey checkKey = CheckKey.create(project, createChange().getPatchSetId(), checkerUuid);
@@ -369,6 +412,30 @@
}
@Test
+ public void messageCanBeUpdated() throws Exception {
+ CheckerUuid checkerUuid = checkerOperations.newChecker().repository(project).create();
+ CheckKey checkKey = CheckKey.create(project, createChange().getPatchSetId(), checkerUuid);
+ checkOperations.newCheck(checkKey).message("original message").upsert();
+
+ checkOperations.check(checkKey).forUpdate().message("updated message").upsert();
+
+ Optional<String> currentMessage = checkOperations.check(checkKey).get().message();
+ assertThat(currentMessage).hasValue("updated message");
+ }
+
+ @Test
+ public void messageCanBeCleared() throws Exception {
+ CheckerUuid checkerUuid = checkerOperations.newChecker().repository(project).create();
+ CheckKey checkKey = CheckKey.create(project, createChange().getPatchSetId(), checkerUuid);
+ checkOperations.newCheck(checkKey).message("original message").upsert();
+
+ checkOperations.check(checkKey).forUpdate().clearMessage().upsert();
+
+ Optional<String> currentMessage = checkOperations.check(checkKey).get().message();
+ assertThat(currentMessage).isEmpty();
+ }
+
+ @Test
public void urlCanBeUpdated() throws Exception {
CheckerUuid checkerUuid = checkerOperations.newChecker().repository(project).create();
CheckKey checkKey = CheckKey.create(project, createChange().getPatchSetId(), checkerUuid);
@@ -452,6 +519,7 @@
checkOperations
.newCheck(checkKey)
.state(CheckState.RUNNING)
+ .message("some message")
.url("http://example.com/my-check")
.started(new Timestamp(1234567L))
.finished(new Timestamp(7654321L))
@@ -463,6 +531,7 @@
assertThat(checkInfo.patchSetId).isEqualTo(check.key().patchSet().get());
assertThat(checkInfo.checkerUuid).isEqualTo(check.key().checkerUuid().get());
assertThat(checkInfo.state).isEqualTo(check.state());
+ assertThat(checkInfo.message).isEqualTo(check.message().get());
assertThat(checkInfo.url).isEqualTo(check.url().get());
assertThat(checkInfo.started).isEqualTo(check.started().get());
assertThat(checkInfo.finished).isEqualTo(check.finished().get());
@@ -478,6 +547,7 @@
CheckInput checkInput = new CheckInput();
checkInput.checkerUuid = checkerUuid.get();
checkInput.state = CheckState.SCHEDULED;
+ checkInput.message = "some message";
checkInput.url = "http://example.com/my-check";
checkInput.started = TimeUtil.nowTs();
return checkInput;
diff --git a/src/main/resources/Documentation/rest-api-checks.md b/src/main/resources/Documentation/rest-api-checks.md
index c785e75..4e30e94 100644
--- a/src/main/resources/Documentation/rest-api-checks.md
+++ b/src/main/resources/Documentation/rest-api-checks.md
@@ -168,6 +168,7 @@
| `patch_set_id` | | The patch set that this check applies to.
| `checker_uuid` | | The [UUID](./rest-api-checkers.md#checker-id) of the checker that reported this check.
| `state` | | The state as string-serialized form of [CheckState](#check-state)
+| `message` | optional | Short message explaining the check state.
| `url` | optional | A fully-qualified URL pointing to the result of the check on the checker's infrastructure.
| `started` | optional | The [timestamp](../../../Documentation/rest-api.html#timestamp) of when the check started processing.
| `finished` | optional | The [timestamp](../../../Documentation/rest-api.html#timestamp) of when the check finished processing.
@@ -184,6 +185,7 @@
| --------------- | -------- | ----------- |
| `checker_uuid` | optional | The name of the checker. Must be specified for checker creation. Optional only if updating a check and referencing the checker using the [UUID](./rest-api-checkers.md#checker-id) in the URL.
| `state` | optional | The state as string-serialized form of [CheckState](#check-state)
+| `message` | optional | Short message explaining the check state.
| `url` | optional | A fully-qualified URL pointing to the result of the check on the checker's infrastructure.
| `started` | optional | The [timestamp](../../../Documentation/rest-api.html#timestamp) of when the check started processing.
| `finished` | optional | The [timestamp](../../../Documentation/rest-api.html#timestamp) of when the check finished processing.
