Fix alerting rule update crash for 4xx HTTP errors. #649
Conversation
|
|
||
| // The method stubs are generated initially by the VS Code Quick Fix for the below Blank Identifier definition. | ||
| // https://stackoverflow.com/a/77393824 | ||
| var _ alerting.AlertingAPI = (*FakeAlertingAPI)(nil) |
There was a problem hiding this comment.
Can we use gomock or similar to generate a proper mock implementation rather than this?
| if diags := utils.CheckHttpError(res, "Unable to update alerting rule"); diags.HasError() { | ||
| return nil, diags | ||
| } | ||
|
|
||
| shouldBeEnabled := rule.Enabled != nil && *rule.Enabled | ||
|
|
||
| if shouldBeEnabled && !ruleRes.Enabled { |
There was a problem hiding this comment.
We're assuming here that anything where ruleRes == nil will be caught by utils.CheckHttpError above? I wonder if it's worth having an explicit check as well?
There was a problem hiding this comment.
We're assuming here that anything where ruleRes == nil will be caught by utils.CheckHttpError above?
I'm not sure actually - CheckHttpError just checks HTTP status code.
There was a problem hiding this comment.
I've added logic to explicitly handle an empty response with HTTP 200.
There was a problem hiding this comment.
Sorry - the new logic just returns an error in case of an empty response for Create/Update call, HTTP status code doesn't make any diff in this case.
Still in reality, CheckHttpError should handle the cases when something goes wrong based on the HTTP status code. The new logic just makes sure that crash won't happen.
Handle an empty response with HTTP 200 in CreateRule and UpdateRule.
|
@tobio , I addressed the comments, could you take a look at it again when you have time? |
This fixed the problem for
CreateRule.This PR should fix it for
UpdateRule.Also it adds unit tests to cover the case.
Rel: #647