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(routing/http/contentrouter): additional FindPeer checks #616

Merged
merged 6 commits into from
Jun 20, 2024

Conversation

aschmahmann
Copy link
Contributor

Noticed a few issues while doing some tracing (e.g. errors not being returned when no data was returned) that caused me to look a little closer here.

@aschmahmann aschmahmann requested a review from a team as a code owner June 7, 2024 21:43
Copy link

codecov bot commented Jun 7, 2024

Codecov Report

Attention: Patch coverage is 77.77778% with 2 lines in your changes missing coverage. Please review.

Project coverage is 59.79%. Comparing base (d99b7e8) to head (4721a79).

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #616      +/-   ##
==========================================
+ Coverage   59.75%   59.79%   +0.03%     
==========================================
  Files         238      238              
  Lines       29979    29984       +5     
==========================================
+ Hits        17914    17928      +14     
+ Misses      10446    10434      -12     
- Partials     1619     1622       +3     
Files Coverage Δ
routing/http/contentrouter/contentrouter.go 50.56% <77.77%> (+3.74%) ⬆️

... and 8 files with indirect coverage changes

Copy link
Contributor

@gammazero gammazero left a comment

Choose a reason for hiding this comment

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

Just a couple of minor log formatting changes to make, otherwise LGTM.

routing/http/contentrouter/contentrouter.go Outdated Show resolved Hide resolved
routing/http/contentrouter/contentrouter.go Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
@lidel lidel changed the title fix: routing-v1 findpeer fix(routing/http/contentrouter): additional FindPeer checks Jun 20, 2024
@lidel lidel merged commit fa17571 into main Jun 20, 2024
16 checks passed
@lidel lidel deleted the fix/httpfindpeer branch June 20, 2024 21:09
wenyue pushed a commit to wenyue/boxo that referenced this pull request Oct 17, 2024
* fix(routing/http/contentrouter): return error when no responses are found in FindPeer
* fix(routing/http/contentrouter): fix findpeer iteration log message
* fix(routing/http/contentrouter): only return information for FindPeer that matches the requested peerID
* fix(routing/http/contentrouter): only return information for FindPeer if there are actual addresses
* fix(routing/http/contentrouter): switch logging from Warnw -> Warnf where there aren't key pairs
* docs: update CHANGELOG.md

---------

Co-authored-by: Marcin Rataj <lidel@lidel.org>
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.

3 participants