Throw exceptions on deptrack failures #9
Conversation
status (and not notifying caller of failure). Allows pipeline builds to both abort on deptrack failure, as well as catch and suppress deptrack errors, if desired. Make ApiClientException an IOException, and don't catch it anymore, either.
| public class ApiClientException extends Exception { | ||
| import java.io.IOException; | ||
|
|
||
| public class ApiClientException extends IOException { |
There was a problem hiding this comment.
The BuildStep.perform() method expects IOExceptions to be thrown, and as ApiClientExceptions seem like IOExceptions, marking it as such reduces the need for special handling.
| @@ -15,6 +15,7 @@ | |||
| */ | |||
| package org.jenkinsci.plugins.DependencyTrack; | |||
|
|
|||
| import hudson.AbortException; | |||
There was a problem hiding this comment.
Per the documentation in BuildStep.perform(), an AbortException should be thrown if an error has been handled and diagnosed by the build step. Using AbortException (apparently) suppresses the printing of a stack trace to the build output. Other exceptions that cause build aborts cause a stack trace to be a dumped.
| @@ -178,91 +179,90 @@ public void perform(@Nonnull final Run<?, ?> build, | |||
|
|
|||
| if (StringUtils.isBlank(artifact)) { | |||
| logger.log(Messages.Builder_Artifact_Unspecified()); | |||
| build.setResult(Result.FAILURE); | |||
| return; | |||
| throw new AbortException("Artifact not specified"); | |||
There was a problem hiding this comment.
similar pattern here and below. Rather than setting build status and not informing the pipeline flow, throw exceptions instead.
| } | ||
| try { |
There was a problem hiding this comment.
remove this try/catch block. Just let exceptions, including ApiClientExceptions, percolate up to the build step runner to be handled.
| if (timeout < System.currentTimeMillis()) { | ||
| logger.log(Messages.Builder_Polling_Timeout_Exceeded()); | ||
| // XXX this seems like a fatal error | ||
| throw new AbortException("Dependency Track server response timeout"); |
There was a problem hiding this comment.
It seemed strange that this wasn't being treated as a build failure. If we don't hear back from the deptrack server about whether this build has CVEs or not, seems like we should halt the build.
| throw new AbortException("Severity distribution failure thresholds exceeded"); | ||
| } else { | ||
| // allow build to proceed, but mark overall build unstable | ||
| build.setResult(result); |
There was a problem hiding this comment.
The only place where I think it's valid to set the build result is when setting it to UNSTABLE. This will allow the build to proceed, but it will be marked unstable when complete.
Switch to throw exceptions rather than unilaterally setting build status
(and not notifying caller of failure). Allows pipeline builds
to both abort on deptrack failure, as well as catch and suppress
deptrack errors, if desired. Make ApiClientException an IOException,
and don't catch it anymore, either.