-
Notifications
You must be signed in to change notification settings - Fork 707
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
protovalidate: support new protovalidate-go Validator interface #746
protovalidate: support new protovalidate-go Validator interface #746
Conversation
21dd301
to
8cd57e5
Compare
Signed-off-by: Koichi Shiraishi <[email protected]>
8cd57e5
to
d89418e
Compare
Why diff --git a/README.md b/README.md
index 8c4ac16..c923991 100644
--- a/README.md
+++ b/README.md
@@ -17,22 +17,22 @@ ## Middleware
Additional great feature of interceptors is the fact we can chain those. For example below you can find example server side chain of interceptors with full observabiliy correlation, auth and panic recovery:
```go mdox-exec="sed -n '136,151p' examples/server/main.go"
- grpcSrv := grpc.NewServer(
- grpc.ChainUnaryInterceptor(
- // Order matters e.g. tracing interceptor have to create span first for the later exemplars to work.
- otelgrpc.UnaryServerInterceptor(),
- srvMetrics.UnaryServerInterceptor(grpcprom.WithExemplarFromContext(exemplarFromContext)),
- logging.UnaryServerInterceptor(interceptorLogger(rpcLogger), logging.WithFieldsFromContext(logTraceID)),
- selector.UnaryServerInterceptor(auth.UnaryServerInterceptor(authFn), selector.MatchFunc(allButHealthZ)),
- recovery.UnaryServerInterceptor(recovery.WithRecoveryHandler(grpcPanicRecoveryHandler)),
- ),
- grpc.ChainStreamInterceptor(
- otelgrpc.StreamServerInterceptor(),
- srvMetrics.StreamServerInterceptor(grpcprom.WithExemplarFromContext(exemplarFromContext)),
- logging.StreamServerInterceptor(interceptorLogger(rpcLogger), logging.WithFieldsFromContext(logTraceID)),
- selector.StreamServerInterceptor(auth.StreamServerInterceptor(authFn), selector.MatchFunc(allButHealthZ)),
- recovery.StreamServerInterceptor(recovery.WithRecoveryHandler(grpcPanicRecoveryHandler)),
- ),
+ )
+ t := &testpb.TestPingService{}
+ testpb.RegisterTestServiceServer(grpcSrv, t)
+ srvMetrics.InitializeMetrics(grpcSrv)
+
+ g := &run.Group{}
+ g.Add(func() error {
+ l, err := net.Listen("tcp", grpcAddr)
+ if err != nil {
+ return err
+ }
+ logger.Info("starting gRPC server", "addr", l.Addr().String())
+ return grpcSrv.Serve(l)
+ }, func(err error) {
+ grpcSrv.GracefulStop()
+ grpcSrv.Stop() This pattern offers clean and explicit shared functionality for all your gRPC methods. Full, buildable examples can be found in examples directory.
|
Would love to get this version upgrade merged as it's blocking package upgrades on our projects. |
go 1.21.1 | ||
|
||
toolchain go1.21.13 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please revert these changes, as a library we should not specify subversions of the Go version. See grpc/grpc-go#7831.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Revert but failed CI:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you please re-revert it and we can figure out the CI failure. CI is unstable as it is, don't trust it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@johanbrandhorst done.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks. Clearly we still need to run go mod tidy
, and clearly that introduces the toolchain
directive (and minor version requirement). Could you try to figure out which of the upgrades here is introducing the toolchain directive? It shouldn't be added just on its own.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@johanbrandhorst This issue not related to upgrade protovalidate-go. I think should be separate PR or issue and fix until next release if you want (I don't want do it at this PR
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are you saying that the toolchain is being added when running go mod tidy
on main
? I think this is the first time we've seen it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, as I said on #746 (comment), it seems to be related to:
- deps: Remove Go minor version from go.mods grpc/grpc-go#7831 (comment)
- cmd/go: toolchain directive in go.mod being updated unnecessarily golang/go#65847
From the updated go.mod
, the cel.dev/expr
has a toolchain
directive:
I don't know which is the root cause.
Could you try to figure out ...
Several developers are already waiting for this PR via emoji action. The go-grpc-middleware maintainers are concerned about the issue of the "toolcahin" directive in go.mod due to go-grpc-ecosystem policy, but users (including me) just want to upgrade the protovalidate-go. Those are separate issues, and the main
branch is not stable, so if the concerns about the toolchain
directive take precedence, please ensure the maintainers understand and resolve those issues, then upgrade the protovalidate-go by maintainers until the next release. Please feel free to close this PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cel-spec does seems to be the culprit. I think it's fair to say that we could merge this with the new toolchain directive if we can plan to remove it again before the next release. We'd need to create an issue with cel-spec to get them to remove their spec (or potentially look into their dependencies which may have forced its addition). In any case, if you'd like to just run go mod tidy
in this PR and push that, we can get this upgrade merged.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, done.
50d28b3
to
1a6bb90
Compare
Signed-off-by: Koichi Shiraishi <[email protected]>
1a6bb90
to
f99b389
Compare
@johanbrandhorst PTAL. |
Signed-off-by: Koichi Shiraishi <[email protected]>
This reverts commit 7ba0f46. Signed-off-by: Koichi Shiraishi <[email protected]>
@johanbrandhorst
|
Yeah these just started happening, I'll get that fixed separately. |
Changes
protovalidate: support new protovalidate-go Validator interface.
Verification
protovalidate-go v0.9 release said: