refactor: circle ci scripts #1510
Conversation
WalkthroughThe changes in this pull request primarily involve modifications to the CircleCI configuration, task dependencies in Changes
Possibly related PRs
Suggested reviewers
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
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
CodeRabbit Configuration File (
|
github-actions
bot
requested review from
aimensahnoun,
alexandre-abrioux,
leoslr and
yomarion
![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
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
.circleci/config.yml(2 hunks)
🔇 Additional comments (1)
.circleci/config.yml (1)
9-10: Memory allocation increase looks good!
The addition of NODE_OPTIONS with 4GB memory allocation is a reasonable solution to address the out-of-memory issues in CircleCI.
Let's verify the memory settings are sufficient by checking the CircleCI job logs for any memory-related warnings or errors:
✅ Verification successful
Memory allocation increase is appropriate and well-placed
The 4GB memory allocation is justified based on the codebase analysis:
- The project is a monorepo with multiple packages that need to be built
- There's an extensive build chain with numerous workspace packages being compiled
- All test and build jobs inherit from the node_image configuration, ensuring consistent memory settings across the pipeline
- The memory setting is particularly important for operations like:
- Concurrent builds of multiple TypeScript packages
- Smart contract compilation and type generation
- Integration tests that run multiple packages together
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Search for memory-related issues in recent CircleCI builds
# Use GitHub API to fetch recent CircleCI build logs
gh api graphql -f query='
{
repository(owner: "RequestNetwork", name: "requestNetwork") {
pullRequest(number: 1510) {
commits(last: 1) {
nodes {
commit {
statusCheckRollup {
contexts(last: 100) {
nodes {
... on CheckRun {
name
text
}
}
}
}
}
}
}
}
}
}' | jq -r '..|.text? | select(.)' | grep -iE "heap|memory|javascript heap out of memory"
Length of output: 561
Script:
#!/bin/bash
# Let's verify the CircleCI configuration for memory settings and their usage across jobs
# First, let's see all jobs that might be affected by this NODE_OPTIONS setting
rg "<<: \*node_image" .circleci/config.yml -A 5
# Also check for any existing memory-related settings
rg -i "memory|heap|node_options" .circleci/config.yml
# Let's also check the size of build artifacts to understand memory requirements
rg "build|dist|compile" .circleci/config.yml -A 2
Length of output: 5712
|
What does canary do? |
It will conflict with --conventional-prerelease and produce the currently broken versioning |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
.circleci/config.yml (1)
119-180: LGTM: Test job consolidation improves pipeline efficiencyThe consolidation of multiple test jobs into a single job with proper service dependencies and test result storage is a good improvement. However, consider the following enhancement:
Consider using CircleCI's test splitting feature to parallelize the tests while maintaining the consolidated structure:
test: docker: - *node_image - *ipfs_image - *ganache_image - *postgres_image - *graph_image resource_class: large + parallelism: 4 working_directory: *working_directory steps: - attach_workspace: at: *working_directory - run: name: 'Deploy test contract from smart-contracts' command: 'yarn workspace @requestnetwork/smart-contracts run deploy' - run: *step_graph_deploy - run: name: 'Start request-node' command: 'yarn start:request-node' background: true - run: *step_wait_for_node - run: name: 'Test all' - command: 'yarn test' + command: | + TESTFILES=$(circleci tests glob "packages/*/test/**/*.test.ts" | circleci tests split --split-by=timings) + yarn test $TESTFILES
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
.circleci/config.yml(5 hunks)
🔇 Additional comments (2)
.circleci/config.yml (2)
Line range hint 244-280: LGTM: Workflow configuration properly maintains all necessary jobs and dependencies
The workflow configuration correctly:
- Maintains proper job dependencies
- Preserves the next-release job configuration
- Keeps the nightly workflow intact
9-10: LGTM: Memory allocation increase addresses OOM issues
The addition of NODE_OPTIONS: '--max-old-space-size=4096' aligns with the PR objective to resolve out-of-memory issues during the build process.
Let's verify if 4GB is sufficient for the build process:
✅ Verification successful
Based on the analysis of the package.json scripts and build configurations, I can now provide a final response:
Memory allocation of 4GB is appropriate for the build process
The codebase shows characteristics that justify the 4GB memory allocation:
- Monorepo structure with multiple packages using Lerna
- Concurrent TypeScript compilation across packages
- Complex test suites including blockchain interactions
- Multiple UMD and CommonJS builds
- Resource-intensive operations like GraphQL codegen
The 4GB limit is a standard and sufficient allocation for these types of operations, especially considering:
- Tests are run with
--concurrency=1in the root package.json - Some test suites use
--maxWorkers=1to limit resource usage - Build processes are properly segregated into distinct steps
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for memory-related build failures in recent CircleCI runs
# Search for memory-related errors in build logs
gh api -X GET /repos/RequestNetwork/requestNetwork/actions/runs \
--jq '.workflow_runs[] | select(.conclusion == "failure") | .id' | \
while read -r run_id; do
gh api -X GET "/repos/RequestNetwork/requestNetwork/actions/runs/$run_id/jobs" \
--jq '.jobs[] | select(.conclusion == "failure") | .steps[] | select(.conclusion == "failure") | .name + ": " + .conclusion'
done
Length of output: 147
Script:
#!/bin/bash
# Let's try a different approach to check for memory-related issues
# 1. Look for any memory-related configurations in the codebase
# 2. Check for memory-related comments or discussions in the config
# Search for memory-related configurations
echo "=== Memory configurations across the codebase ==="
rg -i "max.*space.*size|memory.*limit|heap.*size" -g "!package-lock.json"
# Search for memory-related comments in CircleCI config
echo -e "\n=== Memory-related comments in CircleCI config ==="
rg -i "# .*memory|// .*memory" .circleci/config.yml
# Check if there are other similar memory configurations
echo -e "\n=== Other NODE_OPTIONS configurations ==="
rg "NODE_OPTIONS" -g "!package-lock.json"
Length of output: 496
Script:
#!/bin/bash
# Let's check the build and test job configurations to understand the memory requirements
# Look at the test commands and build steps
echo "=== Build and test configurations ==="
rg "npm (run |test|build)" .circleci/config.yml -A 2
# Check package.json scripts to understand the complexity of build/test operations
echo -e "\n=== Package.json scripts ==="
fd package.json --exec cat {} | jq -r '.scripts // empty'
# Look for any previous memory configurations that might have been removed
echo -e "\n=== Git history for memory configurations ==="
git log -p .circleci/config.yml | rg -i "memory|node_options" -C 2
Length of output: 9741
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (2)
nx.json (1)
11-13: Review dependency chain implicationsThe new dependency chain (test → build → lint → ^lint) establishes stricter quality gates but might increase CI time. Consider:
- Running lint in parallel with build where possible
- Ensuring fast feedback by failing early on lint errors
Also applies to: 15-15, 18-18
.circleci/config.yml (1)
102-163: Test job consolidation looks comprehensive but consider monitoring execution time.The consolidation of test jobs simplifies the workflow, but it might impact the ability to parallelize tests and could make debugging failures more challenging.
Consider:
- Monitoring total execution time compared to previous parallel execution
- Adding test timing metrics to identify bottlenecks
- Implementing test result aggregation for better visibility
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
.circleci/config.yml(6 hunks)nx.json(1 hunks)package.json(1 hunks)packages/request-logic/package.json(1 hunks)
🔇 Additional comments (5)
nx.json (1)
6-6: Verify impact of removing "clean" from cacheableOperations
While adding "lint" to cacheableOperations is beneficial for CI performance, removing "clean" might lead to stale artifacts affecting builds.
package.json (2)
21-24: LGTM: Improved lint management structure
The new linting structure using Lerna provides better organization and consistency across packages.
Line range hint 1-85: Missing NODE_OPTIONS configuration mentioned in PR objectives
The PR objectives mention adding NODE_OPTIONS='--max-old-space-size=4096' to address memory issues, but this configuration is not present in the package.json.
Consider adding a cross-platform solution by:
- Adding an
.envfile with the memory configuration - Setting it in the npm/yarn configuration
- Documenting the memory requirements in the README
.circleci/config.yml (2)
Line range hint 220-255: Workflow structure is well-organized with proper dependencies.
The consolidated test job is correctly positioned as a dependency for other jobs, maintaining the logical flow of the CI pipeline.
Let's verify the workflow execution order:
✅ Verification successful
Workflow dependencies are correctly configured and maintain proper execution order
The verification confirms that:
- The consolidated
testjob is correctly positioned as a prerequisite forlint,test-usage-examples, andtest-toolboxjobs - The
next-releasejob properly depends on thetestjob and maintains its branch filtering for master - The nightly workflow is independent and properly configured with its cron schedule
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check recent workflow execution patterns
# Check workflow execution times and success rates
gh api repos/RequestNetwork/requestNetwork/actions/workflows \
--jq '.workflows[] | select(.name == "build-and-test") | .id' \
| while read -r workflow_id; do
gh api repos/RequestNetwork/requestNetwork/actions/workflows/$workflow_id/runs \
--jq '.workflow_runs[] | select(.conclusion != null) | {status: .conclusion, duration: (.updated_at | fromdate - (.created_at | fromdate))}'
done
Length of output: 146
Script:
#!/bin/bash
# Let's verify the workflow structure and dependencies directly from the config file
# Check the workflow structure and job dependencies
cat .circleci/config.yml | grep -A 30 "build-and-test:"
# Also check for any other workflow definitions that might interact
cat .circleci/config.yml | grep -A 10 "workflows:"
Length of output: 986
9-10: Memory and resource configuration looks appropriate.
The addition of NODE_OPTIONS with 4GB memory limit and large resource class should help address the out-of-memory issues.
Let's verify the memory requirements:
Also applies to: 65-65
✅ Verification successful
Memory configuration is appropriate for the project's build requirements
The verification shows that:
- The CircleCI config sets
NODE_OPTIONS='--max-old-space-size=4096'(4GB) - The project uses multiple TypeScript builds across packages (lerna-based monorepo)
- Complex build processes including:
- Concurrent package builds
- Multiple test suites
- UMD and CommonJS builds
- Smart contract compilation
- GraphQL codegen
The 4GB heap size is a reasonable default for this type of monorepo with multiple TypeScript packages and complex build processes.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check build memory usage patterns in recent builds
# Check for memory-related errors in recent builds
gh api repos/RequestNetwork/requestNetwork/actions/runs \
--jq '.workflow_runs[] | select(.conclusion == "failure") | .id' \
| while read -r run_id; do
gh api repos/RequestNetwork/requestNetwork/actions/runs/$run_id/jobs \
--jq '.jobs[] | select(.conclusion == "failure") | .steps[].name + ": " + .steps[].conclusion'
done
Length of output: 139
Script:
#!/bin/bash
# Let's try a different approach to verify memory-related configurations and issues
# Check for any memory-related configurations in the CircleCI config
echo "Checking CircleCI memory configurations:"
rg -A 2 -B 2 "memory|heap|NODE_OPTIONS" .circleci/config.yml
# Look for build-related scripts that might indicate memory requirements
echo -e "\nChecking package.json scripts:"
fd package.json --exec cat {} | jq '.scripts'
# Check for other memory-related configurations in the project
echo -e "\nChecking for other memory configurations:"
rg -l "max-old-space-size" --type yaml --type json
Length of output: 10093
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
.circleci/config.yml (1)
272-272: Incorrect workflow commentThe comment "Lerna will run lint, build and test" is misleading as these steps are executed separately in the workflow, not automatically by Lerna.
Consider updating the comment to:
- ## Lerna will run lint, build and test + ## Build, lint and test workflow
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
.circleci/config.yml(7 hunks)
🧰 Additional context used
📓 Learnings (1)
.circleci/config.yml (2)
Learnt from: MantisClone
PR: RequestNetwork/requestNetwork#1510
File: .circleci/config.yml:80-81
Timestamp: 2024-12-09T23:23:12.207Z
Learning: There are no circular dependencies among the packages via TypeScript project references.
Learnt from: rodrigopavezi
PR: RequestNetwork/requestNetwork#1510
File: .circleci/config.yml:0-0
Timestamp: 2024-12-06T10:32:50.647Z
Learning: It's acceptable to exclude the following packages from unit tests in CI: `@requestnetwork/smart-contracts`, `@requestnetwork/payment-detection`, `@requestnetwork/payment-processor`, and `@requestnetwork/integration-test`.
🔇 Additional comments (2)
.circleci/config.yml (2)
Line range hint 8-9: LGTM: Memory limit increase addresses OOM issues
The addition of NODE_OPTIONS: '--max-old-space-size=4096' aligns with the PR objective of resolving out-of-memory problems during the build process.
108-115: Well-organized test segregation
The test exclusions are well-structured, separating unit tests from integration tests that require additional services. This organization aligns with the accepted practice of excluding certain packages from unit tests.
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
.circleci/config.yml(7 hunks)
🧰 Additional context used
📓 Learnings (1)
.circleci/config.yml (2)
Learnt from: MantisClone
PR: RequestNetwork/requestNetwork#1510
File: .circleci/config.yml:80-81
Timestamp: 2024-12-09T23:23:12.207Z
Learning: There are no circular dependencies among the packages via TypeScript project references.
Learnt from: rodrigopavezi
PR: RequestNetwork/requestNetwork#1510
File: .circleci/config.yml:0-0
Timestamp: 2024-12-06T10:32:50.647Z
Learning: It's acceptable to exclude the following packages from unit tests in CI: `@requestnetwork/smart-contracts`, `@requestnetwork/payment-detection`, `@requestnetwork/payment-processor`, and `@requestnetwork/integration-test`.
🔇 Additional comments (5)
.circleci/config.yml (5)
Line range hint 8-9: LGTM: Memory limit configuration addresses OOM issues
The addition of NODE_OPTIONS: '--max-old-space-size=4096' directly addresses the out-of-memory problems mentioned in the PR objectives.
63-63: LGTM: Build optimizations look good
The changes improve build performance:
- Using
largeresource class provides more resources for memory-intensive operations - Using
--since masteroptimizes build time by only building changed packages, which is safe given the absence of circular dependencies
Also applies to: 80-81
108-116: LGTM: Test consolidation with appropriate exclusions
The test consolidation with package exclusions aligns with the documented acceptable exclusions for unit tests.
129-129: LGTM: Integration test improvements
Good improvements to integration test reliability:
- Using
largeresource class ensures sufficient resources - Running request-node in background with wait step ensures service readiness
Also applies to: 170-172
Line range hint 275-307: LGTM: Workflow changes maintain proper job sequencing
The workflow changes:
- Maintain proper job dependencies
- Ensure all tests pass before next-release
- Preserve nightly test workflow
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
.circleci/config.yml(7 hunks)
🧰 Additional context used
📓 Learnings (1)
.circleci/config.yml (2)
Learnt from: MantisClone
PR: RequestNetwork/requestNetwork#1510
File: .circleci/config.yml:80-81
Timestamp: 2024-12-09T23:23:12.207Z
Learning: There are no circular dependencies among the packages via TypeScript project references.
Learnt from: rodrigopavezi
PR: RequestNetwork/requestNetwork#1510
File: .circleci/config.yml:0-0
Timestamp: 2024-12-06T10:32:50.647Z
Learning: It's acceptable to exclude the following packages from unit tests in CI: `@requestnetwork/smart-contracts`, `@requestnetwork/payment-detection`, `@requestnetwork/payment-processor`, and `@requestnetwork/integration-test`.
🔇 Additional comments (5)
.circleci/config.yml (5)
Line range hint 8-8: LGTM: Memory limit increase addresses OOM issues
The addition of NODE_OPTIONS: '--max-old-space-size=4096' aligns with the PR objective of fixing out-of-memory problems during the build process.
63-63: LGTM: Build optimization with selective package building
The changes optimize the build process by:
- Using
largeresource class for better performance - Using
--since masterflag to build only changed packages and their dependencies
Also applies to: 80-81
108-116: LGTM: Test job consolidation with appropriate exclusions
The test job consolidation with package exclusions is well-structured and aligns with the documented requirements. The excluded packages are: request-client.js, smart-contracts, payment-detection, payment-processor, and integration-test.
Line range hint 198-226: LGTM: Comprehensive nightly test configuration
The nightly test configuration is well-structured with:
- Complete environment setup (node, ipfs, ganache, postgres, graph)
- Specific integration tests for erc20 and btc
- Appropriate scheduling (daily at 22:00)
260-260: LGTM: Well-structured release workflow
The release workflow is properly configured with:
- Correct dependency ordering (lint, build, all tests)
- Release triggered only on master branch
- Required context for protocol release
Also applies to: 279-292
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
.circleci/config.yml(7 hunks)
🧰 Additional context used
📓 Learnings (1)
.circleci/config.yml (2)
Learnt from: MantisClone
PR: RequestNetwork/requestNetwork#1510
File: .circleci/config.yml:80-81
Timestamp: 2024-12-09T23:23:12.207Z
Learning: There are no circular dependencies among the packages via TypeScript project references.
Learnt from: rodrigopavezi
PR: RequestNetwork/requestNetwork#1510
File: .circleci/config.yml:0-0
Timestamp: 2024-12-06T10:32:50.647Z
Learning: It's acceptable to exclude the following packages from unit tests in CI: `@requestnetwork/smart-contracts`, `@requestnetwork/payment-detection`, `@requestnetwork/payment-processor`, and `@requestnetwork/integration-test`.
🔇 Additional comments (4)
.circleci/config.yml (4)
Line range hint 8-8: LGTM: Memory limit increase addresses OOM issues
Setting NODE_OPTIONS='--max-old-space-size=4096' is appropriate for resolving the out-of-memory problems mentioned in the PR objectives.
80-81: LGTM: Selective build strategy approved
Using --since master for builds has been previously discussed and approved, as there are no circular dependencies among packages.
108-116: LGTM: Test organization and package exclusions
The test organization and package exclusions align with the established guidelines.
Line range hint 260-292: LGTM: Workflow dependencies are properly configured
The workflow configuration:
- Maintains proper job dependencies
- Ensures all tests pass before next-release
- Preserves nightly testing
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
package.json(1 hunks)
🔇 Additional comments (3)
package.json (3)
21-23: LGTM: Changes align with previous review suggestions.
The updated linting scripts properly delegate to package-level scripts via Lerna, as previously suggested.
27-27: LGTM: Aligned with suggested publish command format.
The publish-npm script now matches the suggested format from the CircleCI config and release tools.
28-29: Verify the force publish approach for prereleases.
The publish-prerelease script uses a complex approach with jq to force publish changed packages. While this aligns with the PR's goal of fixing next release issues, we should verify:
- The shell command substitution for FORCE_PUBLISH
- The impact of force publishing on package versioning
✅ Verification successful
Let me try a different approach to verify the package.json scripts.
Based on the examination of package.json and lerna.json, I can now provide a final response:
The force publish approach for prereleases is appropriate and well-structured
The implementation is correct because:
- The monorepo uses independent versioning (lerna.json:
"version": "independent") - The force publish script correctly identifies changed packages using
lerna changed --json - The jq command properly formats the package names into a comma-separated list
- The prerelease flags (--conventional-prerelease, --preid next, --dist-tag next) ensure proper version bumping and npm tagging
- The safety flags (--no-git-tag-version, --no-push) prevent unintended git operations
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the changed packages detection logic
# Test 1: Check if jq command works as expected
echo "Test 1: Verify jq command output format"
$(yarn bin)/lerna changed --json | jq '. | map(.name) | join(",")'
# Test 2: List all packages that would be force published
echo "Test 2: List packages affected by force publish"
$(yarn bin)/lerna changed --json | jq '. | map(.name)'
Length of output: 2040
Script:
#!/bin/bash
# Let's examine the package.json and lerna.json configuration
# Check package.json content
echo "Package.json content:"
cat package.json
# Check lerna.json if exists
echo -e "\nLerna.json content (if exists):"
if [ -f lerna.json ]; then
cat lerna.json
fi
# List all package directories to understand the monorepo structure
echo -e "\nPackage directories:"
find . -name "package.json" -not -path "*/node_modules/*"
Length of output: 4555
|
Test reports are being uploaded. 
|
Co-authored-by: MantisClone <david.huntmateo@request.network>
Problem
The next release is not working on Circle Ci due to out-of-memory issues. Furthermore, it does not use lerna for running build and tests.
Changes
lernato run tests - eliminate the need to update CircleCI when adding new tests--since masterto run tests for only changed code.Summary by CodeRabbit
Summary by CodeRabbit
New Features
lint:checktarget for improved linting processes.lint:ciscript for comprehensive linting checks.Improvements
testjob in the CI configuration.lit-protocol-cipherpackage with new dependencies.NODE_OPTIONSfor better memory management in CI configuration.lint:checkscript to exclude thespecsdirectory during checks.testscript for thelit-protocol-cipherpackage for a more straightforward execution.It will have releases as such