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

feature: 'ipfs swarm peering' command #8147

Merged
merged 11 commits into from
Sep 15, 2021

Conversation

TakashiMatsuda
Copy link
Contributor

@TakashiMatsuda TakashiMatsuda commented May 19, 2021

Overview

This PR is intended to implement a part of #7880.

Detail

New command ipfs swarm peering is added in this commit.
There are three subcommands under ipfs swarm peering. :

ipfs swarm peering add <address>... - add peers into the peering service.
ipfs swarm peering ls               - list peers registered in the peering service.
ipfs swarm peering rm <ID>...       - remove a peer from the peering service.
Usage of 'ipfs swarm peering add'

Example:

$ ./cmd/ipfs/ipfs swarm peering add /ip4/1.2.3.4/tcp/1234/p2p/QmYyQSo1c1Ym7orWxLYvCrM2EmxFTANf8wXmmE7DWjhx5N
add QmYyQSo1c1Ym7orWxLYvCrM2EmxFTANf8wXmmE7DWjhx5N success

Help text:

USAGE
ipfs swarm peering add <address>... - add peers into the peering service.

ipfs swarm peering add [--] <address>...

'ipfs swarm peering add' adds peers into the peering service.

For more information about each command, use:
'ipfs swarm peering add <subcmd> --help'
Usage of 'ipfs swarm peering rm'

Example:

$ ./cmd/ipfs/ipfs swarm peering rm QmYyQSo1c1Ym7orWxLYvCrM2EmxFTANf8wXmmE7DWjhx5N
remove QmYyQSo1c1Ym7orWxLYvCrM2EmxFTANf8wXmmE7DWjhx5N success

Help text:

> ipfs swarm peering rm IDs...
USAGE
  ipfs swarm peering rm <ID>... - remove a peer from the peering subsystem.

  ipfs swarm peering rm [--] <ID>...

  'ipfs swarm peering rm' remove peers from the peering subsystem.

  For more information about each command, use:
  'ipfs swarm peering rm <subcmd> --help'
Usage of 'ipfs swarm peering ls'

Example:

$ ./cmd/ipfs/ipfs swarm peering ls
12D3KooWPBxKtYk9ptRH2B9tEJvBiQnPxKL3JxaVvJgmh3bpeGmn
        /ip4/35.213.90.5/tcp/4001

Help text:

USAGE
  ipfs swarm peering ls - list peers registered in the peering service.

  ipfs swarm peering ls

  'ipfs swarm peering ls' lists peers registered in the peering service.

  For more information about each command, use:
  'ipfs swarm peering ls <subcmd> --help'

Comment

  • I have not added the --save option and the add command.
  • ipfs swarm peering ls command is not requested in [Enhancement] Dynamic Peerlist adding / removing #7880. If ls was unnecessary, I would remove it.
  • Please let me know if I have broken any compatibility with the output format of the command.

@TakashiMatsuda TakashiMatsuda changed the title add peering rm command without save option feature: 'ipfs swarm peering rm' command without save option May 19, 2021
@TakashiMatsuda TakashiMatsuda changed the title feature: 'ipfs swarm peering rm' command without save option feature: 'ipfs swarm peering rm' command without '--save' option May 19, 2021
@aschmahmann aschmahmann self-assigned this May 24, 2021
@aschmahmann aschmahmann added P3 Low: Not priority right now need/maintainer-input Needs input from the current maintainer(s) labels May 24, 2021
@Stebalien
Copy link
Member

  1. This will need sharness tests (test/sharness).
  2. Punting --save to a future patch is fine, punting the add command is not. It makes it hard to test will likely confuse users.

@Stebalien Stebalien added need/author-input Needs input from the original author and removed need/maintainer-input Needs input from the current maintainer(s) labels May 24, 2021
@TakashiMatsuda
Copy link
Contributor Author

Thank you for your comment. I understand.
I'll work on adding sharness tests and the add command. I'll leave this PR as Draft until the add is done.

@TakashiMatsuda TakashiMatsuda marked this pull request as draft May 25, 2021 12:00
@TakashiMatsuda TakashiMatsuda force-pushed the feat/peering-rm branch 3 times, most recently from d70ae1b to 6721d11 Compare August 17, 2021 17:13
@TakashiMatsuda TakashiMatsuda marked this pull request as ready for review August 17, 2021 17:25
@TakashiMatsuda TakashiMatsuda changed the title feature: 'ipfs swarm peering rm' command without '--save' option feature: 'ipfs swarm peering' command Aug 17, 2021
@TakashiMatsuda
Copy link
Contributor Author

TakashiMatsuda commented Aug 17, 2021

  1. This will need sharness tests (test/sharness).
  2. Punting --save to a future patch is fine, punting the add command is not. It makes it hard to test will likely confuse users.

@Stebalien ( @aschmahmann )
I've added the missing implementation for 1 and 2. Could you review it again?

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.

Thanks!

peering/peering.go Outdated Show resolved Hide resolved
peering/peering.go Outdated Show resolved Hide resolved
peering/peering.go Outdated Show resolved Hide resolved
peering/peering.go Outdated Show resolved Hide resolved
peering/peering.go Outdated Show resolved Hide resolved
core/commands/swarm.go Outdated Show resolved Hide resolved
core/commands/swarm.go Show resolved Hide resolved
core/commands/swarm.go Outdated Show resolved Hide resolved
core/commands/swarm.go Outdated Show resolved Hide resolved
core/commands/swarm.go Outdated Show resolved Hide resolved
@TakashiMatsuda TakashiMatsuda force-pushed the feat/peering-rm branch 2 times, most recently from 3d459e3 to 2f6be1a Compare August 18, 2021 12:26
@BigLep
Copy link
Contributor

BigLep commented Aug 27, 2021

@TakashiMatsuda : let us know when this is ready to be looked at again.

@TakashiMatsuda
Copy link
Contributor Author

@BigLep Okay. Sorry I kept you waiting. I will finish in a few days.

@TakashiMatsuda
Copy link
Contributor Author

TakashiMatsuda commented Sep 2, 2021

@BigLep @Stebalien
I have finished the corrections. It's ready to be looked at.

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.

https://github.com/ipfs/go-ipfs/pull/8147/files#r690637152 still needs to be addressed and I've added one help-text nit. Other than that, this looks great! And thanks for writing the tests.

core/commands/swarm.go Show resolved Hide resolved
@Stebalien
Copy link
Member

Could you rebase on master one more time to fix the tests?

@TakashiMatsuda
Copy link
Contributor Author

TakashiMatsuda commented Sep 2, 2021

Sure, can I push -f it to the repository?

@BigLep BigLep requested a review from lidel September 10, 2021 15:18
@BigLep BigLep assigned lidel and unassigned aschmahmann Sep 10, 2021
@Stebalien Stebalien removed the need/author-input Needs input from the original author label Sep 15, 2021
@aschmahmann aschmahmann merged commit a651045 into ipfs:master Sep 15, 2021
@aschmahmann
Copy link
Contributor

Thanks @TakashiMatsuda 🙏

@Wondertan
Copy link
Member

ListPeers does not hold a lock when reads from the peers map.

@Stebalien
Copy link
Member

Thanks for catching that! Fixed in #8438.

@TakashiMatsuda TakashiMatsuda deleted the feat/peering-rm branch September 17, 2021 16:51
@aschmahmann aschmahmann mentioned this pull request Sep 27, 2021
5 tasks
aschmahmann pushed a commit that referenced this pull request Sep 27, 2021
* feat: added swarm peering command supporting add, ls and rm

Co-authored-by: Steven Allen <[email protected]>
(cherry picked from commit a651045)
aschmahmann pushed a commit that referenced this pull request Sep 27, 2021
* feat: added swarm peering command supporting add, ls and rm

Co-authored-by: Steven Allen <[email protected]>
(cherry picked from commit a651045)
@aschmahmann aschmahmann mentioned this pull request Sep 30, 2021
62 tasks
@aschmahmann aschmahmann mentioned this pull request Dec 1, 2021
80 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
P3 Low: Not priority right now
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants