feature: Implement --save option in 'ipfs swarm peering add/rm' #9192
Conversation
|
Thank you for submitting this PR!
Getting other community members to do a review would be great help too on complex PRs (you can ask in the chats/forums). If you are unsure about something, just leave us a comment.
We currently aim to provide initial feedback/triaging within two business days. Please keep an eye on any labelling actions, as these will indicate priorities and status of your contribution. |
|
All tests pass for me locally except for generated completions defines '_ipfs'. It fails with this error output: I suspect, based on this thread on the Homebrew mailing list, that this is because I have Bash 3 installed on my Mac. I'll be interested to see if the test passes in CI when someone comes around who is authorized to run them. Marking this as open for review now in hopes that this is just a me issue. |
samuelbacaner
force-pushed
the
swarm-peering-save
branch
from
9bd1404 to
9bc188c
Compare
core/commands/swarm.go
Outdated
| if save { | ||
| r, err := fsrepo.Open(env.(*commands.Context).ConfigRoot) | ||
| if err != nil { | ||
| return err |
There was a problem hiding this comment.
Could you wrap this with a descriptive error?
core/commands/swarm.go
Outdated
| defer r.Close() | ||
| cfg, err := r.Config() | ||
| if err != nil { | ||
| return err |
There was a problem hiding this comment.
Could you wrap this with a descriptive error?
core/commands/swarm.go
Outdated
| defer r.Close() | ||
| cfg, err := r.Config() | ||
| if err != nil { | ||
| return err |
There was a problem hiding this comment.
Could you wrap this with a descriptive error?
core/commands/swarm.go
Outdated
| if save { | ||
| r, err := fsrepo.Open(env.(*commands.Context).ConfigRoot) | ||
| if err != nil { | ||
| return err |
There was a problem hiding this comment.
Could you wrap this with a descriptive error?
core/commands/swarm.go
Outdated
| save, _ := req.Options[swarmSaveOptionName].(bool) | ||
| if save { | ||
| r, err := fsrepo.Open(env.(*commands.Context).ConfigRoot) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| defer r.Close() | ||
| cfg, err := r.Config() | ||
| if err != nil { | ||
| return err | ||
| } |
There was a problem hiding this comment.
I think we can avoid repeating all this code and separate it into a function.
There was a problem hiding this comment.
Factored out into new method updateAndPersistConfig in this commit.
core/commands/swarm.go
Outdated
| return err | ||
| } | ||
|
|
||
| addrInfos, err := mergeAddrInfo(addInfos, cfg.Peering.Peers) |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Good catch! Simplified in this commit.
samuelbacaner
requested review from
ajnavarro and
Jorropo
and removed request for
ajnavarro and
Jorropo
| } | ||
| update(cfg) | ||
| if err := r.SetConfig(cfg); err != nil { | ||
| return fmt.Errorf("error removing peers from repo config: %w", err) |
There was a problem hiding this comment.
If this method is generic for any config param, we should update this error message.
|
Superseded by #9425 |
Resolves #7880 where @Stebalien proposes several new CLI commands. #8147 implements most of the actual functionality.
This PR adds the
--saveoption to bothipfs swarm peering addandipfs swarm peering rm. With this boolean option set, the mutated peering set will be written to the configuration on disk.