[Fixed JENKINS-49754] - Prevent PowerShell stdout pollution when using returnStdout #64
Conversation
gabloe
changed the title
| @@ -124,7 +124,7 @@ | |||
| } | |||
|
|
|||
| @Test public void implicitError() throws Exception { | |||
| Controller c = new PowershellScript("MyBogus-Cmdlet").launch(new EnvVars(), ws, launcher, listener); | |||
| Controller c = new PowershellScript("$ErrorActionPreference = 'Stop'; MyBogus-Cmdlet").launch(new EnvVars(), ws, launcher, listener); | |||
There was a problem hiding this comment.
does this mean that people have to change all their scripts as this code was removed?
There was a problem hiding this comment.
No. No changes should be required here. The issue is that regardless of the value of $ErrorActionPreference or the ErrorAction parameter provided by all cmdlets a job would fail even if the error was non fatal. Fatal errors should be controlled via ErrorAction, and this is the standard practice in PowerShell. So, this is definitely a bug.
| c.writeLog(ws, baos); | ||
| assertTrue(c.exitStatus(ws, launcher, listener).intValue() == 0); | ||
| assertThat(baos.toString(), containsString("Hello, World!")); | ||
| assertEquals("Success\r\n", new String(c.getOutput(ws, launcher))); |
There was a problem hiding this comment.
does powershell on Linux use \r\n or just \n (ie will this test fail if run on Linux?)
There was a problem hiding this comment.
Good point. I will verify this and update the PR if needed.
There was a problem hiding this comment.
PowerShell on Linux uses \n. I'll update these tests.
There was a problem hiding this comment.
In a related note it would be nice if PowerShell core could be installed on the Linux VM's used in testing with a symlink added for powershell->pwsh, because as it stands right now the tests are skipped on Linux.
There was a problem hiding this comment.
@gabloe That can be raised as a Jenkins INFRA ticket for action by the community infra maintainers. Be aware though that turnaround times may very because the number of people who can handle those tickets is limited and their time is heavily committed usually.
| c.cleanup(ws); | ||
| } | ||
|
|
||
| @Test public void verboseNegativeTest() throws Exception { |
There was a problem hiding this comment.
how is this a negative test (negative tests generally check for error handling)?
There was a problem hiding this comment.
That's right. This is poorly named. I am trying to ensure that when using returnStdout another PowerShell output stream does not get mixed in. I'll rename the test.
|
Was there any other changes left for this before it can be merged? We're looking to upgrade a number of other plugins in our Jenkins environment for bug fixes, but due to the dependency matrix between them, we're kind of stuck until we have a working version of the Durable Task Plugin. |
|
I'd like to see the automated tests come back successful. It looks like the Linux tests (non-powershell) are aborting early, which is causing an early failure of all parallel branches. @svanoort do you know why this is happening? |
|
I'd like to see a clean test pass too and a confirmation from @jtnord that this looks sane. I don't feel comfortable signing off on Powershell logic myself due to lack of familiarity (I'm heavily rooted in Linux land and more likely to use the new Linux subsystem for Windows). @gabloe
|
|
@jakauppila For the same reasons why it's important to you that Durable tasks be reliable, we're generally cautious about merging changes. Please have patience -- a slower but stable release of Durable Task is much better than cutting frequent but buggy releases. |
|
@svanoort That failure likely has to do with the bugfix that I've added to rely on ErrorActionPreference when failing the task. The frustration with this particular change is that PowerShell 5 has a different behavior from PowerShell 3-4, and $LASTEXITCODE can be 0 even if an exception has been thrown, and if an exception has been thrown the exception may not be exposed in the wrapping PowerShell script depending on the version of PowerShell. So in PowerShell 4 the process may have failed and $LASTEXITCODE could be zero and the thrown exception may be hidden, which results in a false positive. I will revert that bugfix and bring it in as a separate PR. |
|
The error handling bug fix has been sent in another PR: |
|
Thanks @jtnord ! Okay, I'm going ahead and merging, and will aim to cut a release soonishly. |
|
Thanks for all your help everyone, really appreciate it! |
PowerShell output consists of 6 separate streams (success, error, warning, debug, verbose, and information), but only the success stream is conventionally considered to be stdout. The usage of returnStdout option of the PowerShell pipeline step should disregard all streams except the success stream to be consistent with other pipeline steps (bat, sh, etc.)
This PR prevents the warning, debug, verbose, or information streams from polluting stdout.
Caveats:
Normally in PowerShell when you use one of the various Write-* cmdlets, such as Write-Verbose or Write-Warning, the stream designation is prepended to the output. For example:
Write-Verbose "This is some verbose output"Would yield:
There is a known issue when you invoke a PowerShell script and redirect the various streams to disk where this stream designation is omitted from the output. The solution for this is to wrap the execution of the script, which causes all of the non-error streams to be merged with stdout along with their corresponding designations. However, this adds the side effect of treating all streams as normal stdout, which is undesirable when using returnStdout. So, with this change the stream designations will not be provided in the console log if using returnStdout. If not using returnStdout then the behavior remains the same as it is now.
I've also added a few extra unit tests to ensure that verbose, warning, and debug output continue to be correct.