Clean up exception handling in StashApiClient

src/main/java/stashpullrequestbuilder/stashpullrequestbuilder/StashBuildListener.java

@@ -1,5 +1,7 @@
 package stashpullrequestbuilder.stashpullrequestbuilder;
 
+import static java.lang.String.format;
+
 import hudson.Extension;
 import hudson.Util;
 import hudson.model.AbstractBuild;
@@ -10,11 +12,11 @@
 import java.io.IOException;
 import java.io.PrintStream;
 import java.lang.invoke.MethodHandles;
-import java.util.logging.Level;
 import java.util.logging.Logger;
 import javax.annotation.Nonnull;
 import jenkins.model.JenkinsLocationConfiguration;
 import jenkins.model.ParameterizedJobMixIn;
+import stashpullrequestbuilder.stashpullrequestbuilder.stash.StashApiClient.StashApiException;
 
 /** Created by Nathan McCarthy */
 @Extension
@@ -42,6 +44,8 @@ public void onStarted(Run<?, ?> run, TaskListener listener) {
 
   @Override
   public void onCompleted(Run<?, ?> run, @Nonnull TaskListener listener) {
+    PrintStream buildLogger = listener.getLogger();
+
     StashCause cause = run.getCause(StashCause.class);
     if (cause == null) {
       return;
@@ -65,7 +69,19 @@ public void onCompleted(Run<?, ?> run, @Nonnull TaskListener listener) {
     } else {
       buildUrl = rootUrl + run.getUrl();
     }
-    repository.deletePullRequestComment(cause.getPullRequestId(), cause.getBuildStartCommentId());
+
+    // Delete the "Build Started" comment, as it gets replaced with a comment
+    // about the build result. Failure to delete the comment is not fatal, it's
+    // reported to the build log.
+    try {
+      repository.deletePullRequestComment(cause.getPullRequestId(), cause.getBuildStartCommentId());
+    } catch (StashApiException e) {
+      buildLogger.println(
+          format(
+              "%s: cannot delete Build Start comment for pull request %s",
+              run.getParent().getDisplayName(), cause.getPullRequestId()));
+      e.printStackTrace(buildLogger);
+    }
 
     String additionalComment = "";
     StashPostBuildComment comments = null;
@@ -103,31 +119,30 @@ public void onCompleted(Run<?, ?> run, @Nonnull TaskListener listener) {
         additionalComment,
         duration);
 
-    // Merge PR
+    // Request the server to merge the pull request on success if that option is
+    // enabled. Log the result to the build log. Handle exceptions here, report
+    // them to the build log as well.
     if (trigger.getMergeOnSuccess() && run.getResult() == Result.SUCCESS) {
-      boolean mergeStat =
-          repository.mergePullRequest(cause.getPullRequestId(), cause.getPullRequestVersion());
-      if (mergeStat == true) {
-        String logmsg =
-            "Merged pull request "
-                + cause.getPullRequestId()
-                + "("
-                + cause.getSourceBranch()
-                + ") to branch "
-                + cause.getTargetBranch();
-        logger.log(Level.INFO, logmsg);
-        listener.getLogger().println(logmsg);
-      } else {
-        String logmsg =
-            "Failed to merge pull request "
-                + cause.getPullRequestId()
-                + "("
-                + cause.getSourceBranch()
-                + ") to branch "
-                + cause.getTargetBranch()
-                + " because it's out of date";
-        logger.log(Level.INFO, logmsg);
-        listener.getLogger().println(logmsg);
+      try {
+        boolean mergeStat =
+            repository.mergePullRequest(cause.getPullRequestId(), cause.getPullRequestVersion());
+        if (mergeStat == true) {
+          buildLogger.println(
+              format(
+                  "Successfully merged pull request %s (%s) to branch %s",
+                  cause.getPullRequestId(), cause.getSourceBranch(), cause.getTargetBranch()));
+        } else {
+          buildLogger.println(
+              format(
+                  "Failed to merge pull request %s (%s) to branch %s, it may be out of date",
+                  cause.getPullRequestId(), cause.getSourceBranch(), cause.getTargetBranch()));
+        }
+      } catch (StashApiException e) {
+        buildLogger.println(
+            format(
+                "Failed to merge pull request %s (%s) to branch %s",
+                cause.getPullRequestId(), cause.getSourceBranch(), cause.getTargetBranch()));
+        e.printStackTrace(buildLogger);
       }
     }
   }

src/main/java/stashpullrequestbuilder/stashpullrequestbuilder/StashRepository.java

@@ -94,6 +94,9 @@ public Collection<StashPullRequestResponseValue> getTargetPullRequests() {
     List<StashPullRequestResponseValue> targetPullRequests =
         new ArrayList<StashPullRequestResponseValue>();
 
+    // Fetch "OPEN" pull requests from the server. Failure to get the list will
+    // prevent builds from being scheduled. However, the call will be retried
+    // during the next cycle, as determined by the cron settings.
     List<StashPullRequestResponseValue> pullRequests;
     try {
       pullRequests = client.getPullRequests();
@@ -110,7 +113,15 @@ public Collection<StashPullRequestResponseValue> getTargetPullRequests() {
     return targetPullRequests;
   }
 
-  public String postBuildStartCommentTo(StashPullRequestResponseValue pullRequest) {
+  /**
+   * Post "BuildStarted" comment to Bitbucket Server
+   *
+   * @param pullRequest pull request
+   * @return comment ID
+   * @throws StashApiException if posting the comment fails
+   */
+  public String postBuildStartCommentTo(StashPullRequestResponseValue pullRequest)
+      throws StashApiException {
     String sourceCommit = pullRequest.getFromRef().getLatestCommit();
     String destinationCommit = pullRequest.getToRef().getLatestCommit();
     String comment =
@@ -269,6 +280,9 @@ public void addFutureBuildTasks(Collection<StashPullRequestResponseValue> pullRe
     for (StashPullRequestResponseValue pullRequest : pullRequests) {
       Map<String, String> additionalParameters;
 
+      // Get parameter values for the build from the pull request comments.
+      // Failure to do so causes the build to be skipped, as we would not run
+      // the build with incorrect parameters.
       try {
         additionalParameters = getAdditionalParameters(pullRequest);
       } catch (StashApiException e) {
@@ -281,6 +295,8 @@ public void addFutureBuildTasks(Collection<StashPullRequestResponseValue> pullRe
         continue;
       }
 
+      // Delete comments about previous build results, if that option is
+      // enabled. Run the build even if those comments cannot be deleted.
       if (trigger.getDeletePreviousBuildFinishComments()) {
         try {
           deletePreviousBuildFinishedComments(pullRequest);
@@ -294,7 +310,25 @@ public void addFutureBuildTasks(Collection<StashPullRequestResponseValue> pullRe
         }
       }
 
-      String commentId = postBuildStartCommentTo(pullRequest);
+      // Post a comment indicating the build start. Strictly speaking, we are
+      // just adding the build to the queue, it will start after the quiet time
+      // expires and there are executors available. Failure to post the comment
+      // prevents the build for safety reasons. If the plugin cannot post this
+      // comment, chances are it won't be able to post the build results, which
+      // would trigger the build again and again, wasting Jenkins resources.
+      String commentId;
+      try {
+        commentId = postBuildStartCommentTo(pullRequest);
+      } catch (StashApiException e) {
+        logger.log(
+            Level.INFO,
+            format(
+                "%s: cannot post Build Start comment for pull request %s, not building",
+                job.getName(), pullRequest.getId()),
+            e);
+        continue;
+      }
+
       StashCause cause =
           new StashCause(
               trigger.getStashHost(),
@@ -315,7 +349,15 @@ public void addFutureBuildTasks(Collection<StashPullRequestResponseValue> pullRe
     }
   }
 
-  public void deletePullRequestComment(String pullRequestId, String commentId) {
+  /**
+   * Deletes pull request comment from Bitbucket Server
+   *
+   * @param pullRequestId pull request ID
+   * @param commentId comment to be deleted
+   * @throws StashApiException if deleting the comment fails
+   */
+  public void deletePullRequestComment(String pullRequestId, String commentId)
+      throws StashApiException {
     this.client.deletePullRequestComment(pullRequestId, commentId);
   }
 
@@ -359,14 +401,41 @@ public void postFinishedComment(
 
     comment = comment.concat(additionalComment);
 
-    this.client.postPullRequestComment(pullRequestId, comment);
+    // Post the "Build Finished" comment. Failure to post it can lead to
+    // scheduling another build for the pull request unnecessarily.
+    try {
+      this.client.postPullRequestComment(pullRequestId, comment);
+    } catch (StashApiException e) {
+      logger.log(
+          Level.WARNING,
+          format(
+              "%s: cannot post Build Finished comment for pull request %s",
+              job.getDisplayName(), pullRequestId),
+          e);
+    }
   }
 
-  public boolean mergePullRequest(String pullRequestId, String version) {
+  /**
+   * Instructs Bitbucket Server to merge pull request
+   *
+   * @param pullRequestId pull request ID
+   * @param version pull request version
+   * @return true if the merge succeeds, false if the server reports an error
+   * @throws StashApiException if cannot communicate to the server
+   */
+  public boolean mergePullRequest(String pullRequestId, String version) throws StashApiException {
     return this.client.mergePullRequest(pullRequestId, version);
   }
 
-  private boolean isPullRequestMergeable(StashPullRequestResponseValue pullRequest) {
+  /**
+   * Inquiries Bitbucket Server whether the pull request can be merged
+   *
+   * @param pullRequest pull request
+   * @return true if the merge is allowed, false otherwise
+   * @throws StashApiException if cannot communicate to the server
+   */
+  private boolean isPullRequestMergeable(StashPullRequestResponseValue pullRequest)
+      throws StashApiException {
     if (trigger.getCheckMergeable()
         || trigger.getCheckNotConflicted()
         || trigger.getCheckProbeMergeStatus()) {
@@ -454,8 +523,21 @@ private boolean isBuildTarget(StashPullRequestResponseValue pullRequest) {
       return false;
     }
 
-    if (!isPullRequestMergeable(pullRequest)) {
-      logger.info("Skipping PR: " + pullRequest.getId() + " as cannot be merged");
+    // Check whether the pull request can be merged and whether it's in the
+    // "conflicted" state. If that information cannot be retrieved, don't build
+    // the pull request in this cycle.
+    try {
+      if (!isPullRequestMergeable(pullRequest)) {
+        logger.info("Skipping PR: " + pullRequest.getId() + " as cannot be merged");
+        return false;
+      }
+    } catch (StashApiException e) {
+      logger.log(
+          Level.INFO,
+          format(
+              "%s: cannot determine if pull request %s can be merged, skipping",
+              job.getDisplayName(), pullRequest.getId()),
+          e);
       return false;
     }
 
@@ -473,6 +555,10 @@ private boolean isBuildTarget(StashPullRequestResponseValue pullRequest) {
     String destinationCommit = destination.getLatestCommit();
 
     String id = pullRequest.getId();
+
+    // Fetch all comments for the pull request. If it fails, don't build the
+    // pull request in this cycle, as it cannot be determined if it should be
+    // built without checking the comments.
     List<StashPullRequestComment> comments;
     try {
       comments = client.getPullRequestComments(owner, repositoryName, id);