-
Notifications
You must be signed in to change notification settings - Fork 38
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: added custom grpc resolver #1008
feat: added custom grpc resolver #1008
Conversation
fb0f789
to
0da27d8
Compare
Hey @pradeepbbl ! Thanks a lot for this. I was hoping to get to it this week, but I won't be able to. I will review next week. Sorry about that. |
No worries take your time meanwhile I'm exploring way to have e2e test using local envoy container |
745b423
to
8328fee
Compare
added a new e2e test coverage for Test Name: RunFlagdInProcessEnvoyCucumberTest |
...gd/src/main/java/dev/openfeature/contrib/providers/flagd/resolver/common/ChannelBuilder.java
Show resolved
Hide resolved
providers/flagd/src/test/java/dev/openfeature/contrib/providers/flagd/e2e/ContainerConfig.java
Show resolved
Hide resolved
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.
nice, love the e2e tests, just added a suggestion regarding enums.
...gd/src/main/java/dev/openfeature/contrib/providers/flagd/resolver/common/ChannelBuilder.java
Outdated
Show resolved
Hide resolved
I only see one small issue: the way |
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.
Excellent work, thanks so much. This is my only blocking change (which is small and I would do myself, but I can't edit your branch)
...est/java/dev/openfeature/contrib/providers/flagd/e2e/RunFlagdInProcessEnvoyCucumberTest.java
Show resolved
Hide resolved
@pradeepbbl I merged another PR that results in some (I believe minor) conflicts here, mostly with the setup of the providers for e2e tests. Just be sure to use: .deadline(1000)
.streamDeadlineMs(0) // this makes reconnect tests more predictable for the options in those. I think that's the only thing to note, but if you have issues with the conflicts or e2e tests not working, please let me know and I will do what I can to help. |
added custom gRPC resolver to support envoy proxy Signed-off-by: Pradeep Mishra <pradeepbbl@gmail.com> Signed-off-by: Pradeep <pradeepbbl@gmail.com>
Signed-off-by: Pradeep <pradeepbbl@gmail.com>
Signed-off-by: Pradeep <pradeepbbl@gmail.com>
Signed-off-by: Pradeep <pradeepbbl@gmail.com>
Co-authored-by: Michael Beemer <beeme1mr@users.noreply.github.com> Signed-off-by: Pradeep Mishra <pradeepbbl@users.noreply.github.com>
Co-authored-by: Michael Beemer <beeme1mr@users.noreply.github.com> Signed-off-by: Pradeep Mishra <pradeepbbl@users.noreply.github.com>
Co-authored-by: Michael Beemer <beeme1mr@users.noreply.github.com> Signed-off-by: Pradeep Mishra <pradeepbbl@users.noreply.github.com>
Signed-off-by: Pradeep <pradeepbbl@gmail.com>
Signed-off-by: Pradeep <pradeepbbl@gmail.com>
9405359
to
8d7cee6
Compare
Signed-off-by: Pradeep <pradeepbbl@gmail.com>
@toddbaert thanks as suggested fixed the |
Signed-off-by: Pradeep <pradeepbbl@gmail.com>
yeah sorry for that as per the company policy all our open-source contribution need to be done through company github org |
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.
Nothing else to say but amazing work!
I'll merge this today unless anyone objects, and then release.
This PR
Added custom gRPC resolver to support envoy proxy
tragetUri
Related Issues
Fixes #995
How to test
Unit test are already added for the custom resolver for integration test you need a working
envoy
proxy support or any of the supported core resolver mentioned here