Skip to content

Another Config File PR#3079

Closed
realbigsean wants to merge 4 commits intosigp:unstablefrom
realbigsean:argfile
Closed

Another Config File PR#3079
realbigsean wants to merge 4 commits intosigp:unstablefrom
realbigsean:argfile

Conversation

@realbigsean
Copy link
Member

@realbigsean realbigsean commented Mar 11, 2022

Issue Addressed

#2748

Proposed Changes

  • Allows reading from a YAML or TOML config file.
  • CLI arguments take precedence over file arguments if both are provided
  • This also includes a fix to a documentation issue I stumbled on where advanced-pre-releases was linked when advanced-release-candidates.md should be

Additional Info

The solution here is a combination of what @pawanjay176 suggested here and what the argfile crate provides.

Pawan's solution required using clap to parse and validate the config file flag, and then using clap_derive's update_from to update/validate the original command with the file config. But this requires the extensive migration here, which may take a while to get merged.

argfile circumvents the need for clap_derive's update_from by parsing the config file directly from the cli args and expanding it prior to doing any clap parsing. But it provides no ability to override file config with cli args and requires using a syntax along the lines of lighthouse bn @argfile which is unlike what is provided by other consensus clients.

This solution mimics argfile's preprocessing but is more suited to lighthouse's specific needs. It also does not depend on or conflict with #3007.

@realbigsean realbigsean mentioned this pull request Mar 11, 2022
2 tasks
@realbigsean realbigsean added the ready-for-review The code is ready for review label Mar 11, 2022
@paulhauner
Copy link
Member

Ooh, this is interesting! Very simple to implement. It is a shame you can't override with the CI, but it does make things simpler to reason about!

But it provides no ability to override file config with cli args and requires using a syntax along the lines of lighthouse bn @argfile which is unlike what is provided by other consensus clients.

Looking at the PR, it seems we'd use lighthouse bn --config-file rather than the @argfile?

@realbigsean
Copy link
Member Author

But it provides no ability to override file config with cli args and requires using a syntax along the lines of lighthouse bn @argfile which is unlike what is provided by other consensus clients.

^ These are limitations of the argfile crate which is why I didn't use that crate in this PR. In this PR you can override file config with CLI and use lighthouse bn --config-file

@paulhauner
Copy link
Member

I'm going to drop the ready-for-review tags due to conflicts. Sorry about letting this one go stale, we can chat about it sometime :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

backlog PR is not currently prioritized work-in-progress PR is a work-in-progress

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants