[go: nahoru, domu]
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.

Sign up for GitHub

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

Jump to bottom

feature: 'ipfs swarm peering' command #8147

Merged
merged 11 commits into from
Sep 15, 2021
Merged

feature: 'ipfs swarm peering' command #8147

merged 11 commits into from
Sep 15, 2021

Conversation

TakashiMatsuda
Copy link
Contributor
@TakashiMatsuda TakashiMatsuda commented May 19, 2021 •
edited

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 mentioned this pull request Aug 7, 2021
Merged
@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 •
edited
  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?

Stebalien
Stebalien requested changes Aug 17, 2021
View reviewed changes
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 •
edited

@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 •
edited

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

Stebalien
Stebalien requested changes Sep 2, 2021
View reviewed changes
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 •
edited

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

TakashiMatsuda and others added 8 commits September 3, 2021 02:17
@TakashiMatsuda @Stebalien
Update peering/peering.go
93aee8f 
Co-authored-by: Steven Allen <steven@stebalien.com>
@TakashiMatsuda @Stebalien
Update peering/peering.go
4a99978 
Co-authored-by: Steven Allen <steven@stebalien.com>
@TakashiMatsuda @Stebalien
Update core/commands/swarm.go
9121c9e 
Co-authored-by: Steven Allen <steven@stebalien.com>
@TakashiMatsuda @Stebalien
Update core/commands/swarm.go
9c2340c 
Co-authored-by: Steven Allen <steven@stebalien.com>
@TakashiMatsuda
fix: help text and comments
0803da7
@TakashiMatsuda
change return value of swarm peering add
9b9c26c
@TakashiMatsuda
change return value of swarm peering rm
8f32e34
@TakashiMatsuda
Update core/commands/swarm.go
dbdf103
@BigLep BigLep added this to In Review in Maintenance Priorities - Go via automation Sep 3, 2021
@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
Maintenance Priorities - Go automation moved this from In Review to Done 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 Stebalien mentioned this pull request Sep 15, 2021
Merged
@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
Merged
5 tasks
aschmahmann pushed a commit that referenced this pull request Sep 27, 2021
@TakashiMatsuda @aschmahmann
feature: 'ipfs swarm peering' command (#8147)
614e33d 
* feat: added swarm peering command supporting add, ls and rm

Co-authored-by: Steven Allen <steven@stebalien.com>
(cherry picked from commit a651045)
aschmahmann pushed a commit that referenced this pull request Sep 27, 2021
@TakashiMatsuda @aschmahmann
feature: 'ipfs swarm peering' command (#8147)
6288f0b 
* feat: added swarm peering command supporting add, ls and rm

Co-authored-by: Steven Allen <steven@stebalien.com>
(cherry picked from commit a651045)
@aschmahmann aschmahmann mentioned this pull request Sep 30, 2021
Closed
62 tasks
@aschmahmann aschmahmann mentioned this pull request Dec 1, 2021
Closed
80 tasks
@samuelbacaner samuelbacaner mentioned this pull request Aug 14, 2022
Closed
@ajnavarro ajnavarro mentioned this pull request Nov 21, 2022
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Stebalien Stebalien Stebalien approved these changes

@lidel lidel Awaiting requested review from lidel

Assignees

@lidel lidel

Labels
P3 Low: Not priority right now
Projects
No open projects
Maintenance Priorities - Go
Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@TakashiMatsuda @Stebalien @BigLep @aschmahmann @Wondertan @lidel