CLI Improvement #236

dora-gt · 2019-08-22T08:48:29Z

Fixes
- Use gumdrop or structopt for CLI option parsing #171 Use gumdrop or structopt for CLI option parsing
- Fix --version flag in CLI #113 Fix --version flag in CLI
- Allow env vars to override config file values #206 Allow env vars to override config file values
- Make settlement-engine to take environmental values #194 Make settlement-engine to take environmental values
- Add documentation for interledger node configuration #215 Add documentation for interledger node configuration
Version is obtained from crate's version
~~Using structopt to reduce the complexity of the source.~~
~~Using #[allow(clippy::large_enum_variant)] because options of account add command are too many. It may be difficult to fix this unless we make a patch to structopt~~
- ~~There is someone doing the same~~

BREAKING CHANGES

Ethereum settlement engine now doesn't accept key argument. Use private_key instead.
Ethereum settlement engine now doesn't accept watch_incoming flag because it was a flag and default_value was true, which doesn't make sense and structopt considers it as an error. Instead I implemented without_incoming_watcher flag which disables incoming watcher.
- ~~structopt: help: message: default_value is meaningless for bool~~
~~node now doesn't accept ilp_address argument. Instead address argument or ILP_ADDRESS env var will be accepted.~~

emschwartz

Right now the configuration of the node and SE works differently. I think we should make them more consistent.

I would suggest allowing the user to use all three methods: CLI arguments, a config file, and env vars. We could make it so you pass the path to the config file as a positional argument (interledger node ./path/to/config) so that it doesn't conflict with the CLI arguments.

The main benefit I see of allowing all options to be passed in as CLI args is that you could get the explanation of all of them by running --help

emschwartz · 2019-08-26T17:05:19Z

crates/interledger-settlement-engines/src/main.rs

+        let mut config = config::Config::new();
+        config.merge(self.clone()).unwrap();
+        config
+            .merge(config::Environment::with_prefix("ILP"))


Should these be prefixed with ILP or something Ethereum-related?

I thought ILP was required at least and adding ETH_SE or something would be too much (it becomes like ILP_ETH_SE_ETHEREUM_ENDPOINT), so... I think just ILP is enough🤔

I just left it as ILP but if there are more suitable options, it would be easy to fix.

crates/interledger-settlement-engines/src/main.rs

crates/interledger-settlement-engines/tests/eth_ledger_settlement.rs

crates/interledger-settlement-engines/src/main.rs

docs/operating-manuals/README.md

docs/operating-manuals/eth-se-configurations.md

docs/operating-manuals/node-configurations.md

examples/eth_xrp_three_nodes/README.md

dora-gt · 2019-08-28T10:57:47Z

I fixed some of the issues but I still have to fix documents. I'm working on this.

The biggest changes are:

We no longer use structopt because recognition complexity problem wasn't solved. Now it looks simpler, I think.
Now every command accepts 1) command line arguments, 2) config file, 3) environment variables.

dora-gt · 2019-08-28T14:00:22Z

I think I've addressed issues, if you have anything else, please tell me.

emschwartz

The direction looks good to me. The main points I'd make are that:

configuration files should be passed in with node config.yml instead of node --config config.yml
we should try to use the required flag on Clap so that the --help prints correctly

crates/interledger-settlement-engines/src/main.rs

crates/interledger/src/main.rs

docs/operating-manuals/README.md

dora-gt · 2019-08-28T23:43:05Z

configuration files should be passed in with node config.yml instead of node --config config.yml

Currently it accepts like this.

we should try to use the required flag on Clap so that the --help prints correctly

I've tried for several hours to hack this... but I couldn't come up with how to cleanly and entirely realize this... Anyway, let me try some more.

dora-gt · 2019-08-28T23:54:52Z

The problem is that if we set required flag, it is parsed BEFORE parameters from configuration files are given. So even if we set a configuration file, it warns that some required parameters are not set. To prevent this,

We might need to hack help
- Negative, there are so many points that generate help strings, we cannot hack this entirely.
We might need to use env or get_matches_from
- Negative, we might need to parse twice or need to parse on our own, which looks a bit ugly.
- That said, if we have any solution, I think this approach is closer to it.

dora-gt · 2019-08-29T10:41:50Z

Refactored entirely.

Config is a positional arg.
Required fields are shown in the help.
Warns if the required fields are not given.
The methodology is:

tarcieri · 2019-08-29T13:21:07Z

crates/interledger-settlement-engines/src/main.rs

+    Ok(path)
+}
+
+fn merge_args(config: &mut Config, matches: &ArgMatches) {


FYI, you get a lot of the logic being added here for free with Abscissa, which allows you to impl this trait to merge command-line options into the configuration:

https://docs.rs/abscissa_core/0.3.0/abscissa_core/config/trait.Override.html

crates/interledger-settlement-engines/src/main.rs

emschwartz · 2019-08-30T05:52:32Z

crates/interledger-settlement-engines/src/main.rs

+        let file_config = file_config.collect().unwrap();
+
+        // if the key is not defined in the given config already, set it to the config
+        // because the original values override the ones from the config file


Should the file take precedence over the CLI args or vice versa?

I don't see the clear difference because, in either case, we can override it by environment variables. hmmm do you have a preference?

crates/interledger-settlement-engines/src/main.rs

crates/interledger/src/main.rs

emschwartz · 2019-08-30T06:06:07Z

crates/interledger/src/main.rs

+            ("accounts", Some(node_accounts_matches)) => match node_accounts_matches.subcommand() {
+                ("add", Some(node_accounts_add_matches)) => {
+                    merge_args(&mut config, &node_accounts_add_matches);
+                    runner.run(get_or_error(config.try_into::<NodeAccountsAddOpt>()));


I think this code would be clearer without the Runner thing. You could just copy that code into here, since it's not actually abstracting anything away

I wanted to make it readable. In the original code, there were many lines between subcommand matches, so it looked hard to read for me. I think this is clearer than inserting the code here because we can understand it intuitively as:

Here is to parse subcommands

There are many subcommands and some of these also have its own subcommands

Subcommands execute some logic using options

I think too long code is hard to understand. Isn't breaking the responsibility into some parts a good method? At least, I've believed so.

ummm but it actually doesn't abstract. Don't you also want to make it just a fn?

Yeah I think it would be simpler to just use a fn instead of the Runner trait

emschwartz · 2019-08-30T06:07:35Z

crates/interledger/tests/three_nodes.rs

@@ -47,7 +47,7 @@ fn three_nodes() {

    let node1 = InterledgerNode {
        ilp_address: Address::from_str("example.one").unwrap(),
-        default_spsp_account: Some(0),
+        default_spsp_account: None,


Shouldn't this be set to one of the accounts?

No need, all tests explicitly declare the accounts they're sending to - pushing a PR which requires this type to always be Username

emschwartz · 2019-08-30T06:08:44Z

docs/operating-manuals/README.md

+
+- Environment variables
+- Configuration files
+- Command line arguments


It's slightly confusing that this list is in the opposite order as the one above. I would just write this list once and clarify there what the prioritization is

gakonst · 2019-08-30T12:00:10Z

Feature request from: #108 (comment)

We want to add an option to load config files from stdin, allowing a user to do cat alice.yaml | interledger node. The use case for this is that we're going to have encrypted config files, and the user will run sops -d alice.yaml | interledger node.

dora-gt · 2019-08-30T12:07:27Z

Which format would you like to pass to STDIN?

I'm asking because unfortunately Config file module was private and I can't reuse the code. I have to write some glue code to parse it though it may be possible to copy as it is licensed under MIT/Apache 2.0.

gakonst · 2019-08-30T12:58:37Z

Which format would you like to pass to STDIN?

So the config module understands which deserializer to use based on the config file extension (.yaml, .json, etc.). Us passing it from STDIN means that the config module cannot parse it. We could manually import serde_yaml and serde_json, and try all until one works.

Hacky idea (doesn't compile)

    let parsed = serde_yaml::from_str(&s);
    if parsed.is_err() {
        parsed = serde_json::from_str(&s);
    }

dora-gt · 2019-08-30T13:37:20Z

Since our CLI supports YAML, TOML, JSON, etc... thanks to Config module, it seems natural that we support these format for STDIN.

Ok, I'll try.

crates/interledger-settlement-engines/src/main.rs

gakonst · 2019-09-05T07:53:07Z

FYI: I've tested encrypting a YAML configuration file with sops and then running a node with sops -d alice.yaml | cargo run -- node and it's working well. It also similarly works well for the ethereum engine, sops -d engine.yaml | cargo run --package interledger-settlement-engines -- ethereum-ledger.

This looks good to me, do we need to add more things?

dora-gt · 2019-09-05T08:55:40Z

Thank you @gakonst , unfortunately I found a bug. If it is run on Docker, it tries to read from STDIN which causes an IO-block. It is because isatty is false on Docker even when it is not piped.

I'm trying to fix this.

dora-gt · 2019-09-05T10:35:41Z

^ was not a problem, just using -t option for Docker.

crates/interledger-settlement-engines/Cargo.toml

crates/interledger-settlement-engines/src/engines/ethereum_ledger/eth_engine.rs

crates/interledger/src/main.rs

emschwartz · 2019-09-05T18:59:32Z

crates/interledger/src/main.rs

+            ("accounts", Some(node_accounts_matches)) => match node_accounts_matches.subcommand() {
+                ("add", Some(node_accounts_add_matches)) => {
+                    merge_args(&mut config, &node_accounts_add_matches);
+                    runner.run(get_or_error(config.try_into::<NodeAccountsAddOpt>()));


Yeah I think it would be simpler to just use a fn instead of the Runner trait

crates/interledger/src/main.rs

docs/operating-manuals/README.md

dora-gt · 2019-09-06T00:25:48Z

Thank you Evan for taking time, I'll check and fix.

dora-gt · 2019-09-06T05:33:14Z

Fixed most points, I'm wondering just: #236 (comment)

emschwartz

Looking good. Let's just change the parameter names and then get this merged in

crates/interledger/src/main.rs

crates/interledger-settlement-engines/src/engines/ethereum_ledger/eth_engine.rs

crates/interledger-settlement-engines/src/main.rs

crates/interledger/src/main.rs

docs/operating-manuals/README.md

examples/eth-settlement/README.md

crates/interledger/src/main.rs

docs/operating-manuals/README.md

Every command accepts arguments, config file settings, env vars. `interledger` and `interledger-settlement-engines` are almost consistent. BREAKING CHANGE: Some arguments are now obsolete. On interledger package: server_secret -> secret_seed, btp_server -> btp_server_url, http_server -> http_server_url, redis_connection -> redis_url, http_address -> http_bind_address, settlement_address -> settlement_api_bind_address, btp_address -> btp_bind_address, btp_uri -> btp_server_url, http_url -> http_server_url. On interledger-settlement-engines package: http_address -> settlement_api_bind_address, ethereum_endpoint -> ethereum_url, redis_uri -> redis_url. Signed-off-by: Taiga Nakayama <dora@dora-gt.jp>

emschwartz · 2019-09-09T16:35:42Z

Merged in as #290

dora-gt requested a review from emschwartz as a code owner August 22, 2019 08:48

dora-gt force-pushed the feature/cli-improvement/main branch 4 times, most recently from 6df6bab to c3641d3 Compare August 23, 2019 14:14

dora-gt changed the title ~~[WIP] DO NOT MERGE: CLI Improvement~~ CLI Improvement Aug 24, 2019

gakonst mentioned this pull request Aug 26, 2019

Improve key storage approach #108

Closed

emschwartz requested changes Aug 26, 2019

View reviewed changes

emschwartz mentioned this pull request Aug 27, 2019

Hackathon prep #249

Closed

15 tasks

dora-gt force-pushed the feature/cli-improvement/main branch from 4557bf6 to b5d75d3 Compare August 28, 2019 13:55

emschwartz requested changes Aug 28, 2019

View reviewed changes

dora-gt force-pushed the feature/cli-improvement/main branch 4 times, most recently from 945b207 to 942ec2a Compare August 29, 2019 13:07

tarcieri reviewed Aug 29, 2019

View reviewed changes

emschwartz reviewed Aug 30, 2019

View reviewed changes

dora-gt mentioned this pull request Aug 30, 2019

feat(eth se): port is deprecated, use http_address instead #259

Merged

gakonst added this to To do in Hackathon Kit Aug 30, 2019

dora-gt force-pushed the feature/cli-improvement/main branch from 8c55ac4 to f55d3c4 Compare August 30, 2019 12:27

gakonst moved this from To do to Under Review in Hackathon Kit Aug 30, 2019

dora-gt force-pushed the feature/cli-improvement/main branch from bf7f5f1 to 04adda5 Compare September 4, 2019 10:56

gakonst reviewed Sep 5, 2019

View reviewed changes

crates/interledger-settlement-engines/src/main.rs Outdated Show resolved Hide resolved

dora-gt force-pushed the feature/cli-improvement/main branch from 1da19d8 to c4b1c36 Compare September 5, 2019 12:02

emschwartz requested changes Sep 5, 2019

View reviewed changes

dora-gt force-pushed the feature/cli-improvement/main branch from 22d7463 to 1d0c8f4 Compare September 6, 2019 06:04

emschwartz reviewed Sep 6, 2019

View reviewed changes

emschwartz mentioned this pull request Sep 6, 2019

Should we make the API field names more consistent with the node configuration? #286

Closed

dora-gt force-pushed the feature/cli-improvement/main branch 7 times, most recently from 61ef567 to f1f5b87 Compare September 9, 2019 04:36

emschwartz approved these changes Sep 9, 2019

View reviewed changes

examples/eth-settlement/README.md Show resolved Hide resolved

crates/interledger/src/main.rs Show resolved Hide resolved

crates/interledger/src/main.rs Show resolved Hide resolved

docs/operating-manuals/README.md Outdated Show resolved Hide resolved

Hackathon Kit automation moved this from Needs Review (PRs only) to Reviewed Sep 9, 2019

dora-gt force-pushed the feature/cli-improvement/main branch 2 times, most recently from 3e66dd8 to d129e09 Compare September 9, 2019 14:26

dora-gt force-pushed the feature/cli-improvement/main branch from d129e09 to 7e2b46e Compare September 9, 2019 14:41

emschwartz mentioned this pull request Sep 9, 2019

CLI Improvement #290

Merged

emschwartz closed this Sep 9, 2019

dora-gt deleted the feature/cli-improvement/main branch September 20, 2019 19:29

emschwartz moved this from Reviewed to Done in Hackathon Kit Oct 2, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

CLI Improvement #236

CLI Improvement #236

dora-gt commented Aug 22, 2019 •

edited

emschwartz left a comment

emschwartz Aug 26, 2019

dora-gt Aug 27, 2019

dora-gt Aug 28, 2019

dora-gt commented Aug 28, 2019 •

edited

dora-gt commented Aug 28, 2019 •

edited

emschwartz left a comment

dora-gt commented Aug 28, 2019 •

edited

dora-gt commented Aug 28, 2019 •

edited

dora-gt commented Aug 29, 2019 •

edited

tarcieri Aug 29, 2019

emschwartz Aug 30, 2019

dora-gt Aug 30, 2019

emschwartz Aug 30, 2019

dora-gt Aug 30, 2019

dora-gt Aug 30, 2019

emschwartz Sep 5, 2019

emschwartz Aug 30, 2019

gakonst Aug 30, 2019 •

edited

dora-gt Aug 30, 2019

emschwartz Aug 30, 2019

gakonst commented Aug 30, 2019

dora-gt commented Aug 30, 2019

gakonst commented Aug 30, 2019 •

edited

dora-gt commented Aug 30, 2019

gakonst commented Sep 5, 2019 •

edited

dora-gt commented Sep 5, 2019 •

edited

dora-gt commented Sep 5, 2019

emschwartz Sep 5, 2019

dora-gt commented Sep 6, 2019

dora-gt commented Sep 6, 2019

emschwartz left a comment

emschwartz commented Sep 9, 2019

CLI Improvement #236

CLI Improvement #236

Conversation

dora-gt commented Aug 22, 2019 • edited

emschwartz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dora-gt commented Aug 28, 2019 • edited

dora-gt commented Aug 28, 2019 • edited

emschwartz left a comment

Choose a reason for hiding this comment

dora-gt commented Aug 28, 2019 • edited

dora-gt commented Aug 28, 2019 • edited

dora-gt commented Aug 29, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gakonst Aug 30, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gakonst commented Aug 30, 2019

dora-gt commented Aug 30, 2019

gakonst commented Aug 30, 2019 • edited

dora-gt commented Aug 30, 2019

gakonst commented Sep 5, 2019 • edited

dora-gt commented Sep 5, 2019 • edited

dora-gt commented Sep 5, 2019

Choose a reason for hiding this comment

dora-gt commented Sep 6, 2019

dora-gt commented Sep 6, 2019

emschwartz left a comment

Choose a reason for hiding this comment

emschwartz commented Sep 9, 2019

dora-gt commented Aug 22, 2019 •

edited

dora-gt commented Aug 28, 2019 •

edited

dora-gt commented Aug 28, 2019 •

edited

dora-gt commented Aug 28, 2019 •

edited

dora-gt commented Aug 28, 2019 •

edited

dora-gt commented Aug 29, 2019 •

edited

gakonst Aug 30, 2019 •

edited

gakonst commented Aug 30, 2019 •

edited

gakonst commented Sep 5, 2019 •

edited

dora-gt commented Sep 5, 2019 •

edited