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.

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

Support stopOnError for powershell scripts #198

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Support stopOnError for powershell scripts #198

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
f902cf2
Support stopOnError for PS
mbaitelman-bitburst-ops Jan 20, 2022
d9d912f
Updated and added additional tests
mbaitelman-bitburst-ops Jan 24, 2022
06ef199
Update tests to split methods
mbaitelman-bitburst-ops Jan 24, 2022
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
9 changes: 9 additions & 0 deletions src/main/java/org/jenkinsci/plugins/workflow/steps/durable_task/PowershellScriptStep.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,18 +29,27 @@
import org.jenkinsci.plugins.durabletask.DurableTask;
import org.jenkinsci.plugins.durabletask.PowershellScript;
import org.kohsuke.stapler.DataBoundConstructor;
import org.kohsuke.stapler.DataBoundSetter;

/**
* Asynchronous batch script execution.
*/
public class PowershellScriptStep extends DurableTaskStep {

private final String script;
private boolean stopOnError = false;

@DataBoundSetter public void setStopOnError(boolean stopOnError) {
this.stopOnError = stopOnError;
}

@DataBoundConstructor public PowershellScriptStep(String script) {
if (script == null) {
throw new IllegalArgumentException();
}
if(this.stopOnError){
script = "$ErrorActionPreference=\"Stop\"" + script;
}
Comment on lines +50 to +52
Copy link
Member

@dwnusbaum dwnusbaum Jan 25, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Does this do anything? The constructor will run before setStopOnError could be called, so I think this branch is never entered. That said, if your new tests pass, then maybe the existing behavior is already fine and this PR is not needed, or maybe your tests don't quite cover the right scenario.

Also more generally, I don't think you should modify the script like this here. I think the right way to do this would be to modify powershellHelper.ps1 (or do something like this) over in durable-task plugin and expose a new @DataBoundSetter on PowershellScript, and then consume that from this plugin.

(I don't know enough about Powershell to comment on the behavior of ErrorActionPreference itself).

Copy link
Collaborator

Choose a reason for hiding this comment

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

Good points. If I had to choose, I'd go with the second option
powershellHelper.ps1 does not exist in the binary version of durabletask, so you would lose support there.

this.script = script;
}

Expand Down
16 changes: 16 additions & 0 deletions src/test/java/org/jenkinsci/plugins/workflow/steps/durable_task/PowerShellStepTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,6 +66,22 @@ public class PowerShellStepTest {
j.assertLogContains("bogus error", j.assertBuildStatus(Result.FAILURE, p.scheduleBuild2(0)));
}

// Test that a powershell step that fails indeed causes the underlying build to fail with stopOnError: true
@Test public void testStopOnFailure() throws Exception {
WorkflowJob p = j.jenkins.createProject(WorkflowJob.class, "stopOnError");
p.setDefinition(new CpsFlowDefinition("node {powershell( script: 'throw \"bogus error\"', stopOnError: true) powershell( script: 'Write-output \"We made it past the error\"') }", true));
j.assertLogNotContains("We made it past the error", j.assertBuildStatus(Result.FAILURE, p.scheduleBuild2(0)));
j.assertLogContains("bogus error", j.assertBuildStatus(Result.FAILURE, p.scheduleBuild2(0)));
}

// Test that a powershell step that fails does not cause the underlying build to fail with stopOnError: false
@Test public void testStopOnFailureFalse() throws Exception {
WorkflowJob p = j.jenkins.createProject(WorkflowJob.class, "stopOnErrorFalse");
p.setDefinition(new CpsFlowDefinition("node {powershell( script: 'throw \"bogus error\"', stopOnError: false) powershell( script: 'Write-output \"We made it past the error\"') }", true));
j.assertLogContains("We made it past the error", j.assertBuildStatus(Result.FAILURE, p.scheduleBuild2(0)));
j.assertLogContains("bogus error", j.assertBuildStatus(Result.FAILURE, p.scheduleBuild2(0)));
}

@Test public void testUnicode() throws Exception {
WorkflowJob p = j.jenkins.createProject(WorkflowJob.class, "foobar");
p.setDefinition(new CpsFlowDefinition("node {def x = powershell(returnStdout: true, script: 'write-output \"Hëllö Wórld\"'); println x.replace(\"\ufeff\",\"\")}", true));
Expand Down