Skip to content

Handle failure in publish invocation #83

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
mrginglymus wants to merge 3 commits into from
Closed

Handle failure in publish invocation #83

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
69d14a5
Handle failure in publish invocation
mrginglymus Feb 13, 2021
55790f5
Only call onFailure if we know we haven't already
mrginglymus Feb 13, 2021
f88c550
Alternatively, this
mrginglymus Feb 13, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
40 changes: 26 additions & 14 deletions src/main/java/io/jenkins/plugins/checks/steps/WithChecksStep.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
package io.jenkins.plugins.checks.steps;

import edu.hm.hafner.util.VisibleForTesting;
import edu.umd.cs.findbugs.annotations.CheckForNull;
import edu.umd.cs.findbugs.annotations.NonNull;
import hudson.Extension;
import hudson.model.Run;
Expand Down Expand Up @@ -108,15 +109,17 @@ ChecksInfo extractChecksInfo() {

@Override
public void stop(final Throwable cause) {
if (publish(getContext(), new ChecksDetails.ChecksDetailsBuilder()
Exception publishFailure = publish(getContext(), new ChecksDetails.ChecksDetailsBuilder()
Copy link
Member

@timja timja Feb 13, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is a bit odd for java, normally you would throw the exception from the publish method and catch it here, possibly a known checked exception

.withName(step.getName())
.withStatus(ChecksStatus.COMPLETED)
.withConclusion(ChecksConclusion.CANCELED))) {
getContext().onFailure(cause);
.withConclusion(ChecksConclusion.CANCELED));
if (publishFailure != null) {
cause.addSuppressed(publishFailure);
}
getContext().onFailure(cause);
}

private boolean publish(final StepContext context, final ChecksDetails.ChecksDetailsBuilder builder) {
private @CheckForNull Exception publish(final StepContext context, final ChecksDetails.ChecksDetailsBuilder builder) {
TaskListener listener = TaskListener.NULL;
try {
listener = fixNull(context.get(TaskListener.class), TaskListener.NULL);
Expand All @@ -135,22 +138,25 @@ private boolean publish(final StepContext context, final ChecksDetails.ChecksDet
String msg = "Failed getting Run from the context on the start of withChecks step: " + e;
pluginLogger.log(msg);
SYSTEM_LOGGER.log(Level.WARNING, msg);
context.onFailure(new IllegalStateException(msg));
return false;
return new IllegalStateException(msg);
}

if (run == null) {
String msg = "No Run found in the context.";
pluginLogger.log(msg);
SYSTEM_LOGGER.log(Level.WARNING, msg);
context.onFailure(new IllegalStateException(msg));
return false;
return new IllegalStateException(msg);
}

ChecksPublisherFactory.fromRun(run, listener)
.publish(builder.withDetailsURL(DisplayURLProvider.get().getRunURL(run))
.build());
return true;
try {
ChecksPublisherFactory.fromRun(run, listener)
.publish(builder.withDetailsURL(DisplayURLProvider.get().getRunURL(run))
.build());
return null;
}
catch (RuntimeException e) {
Copy link
Contributor

@XiongKezhi XiongKezhi Feb 14, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is this catch needed? if u are catching the exception outside this whole function (like in line 112) instead of returning the exception, this is not needed?

return e;
}
}

class WithChecksCallBack extends BodyExecutionCallback {
Expand All @@ -166,10 +172,13 @@ class WithChecksCallBack extends BodyExecutionCallback {

@Override
public void onStart(final StepContext context) {
publish(context, new ChecksDetails.ChecksDetailsBuilder()
Exception publishFailure = publish(context, new ChecksDetails.ChecksDetailsBuilder()
.withName(info.getName())
.withStatus(ChecksStatus.IN_PROGRESS)
.withConclusion(ChecksConclusion.NONE));
if (publishFailure != null) {
context.onFailure(publishFailure);
}
}

@Override
Expand Down Expand Up @@ -203,7 +212,10 @@ public void onFailure(final StepContext context, final Throwable t) {
.withTitle("Failed")
.withText(t.toString()).build());
}
publish(context, builder);
Exception publishFailure = publish(context, builder);
if (publishFailure != null) {
t.addSuppressed(publishFailure);
}
context.onFailure(t);
}
}
Expand Down