tox-node icon indicating copy to clipboard operation
tox-node copied to clipboard

`config` should be an option

Open suhr opened this issue 5 years ago • 8 comments

This issue proposes to remove config subcommand in favour of --config option.

Motivation

An english dictionary gives the following definition of the word “subcommand”:

(computing) A command that makes up part of a larger command. This command accepts additional subcommands as parameters.

In command line interfaces, subcommands are used to execute specific actions. Some examples of various subcommands:

cargo build --example=foo
git commit -m "Initial commit"
http get https://github.com/

At this moment, tox-node uses config subcommand for running a node with a configuration file, and command line options to run a node without a configuration file. The main drawback of this approach is that it violates the subcommand semantics: config is not an action. Beside that, this approach provides no way to override the parameters specified in the configuration file.

The distinction between the confiuration file option and other options was created to avoid the problem of mixing the configuration file with configuration options. This issue proposes a different solution: configuration merging.

Detailed design

  1. Remove the config subcommand
  2. Add the --config cli option instead
  3. Implement merging cli options with options in an configuration file

The left merge of configurations

A configuration A is a set of pairs (N, V), such as if (N, V_1) ∈ A and (N, V_2) ∈ A, than V_1 = V_2. The first element of the pair is called a name and the second is called a value. We will write configurations as {N_1 = V_1, N_2 = V_2}.

We say that name n is in A if there exists such v that (n, v) ∈ A.

A symmetric difference of configurations A and B is a configuration that contains pairs from both sets with names only in A or only in B. Symbolically, this is can be written as A ∆ B = {(n, v) | ((n, v) ∈ A ∧ (n, _) ∉ B) ∨ ((n, v) ∈ B ∧ (n, _) ∉ A)}.

A left merge of configurations A and B, written A < B, is defined as this:

  1. A ∆ B ⊂ A < B
  2. If (n, a) ∈ A and (n, b) ∈ B than (n, b) ∈ A < B

Proposition: a left merge is a monoid with empty configuration as the identity.

Indeed:

  1. {} < A = A < {} = A, since A ∆ {} = A
  2. ({x = a} < {x = b}) < {x = c} = {x = a} < ({x = b} < {x = c}) = {x = c}

Merging the configuration file with cli options

The result of merging the configuration file with cli options is the left merge of configurations. In other words, cli options have a priority over the configuration file, so when a parameter is defined by both the configuration file and a cli option, the cli option overrides parameter specified in the configuration file.

This allows to use the same configuration file for different configurations of the tox node. For example, a user may change the used key file with --keys-file option, without changing the configuration file.

Alternatives

  • Deprecate the config subcommand instead of removing
  • Do not merge the configuration, instead return an error when the --config option is used with other options.

How do we teach this?

The README.md already describes config to be an option instead of a subcommand.

Drawbacks

  • The change breaks scripts that use config as a subcommand
  • Slight increase of cli code complexy because of configuration merging

suhr avatar Mar 28 '19 16:03 suhr

The main drawback of this approach is that it violates the subcommand semantics: config is not an action.

It can be considered as action "read config and run node", when default action is "run node". Your example with cargo demonstrates exact same thing - cargo test builds project first with cargo build --tests and only then runs tests. git also has such commands, e.g. git pull = git fetch + git merge. So it's not very clear when one command ends and another begins.

Beside that, this approach provides no way to override the parameters specified in the configuration file.

Do we really need it? Also there is another feature that often present in unix utils: arguments that are supposed to be unique can be specified multiple times. This way later arguments override earlier ones. This allows to alias commands with arguments and then override these arguments running aliases. Also to be able to override flags there are reversed flags, like --arg and --no-arg. So how far should we go? I have an impression that all this is contrary to the clap's approach.

Proposition: a left merge is a monoid with empty configuration as the identity.

This might be confusing for lists. For example, we have bootstrap nodes list and one might expect that specifying node via CLI will add it to the list but not override the whole list of nodes specified in the config.

Drawbacks

  • The change breaks scripts that use config as a subcommand
  • Slight increase of cli code complexy because of configuration merging

There is one more major drawback - this way we won't be able to use clap's validation and default arguments declaration. We'll have to assume that all CLI parameters are optional because they can be specified in config. So we'll have to check each required parameter manually and if it's not present in both config and CLI we'll have to panic. That was the reason why we used subcommand in the first place.

kurnevsky avatar Mar 28 '19 17:03 kurnevsky

So it's not very clear when one command ends and another begins.

Still there's a very clear generic action (test the project, pull from a remote), unlike config.

There is one more major drawback - this way we won't be able to use clap's validation.

I see. Well, then it's probably easier to just return an error when --config is used with other options.

suhr avatar Mar 28 '19 18:03 suhr

Still there's a very clear generic action (test the project, pull from a remote), unlike config.

But what if it was called run-with-config? Also can we name it with dashes, just like --config argument?

I see. Well, then it's probably easier to just return an error when --config is used with other options.

This also messes with parsing a bit. --config will have to conflict with all arguments that don't have default value. Also instead of require we will have to use require_unless("--config") (though we don't have much requires right now).

kurnevsky avatar Mar 28 '19 18:03 kurnevsky

This allows to use the same configuration file for different configurations of the tox node. For example, a user may change the used key file with --keys-file option, without changing the configuration file.

This is clear for --keys-file, how about --bootstrap-node? How about default values? --threads = 1 if nothing is set.

kpp avatar Mar 28 '19 18:03 kpp

Also can we name it with dashes, just like --config argument?

Sounds like a hack.

How about default values? --threads = 1 if nothing is set.

Defaults < configuration file < cli arguments.

suhr avatar Mar 29 '19 11:03 suhr

Defaults < configuration file < cli arguments.

Why? When there could be an easy way to use only Defaults < configuration file. Because simple logic > complicated logic.

kpp avatar Mar 29 '19 12:03 kpp

Yes, I agree that this kind of logic is probably too complicated. So the most reasonable solution is indeed to forbid using --config with other arguments.

suhr avatar Mar 29 '19 13:03 suhr

This is possible with https://docs.rs/clap/2.32.0/clap/struct.Arg.html#method.conflicts_with_all , but we will have to list every arg in conflicts_with_all(&[arg1, arg2, arg3]) and add require_unless("--config") for required args. Or ask clap-rs to add a method named unique for such args =)

It will bloat the code, so we have to cover it with tests.

But what if it was called run-with-config?

How about that? or with-config?

kpp avatar Mar 29 '19 16:03 kpp