Skip to content

Split gRPC and HTTP error utility into seperate packages#5

Merged
dmcgowan merged 1 commit intocontainerd:mainfrom
austinvazquez:split-grpc-to-package
Apr 26, 2024
Merged

Split gRPC and HTTP error utility into seperate packages#5
dmcgowan merged 1 commit intocontainerd:mainfrom
austinvazquez:split-grpc-to-package

Conversation

@austinvazquez
Copy link
Member

@austinvazquez austinvazquez commented Feb 9, 2024 •
edited
Loading

Issue:

#4

Description:

By having a seperate packages, users can consume base package without pulling gRPC or HTTP as a dependency if not required.

This approach creates errgrpc and errhttp. The imports for users would be the following:

import (
    "github.com/containerd/errdefs"
    "github.com/containerd/errdefs/errgrpc"
    "github.com/containerd/errdefs/errhttp"
)

Other approaches considered:

Top level grpc and http packages. The imports for users would be the following:

import (
    "github.com/containerd/errdefs"
    "github.com/containerd/errdefs/grpc"
    "github.com/containerd/errdefs/http"
)

My thought was this may clutter the top level directory, but this may be acceptable as the number of protocol packages would be finite.

Testing:

Adds basic unit tests for there and back again error conversions for gRPC and HTTP error packages.

AkihiroSuda
AkihiroSuda reviewed Feb 9, 2024
View reviewed changes
pkg/grpc/grpc.go Outdated
*/

package errdefs
package grpc
Copy link
Member

@AkihiroSuda AkihiroSuda Feb 9, 2024

Choose a reason for hiding this comment

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

errdefs.FromGRPC should be now called like grpc.ToNative ?

Copy link
Member Author

@austinvazquez austinvazquez Feb 9, 2024

Choose a reason for hiding this comment

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

Yeah, grpc.FromGRPC can be redundant. grpc.ToNative is nice naming imo. Pushed to try it out.

Copy link
Member

@cpuguy83 cpuguy83 Apr 11, 2024

Choose a reason for hiding this comment

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

Nit: maybe grpcerror or something?
I just dislike having package name conflicts that makes goimports trip up.

@austinvazquez austinvazquez changed the title Split gRPC error utility into seperate package Split gRPC and HTTP error utility into seperate packages Feb 9, 2024
@austinvazquez austinvazquez marked this pull request as ready for review February 9, 2024 16:47
@dmcgowan
Copy link
Member

dmcgowan commented Apr 19, 2024

Thanks for doing the split

Top level grpc and http packages. The imports for users would be the following:

+1 to keeping it at the top. I see pkg more for packages that could live on their own or not directly extending the root package.

Nit: maybe grpcerror or something?

+1 for keeping err (or error) in the package name (not the directory name). Maybe to be like...

import (
    "github.com/containerd/errdefs"
    errgrpc "github.com/containerd/errdefs/grpc"
    errhttp "github.com/containerd/errdefs/http"
)

?

@cpuguy83 that what you were thinking?

@cpuguy83
Copy link
Member

cpuguy83 commented Apr 22, 2024

Not really, but that's fine.
For me I just end up fighting with goimports constantly with packages that have the same name and have to manually write the import statement.

@austinvazquez austinvazquez force-pushed the split-grpc-to-package branch 2 times, most recently from 334de29 to 8b9db8f Compare April 26, 2024 14:49
@austinvazquez
Split gRPC and HTTP error utility into seperate packages
fd0e482 
By having seperate packages, users can consume base package without pulling
gRPC or HTTP as a dependency if not required.

Signed-off-by: Austin Vazquez <[email protected]>
@austinvazquez austinvazquez force-pushed the split-grpc-to-package branch from 8b9db8f to fd0e482 Compare April 26, 2024 14:50
@austinvazquez
Copy link
Member Author

austinvazquez commented Apr 26, 2024

@cpuguy83, @dmcgowan thanks for the suggestions. Pushed changes to address. Landed with errXXX package naming to avoid conflicts with common packages like http and grpc. LMK your thoughts.

@dmcgowan
Copy link
Member

dmcgowan commented Apr 26, 2024

Ok, works for me

@dmcgowan dmcgowan merged commit 2dc9c17 into containerd:main Apr 26, 2024
@austinvazquez austinvazquez deleted the split-grpc-to-package branch April 26, 2024 16:45
@akhilerm akhilerm mentioned this pull request Aug 13, 2024
Closed
This was referenced Sep 12, 2024
Closed
Closed
@dmcgowan dmcgowan mentioned this pull request Oct 8, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@AkihiroSuda AkihiroSuda AkihiroSuda left review comments

@dmcgowan dmcgowan dmcgowan approved these changes

@cpuguy83 cpuguy83 cpuguy83 approved these changes

Assignees

No one assigned

Labels

None yet

Projects

Pull Request Review
Archived in project

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@austinvazquez @dmcgowan @cpuguy83 @AkihiroSuda