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

fix: register gRPC Gateway routes #2838

Merged

Conversation

jaybxyz
Copy link
Contributor

@jaybxyz jaybxyz commented Nov 13, 2023

Overview

This PR fixes #2837.

Both blob and blobstream modules lack registration for the gRPC Gateway routes, leading to the inability to query endpoints. Let me know if there is anything i need to do to merge this PR.

CI checks require 3 workflows awaiting approval.

Checklist

  • New and updated code has appropriate documentation
  • New and updated code has new and/or updated testing
  • Required CI checks are passing
  • Visual proof for any user facing features like CLI or documentation updates
  • Linked issues closed with keywords

@jaybxyz jaybxyz mentioned this pull request Nov 13, 2023
4 tasks
@jaybxyz jaybxyz marked this pull request as ready for review November 13, 2023 14:52
Copy link
Contributor

@cmwaters cmwaters left a comment

Choose a reason for hiding this comment

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

Thanks for the contribution! LGTM

@cmwaters cmwaters added the backport:v1.x PR will be backported automatically to the v1.x branch upon merging label Nov 13, 2023
Copy link
Member

@evan-forbes evan-forbes left a comment

Choose a reason for hiding this comment

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

I think we can go ahead and approve these since we have not gathered user data on who actually needs the rest adapters. If there are not many users, then it might make sense to disable them entirely to focus on only supporting the canonical grpc ones

@jaybxyz
Copy link
Contributor Author

jaybxyz commented Nov 14, 2023

Last approval is remaining from @rootulp. Remaining CI checks haven't run yet but i expect there won't be any issue with that.

@codecov-commenter
Copy link

Codecov Report

Attention: 6 lines in your changes are missing coverage. Please review.

Comparison is base (b786b08) 19.70% compared to head (7c0ce1e) 19.16%.
Report is 28 commits behind head on main.

Files Patch % Lines
x/blob/module.go 0.00% 3 Missing ⚠️
x/blobstream/module.go 0.00% 3 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2838      +/-   ##
==========================================
- Coverage   19.70%   19.16%   -0.54%     
==========================================
  Files         139      142       +3     
  Lines       17005    17291     +286     
==========================================
- Hits         3350     3313      -37     
- Misses      13333    13676     +343     
+ Partials      322      302      -20     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@jaybxyz
Copy link
Contributor Author

jaybxyz commented Nov 14, 2023

The CI test has failed due to the TestMaxTotalBlobSizeSuite. Is there a potential resolution in #2834?

@evan-forbes evan-forbes merged commit b75c7e3 into celestiaorg:main Nov 14, 2023
33 checks passed
Copy link

gitpoap-bot bot commented Nov 14, 2023

Congrats, your important contribution to this open-source project has earned you a GitPOAP!

GitPOAP: 2023 Celestia Contributor:

GitPOAP: 2023 Celestia Contributor GitPOAP Badge

Head to gitpoap.io & connect your GitHub account to mint!

Learn more about GitPOAPs here.

mergify bot pushed a commit that referenced this pull request Nov 14, 2023
<!--
Please read and fill out this form before submitting your PR.

Please make sure you have reviewed our contributors guide before
submitting your
first PR.
-->

## Overview

<!--
Please provide an explanation of the PR, including the appropriate
context,
background, goal, and rationale. If there is an issue with this
information,
please provide a tl;dr and link the issue.
-->
This PR fixes #2837.

Both `blob` and `blobstream` modules lack registration for the gRPC
Gateway routes, leading to the inability to query endpoints. Let me know
if there is anything i need to do to merge this PR.

CI checks require 3 workflows awaiting approval.

## Checklist

<!--
Please complete the checklist to ensure that the PR is ready to be
reviewed.

IMPORTANT:
PRs should be left in Draft until the below checklist is completed.
-->

- [ ] New and updated code has appropriate documentation
- [ ] New and updated code has new and/or updated testing
- [ ] Required CI checks are passing
- [ ] Visual proof for any user facing features like CLI or
documentation updates
- [x] Linked issues closed with keywords

(cherry picked from commit b75c7e3)
evan-forbes pushed a commit that referenced this pull request Nov 14, 2023
This is an automatic backport of pull request #2838 done by
[Mergify](https://mergify.com).


---


<details>
<summary>Mergify commands and options</summary>

<br />

More conditions and actions can be found in the
[documentation](https://docs.mergify.com/).

You can also trigger Mergify actions by commenting on this pull request:

- `@Mergifyio refresh` will re-evaluate the rules
- `@Mergifyio rebase` will rebase this PR on its base branch
- `@Mergifyio update` will merge the base branch into this PR
- `@Mergifyio backport <destination>` will backport this PR on
`<destination>` branch

Additionally, on Mergify [dashboard](https://dashboard.mergify.com) you
can:

- look at your merge queues
- generate the Mergify configuration with the config editor.

Finally, you can contact us on https://mergify.com
</details>

Co-authored-by: JayB <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport:v1.x PR will be backported automatically to the v1.x branch upon merging
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Register gRPC gateway routes
5 participants