-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
feat: cli/net: implement 'net ping' command #8357
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't understand the go mock reflect package thing but as long as you're ok with that this LGTM
pkg_ "github.com/filecoin-project/lotus/api/v0api" | ||
) | ||
|
||
var output = flag.String("output", "", "The output file name, or empty to use stdout.") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When I run this on standard out I get a bunch of errors from binary output as you'd expect from gob output. stdout doesn't seem like a useful default
@@ -0,0 +1,64 @@ | |||
package main |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm confused about the purpose of this tool. Is this file supposed to be checked in? I'm not seeing a big change to the mocking framework in this PR that would indicate we need this binary. If we do want this behavior wouldn't it make sense to put this in the cmd
directory as its own binary or a tool in lotus-shed? If it needs to stay here why the 355.. magic number?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, this is just a temp file created by gomock when generating mocks, probably got here through git add -a ..
; Shouldn't be here
@@ -247,6 +290,51 @@ var NetConnect = &cli.Command{ | |||
}, | |||
} | |||
|
|||
func addrInfoFromArg(ctx context.Context, cctx *cli.Context) ([]peer.AddrInfo, error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know this is just copied over from existing code but this method is surprising. It looks like it will either handle a slice of libp2p peer addr infos or it will take a single miner addr. Either the name should carry over some of the complexity ("addrInfoFromAddrInfosOrMinerAddr") or you should add a comment describing what's going on.
Related Issues
Based on #8125, with conflicts/small issues resolved