setting config options from cli may break config

[jbenet]

setting the repo's Log.MaxSizeMB from the cli breaks the config:

> ipfs config Log.MaxSizeMB
0
> ipfs config Log.MaxSizeMB 100
Error: Failed to set config value: Failure to decode config: json: cannot unmarshal string into Go value of type uint64
> ipfs config Log.MaxSizeMB
Error: Failure to decode config: json: cannot unmarshal string into Go value of type uint64
> ipfs config edit
Error: Failure to decode config: json: cannot unmarshal string into Go value of type uint64

the config got a string:

  "Log": {
    "MaxAgeDays": 0,
    "MaxBackups": 0,
    "MaxSizeMB": "100"
  },

changing it to an int fixes it. BUT:

• this shouldnt break, and
• this should be recoverable from the cli.
  objects retrieving values from the config should try to parse them from the string.

[btc]

One part of this problem is that the config is written to disk before it is validated against the Config struct. (Recall: users are allowed to set keys that don't exist on the struct) This is fixed in #742.

[jbenet]

@rht thanks for picking this up.

I've since implemented the --json option, so maybe we could ask users to use --json. Check it out at:
https://github.com/ipfs/go-ipfs/blob/master/core/commands/config.go#L79-L87

not sure, what do you think is most intuitive?

[rht]

Depends on how the config is used. I will check how postfix, vault, etc nail this, and why.

[rht]

• current state
  Since '--json' supersedes '--bool', should we get rid of the latter?
  Otherwise if '--bool' is kept then '--integer' (this issue) and '--number' (float type) need to exist, for consistency.
  The bug (for integer case) itself has been fixed by when setting config keys, validate against struct before writing to disk #742
• what other tools use
  • vault
    value is always stored as string, see https://github.com/hashicorp/vault/blob/master/helper/kv-builder/builder.go#L74-L107
    vault write secret/alice bob={false:true}
    Will be stored as
    {"bob":"{false:true}"}
  • nginx nginx -g "pid /var/run/nginx.pid; worker_processes 2;"
  • postfix postconf -e "postfix_mydestination = localhost"
  • nixos-option not editable from cli
• tests
  • integer
    ipfs config atoms/mol 11666 -> "atoms/mol": 11666
  • number
    • ipfs config atoms/mol 11666.66 -> "atoms/mol": "11666.66"
    • ipfs config --json atoms/mol 11666.66 -> "atoms/mol": 11666.66
    • ipfs config --json atoms/mol "11666.66" -> "atoms/mol": 11666.66
    • ipfs config --json atoms/mol "\"11666.66\"" -> "atoms/mol": "11666.66"

From the test result, for intuitiveness sake I'd say why not assume the value is json by default and then put escaped quote around numbers when you need them to be string (e.g. the value of Version.CheckPeriod? Why this is string I have no idea).

[rht]

@jbenet This is fixed.

[jbenet]

@rht, that's right, thanks!