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

Support {:all, endpoint} in :services to auto-discover #49

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

v0idpwn
Copy link

@v0idpwn v0idpwn commented Jan 27, 2025

No description provided.

Comment on lines +21 to -22
GrpcReflection,
{GRPC.Server.Supervisor, endpoint: Helloworld.Endpoint, port: 50051, start_server: true},
GrpcReflection
Copy link
Author

Choose a reason for hiding this comment

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

Not related to the PR, but I believe this is the correct order, as the server has dependency on infra started by the GrpcReflection supervisor.

@v0idpwn
Copy link
Author

v0idpwn commented Jan 27, 2025

Closes #48

@v0idpwn v0idpwn changed the title Add {:all, MyApp.Endpoint} to auto-detect gRPC services Add {:all, MyApp.Endpoint} to auto-discover gRPC services Jan 27, 2025
@v0idpwn v0idpwn changed the title Add {:all, MyApp.Endpoint} to auto-discover gRPC services Support {:all, module()} in :services to auto-discover Jan 27, 2025
@v0idpwn v0idpwn changed the title Support {:all, module()} in :services to auto-discover Support {:all, endpoint} in :services to auto-discover Jan 27, 2025
@@ -47,14 +47,15 @@ This is written and tested using [grpcurl](https://github.com/fullstorydev/grpcu
end
```
or both as desired. `version` is the grpc reflection spec, which can be `v1` or `v1alpha`. `services` is the services that will be exposed by that server by reflection. You can expose a service through both services if desired.
1. Add the reflection supervisor to your supervision tree to host the cached reflection state
Instead of using an explicit list of services, you may alternatively use `{:all, MyApp.GrpcEndpoint}`, to include all services from an endpoint. This is a more succint alternative if you want to expose reflection for all services in your endpoint. On the other hand, you lose the fine-grained control of the ehaustive list.
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think this list is starting to get a bit dense with the config options inlined in these paragraphs. Now that services has multiple forms what do you think about simplifying this instruction list and moving the config descriptions into a separate block? I think the two forms for the content of the services entry should be together.

@@ -11,15 +11,15 @@ defmodule GrpcReflection do
defmodule Helloworld.Reflection.Server do
use GrpcReflection.Server,
version: :v1,
services: [Helloworld.Greeter.Service]
services: [Helloworld.Greeter.Service] # alternatively, `{:all, HelloWorld.Endpoint}` can be used to include all services from the endpoint.
Copy link
Collaborator

Choose a reason for hiding this comment

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

We don't describe version: :v1 vs version: v1alpha here. If you want to include this breakout here I would suggest putting in a stand-in for services and version that can be described in text outside of the code block.

Maybe something like:

 defmodule Helloworld.Reflection.Server do
     use GrpcReflection.Server,
         version: <version>,
         services: <services>
 end

Where <version> is either :v1 or :v1alpha, and <services> is either a list of GRPC services to reflect, or the tuple {:all, Endpoint} to reflect all services from that endpoint

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants