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

Vendored gRPC services aren't present in reflection #4422

Closed
zbuc opened this issue May 20, 2024 · 0 comments · Fixed by #4425
Closed

Vendored gRPC services aren't present in reflection #4422

zbuc opened this issue May 20, 2024 · 0 comments · Fixed by #4425
Labels
needs-refinement unclear, incomplete, or stub issue that needs work

Comments

@zbuc
Copy link
Contributor

zbuc commented May 20, 2024

While testing the new IBC interfaces added in #4391 I noticed that third-party gRPC services don't appear in gRPC reflection because the list of reflected services comes from the Penumbra gen/proto_descriptor.bin.no_lfs file, and since these services' Protobuf files are vendored they aren't present.

@conorsch suggested that we might be able to accomplish this if we can shim references to the vendored services in to the proto descriptor file via tonic-reflection.

@github-project-automation github-project-automation bot moved this to Backlog in Penumbra May 20, 2024
@github-actions github-actions bot added the needs-refinement unclear, incomplete, or stub issue that needs work label May 20, 2024
zbuc added a commit that referenced this issue May 21, 2024
## Describe your changes

This changes the proto build process to include cosmos/ibc-go proto
files in the vendored proto file list used to generate the
proto_descriptor file.

As-implemented, there are some duplicate paths that appear both as
dependencies of the penumbra protos and in ibc-go. The ibc-go protos
will be included first so the penumbra protos take priority. This
shouldn't be an issue in the way we currently use the protos but this
seems important to document for future reference.

## Issue ticket number and link

Closes #4422

## Checklist before requesting a review

- [x] If this code contains consensus-breaking changes, I have added the
"consensus-breaking" label. Otherwise, I declare my belief that there
are not consensus-breaking changes, for the following reason:

  > Just changes gRPC reflection returns
@github-project-automation github-project-automation bot moved this from Backlog to Done in Penumbra May 21, 2024
conorsch added a commit that referenced this issue May 21, 2024
Refs #4392. We want to ensure that server reflection remains
working, despite changes to the tonic dependencies (#4400)
and proto compiling logic #4422. While the most effective test
is exercising all these protos regularly, we currently lack
solid coverage, so this superficial integration test is a sanity-check
to save time versus building locally and inspecting the output manually.
conorsch added a commit that referenced this issue May 23, 2024
Refs #4392. We want to ensure that server reflection remains
working, despite changes to the tonic dependencies (#4400)
and proto compiling logic #4422. While the most effective test
is exercising all these protos regularly, we currently lack
solid coverage, so this superficial integration test is a sanity-check
to save time versus building locally and inspecting the output manually.
conorsch added a commit that referenced this issue May 23, 2024
Refs #4392. We want to ensure that server reflection remains
working, despite changes to the tonic dependencies (#4400)
and proto compiling logic #4422. While the most effective test
is exercising all these protos regularly, we currently lack
solid coverage, so this superficial integration test is a sanity-check
to save time versus building locally and inspecting the output manually.
conorsch added a commit that referenced this issue May 23, 2024
Refs #4392. We want to ensure that server reflection remains
working, despite changes to the tonic dependencies (#4400)
and proto compiling logic #4422. While the most effective test
is exercising all these protos regularly, we currently lack
solid coverage, so this superficial integration test is a sanity-check
to save time versus building locally and inspecting the output manually.
conorsch added a commit that referenced this issue May 23, 2024
Refs #4392. We want to ensure that server reflection remains
working, despite changes to the tonic dependencies (#4400)
and proto compiling logic #4422. While the most effective test
is exercising all these protos regularly, we currently lack
solid coverage, so this superficial integration test is a sanity-check
to save time versus building locally and inspecting the output manually.
conorsch added a commit that referenced this issue May 23, 2024
Refs #4392. We want to ensure that server reflection remains
working, despite changes to the tonic dependencies (#4400)
and proto compiling logic #4422. While the most effective test
is exercising all these protos regularly, we currently lack
solid coverage, so this superficial integration test is a sanity-check
to save time versus building locally and inspecting the output manually.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs-refinement unclear, incomplete, or stub issue that needs work
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

1 participant