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
[JENKINS-52165] Disable watch mode by default for now #84
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -31,6 +31,7 @@ | |
import hudson.FilePath; | ||
import hudson.Functions; | ||
import hudson.Launcher; | ||
import hudson.Main; | ||
import hudson.Util; | ||
import hudson.model.Node; | ||
import hudson.model.TaskListener; | ||
|
@@ -170,7 +171,7 @@ interface ExecutionRemotable { | |
/** If set to false, disables {@link Execution#watching} mode. */ | ||
@SuppressFBWarnings(value = "MS_SHOULD_BE_FINAL", justification = "public & mutable only for tests") | ||
@Restricted(NoExternalUse.class) | ||
public static boolean USE_WATCHING = !"false".equals(System.getProperty(DurableTaskStep.class.getName() + ".USE_WATCHING")); | ||
public static boolean USE_WATCHING = Boolean.parseBoolean(System.getProperty(DurableTaskStep.class.getName() + ".USE_WATCHING", Main.isUnitTest ? "true" : /* JENKINS-52165 turn back on by default */ "false")); | ||
|
||
/** | ||
* Represents one task that is believed to still be running. | ||
|
@@ -466,14 +467,7 @@ private void check() { | |
if (controller.writeLog(workspace, listener.getLogger())) { | ||
LOGGER.log(Level.FINE, "last-minute output in {0} on {1}", new Object[] {remote, node}); | ||
} | ||
if (returnStatus || exitCode == 0) { | ||
getContext().onSuccess(returnStatus ? exitCode : returnStdout ? new String(controller.getOutput(workspace, launcher()), StandardCharsets.UTF_8) : null); | ||
} else { | ||
if (returnStdout) { | ||
listener.getLogger().write(controller.getOutput(workspace, launcher())); // diagnostic | ||
} | ||
getContext().onFailure(new AbortException("script returned exit code " + exitCode)); | ||
} | ||
handleExit(exitCode, () -> controller.getOutput(workspace, launcher())); | ||
recurrencePeriod = 0; | ||
controller.cleanup(workspace); | ||
} | ||
|
@@ -507,12 +501,20 @@ private void check() { | |
return; | ||
} | ||
recurrencePeriod = 0; | ||
handleExit(exitCode, () -> output); | ||
} | ||
|
||
@FunctionalInterface | ||
private interface OutputSupplier { | ||
byte[] produce() throws IOException, InterruptedException; | ||
} | ||
private void handleExit(int exitCode, OutputSupplier output) throws IOException, InterruptedException { | ||
Throwable originalCause = causeOfStoppage; | ||
if ((returnStatus && originalCause == null) || exitCode == 0) { | ||
getContext().onSuccess(returnStatus ? exitCode : returnStdout ? new String(output, StandardCharsets.UTF_8) : null); | ||
getContext().onSuccess(returnStatus ? exitCode : returnStdout ? new String(output.produce(), StandardCharsets.UTF_8) : null); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🐜 Nested ternary operators are super hard to read for most developers without line breaks between them There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Perhaps. Anyway that predates this PR. |
||
} else { | ||
if (returnStdout) { | ||
listener().getLogger().write(output); // diagnostic | ||
listener().getLogger().write(output.produce()); // diagnostic | ||
} | ||
if (originalCause != null) { | ||
// JENKINS-28822: Use the previous cause instead of throwing a new AbortException | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -541,7 +541,7 @@ private Object writeReplace() { | |
j.waitForCompletion(b); | ||
// Would have succeeded before https://github.com/jenkinsci/workflow-durable-task-step-plugin/pull/75. | ||
j.assertBuildStatus(Result.ABORTED, b); | ||
j.assertLogContains("Timeout has been exceeded", b); | ||
j.waitForMessage("Timeout has been exceeded", b); // TODO assertLogContains fails unless a sleep is introduced; possible race condition in waitForCompletion | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I saw this as well, although it passed sometimes in my local testing, so some kind of race condition seems likely. |
||
} | ||
|
||
/** | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems undesirable to not actually test using the mode that we are shipping to users, but I also see why it would be good to test against watch mode since any issues in downstream plugins are likely to be specific to that mode, so I am somewhat split on this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indeed. On the other hand the non-watch mode has been tested for years, and I certainly hope this situation will be temporary.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm okay with this part -- not perfect but it'll do. Just trying to check that the exit condition stuff is 100% the same internally.