-
Notifications
You must be signed in to change notification settings - Fork 3.9k
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.
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
Fix ErrorAction issue #8399
Fix ErrorAction issue #8399
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, we should have a unit test to verify both that expected errors are written to the error stream and terminating error are thrown.
@@ -423,7 +423,8 @@ private void HandleException(ExceptionDispatchInfo capturedException) | |||
var errorResponseException = capturedException.SourceException as ErrorResponseMessageException; | |||
if (errorResponseException != null) | |||
{ | |||
this.ThrowTerminatingError(errorResponseException.ToErrorRecord()); | |||
this.WriteError(errorResponseException.ToErrorRecord()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So, there are cases in which we will want to throw a terminating exception (such as PipelineStoppedException)
If this is extending AzurePSCmdlet and usign the default ProcessRecord implementation, we can make use of it's default error handling, and simply throw these exceptions (it will write them to the error stream): https://github.com/Azure/azure-powershell-common/blob/master/src/Common/AzurePSCmdlet.cs#L735.
Otherwise, we want to throw any exception that is a terminating exception and write others to the error stream. We can use the inherited IsTerminatingException method to do this.
@markcowl Looks like I can throw and the ErrorAction preference is working corrects (this base class inherits from AzurePSCmdlet and does not override ProcessRecord). Do we still need unit tests specifically for Resources now that we are using the common implementation? |
I believe we already have tests for the default implementation of this, so this is fine |
Description
#8235
Checklist
CONTRIBUTING.md
platyPS
module