REST endpoint: Give a more descriptive response
A request to the plugin returns "204 No content" on success, and a
generic 500 on any error without any more details. Provide the caller
with more information, so they know what happened and if they should
retry.
New responses:
* A noop call (no projects to update) returns "204 No content"
* A successfull update returns "200 OK"
* A problem in conf is a "412 Precondition Failure"
* An exception return 500 (as before) but now with an error message.
Change-Id: I453b3a674cb252b79d02b65d5255675a1d7dd291
diff --git a/java/com/googlesource/gerrit/plugins/supermanifest/SuperManifestRefUpdatedListener.java b/java/com/googlesource/gerrit/plugins/supermanifest/SuperManifestRefUpdatedListener.java
index 57ee1b6..a15b402 100644
--- a/java/com/googlesource/gerrit/plugins/supermanifest/SuperManifestRefUpdatedListener.java
+++ b/java/com/googlesource/gerrit/plugins/supermanifest/SuperManifestRefUpdatedListener.java
@@ -27,6 +27,7 @@
import com.google.gerrit.extensions.events.GitReferenceUpdatedListener;
import com.google.gerrit.extensions.events.LifecycleListener;
import com.google.gerrit.extensions.restapi.AuthException;
+import com.google.gerrit.extensions.restapi.PreconditionFailedException;
import com.google.gerrit.extensions.restapi.Response;
import com.google.gerrit.extensions.restapi.RestModifyView;
import com.google.gerrit.metrics.Counter1;
@@ -158,6 +159,11 @@
}
@FormatMethod
+ private void errorWithCause(Exception e, @FormatString String formatStr, Object... args) {
+ logger.atSevere().withCause(e).log("%s: %s", canonicalWebUrl, String.format(formatStr, args));
+ }
+
+ @FormatMethod
private void info(@FormatString String formatStr, Object... args) {
logger.atInfo().log("%s: %s", canonicalWebUrl, String.format(formatStr, args));
}
@@ -299,22 +305,37 @@
@Override
public Response<?> apply(BranchResource resource, BranchInput input)
- throws IOException, ConfigInvalidException, GitAPIException, AuthException,
- PermissionBackendException {
+ throws AuthException, PermissionBackendException, PreconditionFailedException {
permissionBackend.currentUser().check(GlobalPermission.ADMINISTRATE_SERVER);
+ String manifestProject = resource.getBranchKey().project().get();
+ String manifestBranch = resource.getBranchKey().branch();
info(
"manual trigger for %s:%s by %d. Config: %s",
- resource.getBranchKey().project().get(),
- resource.getBranchKey().branch(),
+ manifestProject,
+ manifestBranch,
identifiedUser.get().getAccountId().get(),
configurationToString());
List<ConfigEntry> relevantConfigs =
findRelevantConfigs(resource.getProjectState().getProject().getName(), resource.getRef());
- for (ConfigEntry config : relevantConfigs) {
- updateForConfig(config, resource.getRef());
+ if (relevantConfigs.isEmpty()) {
+ info(
+ "manual trigger for %s:%s: no configs found, nothing to do.",
+ manifestProject, manifestBranch);
+ return Response.none();
}
- return Response.none();
+ for (ConfigEntry config : relevantConfigs) {
+ try {
+ updateForConfig(config, resource.getRef());
+ } catch (ConfigInvalidException e) {
+ errorWithCause(e, "Invalid conf processing %s:%s", manifestProject, manifestBranch);
+ throw new PreconditionFailedException(e.getMessage());
+ } catch (GitAPIException | IOException e) {
+ errorWithCause(e, "Internal error processing %s:%s", manifestProject, manifestBranch);
+ return Response.withStatusCode(500, "Internal error: " + e.getMessage());
+ }
+ }
+ return Response.ok();
}
private List<ConfigEntry> findRelevantConfigs(String project, String refName) {
diff --git a/javatests/com/googlesource/gerrit/plugins/supermanifest/RepoSuperManifestIT.java b/javatests/com/googlesource/gerrit/plugins/supermanifest/RepoSuperManifestIT.java
index 8f40cdc..ba9593a 100644
--- a/javatests/com/googlesource/gerrit/plugins/supermanifest/RepoSuperManifestIT.java
+++ b/javatests/com/googlesource/gerrit/plugins/supermanifest/RepoSuperManifestIT.java
@@ -244,6 +244,10 @@
.create(admin.newIdent(), manifestRepo, "Subject", "default.xml", xml)
.to("refs/heads/srcbranch")
.assertOkStatus();
+ pushFactory
+ .create(admin.newIdent(), manifestRepo, "Subject", "default.xml", xml)
+ .to("refs/heads/anotherbranch")
+ .assertOkStatus();
// Push config after XML. Needs a manual trigger to create the destination.
pushConfig(
@@ -260,6 +264,10 @@
userRestSession.post("/projects/" + manifestKey + "/branches/srcbranch/update_manifest");
r.assertForbidden();
r = adminRestSession.post("/projects/" + manifestKey + "/branches/srcbranch/update_manifest");
+ r.assertOK();
+ r =
+ adminRestSession.post(
+ "/projects/" + manifestKey + "/branches/anotherbranch/update_manifest");
r.assertNoContent();
BranchApi branch = gApi.projects().name(superKey.get()).branch("refs/heads/destbranch");
@@ -313,7 +321,7 @@
adminRestSession
.post("/projects/" + manifestKey + "/branches/srcbranch/update_manifest")
- .assertNoContent();
+ .assertOK();
BranchApi branch = gApi.projects().name(superKey.get()).branch("refs/heads/destbranch");
assertThat(branch.file("project1").getContentType()).isEqualTo("x-git/gitlink; charset=UTF-8");
@@ -370,7 +378,7 @@
+ " ignoreRemoteFailures = true\n");
r = adminRestSession.post("/projects/" + manifestKey + "/branches/srcbranch/update_manifest");
- r.assertNoContent();
+ r.assertOK();
BranchApi branch = gApi.projects().name(superKey.get()).branch("refs/heads/destbranch");
assertThat(branch.file("project1").getContentType()).isEqualTo("x-git/gitlink; charset=UTF-8");
