feat: Add ability to display peer id #719

fredcarle · 2022-08-03T07:00:53Z

Relevant issue(s)

Resolves #663

Description

This PR adds the ability to display the peer id of a given node both through the HTTP API and CLI.

Tasks

I made sure the code is well commented, particularly hard-to-understand areas.
I made sure the repository-held documentation is changed accordingly.
I made sure the pull request title adheres to the conventional commit style (the subset used in the project can be found in tools/configs/chglog/config.yml).
I made sure to discuss its limitations such as threats to validity, vulnerability to mistake and misuse, robustness to invalidation of assumptions, resource requirements, ...

How has this been tested?

unit tests and manual tests

Specify the platform(s) on which this was tested:

MacOS

fredcarle · 2022-08-03T07:12:29Z

One of the test is failing for a known reason. Trying to come up with a solution but in the mean time I welcome your feedback on the rest of the code :)

api/http/router.go

cli/peerid.go

cli/peerid_test.go

codecov · 2022-08-05T00:58:06Z

Codecov Report

Merging #719 (98b3fe6) into develop (da7088e) will increase coverage by 0.36%.
The diff coverage is 51.33%.

@@             Coverage Diff             @@
##           develop     #719      +/-   ##
===========================================
+ Coverage    57.66%   58.02%   +0.36%     
===========================================
  Files          143      144       +1     
  Lines        16456    16593     +137     
===========================================
+ Hits          9489     9628     +139     
+ Misses        6083     6052      -31     
- Partials       884      913      +29

Impacted Files	Coverage Δ
node/node.go	`60.10% <ø> (ø)`
cli/peerid.go	`37.28% <37.28%> (ø)`
cli/start.go	`43.17% <43.13%> (+26.41%)`	⬆️
logging/logger.go	`75.77% <60.00%> (-1.05%)`	⬇️
api/http/errors.go	`100.00% <100.00%> (ø)`
api/http/handler.go	`100.00% <100.00%> (ø)`
api/http/handlerfuncs.go	`89.01% <100.00%> (+0.91%)`	⬆️
api/http/router.go	`100.00% <100.00%> (ø)`
api/http/server.go	`100.00% <100.00%> (ø)`
... and 6 more

cli/start.go

orpheuslummis · 2022-08-06T01:03:03Z

api/http/server.go

 	http.Server
 }

 type serverOptions struct {
 	allowedOrigins []string
+	peerID         string


question (non-blocking): I wonder what is best: to pass the peerID as param to http API and for it to be part of context of each request, or for the http API to not hold the info and instead obtain the info from node every time.

Adding the info to the context is very lightweight. Not sure how it would obtain the info from node.

Adding a .PeerID() func to the node interface, which replies either with an empty string or its PeerID?

If the public key of a node changes, its PeerID would have to change. When that happens, with the construction of this PR, we would also have to restart the HTTP API. I'm not convinced that supporting live changing the public key and PeerID of a node is a good idea, but I do want to acknowledge the dependency that is made here.

This PR already adds a PeerID method to the node. But the HTTP API isn't aware of the node at the moment. In the future it probably should be though. So right now the only way this can work is if we pass along the PeerID to the HTTP API when we start the instance.

core/api/http.go

cli/peerid.go

api/http/errors.go

api/http/handler.go

api/http/handler_test.go

cli/start.go

orpheuslummis · 2022-08-12T14:57:29Z

logging/logger.go

@@ -91,34 +91,44 @@ func (l *logger) FatalE(ctx context.Context, message string, err error, keyvals

 func (l *logger) FeedbackInfo(ctx context.Context, message string, keyvals ...KV) {
 	l.Info(ctx, message, keyvals...)
+	l.syncLock.RLock()


question: why?

Because with the new test there was a race condition error caused by trying to read consoleLogger while it was being set.

orpheuslummis · 2022-08-12T14:59:00Z

suggestion: Because the PR now has more changes than what stated in its title, the title should be update to represent that

fredcarle · 2022-08-12T15:16:00Z

suggestion: Because the PR now has more changes than what stated in its title, the title should be update to represent that

All changes are related to what is represented in the PR title. Not sure it really has to change.

cli/start.go

orpheuslummis

LGTM
Just a few typos to fix

api/http/errors.go

api/http/handler.go

api/http/server.go

fredcarle · 2022-08-19T13:49:34Z

LGTM Just a few typos to fix

Very glad that we have someone on the team that has a radar for that kind of thing. 😄

Description This PR adds the ability to display the peer id of a given node both through the HTTP API and CLI.

fredcarle added feature New feature or request area/api Related to the external API component area/p2p Related to the p2p networking system action/no-benchmark Skips the action that runs the benchmark. area/cli Related to the CLI binary labels Aug 3, 2022

fredcarle added this to the DefraDB v0.3.1 milestone Aug 3, 2022

fredcarle requested a review from a team August 3, 2022 07:00

fredcarle self-assigned this Aug 3, 2022

shahzadlone reviewed Aug 3, 2022

View reviewed changes

api/http/router.go Show resolved Hide resolved

cli/peerid.go Show resolved Hide resolved

cli/peerid.go Outdated Show resolved Hide resolved

cli/peerid_test.go Outdated Show resolved Hide resolved

fredcarle force-pushed the fredcarle/feat/I663-display-peerid branch from 1e8951c to 100d6d9 Compare August 5, 2022 00:53

fredcarle force-pushed the fredcarle/feat/I663-display-peerid branch from 3942f22 to 2b49372 Compare August 5, 2022 16:49

orpheuslummis reviewed Aug 6, 2022

View reviewed changes

fredcarle force-pushed the fredcarle/feat/I663-display-peerid branch 3 times, most recently from 57dd095 to cab57e8 Compare August 8, 2022 19:37

shahzadlone reviewed Aug 9, 2022

View reviewed changes

core/api/http.go Outdated Show resolved Hide resolved

shahzadlone reviewed Aug 9, 2022

View reviewed changes

core/api/http.go Outdated Show resolved Hide resolved

orpheuslummis reviewed Aug 11, 2022

View reviewed changes

cli/peerid.go Outdated Show resolved Hide resolved

orpheuslummis reviewed Aug 11, 2022

View reviewed changes

cli/peerid.go Show resolved Hide resolved

shahzadlone requested changes Aug 11, 2022

View reviewed changes

orpheuslummis reviewed Aug 12, 2022

View reviewed changes

shahzadlone requested changes Aug 12, 2022

View reviewed changes

cli/start.go Outdated Show resolved Hide resolved

shahzadlone reviewed Aug 12, 2022

View reviewed changes

cli/start.go Outdated Show resolved Hide resolved

shahzadlone reviewed Aug 15, 2022

View reviewed changes

cli/start.go Show resolved Hide resolved

shahzadlone approved these changes Aug 15, 2022

View reviewed changes

fredcarle force-pushed the fredcarle/feat/I663-display-peerid branch from e7d60e2 to 667f441 Compare August 16, 2022 03:45

fredcarle changed the title ~~feat: Add ability to display peer id through the HTTP API and CLI~~ feat: Add ability to display peer id Aug 16, 2022

fredcarle force-pushed the fredcarle/feat/I663-display-peerid branch 2 times, most recently from f0b64c7 to 24696a3 Compare August 18, 2022 20:40

fredcarle added 20 commits August 18, 2022 16:53

add peerid cli command

6a5086d

change server listening pattern

a5a1ca9

change cli start flow

465c999

fix possible race condition

0f2ed76

fix race condition on config

084b1e3

lint fix

c62defb

move HTTP API data structs to the core package

49707d1

fix panic on start with —no-p2p

4ffebcb

add core api response to peerid function

309cbcc

fix race condition of peerID tests

358c901

fix error check

fdf13b9

move data structures back to api/http

2f842b3

change Defra for DefraDB

be7529d

change pipe output to simpler object

d52a318

fix trailing new line

d71e7f4

fix logging even when piping

17df178

move DataResponse back to top of file

dd26a61

fix first letter uppercase for logger messages

23119fd

add error check for n.Close()

ef14f48

fix err check

f48bd08

fredcarle force-pushed the fredcarle/feat/I663-display-peerid branch from 24696a3 to f48bd08 Compare August 18, 2022 20:53

orpheuslummis approved these changes Aug 19, 2022

View reviewed changes

api/http/errors.go Outdated Show resolved Hide resolved

api/http/handler.go Outdated Show resolved Hide resolved

api/http/server.go Outdated Show resolved Hide resolved

fix typos

98b3fe6

fredcarle merged commit e9f5f70 into develop Aug 19, 2022

fredcarle deleted the fredcarle/feat/I663-display-peerid branch August 19, 2022 14:48

shahzadlone pushed a commit to shahzadlone/defradb that referenced this pull request Feb 23, 2024

feat: Add ability to display peer id (sourcenetwork#719)

13e1661

Description This PR adds the ability to display the peer id of a given node both through the HTTP API and CLI.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Add ability to display peer id #719

feat: Add ability to display peer id #719

fredcarle commented Aug 3, 2022

fredcarle commented Aug 3, 2022

codecov bot commented Aug 5, 2022 •

edited

Loading

orpheuslummis Aug 6, 2022

fredcarle Aug 8, 2022

orpheuslummis Aug 11, 2022

fredcarle Aug 11, 2022

orpheuslummis Aug 12, 2022

fredcarle Aug 12, 2022 •

edited

Loading

orpheuslummis commented Aug 12, 2022

fredcarle commented Aug 12, 2022

orpheuslummis left a comment

fredcarle commented Aug 19, 2022

feat: Add ability to display peer id #719

feat: Add ability to display peer id #719

Conversation

fredcarle commented Aug 3, 2022

Relevant issue(s)

Description

Tasks

How has this been tested?

fredcarle commented Aug 3, 2022

codecov bot commented Aug 5, 2022 • edited Loading

Codecov Report

orpheuslummis Aug 6, 2022

Choose a reason for hiding this comment

fredcarle Aug 8, 2022

Choose a reason for hiding this comment

orpheuslummis Aug 11, 2022

Choose a reason for hiding this comment

fredcarle Aug 11, 2022

Choose a reason for hiding this comment

orpheuslummis Aug 12, 2022

Choose a reason for hiding this comment

fredcarle Aug 12, 2022 • edited Loading

Choose a reason for hiding this comment

orpheuslummis commented Aug 12, 2022

fredcarle commented Aug 12, 2022

orpheuslummis left a comment

Choose a reason for hiding this comment

fredcarle commented Aug 19, 2022

codecov bot commented Aug 5, 2022 •

edited

Loading

fredcarle Aug 12, 2022 •

edited

Loading