feat: add optional persistence to localnet #2868
Conversation
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughWalkthroughThe pull request introduces several changes across multiple files. Key modifications include the addition of a path to the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Script
participant ConfigFile
participant ExternalJSON
User->>Script: Start Client Script
Script->>ConfigFile: Check for existing config
alt External JSON exists
Script->>ExternalJSON: Load extra-evm-chains.json
ExternalJSON-->>Script: Provide configurations
Script->>ConfigFile: Merge configurations
end
Script->>User: Client setup complete
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? TipsChatThere are 3 ways to chat with CodeRabbit:
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 using PR comments)
Other keywords and placeholders
Documentation and Community
|
gartnera
force-pushed
the
persistent-localnet
branch
2 times, most recently
from
5a69ca9
to
0aeb30c
Compare
orchestrator persistence geth and orchestrator persistence also persist zetaclient dir (observer databases) add extra-evm-chains.json merge into zetaclient config
gartnera
force-pushed
the
persistent-localnet
branch
from
0aeb30c
to
93f5b57
Compare
gartnera
requested review from
brewmaster012,
kingpinXD,
lumtis,
ws4charlie,
skosito,
swift1337 and
fbac
as code owners
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Outside diff range and nitpick comments (1)
contrib/localnet/orchestrator/start-zetae2e.sh (1)
Line range hint
184-193: Consider using a more reliable waiting mechanism and clarify the purpose of runningzetae2etwice.
Using the
deployed_config_pathvariable is consistent and improves maintainability.The 10-second wait might not be sufficient in all scenarios, potentially leading to race conditions. Consider using a more reliable waiting mechanism, such as polling for a specific condition or using an exponential backoff strategy.
The purpose of running
zetae2etwice with different flags is unclear from the given context. Please add comments to clarify the reasoning behind this approach.
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (7)
- .gitignore (1 hunks)
- Makefile (2 hunks)
- contrib/localnet/docker-compose-persistent.yml (1 hunks)
- contrib/localnet/docker-compose.yml (3 hunks)
- contrib/localnet/grafana/grafana.ini (1 hunks)
- contrib/localnet/orchestrator/start-zetae2e.sh (8 hunks)
- contrib/localnet/scripts/start-zetaclientd.sh (1 hunks)
Files skipped from review due to trivial changes (1)
- .gitignore
Additional context used
Path-based instructions (2)
contrib/localnet/orchestrator/start-zetae2e.sh (1)
Pattern
**/*.sh: Review the shell scripts, point out issues relative to security, performance, and maintainability.contrib/localnet/scripts/start-zetaclientd.sh (1)
Pattern
**/*.sh: Review the shell scripts, point out issues relative to security, performance, and maintainability.
Additional comments not posted (12)
contrib/localnet/docker-compose-persistent.yml (5)
1-2: Informative comments.The comments effectively describe the purpose of the file and provide a useful command for clearing the persistent volumes. This enhances the maintainability and usability of the configuration.
4-51: Well-structured services with persistent volumes.The services section is properly configured with appropriate volume mappings for each service. The volume names are descriptive and follow a consistent naming convention. This setup ensures data persistence for critical directories, enhancing the reliability and robustness of the services.
53-71: Properly declared persistent volumes.The volumes section correctly declares all the required named volumes without any extraneous configuration. The volume names match the mappings used in the services section, ensuring proper data persistence for each service.
1-71: Well-structured and organized configuration file.The
docker-compose-persistent.ymlfile is well-organized with clear sections for services and volumes. The separation of concerns and the informative comments at the top enhance the readability and maintainability of the configuration. This structure makes it easy for users and maintainers to understand and work with the file.
1-71: No issues or improvements identified.After a thorough review of the
docker-compose-persistent.ymlfile, no issues or areas for improvement were found. The configuration follows best practices, and the persistent storage setup is properly implemented. The file is ready for integration into the project.contrib/localnet/docker-compose.yml (2)
196-196: Approve the--datadirand--additions to thegethservice.The inclusion of the
--datadiroption and the trailing--in thegethservice'sentrypointis a welcome enhancement. It improves data management and allows for further customization of thegethprocess.
316-316: Approve the Grafana version upgrade andgrafana.inivolume mapping.Updating the Grafana image to version
10.4.8is a positive change that likely introduces new features, improvements, and security patches. This upgrade enhances the overall functionality and stability of the monitoring service.Furthermore, the addition of the
grafana.inivolume mapping is a valuable enhancement. It allows for custom configuration of the Grafana instance, improving its configurability and adaptability to specific requirements.Also applies to: 325-325
contrib/localnet/orchestrator/start-zetae2e.sh (3)
163-165: LGTM!The changes ensure the required directory exists and improve the script's maintainability by using a variable for the file path.
173-174: Looks good!The changes consistently use the
deployed_config_pathvariable, improving the script's maintainability.
217-218: LGTM!The changes consistently use the
deployed_config_pathvariable across the script, improving its maintainability and readability.Also applies to: 232-232, 271-271, 273-273, 289-290, 305-305
Makefile (2)
8-11: Excellent enhancement to improve flexibility!The introduction of the
NODE_COMPOSE_ARGSvariable enables users to pass additional arguments to Docker Compose, such as setting profiles and optional overlays. The provided example in the comments clearly illustrates the usage of this new variable.
228-228: Clear and explicit file resolution for stopping the local network.The inclusion of the
-f docker-compose.ymlflag in thestop-localnettarget ensures that the correct Docker Compose file is used when bringing down the local network. This change enhances clarity and mitigates potential issues related to file resolution.
There was a problem hiding this comment.
Can we add mention of the option in https://github.com/zeta-chain/node/blob/develop/docs/development/LOCAL_TESTING.md?
Also maybe we could consider to have persistence by default when using start-localnet?
Done
I think it might be a bit confusing to change the default. I'd like to get a bit more feedback and fully document the localnet -> real network process before doing that. |
This enables you to change the binaries without resetting the data. Use like this
This will enable you to easily connect your localnet to external chains.
Also upgrade grafana and enable anonymous auth so you don't get prompted for credentials. Example of using grafana too:
Summary by CodeRabbit
Release Notes
New Features
docker-compose-persistent.ymlfor deploying services with persistent data storage.Improvements
Chores
.gitignoreto exclude environment-specific files from version control.