Errors: Update errutil to be compatible with k8s errors #87605
Conversation
| // StatusNotFound means that the server does not have any | ||
| // corresponding document to return to the request. | ||
| // HTTP status code 404. | ||
| StatusNotFound CoreStatus = "Not found" | ||
| StatusNotFound CoreStatus = CoreStatus(metav1.StatusReasonNotFound) |
There was a problem hiding this comment.
^^ this is a real change in the response. The k8s reason strings are capital camel case sentences, and our existing reasons have spaces.
It seems better to just use the k8s flavors 🤷🏻
There was a problem hiding this comment.
🤔 I'm not sure about this - This package is used for user-facing errors; the whole point is that messages returned here should be human readable. I don't think we should be exposing these k8s implementation details to end users.
There was a problem hiding this comment.
Ya -- torn on this also. If the train was not heading towards apiserver for everything I would agree. BUT given that the goal is to remove everythign from /api -- should we just get our UI elements ready to add spaces to camel case reasons?
There was a problem hiding this comment.
I tend to agree that switching to the "standard" error text format makes sense, and that we are going to need to figure out how to present the error messages in a friendly format on the frontend (including i18n) so passing "user readable" strings through from the backend makes less sense to me than having standard strings and a frontend switch case to turn them into human-readable strings.
ryantxu
changed the title
ryantxu
added
no-backport
| @@ -105,14 +104,14 @@ func (r *queryREST) Connect(ctx context.Context, name string, opts runtime.Objec | |||
| Message: "datasource not found", | |||
| }} | |||
| } | |||
| responder.Error(convertToK8sError(err)) | |||
| responder.Error(err) | |||
There was a problem hiding this comment.
🎉 the errors are now returned properly -- rather than getting returned as 500, it is now 400
This PR updates our existing errutil package so that the errors render properly when returned in an apiserver.
To support this behavior, we made three changes:
Status()method that returns ametav1.Status{}objectTODO? (but not this PR) The k8s docs suggest that the error details can be anything, however it is not clear how to implement that. This PR just shoves the values into the causes arrary 🤷🏻