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: Implement --save option in 'ipfs swarm peering add/rm' #9192

Closed
wants to merge 4 commits into from
Closed
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
66 changes: 65 additions & 1 deletion core/commands/swarm.go
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,7 @@ const (
swarmStreamsOptionName = "streams"
swarmLatencyOptionName = "latency"
swarmDirectionOptionName = "direction"
swarmSaveOptionName = "save"
)

type peeringResult struct {
Expand Down Expand Up @@ -101,6 +102,9 @@ var swarmPeeringAddCmd = &cmds.Command{
Arguments: []cmds.Argument{
cmds.StringArg("address", true, true, "address of peer to add into the peering subsystem"),
},
Options: []cmds.Option{
cmds.BoolOption(swarmSaveOptionName, "if set, address(es) will be saved to peering config"),
},
Run: func(req *cmds.Request, res cmds.ResponseEmitter, env cmds.Environment) error {
addrs := make([]ma.Multiaddr, len(req.Arguments))

Expand Down Expand Up @@ -130,6 +134,30 @@ var swarmPeeringAddCmd = &cmds.Command{
return err
}
}

save, _ := req.Options[swarmSaveOptionName].(bool)
Jorropo marked this conversation as resolved.
Show resolved Hide resolved
if save {
r, err := fsrepo.Open(env.(*commands.Context).ConfigRoot)
if err != nil {
return err
Copy link
Member

Choose a reason for hiding this comment

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

Could you wrap this with a descriptive error?

}
defer r.Close()
cfg, err := r.Config()
if err != nil {
return err
Copy link
Member

Choose a reason for hiding this comment

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

Could you wrap this with a descriptive error?

Copy link
Author

Choose a reason for hiding this comment

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

Updated in this commit.

}

addrInfos, err := mergeAddrInfo(addInfos, cfg.Peering.Peers)
Copy link
Member

Choose a reason for hiding this comment

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

Correct me if I'm wrong, but we can do the same as we are doing in line 248 to set the peers, avoiding even more duplicated lines.

Copy link
Author

Choose a reason for hiding this comment

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

Good catch! Simplified in this commit.

if err != nil {
return fmt.Errorf("error merging peers: %w", err)
}

cfg.Peering.Peers = addrInfos
if err := r.SetConfig(cfg); err != nil {
return fmt.Errorf("error writing new peers to repo config: %w", err)
}
}

return nil
},
Encoders: cmds.EncoderMap{
Expand Down Expand Up @@ -184,6 +212,9 @@ var swarmPeeringRmCmd = &cmds.Command{
Arguments: []cmds.Argument{
cmds.StringArg("ID", true, true, "ID of peer to remove from the peering subsystem"),
},
Options: []cmds.Option{
cmds.BoolOption(swarmSaveOptionName, "if set, address(es) will be removed from peering config"),
},
Run: func(req *cmds.Request, res cmds.ResponseEmitter, env cmds.Environment) error {
node, err := cmdenv.GetNode(env)
if err != nil {
Expand All @@ -201,12 +232,31 @@ var swarmPeeringRmCmd = &cmds.Command{
return err
}
}

save, _ := req.Options[swarmSaveOptionName].(bool)
ajnavarro marked this conversation as resolved.
Show resolved Hide resolved
if save {
r, err := fsrepo.Open(env.(*commands.Context).ConfigRoot)
if err != nil {
return err
Copy link
Member

Choose a reason for hiding this comment

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

Could you wrap this with a descriptive error?

Copy link
Author

Choose a reason for hiding this comment

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

Updated in this commit.

}
defer r.Close()
cfg, err := r.Config()
if err != nil {
return err
Copy link
Member

Choose a reason for hiding this comment

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

Could you wrap this with a descriptive error?

Copy link
Author

Choose a reason for hiding this comment

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

Updated in this commit.

}
Copy link
Member

Choose a reason for hiding this comment

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

I think we can avoid repeating all this code and separate it into a function.

Copy link
Author

Choose a reason for hiding this comment

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

Factored out into new method updateAndPersistConfig in this commit.


cfg.Peering.Peers = node.Peering.ListPeers()
if err := r.SetConfig(cfg); err != nil {
return fmt.Errorf("error removing peers from repo config: %w", err)
}
}

return nil
},
Type: peeringResult{},
Encoders: cmds.EncoderMap{
cmds.Text: cmds.MakeTypedEncoder(func(req *cmds.Request, w io.Writer, pr *peeringResult) error {
fmt.Fprintf(w, "add %s %s\n", pr.ID.String(), pr.Status)
fmt.Fprintf(w, "rm %s %s\n", pr.ID.String(), pr.Status)
return nil
}),
},
Expand Down Expand Up @@ -1057,3 +1107,17 @@ func filtersRemove(r repo.Repo, cfg *config.Config, toRemoveFilters []string) ([

return removed, nil
}

func mergeAddrInfo(addrInfos ...[]peer.AddrInfo) ([]peer.AddrInfo, error) {
var addrs []ma.Multiaddr
for _, infos := range addrInfos {
for _, addrInfo := range infos {
addr, err := peer.AddrInfoToP2pAddrs(&addrInfo)
if err != nil {
return nil, err
}
addrs = append(addrs, addr...)
}
}
return peer.AddrInfosFromP2pAddrs(addrs...)
}
29 changes: 29 additions & 0 deletions test/sharness/t0140-swarm.sh
Original file line number Diff line number Diff line change
Expand Up @@ -160,6 +160,35 @@ test_expect_success 'peering is removed' '

test_kill_ipfs_daemon

test_launch_ipfs_daemon

peeringID='QmYyQSo1c1Ym7orWxLYvCrM2EmxFTANf8wXmmE7DWjhx5N'
peeringID2='QmYyQSo1c1Ym7orWxLYvCrM2EmxFTANf8wXmmE7DWjhx5K'
peeringAddr='/ip4/1.2.3.4/tcp/1234/p2p/QmYyQSo1c1Ym7orWxLYvCrM2EmxFTANf8wXmmE7DWjhx5N'
peeringAddr2='/ip4/1.2.3.4/tcp/1234/p2p/QmYyQSo1c1Ym7orWxLYvCrM2EmxFTANf8wXmmE7DWjhx5K'

test_expect_success "'ipfs swarm peering add' with save option mutates config" '
ipfs config Peering.Peers > start-peers &&
ipfs swarm peering add ${peeringAddr} ${peeringAddr2} --save &&
ipfs config Peering.Peers > end-peers &&
! test_cmp start-peers end-peers &&
test_should_contain "${peeringID}" end-peers &&
test_should_contain ${peeringID2} end-peers &&
rm start-peers end-peers
'

test_expect_success "'ipfs swarm peering rm' with save option mutates config" '
ipfs config Peering.Peers > start-peers &&
ipfs swarm peering rm ${peeringID} --save &&
ipfs config Peering.Peers > end-peers &&
! test_cmp start-peers end-peers &&
test_should_not_contain "${peeringID}" end-peers &&
test_should_contain ${peeringID2} end-peers &&
rm start-peers end-peers
'

test_kill_ipfs_daemon

test_expect_success "set up tcp testbed" '
iptb testbed create -type localipfs -count 2 -force -init
'
Expand Down