CI : fix build cache #37712
CI : fix build cache #37712
Conversation
WalkthroughThe pull request modifies two GitHub workflow files for client and server builds. The client build workflow now includes a new input parameter 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
|
![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 (3)
.github/workflows/client-build.yml (1)
209-209: Consider performance impact of Git LFS migrationThe
git lfs migrate import --everythingcommand rewrites Git history and could significantly impact build time. Consider:
- Running this only when necessary
- Using selective migration instead of
--everything.github/workflows/server-build.yml (2)
Line range hint
141-144: Consider using environment variables for PostgreSQL configurationWhile the PostgreSQL setup is correct, consider extracting configuration values (port, password) to environment variables for better maintainability.
docker run --name appsmith-pg -p 5432:5432 -d \ - -e POSTGRES_PASSWORD=password postgres:alpine postgres -N 1500 + -e POSTGRES_PASSWORD=${{ env.PG_PASSWORD }} \ + -e POSTGRES_MAX_CONNECTIONS=1500 \ + postgres:alpine
Line range hint
237-307: Consider modularizing test reporting logicThe test reporting logic is complex and handles multiple responsibilities. Consider extracting this into a separate shell script for better maintainability and reusability.
Create a new file
scripts/test-reporter.shand move the test reporting logic there. Then update the workflow to:- # Process mvn_test.log for FAILURE and SKIPPED statuses - while IFS= read -r line; do - if [[ $line == *"FAILURE"* ]]; then - module_name=$(echo "$line" | awk '{print $2}') - failed_modules+=("$module_name") - elif [[ $line == *"SKIPPED"* ]]; then - module_name=$(echo "$line" | awk '{print $2}') - skipped_modules+=("$module_name") - fi - done < mvn_test.log + ./scripts/test-reporter.sh mvn_test.log
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
.github/workflows/client-build.yml(1 hunks).github/workflows/server-build.yml(1 hunks)
🔇 Additional comments (4)
.github/workflows/client-build.yml (3)
Line range hint 171-184: LGTM: Robust cache implementation
The cache handling implementation is well-structured with:
- Proper cache key versioning
- Clear separation between CE and EE builds
- Efficient artifact restoration strategy
Line range hint 134-157: LGTM: Secure environment configuration
The environment setup demonstrates good security practices:
- Proper use of GitHub secrets
- Clear branch-based configuration
- Secure handling of analytics keys
Line range hint 9-19: Verify workflow calls after skip-tests type change
The type change of skip-tests from boolean to string could break existing workflow calls. Ensure all dependent workflows are updated accordingly.
✅ Verification successful
Type change for skip-tests parameter is safe
The change of skip-tests from boolean to string type is safe as all existing workflow calls are already passing the value as a string:
build-client-server-count.ymlandbuild-client-server.ymluse"true"on-demand-build-docker-image-deploy-preview.ymlpasses a variable that would be stringified- Other workflows don't use the parameter, defaulting to
"false"
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for workflow files that might be calling this workflow
rg -l "client-build.yml" .github/workflows/
Length of output: 487
Script:
#!/bin/bash
# Check how client-build.yml is referenced in the identified workflow files
rg -A 5 "client-build.yml" .github/workflows/ad-hoc-docker-image.yml .github/workflows/test-build-docker-image.yml .github/workflows/pr-cypress.yml .github/workflows/on-demand-build-docker-image-deploy-preview.yml .github/workflows/quality-checks.yml .github/workflows/build-client-server.yml .github/workflows/build-client-server-count.yml
Length of output: 3297
.github/workflows/server-build.yml (1)
Line range hint 12-14: Verify compatibility with existing workflow calls
The skip-tests parameter type has been changed from boolean to string. This might break existing workflow calls that pass boolean values.
Also applies to: 21-24
✅ Verification successful
Type change for skip-tests parameter appears safe to proceed
From the workflow calls inspection, none of the calling workflows are currently passing the skip-tests parameter, making this type change from boolean to string safe for existing usage.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for existing workflow calls that might be affected
rg -l 'uses: .*server-build.yml.*\n.*skip-tests:\s*(true|false)\s*$' .github/workflows/
Length of output: 281
Script:
#!/bin/bash
# Search for workflow calls with multiline mode enabled
rg -U 'uses:.*server-build\.yml.*\nskip-tests:' .github/workflows/
# Also search for direct references to the workflow
rg 'server-build\.yml' .github/workflows/
Length of output: 1322
Script:
#!/bin/bash
# Check the actual parameters being passed in the workflow calls
for file in .github/workflows/test-build-docker-image.yml .github/workflows/on-demand-build-docker-image-deploy-preview.yml .github/workflows/pr-cypress.yml .github/workflows/quality-checks.yml .github/workflows/build-client-server.yml .github/workflows/build-client-server-count.yml .github/workflows/ad-hoc-docker-image.yml; do
echo "=== $file ==="
rg -A 3 'uses:.*server-build\.yml' "$file"
done
Length of output: 4615
| @@ -338,6 +338,7 @@ jobs: | |||
| git config --global user.name "$gituser" | |||
| git clone https://$cachetoken@github.com/appsmithorg/cibuildcache.git | |||
| git lfs install | |||
| git lfs migrate import --everything | |||
There was a problem hiding this comment.
💡 Codebase verification
Git LFS setup is missing but required for large files
The repository contains multiple large files (>1MB), particularly video files up to 45MB, but Git LFS is not configured (.gitattributes missing). This will cause the git lfs migrate import command to fail.
- Add
.gitattributesfile with appropriate patterns:*.y4m filter=lfs diff=lfs merge=lfs -text *.mov filter=lfs diff=lfs merge=lfs -text *.gif filter=lfs diff=lfs merge=lfs -text - Install Git LFS before running the migration command
🔗 Analysis chain
Verify Git LFS migration impact
The git lfs migrate import --everything command might cause issues if not all files are properly tracked in Git LFS. Consider using selective migration or verifying the tracked patterns first.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check Git LFS tracked patterns
git lfs track
# Verify size of files that would be migrated
git lfs migrate info --everything
Length of output: 221
Script:
#!/bin/bash
# Check if .gitattributes exists and contains LFS configurations
if [ -f ".gitattributes" ]; then
echo "Content of .gitattributes:"
cat .gitattributes
else
echo ".gitattributes not found"
fi
# Search for large files in the repository
echo -e "\nLarge files (>1MB) in the repository:"
find . -type f -size +1M -not -path "*/\.*" -exec ls -lh {} \;
# Check if there are any existing LFS references
echo -e "\nSearching for LFS pointers:"
find . -type f -not -path "*/\.*" -exec grep -l "version https://git-lfs.github.com/spec/" {} \;
Length of output: 4157
Description
fix build cache
Fixes #
Issue Numberor
Fixes
Issue URLAutomation
/ok-to-test tags=""
🔍 Cypress test results
Communication
Should the DevRel and Marketing teams inform users about this change?
Summary by CodeRabbit
New Features
Improvements
Bug Fixes