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

add dns support inipfs p2p forward and refactor code #5533

Merged
merged 2 commits into from
Oct 5, 2018

Conversation

kjzz
Copy link
Contributor

@kjzz kjzz commented Sep 27, 2018

Fix #5524

License: MIT
Signed-off-by: Kejie Zhang 601172892@qq.com

@kjzz kjzz requested a review from Kubuxu as a code owner September 27, 2018 11:55
@kjzz kjzz changed the title refactor p2p command code [WIP]:refactor p2p command code Sep 27, 2018
@kjzz kjzz changed the title [WIP]:refactor p2p command code [WIP]:ipfs p2p forward should accept full multiaddresses as target Sep 27, 2018
Stebalien
Stebalien previously approved these changes Sep 27, 2018
Copy link
Member

@Stebalien Stebalien left a comment

Choose a reason for hiding this comment

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

Fair enough.

@Stebalien Stebalien dismissed their stale review September 28, 2018 19:13

Looks like there's more to come

@kjzz kjzz changed the title [WIP]:ipfs p2p forward should accept full multiaddresses as target add dns support inipfs p2p forward and refactor code Sep 30, 2018
@kjzz kjzz force-pushed the zkj/forward branch 6 times, most recently from b3ee63f to 7db2efe Compare September 30, 2018 07:29
@kjzz
Copy link
Contributor Author

kjzz commented Sep 30, 2018

Hey @Stebalien , i have finished about this pr.If you have any time,please help me review it.Thx a lot.

@@ -19,6 +19,8 @@ import (
ma "gx/ipfs/QmYmsdtJ3HsodkePE3eU3TsCaP2YvPZJ4LoXnNkDE5Tpt7/go-multiaddr"
"gx/ipfs/QmZNkThpqfVXs9GNbexPrfBbXSLNYeKrE7jwFM2oqHbyqN/go-libp2p-protocol"
pstore "gx/ipfs/QmfAQMFpgDU2U4BXG64qVr8HSiictfWvkSBz7Y2oDj65st/go-libp2p-peerstore"
madns "gx/ipfs/QmfXU2MhWoegxHoeMd3A2ytL2P6CY4FfqGWc23LTNWBwZt/go-multiaddr-dns"
"time"
Copy link
Member

Choose a reason for hiding this comment

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

Move this to the top

test/sharness/t0180-p2p.sh Show resolved Hide resolved
test/sharness/t0180-p2p.sh Outdated Show resolved Hide resolved
if err != nil {
return nil, err
}
if strings.Contains(mutiladdr.String(), "/ipfs/Qm") {
Copy link
Member

Choose a reason for hiding this comment

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

I wouldn't check for the Qm prefix as that won't work with Ed keys. Also, we want to migrate form /ipfs/ to /p2p/ in near future so for compatibility this should just be if _, err := multiaddr.ValueForProtocol(ma.P_IPFS); err == nil {

targetAddressOptionName = "target-address"
)

var connectionTimeout = 10 * time.Second
Copy link
Member

Choose a reason for hiding this comment

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

This doesn't appear to be used anywhere but parseIpfsAddr, so maybe call it resolveTimeout?

@kjzz kjzz force-pushed the zkj/forward branch 2 times, most recently from 1ce96f0 to 480d833 Compare October 1, 2018 12:47
@kjzz
Copy link
Contributor Author

kjzz commented Oct 1, 2018

Thx a lot for your great advice @magik6k . I have update my pr please help me review again.And maybe there are something wrong with jenkins ci.

@kjzz kjzz force-pushed the zkj/forward branch 9 times, most recently from 1ea140c to f8d955c Compare October 2, 2018 14:18
License: MIT
Signed-off-by: Kejie Zhang <601172892@qq.com>
License: MIT
Signed-off-by: Kejie Zhang <601172892@qq.com>
@ghost ghost assigned Stebalien Oct 5, 2018
@ghost ghost added the status/in-progress In progress label Oct 5, 2018
@Stebalien Stebalien merged commit 70ebea5 into ipfs:master Oct 5, 2018
@ghost ghost removed the status/in-progress In progress label Oct 5, 2018
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