Support stopOnError for powershell scripts #198
Conversation
|
I was having trouble with this param until I figured out that this plugin isn't https://github.com/jenkinsci/powershell-plugin which is mentioned everywhere else. |
|
Id rather this not be a blocker but if this is accepted the docs will need to be updated as well. |
| // Test that a powershell step that fails indeed causes the underlying build to fail with stopOnFailure: true | ||
| @Test public void testStopOnFailure() throws Exception { | ||
| WorkflowJob p = j.jenkins.createProject(WorkflowJob.class, "stopfailure"); | ||
| p.setDefinition(new CpsFlowDefinition("node {powershell( script: 'throw \"bogus error\"', stopOnFailure: true)}", true)); |
There was a problem hiding this comment.
Maybe make the job do more after the error step (like print a line) and verify that the subsequent actions do not appear
|
Looks good! I really do think though we need to add some documentation. @dwnusbaum If we are updating the documentation to include this label for just the |
| if(this.stopOnError){ | ||
| script = "$ErrorActionPreference=\"Stop\"" + script; | ||
| } |
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
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.
|
@car-roll yeah, if you add new parameters, they need to be added to |
Add support for stopOnError to handle PS errors