-
Notifications
You must be signed in to change notification settings - Fork 24
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
feat: add k6 gRPC stream middleware #142
Conversation
|
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 for this contribution! You will need to sign the CLA before I can merge this.
The core change LGTM. Can you fix up the dependencies to get the test CI to run?
x/k6/go.mod
Outdated
@@ -14,14 +14,15 @@ require ( | |||
require ( | |||
github.com/davecgh/go-spew v1.1.1 // indirect | |||
github.com/grafana/pyroscope-go/godeltaprof v0.1.8 // indirect | |||
github.com/grpc-ecosystem/go-grpc-middleware v1.4.0 // indirect |
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.
I think you need to do a go mod tidy
on this, as github.com/grpc-ecosystem/go-grpc-middleware
is a direct dependency.
Also, we need to use github.com/grpc-ecosystem/go-grpc-middleware/v2
instead, to avoid other dependency conflicts with google.golang.org/grpc
.
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.
Should be good now!
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.
Used the make target, should be good for real :)
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.
Unfortunately the make target doesn't sync x/k6
. See #142 (comment) for next steps to get CI to go green. I'll open a separate PR to improve the make target for the future.
366421e
to
fd325af
Compare
I have signed the CLA, not sure why it displays both signed and pending. |
fd325af
to
7f76d80
Compare
@devgianlu I apologize for more book keeping requests, but it looks like Would you be able to run After that, CI should go green and I can merge this 👍 |
7f76d80
to
3ae4e48
Compare
Third time's the charm? |
3ae4e48
to
75a6419
Compare
Synced |
75a6419
to
1996fb8
Compare
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.
LGTM! Thanks for this contribution 🎉
This PR adds a gRPC stream middleware to the K6 extension.