Skip to content
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.

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

S3BucketPublisher: handle InterruptedException #92

Merged
merged 1 commit into from Jul 19, 2016

Conversation

Projects
None yet
2 participants
@jlebon
Copy link

commented Jul 18, 2016

Previously, if the source file patterns given to the S3 publisher
matched no files, we tried to be nice and called validateAntFileMask(),
which gave a nicely formatted message in case some other pattern would
have matched.

However, if even validateAntFileMask() fails to match anything within
the bound limit, then we would get a big fat InterruptedException that
kills the whole publisher and marks the build as a failure.

This patch makes this consistent by just ignoring the exception if
nothing was found. IOW, if we have nothing to suggest to the user to
help them diagnose the issue, that shouldn't be a reason to die.

@Jimilian

This comment has been minimized.

Copy link

commented Jul 19, 2016

@jlebon , thanks for your PR! Did you test new behaviour? I'm just curious because I'm not quite sure that empty paths work correctly.

if (error != null) {
log(listener.getLogger(), error);
}
} catch (InterruptedException e) {

This comment has been minimized.

Copy link
@Jimilian

Jimilian Jul 19, 2016

Please, use ignored instead of e, if you don't want to process exception.

@@ -187,9 +187,15 @@ public void perform(@Nonnull Run<?, ?> run, @Nonnull FilePath ws, @Nonnull Launc
if (paths.isEmpty()) {
// try to do error diagnostics
log(listener.getLogger(), "No file(s) found: " + expanded);

This comment has been minimized.

Copy link
@Jimilian

Jimilian Jul 19, 2016

Please, extract this code branch to new private method - something like 'printDiagnostics'.

@@ -187,9 +187,15 @@ public void perform(@Nonnull Run<?, ?> run, @Nonnull FilePath ws, @Nonnull Launc
if (paths.isEmpty()) {

This comment has been minimized.

Copy link
@Jimilian

Jimilian Jul 19, 2016

What do you think - maybe it's better to continue in this case (after printing feedback)?

S3BucketPublisher: handle InterruptedException
Previously, if the source file patterns given to the S3 publisher
matched no files, we tried to be nice and called validateAntFileMask(),
which gave a nicely formatted message in case some other pattern would
have matched.

However, if even validateAntFileMask() fails to match anything within
the bound limit, then we would get a big fat InterruptedException that
kills the whole publisher and marks the build as a failure.

This patch makes this consistent by just ignoring the exception if
nothing was found. IOW, if we have nothing to suggest to the user to
help them diagnose the issue, that shouldn't be a reason to die.

@jlebon jlebon force-pushed the jlebon:pr/fix-interrupted-exp branch from d9f80f4 to 9aebf41 Jul 19, 2016

@jlebon

This comment has been minimized.

Copy link
Author

commented Jul 19, 2016

⬆️ New commit with fixups. I didn't factor out the log lines, but did abbreviate them somehow since it seems like that's what done elsewhere in the codebase.

Did you test new behaviour?

Yes, tested and working.

I'm just curious because I'm not quite sure that empty paths work correctly.

I think it was essentially a no-op before, but I made it a continue to make it more explicit.

@Jimilian

This comment has been minimized.

Copy link

commented Jul 19, 2016

I was talking about something like this:

if (paths.isEmpty()) {
    printDiagnostics(console, ws, expanded);
    continue;
}

But nevermind, I will do this microrefactoring by myself.

@Jimilian Jimilian merged commit 86a32de into jenkinsci:master Jul 19, 2016

1 check passed

Jenkins This pull request looks good
Details
@jlebon

This comment has been minimized.

Copy link
Author

commented Jul 19, 2016

Ahhh gotcha, missed that.
Thanks! 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.