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

registry/api: move all errors to "errcode" package, and split "Register" to internal / exported #4040

Merged
merged 2 commits into from Sep 15, 2023

Conversation

thaJeztah
Copy link
Member

Move all errors to the errcode package

Comment on lines -11 to -14
var (
// ErrorCodeDigestInvalid is returned when uploading a blob if the
// provided digest does not match the blob contents.
ErrorCodeDigestInvalid = errcode.Register(errGroup, errcode.ErrorDescriptor{
Copy link
Member Author

Choose a reason for hiding this comment

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

Gonna be honest; I still don't know if there was a specific reason to split these errors between the errcode and api/v2 package; I looked at this various times, because to check for these codes, it's needed to import the api/v2 package (including all the routes it defines), which always stood out as "odd", but perhaps there was a specific reason to split the various errors.

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
Use the non-exported function to all errors; there's currently no external
consumers of this function (perhaps it should be deprecated).

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
@thaJeztah thaJeztah marked this pull request as ready for review September 4, 2023 16:03
@milosgajdos
Copy link
Member

What is the reason for this change @thaJeztah?

@thaJeztah
Copy link
Member Author

thaJeztah commented Sep 5, 2023

This was an attempt not having to import the whole registry api (just to be able to handle errors it returns); https://github.com/docker/cli/blob/f74f88445f8b23fe4804f1072e5f376cfbe46487/cli/registry/client/fetcher.go#L204-L206

But I may need more to be moved, e.g. v2.URLBuilder is also in some places;

func NewURLBuilderFromString(root string, relative bool) (*URLBuilder, error) {

@milosgajdos
Copy link
Member

milosgajdos commented Sep 5, 2023

Oh, right. Yeah, I'd love this project to be more modular(ised), too. IIRC, these used to be used in some docs generator but I may be wrong so feel free to ignore me 🤔

@thaJeztah
Copy link
Member Author

Yeah, some parts are really coupled (in this case, the API server implementation (gorilla/mux) leaks all the way into the client, which always felt "off".

The errors in general are hard to work with (and I'm not even sure if we handle them correctly everywhere); I know where some of the ideas for these "structured" errors originated from at the time, but they're lacking in utilities to handle them.

Copy link
Collaborator

@davidspek davidspek left a comment

Choose a reason for hiding this comment

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

LGTM

@milosgajdos milosgajdos merged commit 612ad42 into distribution:main Sep 15, 2023
12 checks passed
@thaJeztah thaJeztah deleted the move_api_errors branch September 15, 2023 10:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants