Skip to content
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

fix: config.toml and app.toml overwrites #8118

Merged
merged 20 commits into from
Apr 24, 2024
Merged

Conversation

czarcas7ic
Copy link
Member

@czarcas7ic czarcas7ic commented Apr 21, 2024

Closes: #7905

What is the purpose of the change

This code is currently really odd due to being in between the previous and currently expected behavior.

Previously, we only overwrote the config/app values if the value matched a specific value. We now just overwrite regaurdless (unless a specific flag is provided to not overwrite).

This PR standardizes the logic between the two methods, specifically addressing:

  • Single quote matching (fix: Allow single-quotes for arbitrage-min-gas-fee #8056)
  • Configs with special permissions being overwritten with the default config.toml
  • We now don't overwrite commented out configs (IFF its not one of the overwrited values). This allows configs to be commented out and/or allows personal notes in configs without being overwritten.

Testing and Verifying

Tested the following:

  • Initialization still behaves as intended
  • Making a custom change to one of the overwrite values and then removing write permissions prevents the file from being overwritten, but gracefully logs and continues
    Screenshot 2024-04-21 at 8 41 18 PM
  • Special case of wasm config simulation_gas_limit is handled (had to special case logic due to requirement of it being commented out as a valid value)

Summary by CodeRabbit

Summary by CodeRabbit

  • New Features
    • Introduced custom configuration options for better control over application behavior.
  • Improvements
    • Enhanced the configuration overwrite mechanism to prevent unintended changes.
  • Documentation
    • Updated the CHANGELOG with details on recent improvements and features.

@czarcas7ic czarcas7ic added V:state/compatible/backport State machine compatible PR, should be backported A:backport/v24.x backport patches to v24.x branch labels Apr 21, 2024
@czarcas7ic czarcas7ic marked this pull request as ready for review April 22, 2024 05:56
Copy link
Contributor

coderabbitai bot commented Apr 22, 2024

Walkthrough

The osmosis project's latest update focuses on improving configuration management. Enhancements center around Config.toml and app.toml files, with new features for overwriting and initializing configurations. The codebase introduces new structures and functions to support these changes, ensuring more robust and flexible configuration settings for users.

Changes

File Change Summary
CHANGELOG.md Reflects enhancements in overwriting Config.toml and app.toml.
cmd/osmosisd/cmd/root.go Significant updates: new imports, structures, constants, functions for improved configuration management.

🐇✨
To the code, to the files, a change has come,
Configs dance in the light of the Osmosis sun.
New templates bloom, and structures align,
In the realm of toml, a new design.
🌟📝


Recent Review Details

Configuration used: CodeRabbit UI
Review profile: CHILL

Commits Files that changed from the base of the PR and between 2e6fd7b and ae196d5.
Files selected for processing (1)
  • cmd/osmosisd/cmd/root.go (11 hunks)
Additional comments not posted (6)
cmd/osmosisd/cmd/root.go (6)

99-103: New struct SectionKeyValue introduced.

This struct will facilitate the handling of configuration key-value pairs, which is aligned with the PR's objectives to enhance configuration management.


106-124: Addition of recommendedAppTomlValues and recommendedConfigTomlValues.

These slices store default configuration values, which supports the new logic for configuration overwriting. Ensure these values are well-documented and validated against the application's requirements.


459-480: > 📝 NOTE

This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [437-498]

Refactored overwriteConfigTomlValues function to support new configuration logic.

The function now checks against recommended values before overwriting, which is a good practice. However, ensure that the error handling and recovery mechanisms are thoroughly tested, especially the new logic around file permissions and recovery from panics.


504-559: Refactored overwriteAppTomlValues function to align with new configuration logic.

Similar to overwriteConfigTomlValues, this function has been updated to check values before overwriting. It's crucial to verify that the conditions under which the configuration is not overwritten are correctly implemented and tested.


593-623: > 📝 NOTE

This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [583-641]

Introduction of OsmosisMempoolConfig and CustomAppConfig structs.

These new configurations enhance the flexibility of the application by allowing custom settings for the mempool and other components. It's important to ensure that these configurations are compatible with existing setups and that migration paths are clear for users updating to this version.


1062-1143: New function OverwriteWithCustomConfig.

This function introduces a more dynamic way to handle configuration file updates. It's essential to ensure that the function correctly handles different data types and that the file operations are error-proof. Consider adding more robust error handling and possibly unit tests to cover various edge cases.


Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media?

Share
Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai generate interesting stats about this repository and render them as a table.
    • @coderabbitai show all the console.log statements in this repository.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.

Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments.

CodeRabbit Commands (invoked as PR comments)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger a review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai help to get help.

Additionally, you can add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.

CodeRabbit Configration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

Actionable comments outside the diff hunks (3)
CHANGELOG.md (3)

Line range hint 325-325: Correct the punctuation in the heading.

-## [v6.4.0]
+## v6.4.0

Line range hint 986-1064: Adjust heading levels to increment by one level at a time.

-#### Golang API breaks
+### Golang API breaks
-#### Bug Fixes
+### Bug Fixes
-#### Features
+### Features
-#### SDK fork updates
+### SDK fork updates
-#### IAVL fork updates
+### IAVL fork updates

Line range hint 588-646: Provide descriptive text for bare URLs.

-https://github.com/osmosis-labs/osmosis/releases/tag/v6.4.0
+[Osmosis v6.4.0 Release](https://github.com/osmosis-labs/osmosis/releases/tag/v6.4.0)
-https://github.com/osmosis-labs/osmosis/releases/tag/v6.3.1
+[Osmosis v6.3.1 Release](https://github.com/osmosis-labs/osmosis/releases/tag/v6.3.1)

@czarcas7ic
Copy link
Member Author

Going to convert to draft, want to make a couple more improvements

@czarcas7ic czarcas7ic marked this pull request as draft April 23, 2024 17:48
Comment on lines +106 to +124
recommendedAppTomlValues = []SectionKeyValue{
{
Section: "osmosis-mempool",
Key: "arbitrage-min-gas-fee",
Value: "0.1",
},
{
Section: "osmosis-mempool",
Key: "max-gas-wanted-per-tx",
Value: "60000000",
}}

recommendedConfigTomlValues = []SectionKeyValue{
{
Section: "consensus",
Key: "timeout_commit",
Value: "2s",
},
}
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With this abstraction, for all future changes all we need to do is add to this var, and all the rest is taken care of!

// Optionally allow the chain developer to overwrite the SDK's default
// server config.
srvCfg := serverconfig.DefaultConfig()
srvCfg.API.Enable = true
srvCfg.StateSync.SnapshotKeepRecent = 2
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Drive by

@czarcas7ic czarcas7ic marked this pull request as ready for review April 23, 2024 20:13
Copy link
Collaborator

@PaddyMc PaddyMc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

GG, works well!

@czarcas7ic czarcas7ic merged commit abb6ccf into main Apr 24, 2024
1 check passed
@czarcas7ic czarcas7ic deleted the adam/fix-config-overwrites branch April 24, 2024 00:51
mergify bot pushed a commit that referenced this pull request Apr 24, 2024
* fix config and app toml overwrites

* changelog

* more fixes

* changelog name

* push

* push

* clean up

* reduce diff

* comments

* print propper

* lint

* only change the respective lines, rather than the entire file

* reduce diff

* abstraction

* even more abstraction

* add in line comments

* update config

* in line comments

* fix overwrite bug

* reduce diff

(cherry picked from commit abb6ccf)

# Conflicts:
#	CHANGELOG.md
czarcas7ic added a commit that referenced this pull request Apr 24, 2024
* fix: config.toml and app.toml overwrites (#8118)

* fix config and app toml overwrites

* changelog

* more fixes

* changelog name

* push

* push

* clean up

* reduce diff

* comments

* print propper

* lint

* only change the respective lines, rather than the entire file

* reduce diff

* abstraction

* even more abstraction

* add in line comments

* update config

* in line comments

* fix overwrite bug

* reduce diff

(cherry picked from commit abb6ccf)

# Conflicts:
#	CHANGELOG.md

* Update CHANGELOG.md

---------

Co-authored-by: Adam Tucker <[email protected]>
@github-actions github-actions bot mentioned this pull request May 1, 2024
@github-actions github-actions bot mentioned this pull request May 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A:backport/v24.x backport patches to v24.x branch V:state/compatible/backport State machine compatible PR, should be backported
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Investigate config and app toml override logic
2 participants