Changed option is ignored

gdkrmr commented 9 months ago

Checklist

[X] This is a bug report, not a question. Ask questions on discuss.ipfs.io.
[X] I have searched on the issue tracker for my bug.
[X] I am running the latest kubo version or have an issue updating.

Installation method

ipfs-desktop

Version

This was on the docker version. 

Kubo version: 0.24.0-e70db65
Repo version: 15
System version: amd64/linux
Golang version: go1.21.4

Config

No response

Description

Changing the config

ipfs config --json Experimental.FilestoreEnabled true

and then running (without restarting, as in the documentation)

ipfs add --nocopy ...

will ignore the --nocopy and add everything as blocks. This should probably give an error.

lidel commented 9 months ago

Thank you for surfacing this UX problem @gdkrmr. Yes, this looks like a bug – add --nocopy should error.

I think this is something not limited to FilestoreEnabled, we have many flags that don't take effect without restarting, such as entire Routing, including Routing.AcceleratedDHTClient.

Fix proposal

This is a good opportunity to improve UX around config changes.

Create a global config Flag named RequiresRestart and setting it to true every time when config change is applied.
- Have an ignore list of config fields that don't require restart.
- Why? Because it is shorter, and we won't miss new cofing keys added in the future. Default is to require restart, unless your command does not require it.
- :point_right: From the top of my head (we can expand later), configs that work without restart are limited to:
  - Pinning.RemoteServices
  - ...is there anything else?
- Reset the flag during ipfs daemon.
Then, check for flag in sensitive commands like ipfs add --nocopy
- If the RequiresRestart is set to true, error asking user to restart first.
- :point_right: :point_right: From the top of my head (we can expand later), we should check the flag the beginning of all "mutable" and "publish" commands such as:
- ipfs add, ipfs block put, ipfs dag put|import, ipfs files mkdir|write – we will add ability to adjust default CID parmeters in the future, so config impact won't be limited to add --nocopy
- ipfs routing provide (impacted by AcceleratedDHTClient)
- ipfs name publish (impacted by IPNS config params)`
There should also be a periodical check that also warns users who should restart, like we did in https://github.com/ipfs/kubo/pull/9886

Jonath-z commented 7 months ago

@gdkrmr can look into this? First time contributing to open source ;-), I would love just more guidance to work on this

ipfs / kubo