Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

refactor: errors #1738

Merged
merged 6 commits into from
Apr 16, 2024
Merged

refactor: errors #1738

merged 6 commits into from
Apr 16, 2024

Conversation

chesedo
Copy link
Contributor

@chesedo chesedo commented Apr 15, 2024

Description of change

We used to have code that went from ApiError -> ErrorKind -> ApiError. During this conversion to ErrorKind we would remove friendly errors which would have been helpful to our end users. So this removes the ErrorKind enum.

Instead, each module has its own Error know and knows has to convert it to ApiError to be as friendly as possible while not exposing sensitive information to users.

With this it is also easier to isolate the proxy errors so they now return a custom 600 error code if the proxy fails

How has this been tested? (if applicable)

Starting the local stack

@chesedo chesedo changed the title Refactor/errors refactor: errors Apr 15, 2024
Copy link
Contributor

@oddgrd oddgrd left a comment

Choose a reason for hiding this comment

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

This looks great, thanks for cleaning this up, Pieter! I left a few minor comments and some questions to further my own understanding. I think splitting the error up by modules is a great idea, and it's something we're trying to do in the new crates as well. We should be able to use the ApiError there as well.

common/src/models/error.rs Show resolved Hide resolved
common/src/models/error.rs Show resolved Hide resolved
gateway/src/auth.rs Outdated Show resolved Hide resolved
gateway/src/proxy.rs Show resolved Hide resolved
gateway/src/tls.rs Show resolved Hide resolved
gateway/src/service.rs Show resolved Hide resolved
Co-authored-by: Oddbjørn Grødem <29732646+oddgrd@users.noreply.github.com>
Copy link
Contributor

@oddgrd oddgrd left a comment

Choose a reason for hiding this comment

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

Thanks!

@chesedo chesedo merged commit 4e7745f into main Apr 16, 2024
29 of 32 checks passed
@chesedo chesedo deleted the refactor/errors branch April 16, 2024 10:07
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.

2 participants