Server errors should not be tainted by client error messages #1406
Conversation
There was a problem hiding this comment.
@vyasgun: 0 warnings.
In response to this:
Description
In some cases, if an error is from server but contains a substring that matches a client side error condition, the error returned by the server is being accompanied by the client side error message. This should not be the case.
Changes
- Moved the server error parsing case up so it gets processed before we breakdown the error string for client level message.
- Added test cases.
Reference
Fixes #1342
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.
Codecov Report
@@ Coverage Diff @@
## main #1406 +/- ##
==========================================
+ Coverage 78.33% 78.35% +0.02%
==========================================
Files 160 160
Lines 8270 8274 +4
==========================================
+ Hits 6478 6483 +5
Misses 1103 1103
+ Partials 689 688 -1
Continue to review full report at Codecov.
|
75bc639
to
08aa4ae
Compare
08aa4ae
to
8999c43
Compare
6025fe9
to
1ca19a8
Compare
|
@dsimansk sorry I keep forgetting about adding the entry 😆 |
Well, it happens to me all the time. :) |
|
Thanks looks good to me. I'll leave it open for a while for other to take a look. /approve |
| Name: "not internal error", | ||
| Error: mockErrType{}, | ||
| Internal: false, | ||
| }, |
There was a problem hiding this comment.
What about unknown errors? Or the previous covers it?
There was a problem hiding this comment.
The previous function covers the non k8s api errors. This one covers a mock one which implements the k8s api interface.
There was a problem hiding this comment.
@maximilien any more comments to the above please?
|
The following is the coverage report on the affected files.
|
|
/retest |
|
As I don't see any new comments, I'd say it's done. :) Thanks @vyasgun! |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: dsimansk, vyasgun The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
/retest |
|
/retest |
Description
In some cases, if an error is from server but contains a substring that matches a client side error condition, the error returned by the server is being accompanied by the client side error message. This should not be the case.
Changes
Reference
Fixes #1342