-
Notifications
You must be signed in to change notification settings - Fork 291
Feature/dora config command #1299
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Feature/dora config command #1299
Conversation
|
please check the readme.md ,for the understanding of the implementation |
|
ping @haixuanTao @phil-opp |
phil-opp
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the PR!
The config files seems to have no effect right? So you can set your port number etc but this all has no effect.
You also can set arbitrary values with no checking at all. So you can set the log level to "Lorem ipsum" and, the address to true`, and the port number to "I don't care".
I think the proper way to implement a config system is to:
- Define a
Configstruct with all the available options that implements Serialize/Deserialize. Unknown fields should not error for backwards compatibility, but we should warn about them to catch typos (e.g. users writingpotrinstead ofport). - Load the config struct at CLI startup.
- Update the existing commands to default to the config file if the corresponding CLI argument is not given.
- Provide utility commands in the CLI for updating the config file.
This PR seems to implement step 4, which doesn't make much sense without the prior steps.
af865bb to
6903546
Compare
af87742 to
1de7ec7
Compare
|
Thanks for the review! This PR actually implements all 4 steps you outlined: Type-safe Config struct:
Unknown fields: Captured via #[serde(flatten)] and warned about (e.g., Warning: Unknown configuration key: coordinator.potr) to catch typos, as requested. |
#1212
This PR introduces new dora config command that provides unified configuration management for the Dora CLI. This makes it easier to manage coordinator settings and other configuration options without repeatedly specifying command-line flags
The new command provides a complete configuration management interface:
Configuration Operations: Supports list , get, set, and unset subcommands for managing configuration values with intuitive syntax
Layered Configuration: Implements global (~/.dora/config.toml) and project-level (./dora.toml) configuration files, with project settings automatically overriding global ones
Coordinator Settings: Focuses on essential coordinator configuration (coordinator.addr and coordinator.port)
Developer Experience: Automatically creates configuration directories and files, provides clear feedback messages, and supports nested key notation (e.g., coordinator.addr) for intuitive configuration management