-
-
Notifications
You must be signed in to change notification settings - Fork 130
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
Fixing Required Attachments #419
base: release/2.15-alpha
Are you sure you want to change the base?
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.
I added a few inline comments in your code.
Additionally, please consider:
- I didn't test it, but I am reasonably confident, that the 400 response contains the information why the request failed (missing required fields). Why can't we use this information for our output? Adding our own business logic for identifying why a request failed should be avoided as long as the information returned by the API is not good enough. As I don't know how the response looks like (without looking it up), please state your case why you deem necessary to add this logic to the module.
- I have an improvement for the error handling staged on my local machine to improve it a bit. take a look if your code could benefit from it (I didn't check):
$allErrors = ($responseObject.errorMessages + $responseObject.message + $responseObject.errors)
if ($allErrors.Count -eq 0) {
throw "Unable to handle error"
}
foreach ($_error in $allErrors) {
if ($_error.Count -gt 0) {
$writeErrorSplat = @{
Exception = $exception
ErrorId = $errorId
Category = $errorCategory
Message = Out-String -InputObject $_error
TargetObject = $targetObject
Cmdlet = $Cmdlet
}
WriteError @writeErrorSplat
}
}
- You must still update the existing tests to pass with these changes
- You should add new tests (with a mocked 400 response) showing how these errors are handled
|
||
if ($errorArray.count -gt 0) { | ||
$errorParameter = @{ | ||
ExceptionType = "System.Net.Http.HttpRequestException" |
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.
Please use the same exception type as Invoke-WebRequest returns
(System.Net.WebException
for PSVersion -lt 6 and Microsoft.PowerShell.Commands.HttpResponseException
for PSVersion -ge 6)
@@ -44,7 +44,29 @@ function Resolve-ErrorWebResponse { | |||
try { | |||
$responseObject = ConvertFrom-Json -InputObject $responseBody -ErrorAction Stop | |||
|
|||
$errorArray = @() |
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.
Please use [System.Collections.ArrayList]
Reading material on why: https://gist.github.com/kevinblumenfeld/4a698dbc90272a336ed9367b11d91f1c
foreach ($_error in ($responseObject.errorMessages + $responseObject.errors)) { | ||
|
||
foreach ($_err in $_error.PSObject.Properties.Value) { | ||
if ($_err -like "*You must specify a summary of the issue." -Or $_err -like "*is required.") { |
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.
I don't trust parsing text from the answer. What happens when the system is installed in another language?
(Unfortunately I can't test that)
Is there any other way we can identify this?
Category = "AuthenticationError" | ||
Cmdlet = $Cmdlet | ||
} | ||
ThrowError @errorParameter |
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.
You shouldn't use throw
here. Throw
will cause the entire script to fail when this happens (unless used with try {} catch {}
.
Please use WriteError
(can have the same behavior if wanted, by adding -ErrorAction Stop
to the script)
Description
closes #336
Types of changes
Checklist