-
Notifications
You must be signed in to change notification settings - Fork 8
add frontend build to --all #110
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
Conversation
WalkthroughThe Changes
Possibly related PRs
Suggested reviewers
🪧 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 (
|
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: 0
🧹 Outside diff range and nitpick comments (1)
docker-init/build.sh (1)
67-67: LGTM! Consider handling edge cases.The addition of frontend build to the --all option is implemented correctly. However, there are a few considerations:
- The script might trigger frontend build twice if both --all and --frontend options are specified
- There's no explicit error handling if the frontend build fails during --all build
Consider adding these improvements:
# Build all modules if [ "$BUILD_ALL" = true ]; then echo "Building all modules..." sbt assembly sbt dist - sbt "project frontend" buildFrontend + if [ "$BUILD_FRONTEND" != true ]; then + if ! sbt "project frontend" buildFrontend; then + echo "Frontend build failed during --all build" + exit 1 + fi + fi fi
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
docker-init/build.sh(1 hunks)
🔇 Additional comments (1)
docker-init/build.sh (1)
67-67: Verify build dependencies and documentation.
Let's ensure there are no build order dependencies and that documentation is up to date.
✅ Verification successful
Based on the comprehensive review of the codebase, I can now provide a final response regarding the frontend build process:
Frontend build process is properly integrated and has no dependency issues
The frontend build process is well-defined in the build.sbt file with clear dependencies and build order:
- The frontend project is defined separately with its own build task
buildFrontend - The build task handles npm dependencies installation and build process
- The built assets are correctly copied to the hub/public directory
- The hub project (Play application) depends on these frontend assets
- The build.sh script provides proper orchestration of the entire build process including frontend
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for potential build order dependencies in build files
echo "Checking for build dependencies..."
rg -l "frontend|buildFrontend" --type scala --type sbt
# Check if documentation needs updates
echo "Checking documentation for build instructions..."
fd -e md -e rst -e txt | xargs rg -l "build.*frontend|frontend.*build"
Length of output: 343
Script:
#!/bin/bash
# Check for potential build order dependencies in build files
echo "Checking for build dependencies..."
fd "build.sbt|*.scala" | xargs rg -l "frontend|buildFrontend"
# Check for build-related files
echo "Checking build-related files..."
fd "build|sbt" -t f | xargs cat
# Check documentation
echo "Checking documentation..."
fd "README|CONTRIBUTING|DEVELOPMENT" -e md | xargs cat
Length of output: 88829
piyush-zlai
left a comment
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.
Thanks for catching!
Summary
Checklist
Summary by CodeRabbit