feat(test-bins): show config options #346
Conversation
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
2 file(s) reviewed, 2 comment(s)
Edit PR Review Bot Settings | Greptile
test-bins/src/rpc.rs
Outdated
| #[arg( | ||
| long, | ||
| value_name = "COMPONENTS", | ||
| default_value = "(accounts,transactions)", |
There was a problem hiding this comment.
logic: default_value is set but the type is Option<String> - this will always contain a value and never be None
test-bins/src/rpc.rs
Outdated
| // Set environment variables from CLI arguments | ||
| if let Some(keypair) = cli.keypair { | ||
| std::env::set_var("VALIDATOR_KEYPAIR", keypair); | ||
| } | ||
|
|
||
| if let Some(disable_geyser) = cli.disable_geyser { | ||
| std::env::set_var("GEYSER_DISABLE", disable_geyser); | ||
| } | ||
|
|
||
| if let Some(disable_cache) = cli.disable_geyser_cache { | ||
| std::env::set_var("GEYSER_CACHE_DISABLE", disable_cache); | ||
| } |
There was a problem hiding this comment.
logic: setting env vars after parsing them from CLI args could cause issues with override_from_envs() later picking up these modified values unexpectedly
There was a problem hiding this comment.
LGTM
2 file(s) reviewed, no comment(s)
Edit PR Review Bot Settings | Greptile
There was a problem hiding this comment.
Thank for the PR.
I'm not sure that in the long run this makes sense since we're now starting to duplicate some configs that we can either provide via the config file or env vars.
I thought we agreed at some point to find a way to not have to repeat ourselves, but to generate args/env vars + config from one common source so we can maintain that in one place.
I realize that the issue doesn't explain that part at all, but I don't think it's a good idea to add an args implementation that we'll replace most likely fairly soon.
If you want a stop gap for now I think it'd be more useful to create a heavily documented config toml which also mentions the related env vars.
You could then also print that when the user passes -h.
And for that one arg (-h) we don't need to pull in clap, but can manually parse the args for that.
There was a problem hiding this comment.
1 file(s) reviewed, 2 comment(s)
Edit PR Review Bot Settings | Greptile
test-bins/src/rpc.rs
Outdated
| /// Path to the configuration file | ||
| config: Option<String>, |
There was a problem hiding this comment.
style: config parameter needs a long flag (--config) and better help text for discoverability
| Keypair::from_base58_string(&keypair) | ||
| } else { |
There was a problem hiding this comment.
logic: from_base58_string() can panic - needs error handling for invalid base58 strings
| Keypair::from_base58_string(&keypair) | |
| } else { | |
| Keypair::from_base58_string(&keypair).unwrap_or_else(|_| { | |
| panic!("Invalid base58 string provided for validator keypair"); | |
| }) | |
| } else { |
test-bins/src/rpc.rs
Outdated
| #[command(about = "Runs a MagicBlock validator node")] | ||
| struct Cli { | ||
| /// Path to the configuration file | ||
| config: Option<String>, |
There was a problem hiding this comment.
Clap shall support PathBuf. Also I would propose to consider config_path.
Adds a CLI to start the validator. It has a
-hcommand to display configuration and default values. The original interface is preserved to avoid breaking integrations.CLOSES: #318
Greptile Summary
Based on the provided information, I'll create a summary of the pull request and its context:
Adds a CLI interface to the MagicBlock validator to display configuration options and default values, while maintaining backward compatibility with existing configuration methods.
-hflag in/test-bins/src/rpc.rsto display configuration options and defaults, avoiding full clap implementation per maintainer feedback-hflag instead of pulling in clap dependencyThe maintainer's feedback suggests pivoting from a full CLI implementation to a simpler approach focused on documentation and configuration discoverability through a template config file.
💡 (1/5) You can manually trigger the bot by mentioning @greptileai in a comment!