-
Notifications
You must be signed in to change notification settings - Fork 44
randomports: give user ability to init ipfs using random port for swarm. #17
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.
Preliminary review: LGTM.
Hi @Stebalien,
please feel free to ask about more update. |
@tarekbadrshalaan
There are tests in
Is there benefit to this over the randomness inherited from |
196c87c
to
e5e6814
Compare
@@ -159,6 +163,31 @@ fetching may be degraded. | |||
return nil | |||
}, | |||
}, | |||
"randomports": { | |||
Description: `Use a random port number for swarm.`, |
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.
Hello @djdv
as you can see here I updated the discretion as you said.
again sorry be late and please feel free to ask about more update.
|
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.
This seems fine to me. 👍
The considerations that came up during review where:
- Should
net.Listen("tcp", "[::]:0")
trynet.Listen("tcp", "127.0.0.1:")
instead of, or in addition to, the ipv6 port?
But I doubt it's necessary, so I think not. - Lack of type assertion check in
port = ln.Addr().(*net.TCPAddr).Port
But it's pretty clear that we'd always have a*net.TCPAddr
fromListen
here.
@tarekbadrshalaan Patch seems good to me, handing off to @Stebalien |
Issue : ipfs/kubo#5176
It's still under development, just to make sure that the code is going to
do as we expected.