-
Notifications
You must be signed in to change notification settings - Fork 3.6k
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
refactor(simapp,confix): change example of custom app.toml extension #18496
Conversation
WalkthroughThe recent updates involve refining the application's logging behavior and configuration management. A new log level setting has been introduced to minimize the output to error logs by default, with specific exclusions for peer-to-peer and state logs. Additionally, there's an enhancement in the application configuration setup, incorporating a custom configuration structure. The changes also include cleanup in the configuration migration templates by removing non-SDK related configurations. Changes
TipsChat with CodeRabbit Bot (
|
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.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files ignored due to filter (4)
- tools/confix/data/v0.45-app.toml
- tools/confix/data/v0.46-app.toml
- tools/confix/data/v0.47-app.toml
- tools/confix/data/v0.50-app.toml
Files selected for processing (2)
- simapp/simd/cmd/config.go (4 hunks)
- tools/confix/CHANGELOG.md (1 hunks)
Files skipped from review due to trivial changes (1)
- tools/confix/CHANGELOG.md
Additional comments: 4
simapp/simd/cmd/config.go (4)
15-23: The changes to the
initCometBFTConfig
function set a default log level that displays error logs by default, except forp2p
andstate
which are set to info. This is a sensible default for many applications as it reduces the verbosity of the logs, focusing on errors which are typically more important for monitoring and debugging. However, it's important to ensure that this change is communicated to users who may rely on more verbose logging for their use cases.73-89: The introduction of a
CustomConfig
struct within theinitAppConfig
function allows for extending theapp.toml
configuration with additional custom fields. This is a flexible approach that enables developers to add custom configuration options without modifying the core configuration structs. It's important to ensure that the documentation is updated to reflect how developers can use and extend this custom configuration.98-104: The comment explains the implications of setting
srvCfg.MinGasPrices
to an empty string versus a non-empty value. This is a critical configuration that must be clearly documented, as it affects validator node startup and operation. The choice to setMinGasPrices
to"0stake"
insimapp
is appropriate for a simulation application, but it's crucial that this is not used as a default in production environments where it could lead to spam transactions if validators do not override it.108-125: The
CustomAppConfig
struct is set up with a defaultCustomField
value of"anything"
. This serves as an example for developers on how to set up and use custom fields. The appended custom app template includes the custom field in the configuration file, which is a good practice for making the configuration extensible. It's important to ensure that the parsing and usage of this custom field are properly implemented and tested.
@@ -15,6 +15,9 @@ import ( | |||
func initCometBFTConfig() *cmtcfg.Config { | |||
cfg := cmtcfg.DefaultConfig() | |||
|
|||
// only display only error logs by default except for p2p and state | |||
cfg.LogLevel = "*:error,p2p:info,state:info" |
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.
Thought you may like this @tac0turtle as a default for a prettier simapp logger, but does not change the SDK default.=
is there a need to backport this? |
Yes, later on we can tag a new version of Confix with the updated templates. And Confix is tagged from v0.50 branch |
why does confix need to be aware of this change? this is an example or it has been bleeding elsewhere? |
When I created the SDK default config for Confix, I used simapp, which had this config in its app.toml |
…(backport #18496) (#18497) Co-authored-by: Julien Robert <[email protected]>
🌷 thanks! |
Description
Backporting to eventually tag a future version of confix later when we have more new features/fixes.
Closes: #18494
Author Checklist
All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.
I have...
!
to the type prefix if API or client breaking changeCHANGELOG.md
make lint
andmake test
Reviewers Checklist
All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.
I have...
!
in the type prefix if API or client breaking changeSummary by CodeRabbit
New Features
Bug Fixes
Refactor