-
Notifications
You must be signed in to change notification settings - Fork 5
feat: add global startup method configuration for snapshot and mithril #425
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
Conversation
| // Check if this module is the selected startup method | ||
| let startup_method = config | ||
| .get::<StartupMethod>(CONFIG_KEY_START_UP_METHOD) | ||
| .unwrap_or(StartupMethod::Mithril); |
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.
I think it would make sense for a helper function which did this to live in common, specifically so that we automatically get the same default value everywhere. But I don't think that's critical enough to block the PR
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.
I like that, something like this perhaps?
use config::Config;
pub const CONFIG_KEY_STARTUP_METHOD: &str = "startup.method";
impl StartupMethod {
pub fn from_config(config: &Config) -> Self {
config
.get::<StartupMethod>(CONFIG_KEY_STARTUP_METHOD)
.unwrap_or(StartupMethod::Mithril)
}
}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.
Done here ede0b1c
whankinsiv
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.
My only feedback is to move the key definition to common and also include the default value. const CONFIG_KEY_START_UP_METHOD: (&str, &str) = ("startup.method", StartupMethod::Mithril);
Description
Introduces a
[global]configuration section that gets merged as a base layer for every module's config. This allows shared settings to be defined once rather than duplicated across multiple[module.*]sections.Module-specific values take precedence over global values when keys collide. This requires the Caryatid PR to be merged for the new versions to be picked up.
Both modules can now call
config.get("startup.method")to decide whether to proceed or exit early—without duplicating the setting in each module's section.Related Issue(s)
#220 & #395
How was this tested?
"mithril"and"snapshot"as thestartup.methodChecklist
Impact / Side effects