-
Notifications
You must be signed in to change notification settings - Fork 108
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(localnet): reduce number of docker compose files #2365
Conversation
7c1f799
to
39c2e20
Compare
1669503
to
d9b2f23
Compare
Important Review skippedReview was skipped due to path filters Files ignored due to path filters (19)
You can disable this status message by setting the WalkthroughThe updates streamline several workflows by replacing direct Docker Compose commands with Makefile commands, and introduce new environment variables to enhance configuration flexibility. Changes span across GitHub workflow YAML files, Docker Compose configurations, and shell scripts, aiming to improve maintainability and ease of local network management. Changes
Sequence Diagram(s)sequenceDiagram
participant GitHubActions as GitHub Actions
participant Makefile as Makefile
participant DockerCompose as Docker Compose
participant LocalnetEnv as Localnet Environment
GitHubActions->>Makefile: make stop-localnet
Makefile->>DockerCompose: docker compose down
DockerCompose->>LocalnetEnv: Stop services
Poem
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 as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
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.
Actionable comments posted: 6
Outside diff range and nitpick comments (5)
contrib/localnet/docker-compose-monitoring.yml (1)
Line range hint
45-45
: Add a newline at the end of the file.The YAML file should end with a newline character to comply with UNIX text file formatting standards.
contrib/localnet/scripts/start-zetaclientd.sh (1)
Line range hint
10-10
: Consider removing or using theOPTION
variable.The
OPTION
variable is declared but not used in the script, which could lead to confusion and clutter. Consider removing it if it's no longer needed, or ensure it's used appropriately if required.contrib/localnet/docker-compose.yml (2)
Line range hint
26-26
: Formatting issue: Too many spaces after a comma.Ensure consistent formatting by removing the extra spaces after commas to adhere to YAML best practices.
Line range hint
159-159
: Missing new line at end of file.It's a common practice to end files with a newline character to conform to POSIX standards.
contrib/localnet/scripts/start-zetacored.sh (1)
Line range hint
135-135
: Improve the safety of sourcing external scripts.Using
source
with a relative path can lead to issues if the script is not run from the expected directory. Consider using an absolute path or validating the script's presence before sourcing.+ if [[ -f "$HOME/add-keys.sh" ]]; then + source "$HOME/add-keys.sh" + else + echo "Required script add-keys.sh not found." + exit 1 + fi
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (14)
- .github/workflows/build.yml (2 hunks)
- .github/workflows/execute_advanced_tests.yaml (4 hunks)
- .github/workflows/publish-release.yml (1 hunks)
- Makefile (3 hunks)
- contrib/localnet/docker-compose-additionalevm.yml (2 hunks)
- contrib/localnet/docker-compose-monitoring.yml (1 hunks)
- contrib/localnet/docker-compose-stress.yml (5 hunks)
- contrib/localnet/docker-compose-upgrade.yml (3 hunks)
- contrib/localnet/docker-compose.yml (4 hunks)
- contrib/localnet/orchestrator/start-zetae2e.sh (6 hunks)
- contrib/localnet/scripts/import-data.sh (1 hunks)
- contrib/localnet/scripts/start-upgrade-orchestrator.sh (1 hunks)
- contrib/localnet/scripts/start-zetaclientd.sh (2 hunks)
- contrib/localnet/scripts/start-zetacored.sh (3 hunks)
Files skipped from review due to trivial changes (5)
- .github/workflows/build.yml
- .github/workflows/execute_advanced_tests.yaml
- .github/workflows/publish-release.yml
- contrib/localnet/docker-compose-additionalevm.yml
- contrib/localnet/scripts/import-data.sh
Additional context used
yamllint
contrib/localnet/docker-compose-monitoring.yml
[error] 45-45: no new line character at the end of file (new-line-at-end-of-file)
contrib/localnet/docker-compose-upgrade.yml
[error] 45-45: no new line character at the end of file (new-line-at-end-of-file)
contrib/localnet/docker-compose-stress.yml
[error] 73-73: no new line character at the end of file (new-line-at-end-of-file)
contrib/localnet/docker-compose.yml
[warning] 26-26: too many spaces after comma (commas)
[error] 159-159: no new line character at the end of file (new-line-at-end-of-file)
Shellcheck
contrib/localnet/scripts/start-zetaclientd.sh
[warning] 10-10: OPTION appears unused. Verify use (or export if used externally). (SC2034)
contrib/localnet/scripts/start-zetacored.sh
[warning] 135-135: ShellCheck can't follow non-constant source. Use a directive to specify location. (SC1090)
Gitleaks
contrib/localnet/scripts/start-upgrade-orchestrator.sh
4-4: Detected a Generic API Key, potentially exposing access to various services and sensitive operations. (generic-api-key)
Additional comments not posted (6)
contrib/localnet/docker-compose-upgrade.yml (1)
35-43
: Refactor to use environment variable forUPGRADE_HEIGHT
.This change aligns with the PR's objective to manage configurations more effectively through environment variables. Ensure that the
UPGRADE_HEIGHT
variable is properly set in the environment where this Docker Compose is executed.contrib/localnet/docker-compose-stress.yml (1)
7-8
: Use ofZETACORED_REPLICAS
to manage replicas dynamically.Introducing
ZETACORED_REPLICAS
as an environment variable enhances flexibility in configuring the number of node replicas during stress tests. This change is consistent with the PR's goal of better configuration management.Also applies to: 11-12, 25-25, 41-41
contrib/localnet/scripts/start-zetaclientd.sh (1)
66-66
: Conditional use ofADDITIONAL_EVM
for chain configuration.The script now uses the
ADDITIONAL_EVM
environment variable to conditionally apply chain configurations. This change makes the script more flexible and aligns with the PR's objective of improving environment variable management.Also applies to: 85-85
contrib/localnet/orchestrator/start-zetae2e.sh (1)
78-78
: Enhanced conditional logic and environment variable usage.The modifications to use
LOCALNET_MODE
andUPGRADE_HEIGHT
variables improve the flexibility and configurability of the script. The conditional checks and default settings are well-implemented.Also applies to: 83-83, 100-100, 142-142, 162-162, 171-171, 177-177
Makefile (2)
28-33
: Environment variables added for better configuration management.The introduction of environment variables such as
LOCALNET_MODE
,E2E_ARGS
,UPGRADE_HEIGHT
,ZETACORED_IMPORT_GENESIS_DATA
, andZETACORED_START_PERIOD
enhances the flexibility and configurability of the build process.
232-238
: Usage of environment variables in Docker commands.The use of environment variables in Docker compose commands for different testing scenarios (
E2E_ARGS
,LOCALNET_MODE
,UPGRADE_HEIGHT
,ZETACORED_IMPORT_GENESIS_DATA
) is a good practice, making the build process more dynamic and adaptable to different environments.Also applies to: 242-242, 253-261, 267-274
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.
LGTM; Small note: can you briefly describe new env vars in LOCAL_TESTING.md
?
c2f838d
to
1f4987c
Compare
Description
Reduce number of docker compose files by instead injecting environment variables where possible. Use these environment variables in conditionals rather than command line arguments also.
How Has This Been Tested?
Please describe the tests that you ran to verify your changes. Include instructions and any relevant details so others can reproduce.
Checklist:
Summary by CodeRabbit