Config file by realbigsean · Pull Request #2808 · sigp/lighthouse

realbigsean · 2021-11-12T23:09:43Z

Issue Addressed

Resolves: #2748

Proposed Changes

Added some docs here with some info, there are a couple of quirks.

Upgrade clap to 3.0.4
Allows reading from a YAML or TOML config file.
CLI arguments take precedence over file arguments if both are provided
I added constants for argument keys in order to make it easier to update values in the future. Only for the beacon_node, validator_client, and boot_node subcommands for now though. I sort of needed to do this in order to add a layer of validation to config we load from a file. Using the workaround described below, config values provided from file for the wrong subcommand or that are just entirely foreign would silently fail. So I needed the constants to get an idea of all available flags for each subcommand. Seems like we could use these sets of known flags to generate tests like our existing CLI tests. It's a bit tricky to remember to add to those when adding a new flag.

Additional Info

I was able to figure out how to do this within clap with some workarounds.

Get a copy of all cli args (env::args_os())
Create a cli::App with all of lighthouse's commands and flags.
Consume the initial App (this also consumes cli args) and get back matched arguments
- this initial consumption has to include all commands and flags in order to properly validate the entire command, in case users want to input overriding CLI flags with the --config-file flag.
Check this first set of matched arguments for the --config-file flag. If it exists, read from the file.
Instantiate a second, nearly identical cli::App instance, but as the args are being set, set an additional default_value equal to the matching file config value. This way lighthouse only uses file config value if the value is not passed in via CLI. If the arg has an existing default_value, the latest set default_value takes precedence, so the file config would take precedence over the lighthouse-defined default. This behavior makes sense to me.

Relevant `clap` issues

argfile support might be included in v3.1 Add argfile support clap-rs/clap#1693
Partial argument parsing would make the logic here simpler Partially parse args, capturing unknown args into a vec, rather than error clap-rs/clap#1404

Todo:

allow reading from TOML
self review

…nfig-file � Conflicts: � Cargo.lock � beacon_node/src/cli.rs � boot_node/Cargo.toml � boot_node/src/lib.rs

…nfig-file � Conflicts: � Cargo.lock � beacon_node/src/cli.rs � boot_node/src/config.rs � boot_node/src/lib.rs � common/clap_utils/Cargo.toml � lcli/src/main.rs � lighthouse/src/main.rs � validator_client/Cargo.toml

realbigsean · 2022-01-06T19:00:36Z

+
+    Ok(App::new("beacon_node", file_args)
        .visible_aliases(&["b", "bn", "beacon"])
-        .version(crate_version!())


This is no longer available in clap do we want an alternative? I didn't think crate-level versions seemed really relevant

realbigsean · 2022-01-06T19:04:37Z

        .visible_aliases(&["b", "bn", "beacon"])
-        .version(crate_version!())
        .author("Sigma Prime <contact@sigmaprime.io>")
-        .setting(clap::AppSettings::ColoredHelp)


Removed this because this is now the default setting

…nfig-file � Conflicts: � beacon_node/src/cli.rs

…nfig-file

pawanjay176

Tested this out manually with different combinations of cli and config file parameters and it works as expected :)

I wasn't super happy about parsing the parameters twice, but couldn't come up with anything substantively better after spending quite a lot of time with clap.

Another potential design is to use the clap derive feature to create Config structs.
I have a small self-contained example here. The example parses a Config struct which derives clap::Parser and then updates it with additional parameters from the config file.
https://gist.github.com/pawanjay176/0239b7bb80c2244a60216523654df92c

The derive feature seems to contain everything we currently have and is a bit cleaner than the App::new().arg(...) type builder pattern imo. I haven't fully checked if this pattern would give us further issues when used with subcommands, but seems like it should work fine. It would be a substantial refactor though.

pawanjay176 · 2022-01-12T13:47:57Z

+            .map(toml_value_to_string)
+            .collect::<Result<Vec<_>, _>>()?
+            .join(","),
+        TomlValue::Table(_) => return Err("Unable to parse YAML table".to_string()),


Suggested change

TomlValue::Table(_) => return Err("Unable to parse YAML table".to_string()),

TomlValue::Table(_) => return Err("Unable to parse TOML table".to_string()),

pawanjay176 · 2022-01-12T13:48:18Z

+            .map(yaml_value_to_string)
+            .collect::<Result<Vec<_>, _>>()?
+            .join(","),
+        YamlValue::Mapping(_) => return Err("Unable to parse TOML table".to_string()),


Suggested change

YamlValue::Mapping(_) => return Err("Unable to parse TOML table".to_string()),

YamlValue::Mapping(_) => return Err("Unable to parse YAML table".to_string()),

pawanjay176 · 2022-01-12T14:32:51Z

+```
+Would be equivalent to this YAML config:
+```bash
+$ lighthouse --config-file ./beacon-config.yaml


Suggested change

$ lighthouse --config-file ./beacon-config.yaml

$ lighthouse beacon_node --config-file ./beacon-config.yaml

pawanjay176 · 2022-01-12T14:33:48Z

+```
+And this TOML config:
+```bash
+$ lighthouse --config-file ./beacon-config.toml


Suggested change

$ lighthouse --config-file ./beacon-config.toml

$ lighthouse beacon_node --config-file ./beacon-config.toml

Co-authored-by: Divma <26765164+divagant-martian@users.noreply.github.com>

…nfig-file

realbigsean · 2022-01-20T17:04:53Z

really digging the new link checker! It already helped me find a problem here

thanks @ackintosh !

realbigsean · 2022-01-20T17:12:55Z

@divagant-martian this is the difff for most of your feedback: https://github.com/sigp/lighthouse/compare/a23f0424a33da6a0918924b3e6be71a4f36babfe..9930a278965f269eb823fc427e532d173f0458c6

This is a diff for some doc and formatting fixes/updates: https://github.com/sigp/lighthouse/compare/0ae7ae5ece1cb88d44b49b3eaf03b15618870dbb..80a54e52c984599a2011f363a60ddaecf027baa5

divagant-martian

I agree with @pawanjay176 in attempting a version .2 that leverages structopt, but this change looks good to me as is right now :)

This reverts commit c14d255.

…nfig-file # Conflicts: # account_manager/src/validator/exit.rs # lighthouse/src/main.rs

realbigsean · 2022-02-07T19:49:42Z

I am working on the migration to clap derive here: #3007

So I'm guessing we will end up closing this PR and working off of that one with something like what Pawan suggested.

divagant-martian · 2022-02-07T19:56:38Z

I am working on the migration to clap derive here: #3007

So I'm guessing we will end up closing this PR and working off of that one with something like what Pawan suggested.

I suggest then if you can, to limit this PR only to the standarization of flags. That's a lot of work that we can merge anyway

realbigsean · 2022-03-11T16:15:46Z

Closing for #3079

realbigsean added 4 commits October 21, 2021 11:42

initial commit

993c6af

update clap

8b03b14

clap 3.0 updates

42039b2

Tons of fixes

7bf3c64

realbigsean changed the base branch from stable to unstable November 12, 2021 23:09

realbigsean added the work-in-progress PR is a work-in-progress label Nov 12, 2021

realbigsean mentioned this pull request Nov 12, 2021

Config from file #2754

Closed

realbigsean added 2 commits November 12, 2021 17:52

Merge branch 'unstable' of https://github.com/sigp/lighthouse into co…

299277c

…nfig-file � Conflicts: � Cargo.lock � beacon_node/src/cli.rs � boot_node/Cargo.toml � boot_node/src/lib.rs

merge fixes

810142b

realbigsean mentioned this pull request Nov 15, 2021

Ensure difficulty/hash/epoch overrides change the ChainSpec #2798

Merged

realbigsean added 8 commits November 15, 2021 09:07

Add TOML parsing

4d5e86f

update clap version

d620e05

about -> help

78eae84

refactors and add new App wrapper class

9ddb786

lints and refactors

44c06d3

override_help -> about

2dcd2a1

add some file flag validation, fix some tests, add some docs

eaf2306

realbigsean commented Jan 6, 2022

View reviewed changes

self review

417a667

realbigsean added ready-for-review The code is ready for review and removed work-in-progress PR is a work-in-progress labels Jan 6, 2022

realbigsean added 6 commits January 6, 2022 15:21

fix validation

36923b2

fix enr tests

6e6e789

improve file parsing

0f66b34

Merge branch 'unstable' of https://github.com/sigp/lighthouse into co…

25ab81f

…nfig-file � Conflicts: � beacon_node/src/cli.rs

Merge branch 'unstable' of https://github.com/sigp/lighthouse into co…

6ae306d

…nfig-file

use constants for requires and conflicts_with

37b7ac2

pawanjay176 reviewed Jan 18, 2022

View reviewed changes

realbigsean and others added 13 commits January 20, 2022 10:14

Update common/clap_utils/src/flags.rs

c3ec601

Co-authored-by: Divma <26765164+divagant-martian@users.noreply.github.com>

Update lighthouse/src/main.rs

00bf133

Co-authored-by: Divma <26765164+divagant-martian@users.noreply.github.com>

Update lcli/src/main.rs

bbbe1c0

Co-authored-by: Divma <26765164+divagant-martian@users.noreply.github.com>

Update lighthouse/src/main.rs

4b8e264

Co-authored-by: Divma <26765164+divagant-martian@users.noreply.github.com>

Update lighthouse/src/main.rs

3085a78

Co-authored-by: Divma <26765164+divagant-martian@users.noreply.github.com>

Update serde_yaml and remove macro use

3d057a4

fix formatting in beacon node config file

9930a27

Merge branch 'unstable' of https://github.com/sigp/lighthouse into co…

0ae7ae5

…nfig-file

add file-config link to SUMMARY.md

1412c89

doc fixes and updates

2ac4fbd

fix docs

19e58bd

cargo fmt fix

13f2524

cargo fmt fix

80a54e5

realbigsean mentioned this pull request Jan 20, 2022

[Merged by Bors] - Use "release candidate" in book #2920

Closed

add back advanced-pre-releases.md with the correct link

efeaab9

divagant-martian approved these changes Jan 20, 2022

View reviewed changes

realbigsean added 2 commits January 21, 2022 12:21

test commit signing

c14d255

Revert "test commit signing"

ee8e623

This reverts commit c14d255.

realbigsean mentioned this pull request Jan 25, 2022

Rust doppelganger tests #2960

Closed

Merge branch 'unstable' of https://github.com/sigp/lighthouse into co…

4ac38eb

…nfig-file # Conflicts: # account_manager/src/validator/exit.rs # lighthouse/src/main.rs

realbigsean mentioned this pull request Feb 7, 2022

Clap derive #3007

Closed

realbigsean added do-not-merge and removed ready-for-review The code is ready for review labels Feb 8, 2022

realbigsean closed this Mar 11, 2022

realbigsean deleted the config-file branch November 21, 2023 16:13

Navigation Menu

Repository navigation

Config file - #2808

Config file#2808
realbigsean wants to merge 46 commits into
sigp:unstablefrom
realbigsean:config-file

realbigsean commented Nov 12, 2021 •

edited

Loading

Uh oh!

realbigsean Jan 6, 2022

Uh oh!

realbigsean Jan 6, 2022 •

edited

Loading

Uh oh!

pawanjay176 left a comment

Uh oh!

pawanjay176 Jan 12, 2022

Uh oh!

pawanjay176 Jan 12, 2022

Uh oh!

pawanjay176 Jan 12, 2022

Uh oh!

pawanjay176 Jan 12, 2022

Uh oh!

realbigsean commented Jan 20, 2022 •

edited

Loading

Uh oh!

realbigsean commented Jan 20, 2022

Uh oh!

divagant-martian left a comment

Uh oh!

realbigsean commented Feb 7, 2022

Uh oh!

divagant-martian commented Feb 7, 2022

Uh oh!

realbigsean commented Mar 11, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

	TomlValue::Table(_) => return Err("Unable to parse YAML table".to_string()),
	TomlValue::Table(_) => return Err("Unable to parse TOML table".to_string()),

	YamlValue::Mapping(_) => return Err("Unable to parse TOML table".to_string()),
	YamlValue::Mapping(_) => return Err("Unable to parse YAML table".to_string()),

	$ lighthouse --config-file ./beacon-config.yaml
	$ lighthouse beacon_node --config-file ./beacon-config.yaml

Conversation

realbigsean commented Nov 12, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Issue Addressed

Proposed Changes

Additional Info

Relevant clap issues

Uh oh!

realbigsean Jan 6, 2022

Choose a reason for hiding this comment

Uh oh!

realbigsean Jan 6, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pawanjay176 left a comment

Choose a reason for hiding this comment

Uh oh!

pawanjay176 Jan 12, 2022

Choose a reason for hiding this comment

Uh oh!

pawanjay176 Jan 12, 2022

Choose a reason for hiding this comment

Uh oh!

pawanjay176 Jan 12, 2022

Choose a reason for hiding this comment

Uh oh!

pawanjay176 Jan 12, 2022

Choose a reason for hiding this comment

Uh oh!

realbigsean commented Jan 20, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

realbigsean commented Jan 20, 2022

Uh oh!

divagant-martian left a comment

Choose a reason for hiding this comment

Uh oh!

realbigsean commented Feb 7, 2022

Uh oh!

divagant-martian commented Feb 7, 2022

Uh oh!

realbigsean commented Mar 11, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

realbigsean commented Nov 12, 2021 •

edited

Loading

Relevant `clap` issues

realbigsean Jan 6, 2022 •

edited

Loading

realbigsean commented Jan 20, 2022 •

edited

Loading