chore: improve localnet build performance #2928
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 📝 Walkthrough📝 Walkthrough📝 WalkthroughWalkthroughThe pull request introduces significant modifications across several files, primarily focusing on the Docker and Makefile configurations. Key changes include the introduction of new versioning arguments ( Changes
Possibly related PRs
Suggested labels
Suggested reviewers
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
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: 1
🧹 Outside diff range and nitpick comments (4)
version.sh (3)
3-8: Approve the addition of NODE_VERSION check with a minor suggestion.The new conditional check for NODE_VERSION is a valuable addition. It enhances performance and maintainability by allowing version determination without git operations, which is particularly useful for Docker builds.
Consider adding input validation for NODE_VERSION to ensure it's not empty or malformed:
if [[ -n $NODE_VERSION ]]; then + if [[ ! $NODE_VERSION =~ ^[0-9]+\.[0-9]+\.[0-9]+(-[a-zA-Z0-9.-]+)?$ ]]; then + echo "Error: NODE_VERSION is not in a valid semantic versioning format" >&2 + exit 1 + fi echo $NODE_VERSION exit fiThis change would improve robustness by ensuring NODE_VERSION adheres to semantic versioning format before using it.
19-22: Approve the enhancement of version string with a suggestion for clarity.The addition of the short commit hash to the version string is a commendable improvement. It provides more detailed version information, which enhances traceability and debugging capabilities.
To improve clarity and maintainability, consider using more descriptive variable names:
-commit_timestamp=$(git show --no-patch --format=%at) -short_commit=$(git rev-parse --short HEAD) +commit_unix_timestamp=$(git show --no-patch --format=%at) +short_commit_hash=$(git rev-parse --short HEAD)This change makes the purpose of each variable immediately clear to future maintainers.
25-25: Approve the modified version string output with a suggestion for consistency.The changes to the version string output for both dirty and non-dirty builds are beneficial. They provide more detailed and consistent version information, which is crucial for debugging and tracking builds.
To maintain consistency with semantic versioning and improve readability, consider restructuring the version string:
-echo "0.0.${commit_timestamp}-${short_commit}-dirty" +echo "0.0.0-${commit_timestamp}.${short_commit}.dirty" -echo "0.0.${commit_timestamp}-${short_commit}" +echo "0.0.0-${commit_timestamp}.${short_commit}"This format aligns better with semantic versioning principles and improves parsing consistency.
Also applies to: 29-29
Makefile (1)
236-236: Incorporation of version information in Docker buildThe addition of
NODE_VERSIONandNODE_COMMITas build arguments in the Docker build command is a positive change, aligning with the improved versioning strategy. However, the line length could be reduced for better readability.Consider breaking the long command into multiple lines for improved readability:
- $(DOCKER) build -t zetanode --build-arg NODE_VERSION=$(NODE_VERSION) --build-arg NODE_COMMIT=$(NODE_COMMIT) --target latest-runtime -f ./Dockerfile-localnet . + $(DOCKER) build -t zetanode \ + --build-arg NODE_VERSION=$(NODE_VERSION) \ + --build-arg NODE_COMMIT=$(NODE_COMMIT) \ + --target latest-runtime \ + -f ./Dockerfile-localnet .
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (4)
- .dockerignore (1 hunks)
- Dockerfile-localnet (1 hunks)
- Makefile (3 hunks)
- version.sh (2 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
version.sh (1)
Pattern
**/*.sh: Review the shell scripts, point out issues relative to security, performance, and maintainability.
🔇 Additional comments (5)
.dockerignore (3)
7-7: Commendable addition of.gitto the ignore list.This change aligns with the PR objective of not copying the git directory into the build. It will significantly reduce the size of the build context and improve build performance.
Line range hint
9-13: Appropriate exclusion of Dockerfile and docker-compose files.Excluding Dockerfile and docker-compose files from the Docker build context is a good practice. These files are not needed inside the Docker build and their exclusion can help reduce the build context size.
Line range hint
1-6: Verify the necessity of including previously ignored files.The removal of
localnet,package.json,yarn.lock,.github/,.gitignore, anddist/**from the ignore list may increase the build context size and potentially impact build performance. Please confirm that these files are essential for the build process or need to be included in the final image.✅ Verification successful
Verified: Essential Files Necessary for Build Process
The removed
.dockerignoreentries (localnet,package.json,yarn.lock,.github/,.gitignore, anddist/**) are essential for the build process as they are actively used in Dockerfiles and build scripts. Their inclusion ensures proper Docker image construction and build functionality.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check if the removed ignore entries are used in the Dockerfile or build process # Test 1: Check Dockerfile for usage of previously ignored files echo "Checking Dockerfile for usage of previously ignored files:" rg -i -e "localnet|package.json|yarn.lock|\.github|\.gitignore|dist" Dockerfile* # Test 2: Check if these files exist in the repository echo "Checking if previously ignored files exist in the repository:" fd -H "localnet|package.json|yarn.lock|\.github|\.gitignore|dist" # Test 3: Check if these files are used in any build scripts echo "Checking build scripts for usage of previously ignored files:" rg -i -e "localnet|package.json|yarn.lock|\.github|\.gitignore|dist" *.sh MakefileLength of output: 4358
Makefile (2)
4-5: Improved version and commit managementThe introduction of
NODE_VERSIONandNODE_COMMITvariables enhances the flexibility and consistency of version management in the build process. The use of./version.shforNODE_VERSIONand the fallback mechanism forNODE_COMMITare particularly noteworthy improvements.
20-21: Consistent application of new versioning variablesThe ldflags have been updated to use
NODE_VERSIONandNODE_COMMIT, ensuring consistency with the newly introduced variables. This change maintains coherence in version information across different parts of the build process.Also applies to: 23-24
gartnera
force-pushed
the
improve-localnet-build-cache
branch
from
276ff05 to
a151c82
Compare
* chore: improve localnet build performance * propagate NODE_VERSION and NODE_COMMIT
…guration (#2953) * update artillery config * more fixes * feat: integrate authenticated calls smart contract functionality into protocol (#2904) * e2e tests and modifications for authenticated call * extend test with sender check and revert case * separate tests into separate files * cleanup * withdraw and call support and tests * bump protocol contracts * split tests into separate files * small cleanup * fmt * generate * lint * changelog * PR comments * fix case in proto * bump vote inbound gas limit in zetaclient * fix test * generate * fixing tests * call options non empty * generate * test fix * rename gateway caller * pr comments rename tests * PR comment * generate * tests * update tests fixes * tests fixes * fix * test fix * feat!: bank precompile (#2860) * feat: bank precompile * feat: add deposit * feat: extend deposit * PoC: spend amount on behalf of EOA * feat: expand deposit with transferFrom * use CallEVM instead on ZRC20 bindings * divide the contract into different files * initialize e2e testing * remove duplicated funding * add codecov * expand e2e * fix: wait for deposit tx to be mined * apply first round of reviews * cover al error types test * fixes using time.Since * Include CallContract interface * fix eth events in deposit precompile method * emit Deposit event * add withdraw function * finalize withdraw * pack event arguments generically * add high level event function * first round of review fixes * second round of reviews * create bank account when instantiating bank * e2e: add good and bad scenarios * modify fmt * chore: group input into eventData struct * docs: document bank's methods * chore: generate files with suffix .gen.go * chore: assert errors with errorIs * chore: reset e2e test by resetting allowance * test: add first batch of unit test * test: cover all cases * test: complete unit test cases * include review suggestions * include e2e through contract * test: add e2e through contract complete * test: revert balance between tests * Update precompiles/bank/const.go Co-authored-by: Lucas Bertrand <[email protected]> * fix: changed coin denom --------- Co-authored-by: skosito <[email protected]> Co-authored-by: Lucas Bertrand <[email protected]> * feat: add sender to revert context (#2919) * e2e tests and modifications for authenticated call * extend test with sender check and revert case * separate tests into separate files * cleanup * withdraw and call support and tests * bump protocol contracts * split tests into separate files * small cleanup * fmt * generate * lint * changelog * PR comments * fix case in proto * bump vote inbound gas limit in zetaclient * fix test * generate * fixing tests * call options non empty * generate * test fix * rename gateway caller * pr comments rename tests * PR comment * generate * tests * add sender in test contract * extend e2e tests * generate * changelog * PR comment * generate * update tests fixes * tests fixes * fix * test fix * gas limit fixes * PR comment fix * fix bad merge * ci: add option to enable monitoring stack (#2927) * ci: add option to enable monitoring stack * start prometheus faster * update * ci: add rpcimportable test (#2817) * ci: add rpcimportable test * add ci * fmt * use github.com/btcsuite/btcd/btcutil in pkg/chains * remove app imports types tests * use standalone sdkconfig package * fix policies test * move crosschain keeper tests from types to keeper * never seal config in tests * use zeta-chain/ethermint#126 * add some comments * use merged ethermint hash * show resulting go.mod * ci: Add SARIF upload to GitHub Security Dashboard (#2929) * add semgrep sarif upload to GHAS * added comment to clairfy the usage of the utility script * use ghcr.io instead * add tag to image * bad org name --------- Co-authored-by: jkan2 <[email protected]> * fix: add recover to InitChainer to diplay informative message when starting a node from block 1 (#2925) * add recover to InitChainer * generate files * add docs link to error message * move InitChainErrorMessage to app.go * Update app/app.go Co-authored-by: Francisco de Borja Aranda Castillejo <[email protected]> * use const for message --------- Co-authored-by: Francisco de Borja Aranda Castillejo <[email protected]> * test: add wait for block to tss migration test (#2931) * add wait for block to tss migration test * add comments * refactor identifiers * rename checkNumberOfTssGenerated to checkNumberOfTSSGenerated * chore: allow full zetaclient config overlay (#2945) * test(e2e): add gateway upgrade in upgrade test (#2932) * add gateway upgrade * change reference * add v2 setup for all tests * test v2 in light upgrade * refactor setup to use custody v2 directly * chore: improve localnet build performance (#2928) * chore: improve localnet build performance * propagate NODE_VERSION and NODE_COMMIT * update hashes --------- Co-authored-by: skosito <[email protected]> Co-authored-by: Francisco de Borja Aranda Castillejo <[email protected]> Co-authored-by: Lucas Bertrand <[email protected]> Co-authored-by: Alex Gartner <[email protected]> Co-authored-by: jkan2 <[email protected]> Co-authored-by: jkan2 <[email protected]> Co-authored-by: Tanmay <[email protected]>
Description
With this change you should be able to run
make start-e2e-testand if nothing has changedzetanodeshould not be rebuilt.I could also be convinced that the localnet build should always just be static. Like
v22.0.0-nextor something like that.Summary by CodeRabbit
New Features
version.shscript, providing more detailed version information including short commit hashes.Improvements
Bug Fixes
version.shto include commit details for both dirty and non-dirty builds.