Repository navigation
Don't wrap stderr as ErrorRecord - #5190
Steve Lee (SteveL-MSFT) wants to merge 2 commits into
Conversation
Steve Lee (SteveL-MSFT)
added
the
Breaking-Change
There was a problem hiding this comment.
This is being done in #5189. It shouldn't also be in this PR, I don't think.
There was a problem hiding this comment.
I'll remove it, had to change it locally to debug this.
Steve Lee (SteveL-MSFT)
changed the title
updated tests
6804619 to
57c2e19
Compare
What exactly do you mean? Would any |
|
sergei (@vors) I was partially thinking of #3415, but thats more for exit codes, but also noting that currently because stderr is wrapped as error record, you can set erroraction = stop and catch it, but with this change you can’t do that anymore. |
| this.OutputPipe.Add(sendToPipeline); | ||
| } | ||
|
|
||
| internal void _WriteErrorSkipAllowCheck(object sendToPipeline) |
There was a problem hiding this comment.
Please add a comment.
There was a problem hiding this comment.
Will add
| Dbg.Assert(record != null, "ProcessReader should ensure that data is ErrorRecord"); | ||
| record.SetInvocationInfo(this.Command.MyInvocation); | ||
| this.commandRuntime._WriteErrorSkipAllowCheck(record, isNativeError: true); | ||
| this.commandRuntime._WriteErrorSkipAllowCheck(outputValue.Data); |
There was a problem hiding this comment.
Please add a comment stating that it is the intent to not wrap native stderror in an ErrorRecord.
There was a problem hiding this comment.
will add
| { | ||
| $ErrorActionPreference = $origEA | ||
| } | ||
| It "Verify Validate Dollar Error Populated should be in stderr" { |
There was a problem hiding this comment.
Should we also have a test for stream redirection to output?
$err = & $powershell -noprofile -command { wgwg-wrwrhqwrhrh35h3h3} 2>&1
There was a problem hiding this comment.
I can add that
Paul Higinbotham (PaulHigin)
left a comment
Paul Higinbotham (PaulHigin)
left a comment
There was a problem hiding this comment.
LGTM
| /// The Cmdlet should generally just allow PipelineStoppedException | ||
| /// to percolate up to the caller of ProcessRecord etc. | ||
| /// </exception> | ||
| internal void _WriteErrorSkipAllowCheck(object sendToPipeline) |
There was a problem hiding this comment.
Maybe name this WriteNativeCommandErrorSkipAllowCheck instead?
| record.SetInvocationInfo(this.Command.MyInvocation); | ||
| this.commandRuntime._WriteErrorSkipAllowCheck(record, isNativeError: true); | ||
| // write stderr as string instead of as ErrorRecord | ||
| this.commandRuntime._WriteErrorSkipAllowCheck(outputValue.Data); |
There was a problem hiding this comment.
outputValue.Data is still an ErrorRecord - right?
There was a problem hiding this comment.
You're right, outputValue.Data is still an ErrorRecord. I'll need to rework this.
5 participants
Keep stderr as string and don't wrap as ErrorRecord.
This also means that errors from running pwsh.exe within pwsh are now just stderr strings even though they visually look like ErrorRecords so tests had to be changed.
Most customers are likely not running powershell within powershell and expecting errors, but it does mean that try{}catch{} won't work (until we add the enhancement to throw on any stderr).
Fix #3813