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

gRPC: fix server reflection #8945

Merged
merged 18 commits into from
Mar 22, 2021
Merged

Conversation

fdymylja
Copy link
Contributor

@fdymylja fdymylja commented Mar 19, 2021

Description

closes: #8927


Before we can merge this PR, please make sure that all the following items have been
checked off. If any of the checklist items are not applicable, please leave them but
write a little note why.

  • Targeted PR against correct branch (see CONTRIBUTING.md)
  • Linked to Github issue with discussion and accepted design OR link to spec that describes this work.
  • Code follows the module structure standards.
  • Wrote unit and integration tests
  • Updated relevant documentation (docs/) or specification (x/<module>/spec/)
  • Added relevant godoc comments.
  • Added a relevant changelog entry to the Unreleased section in CHANGELOG.md
  • Re-reviewed Files changed in the Github PR explorer
  • Review Codecov Report in the comment section below once CI passes

@orijbot
Copy link

orijbot commented Mar 19, 2021

@orijbot
Copy link

orijbot commented Mar 19, 2021

@codecov
Copy link

codecov bot commented Mar 22, 2021

Codecov Report

Merging #8945 (ba4698c) into master (5bd93bf) will decrease coverage by 0.40%.
The diff coverage is 15.93%.

❗ Current head ba4698c differs from pull request most recent head def738b. Consider uploading reports for the commit def738b to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##           master    #8945      +/-   ##
==========================================
- Coverage   59.27%   58.87%   -0.41%     
==========================================
  Files         571      574       +3     
  Lines       31827    32114     +287     
==========================================
+ Hits        18867    18906      +39     
- Misses      10757    10993     +236     
- Partials     2203     2215      +12     
Impacted Files Coverage Δ
server/grpc/gogoreflection/serverreflection.go 2.81% <2.81%> (ø)
server/grpc/gogoreflection/fix_registration.go 29.09% <29.09%> (ø)
x/staking/simulation/operations.go 73.16% <60.00%> (-0.30%) ⬇️
server/grpc/server.go 62.50% <100.00%> (ø)
x/genutil/client/cli/migrate.go 68.42% <100.00%> (ø)
x/gov/legacy/v043/json.go 100.00% <100.00%> (ø)
x/gov/legacy/v043/store.go 85.71% <100.00%> (+1.09%) ⬆️
x/staking/types/msg.go 55.55% <100.00%> (+0.62%) ⬆️
... and 1 more

Copy link
Contributor

@amaury1093 amaury1093 left a comment

Choose a reason for hiding this comment

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

Wow, this is pretty cool, thanks @fdymylja.

I tested:

grpcurl -plaintext localhost:9090 list
grpcurl -plaintext localhost:9090 describe cosmos.authz.v1beta1.Query
grpcurl -plaintext localhost:9090 describe cosmos.authz.v1beta1.Query.Authorization

they all work.

Before approving, can you update docs/run-node/interact-node.md and add a changelog?

I also added the backport label, as I see this as a nice bugfix.

Copy link
Contributor

@jgimeno jgimeno left a comment

Choose a reason for hiding this comment

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

TOP! Thanks @fdymylja

@fdymylja
Copy link
Contributor Author

@AmauryM @jgimeno I've added support for extensions reflection and updated the docs.

@amaury1093
Copy link
Contributor

A changelog would be nice! After that feel free to put automerge on

@fdymylja fdymylja added the A:automerge Automatically merge PR once all prerequisites pass. label Mar 22, 2021
Copy link
Collaborator

@robert-zaremba robert-zaremba left a comment

Choose a reason for hiding this comment

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

Left few cosmetic comments. Thanks!

server/grpc/gogoreflection/fix_registration.go Outdated Show resolved Hide resolved
// but are imported by other files in a different way.
// NOTE(fdymylja): This fix should not be needed and should be addressed in some CI.
// Currently every cosmos-sdk proto file is importing gogo.proto as gogoproto/gogo.proto,
// but gogo.proto registers itself as gogo.proto, same goes for cosmos.proto.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do you know why this happens? Maybe we should fix it in github.com/regen-network/cosmos-proto?
@aaronc

Copy link
Collaborator

Choose a reason for hiding this comment

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

I mean - let's create an issue if there is a bug we can easily fix in cosmos-proto

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it's not a bug per se, it's a shortcoming when it comes to combo together reflection and proto imports. protofiles register themselve in the registry with a path, but then are imported with another path. this makes reflection fail, because it cannot find certain files.

this would mean either forking gogoproto and changing how we feed the file to protoc, or changing the third_party structure in the sdk, edit how every file imports gogoproto.

and im not sure if this is a breaking change, descriptor wise it is.

probably this should be a blob of work which should be reorganized during the transition to protov2

@fdymylja
Copy link
Contributor Author

fdymylja commented Mar 22, 2021

@aaronc, @alessio it's the 5th iteration of github actions re-run I am currently doing.. and each of them takes about 30mins to 1 hour. Could we please merge this manually instead of relying on automerge? As there is other work depending on this PR..

@alessio
Copy link
Contributor

alessio commented Mar 22, 2021

@fdymylja yes, we are experiencing many issues with gh actions currently. AFAIK @marbar3778 is looking into what's wrong there...

Meanwhile, it'd be great if you could address @robert-zaremba's comments (some are minor, another would just take to open a new issue), then I'd be super happy to merge this manually 👍

@fdymylja
Copy link
Contributor Author

@fdymylja yes, we are experiencing many issues with gh actions currently. AFAIK @marbar3778 is looking into what's wrong there...

Meanwhile, it'd be great if you could address @robert-zaremba's comments (some are minor, another would just take to open a new issue), then I'd be super happy to merge this manually 👍

I'll open a follow up PR regarding proto file registrations. But as I've written in the comment I think it should end up in a bigger batch of work during the transition to protov2.

@mergify mergify bot merged commit ac48ffe into master Mar 22, 2021
@mergify mergify bot deleted the fdymylja/issue-8927-fix-grpc-reflection branch March 22, 2021 19:40
@robert-zaremba
Copy link
Collaborator

Thanks for creating the issue @fdymylja .

@amaury1093
Copy link
Contributor

@Mergifyio backport release/v0.42.x

mergify bot pushed a commit that referenced this pull request May 3, 2021
* add: gogoreflection

* fix: server reflection

* fix: make tests resolve imports in a transitive way

* chore: cleanup getMessageType

* chore: lint

* add: extensions reflection

* chore: update interact-node.md docs

* chore: update CHANGELOG.md

* Update server/grpc/gogoreflection/fix_registration.go

Co-authored-by: Robert Zaremba <[email protected]>

Co-authored-by: Jonathan Gimeno <[email protected]>
Co-authored-by: Alessio Treglia <[email protected]>
Co-authored-by: Robert Zaremba <[email protected]>
(cherry picked from commit ac48ffe)

# Conflicts:
#	CHANGELOG.md
#	go.mod
#	go.sum
@mergify
Copy link
Contributor

mergify bot commented May 3, 2021

Command backport release/v0.42.x: success

Backports have been created

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A:automerge Automatically merge PR once all prerequisites pass.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

gRPC: fix reflection service
6 participants