-
Notifications
You must be signed in to change notification settings - Fork 346
feat: generate a commented out config.toml so defaults can be changed #1536 #1868
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: v0.38.x-celestia
Are you sure you want to change the base?
Conversation
|
does this break the overriding of configs from app, where now we have to modify things here? |
It shouldn't. If a field is empty the default is used which I believe should be the default we override with unless we force write the new default values |
|
don't think we should merge this into v0.38 (originally made sense to target ofc but we've since switched) |
|
I think this could break some validator setup scripts, and people use sed to change configs |
Yes I can imagine it will (for example setting up state sync with the trusted height and hash). If we don't want this feature, I can close the PR. It just makes it difficult to tweak defaults (which we've done a couple times in the past) |
ninabarbakadze
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.
do we still want this? @cmwaters
|
It's not relevant for now. I think we have update-config and the overrides in the short term. I think longer term requires more of a discussion |
Description
Tested this using the single-node.sh script in celestia-app. Note that the moniker should stay as it's set in the init and sometimes used by the CLI
Closes: #1518