chore: dependabot upgrade grpc/go-jose/net [RM-66] #9280
Conversation
✅ Deploy Preview for determined-ui canceled.
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #9280 +/- ##
==========================================
- Coverage 44.58% 44.58% -0.01%
==========================================
Files 1275 1275
Lines 156216 156219 +3
Branches 2451 2451
==========================================
+ Hits 69647 69648 +1
- Misses 86329 86331 +2
Partials 240 240
Flags with carried forward coverage won't be shown. Click here to find out more.
|
| @@ -50,7 +50,10 @@ def _get_error_str(r: requests.models.Response) -> str: | |||
| if mes is not None: | |||
| return str(mes) | |||
| # Try getting GRPC error description if message does not exist. | |||
| return str(json_resp.get("error").get("error")) | |||
| error = json_resp.get("error") | |||
There was a problem hiding this comment.
@azhou-determined this change related to changing the grpc version
With the grpc upgrade status messages get set even if it was wrapped so we can have messages that look like this now
det dev curl /api/v1/tasks/dsaaaaaa/logs
{
"error": {
"details": [],
"grpcCode": 5,
"httpCode": 404,
"httpStatus": "Not Found",
"message": "1 error occurred:\n\t* failed to fetch batch: rpc error: code = NotFound desc = task 'dsaaaaaa' not found\n\n"
}
}
while before this upgrade it would be a 500 error and not hit this code path.
There was a problem hiding this comment.
nit: maybe a comment here would be helpful, i've always found this logic pretty unintelligible.
IIUC, there's a total of 3 different error messages we now have: resp.message for echo-level errors, this resp.error.message for certain (streaming?) grpc errors, then resp.error.error for generic(?) grpc errors.
There was a problem hiding this comment.
btw, i get messages in this format even without this upgrade:
det dev curl /api/v1/tasks/dsaaaaaa/logs
{
"error": {
"details": [],
"grpcCode": 2,
"httpCode": 500,
"httpStatus": "Internal Server Error",
"message": "1 error occurred:\n\t* failed to fetch batch: rpc error: code = NotFound desc = task 'dsaaaaaa' not found\n\n"
}
}
There was a problem hiding this comment.
I guess it would run into this exception before the upgrade?
| @@ -47,7 +47,7 @@ func auditLogMiddleware() echo.MiddlewareFunc { | |||
| } | |||
|
|
|||
| for path := range staticWebDirectoryPaths { | |||
| if strings.HasPrefix(c.Path(), path) { | |||
| if strings.HasPrefix(c.Request().URL.Path, path) { | |||
There was a problem hiding this comment.
@salonig23 this was a failing test I think because of this echo change
"Return an empty string for ctx.path if there is no registered path labstack/echo#2385"
Using the request path is always correct here I think. Other usages of Path look to be correct with the new change.
Ticket
Description
Manually applied these since I think dependabot got bugged out in the prs trying for this #9189
Hopefully new prs won't have the same issue.
Test Plan
CI passes
Checklist
docs/release-notes/.See Release Note for details.