(maint) Fix multiline stderr capture #150
Conversation
Iristyle
changed the title
- Prior to PowerShell 3, the behavior of the Write-Output cmdlet introduces newlines automatically based on the current console width. This behavior is not controllable in the host - as the custom PSHostUserInterface implementation will see multiple calls to WriteLine instead of just a single call to WriteLine. Since the captured output of stdout is used only for logging purposes within Puppet, modify the test to instead use Write-Host on PS2 and earlier versions. This does not introduce extraneous newlines into the original string value. Note that the line ending is also different when using Write-Host rather than Write-Output.
Iristyle
force-pushed
the
maint/stable/fix-multiline-stderr
branch
from
1385792 to
5079619
Compare
| @@ -89,7 +89,7 @@ def execute(powershell_code, timeout_ms = nil, working_dir = nil) | |||
| (prop.text.nil? ? nil : Base64.decode64(prop.text)) | |||
| # if err contains data it must be "real" stderr output | |||
| # which should be appended to what PS has already captured | |||
| value += err if err && (err != []) && (name == 'stderr') | |||
| value = (value || '') + err.join('') if err && (err != []) && (name == 'stderr') | |||
There was a problem hiding this comment.
Completely understand why though - to ensure that the value is non-nil.
It looks like there is a possibility of value.nil? being true still. Were you trying to remove this scenario so that it is at least always an empty string?
There was a problem hiding this comment.
If prop.text.nil?, value remains nil from lines 87-89.
If err is nil or the name doesn't equal stderr.
There was a problem hiding this comment.
Is the check for err != [] still necessary?
There was a problem hiding this comment.
I made a few more changes there after some more testing - would appreciate some more testing!
Iristyle
force-pushed
the
maint/stable/fix-multiline-stderr
branch
from
5079619 to
66fe44c
Compare
| value += err if err && (err != []) && (name == 'stderr') | ||
| if name == 'stderr' | ||
| value = value.nil? ? [] : [value] | ||
| value += err if !err.nil? |
There was a problem hiding this comment.
I made notes in commit message... our previous behavior was to pass an array of strings up through "stderr" - but I accidentally broke it in fa3d618. There were no tests in the provider to catch it.
We should probably rethink that at some point (to just use a string value everywhere, like we do for stdout) - but I'm trying to preserve what we were doing.
- Previous code made two bad assumption that could result in the module crashing when anything is written to the stderr stream via native executables or through [System::Console]::Error. The first assumption in the manager is that the XML includes a non-nil value passed back in the `stderr` element. It's completely valid to have no value present. The second assumption was that the captured stderr is a bare string. It will never be a bare string and should always be a single element string array at the very least. Additionally, code had incorrectly changed in fa3d618 breaking the contract of returning a stderr array from the manager to the calling provider. There were unfortunately no tests that caught this breakage, so one has been added. An additional test was added to produce multiline stderr output. - Note that v1 of the PowerShell module collects output in a different fashion and therefore the provider test handles this.
Iristyle
force-pushed
the
maint/stable/fix-multiline-stderr
branch
from
66fe44c to
b90fe66
Compare
Previous code made two bad assumption that could result in the
module crashing when anything is written to the stderr stream via
native executables.
The first assumption is that there is a non-nil value passed back in
the XML response
stderrelement. It's completely valid to have nopre-existing value present.
The second assumption was that the captured stderr was a bare string.
It will never be a bare string and should always be a single element
string array at the very least.
Adjust the code to handle these situations accordingly and introduce
a new test that creates multiline stderr output.