Unify http/non-http result handling #729
Conversation
Codecov Report
@@ Coverage Diff @@
## master #729 +/- ##
==========================================
+ Coverage 40.1% 40.18% +0.07%
==========================================
Files 69 69
Lines 3922 3932 +10
==========================================
+ Hits 1573 1580 +7
- Misses 2247 2251 +4
+ Partials 102 101 -1
Continue to review full report at Codecov.
|
|
@stevesloka Sanity check on this? The diff looks bigger than it logically is. I did the following:
No tests changed and no logical change in behavior intended. Follow up PR will modify things and add new tests. |
We have two flows for this logic: one for the HTTP server and one for the channel monitoring results. This change places all the logic into a central method so that it can be modified/tested more confidently/succinctly. Signed-off-by: John Schnake <[email protected]>
johnSchnake
changed the title
| return | ||
| return &httpError{ | ||
| err: fmt.Errorf("result %v unexpected", resultID), | ||
| code: http.StatusForbidden, |
There was a problem hiding this comment.
A 403 seems out of place here for this type of error.
There was a problem hiding this comment.
Agree, looking at docs that seems like it is supposed to be auth related, not content like this. Not 100% sure of the right response though but that is for another PR; this just moved the code and didnt chose it here. Maybe https://httpstatuses.com/422 would be most appropriate.
What this PR does / why we need it:
We have two flows for this logic: one for the
HTTP server and one for the channel monitoring
results.
This change places all the logic into a central
method so that it can be modified/tested more
confidently/succinctly.
Which issue(s) this PR fixes
Related to #728
Special notes for your reviewer:
No logical changes in this PR; only doing this in prep of my next change to simplify the diffs. This way we can more easily test the logic used throughout.
Release note: