fix(webhooks): give better error message for monitored webhooks (spin…

…naker#2635) Instead of bubbling semi-useless error message verbatim from JsonPath Make it relevant and clear to the end user why monitoring for webhook failed if the specified json path is not valid
sicp618 · Jan 31, 2019 · d9501fa · d9501fa
1 parent 730f6ec
commit d9501fa
Show file tree

Hide file tree

Showing 2 changed files with 4 additions and 3 deletions.
diff --git a/...ebhook/src/main/groovy/com/netflix/spinnaker/orca/webhook/tasks/MonitorWebhookTask.groovy b/...ebhook/src/main/groovy/com/netflix/spinnaker/orca/webhook/tasks/MonitorWebhookTask.groovy
@@ -38,6 +38,7 @@ class MonitorWebhookTask implements OverridableTimeoutRetryableTask {
 
   long backoffPeriod = TimeUnit.SECONDS.toMillis(1)
   long timeout = TimeUnit.HOURS.toMillis(1)
+  private static final String JSON_PATH_NOT_FOUND_ERR_FMT = "Unable to parse %s: JSON property '%s' not found in response body"
 
   @Override
   long getDynamicBackoffPeriod(Stage stage, Duration taskDuration) {
@@ -104,7 +105,7 @@ class MonitorWebhookTask implements OverridableTimeoutRetryableTask {
     try {
       result = JsonPath.read(response.body, statusJsonPath)
     } catch (PathNotFoundException e) {
-      responsePayload.webhook.monitor << [error: e.message]
+      responsePayload.webhook.monitor << [error: String.format(JSON_PATH_NOT_FOUND_ERR_FMT, "status", statusJsonPath)]
       return new TaskResult(ExecutionStatus.TERMINAL, responsePayload)
     }
     if (!(result instanceof String || result instanceof Number || result instanceof Boolean)) {
@@ -117,7 +118,7 @@ class MonitorWebhookTask implements OverridableTimeoutRetryableTask {
       try {
         progress = JsonPath.read(response.body, progressJsonPath)
       } catch (PathNotFoundException e) {
-        responsePayload.webhook.monitor << [error: e.message]
+        responsePayload.webhook.monitor << [error: String.format(JSON_PATH_NOT_FOUND_ERR_FMT, "progress", statusJsonPath)]
         return new TaskResult(ExecutionStatus.TERMINAL, responsePayload)
       }
       if (!(progress instanceof String)) {

diff --git a/...ok/src/test/groovy/com/netflix/spinnaker/orca/webhook/tasks/MonitorWebhookTaskSpec.groovy b/...ok/src/test/groovy/com/netflix/spinnaker/orca/webhook/tasks/MonitorWebhookTaskSpec.groovy
@@ -176,7 +176,7 @@ class MonitorWebhookTaskSpec extends Specification {
 
     then:
     result.status == ExecutionStatus.TERMINAL
-    result.context.webhook.monitor == [error: 'Missing property in path $[\'doesnt\']', body: [status: "SUCCESS"], statusCode: HttpStatus.OK, statusCodeValue: HttpStatus.OK.value()]
+    result.context.webhook.monitor == [error: 'Unable to parse status: JSON property \'$.doesnt.exist\' not found in response body', body: [status: "SUCCESS"], statusCode: HttpStatus.OK, statusCodeValue: HttpStatus.OK.value()]
   }
 
   def "should return TERMINAL status if jsonPath isn't evaluated to single value"() {