Skip to content

chore: centralized error formatting#866

Open
olevski wants to merge 2 commits into
masterfrom
chore-centralized-error-handling
Open

chore: centralized error formatting#866
olevski wants to merge 2 commits into
masterfrom
chore-centralized-error-handling

Conversation

@olevski
Copy link
Copy Markdown
Member

@olevski olevski commented Apr 30, 2026

/deploy

@RenkuBot
Copy link
Copy Markdown
Contributor

You can access the deployment of this PR at https://renku-ci-gw-866.dev.renku.ch

@olevski
Copy link
Copy Markdown
Member Author

olevski commented Apr 30, 2026

To check this:

Open the browser at: https://renku-ci-gw-866.dev.renku.ch/api/fdfd - this will give you the same response as before.

{
  "message": "Not Found"
}

We can add a nice http page for the errors later. We need to coordinate with the ui for this.

But if you use curl you will get the same format as the data service.

 curl https://renku-ci-gw-866.dev.renku.ch/api/fdfd
{"error":{"code":6404,"message":"Not Found"}}

@olevski
Copy link
Copy Markdown
Member Author

olevski commented Apr 30, 2026

With using python we also get the non-http response:

>>> import requests
>>> res = requests.get("https://renku-ci-gw-866.dev.renku.ch/api/fd")
>>> res.text
'{"error":{"code":6404,"message":"Not Found"}}\n'

@leafty leafty self-requested a review April 30, 2026 12:20
Copy link
Copy Markdown
Member

@leafty leafty left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice, some comments below:

)

// The same error content as the data services API
const gwBaseErrorCode int = 6000
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why 6000? (Do we have a rule for these numbers?)

Comment on lines +32 to +33
accept := c.Request().Header.Get("Accept")
isHTML := strings.Contains(accept, echo.MIMETextHTML)
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remark: weak content negotiation. But there is no standard library method to at least parse the accept header (not even try to perform content negotiation).
See: https://httpwg.org/specs/rfc9110.html#field.accept

if c.Request().Method == http.MethodHead {
cErr = c.NoContent(code)
} else {
cErr = c.JSON(code, errorResponse{Error: errorContent{Code: gwBaseErrorCode + code, Message: message}})
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like we are not using Detail and TraceId. Should we open an issue to add them?

cErr = c.JSON(code, errorResponse{Error: errorContent{Code: gwBaseErrorCode + code, Message: message}})
}
if cErr != nil {
c.Logger().Error("failed to send error page to client", "error", errors.Join(err, cErr))
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use slog.Error() here. We configure the logger for slog but we don't set the Logger used by echo.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note: maybe this file should be in internal/gwerrors/?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants