Gcp build npm #1272
Gcp build npm #1272
Conversation
WalkthroughThe Changes
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 using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![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 comments (3)
cloudbuild.yaml (3)
Line range hint
6-7: Fix the substitution syntax for_FIREBASE_SITE_IDThe current string interpolation syntax
"${_BUILD_ENV}_site_id"might not work as expected in YAML substitutions. Cloud Build substitutions are performed before the YAML is parsed, so dynamic values should be handled in the steps where they're used.Consider removing the interpolation and handling it in the relevant steps:
- _FIREBASE_SITE_ID: "${_BUILD_ENV}_site_id" # Placeholder for Firebase site ID + _FIREBASE_SITE_ID: "kommunicate_site_id" # Define a static default value🧰 Tools
🪛 yamllint (1.35.1)
[error] 25-25: trailing spaces
(trailing-spaces)
Line range hint
53-80: Enhance Firebase configuration validationWhile the current validation checks for file existence, it should also validate the firebase.json structure and ensure the site ID update succeeds.
echo "Updating firebase.json with site ID..." + if [ ! -f "firebase.json" ]; then + echo "Error: firebase.json file does not exist." + exit 1 + fi + # Backup the original file + cp firebase.json firebase.json.bak # Update the site ID dynamically in firebase.json - sed -i 's/"site": ".*"/"site": "'"${_FIREBASE_SITE_ID}"'"/' firebase.json + if ! sed -i 's/"site": ".*"/"site": "'"${_FIREBASE_SITE_ID}"'"/' firebase.json; then + echo "Error: Failed to update site ID in firebase.json" + mv firebase.json.bak firebase.json + exit 1 + fi + # Validate JSON structure + if ! jq empty firebase.json; then + echo "Error: Invalid JSON structure in firebase.json" + mv firebase.json.bak firebase.json + exit 1 + fi + rm firebase.json.bak
Firebase deployment configuration needs updates
The verification revealed several configuration issues that need attention:
- The
_FIREBASE_PROJECT_IDis set to a placeholder value "your-project-id"- The
_FIREBASE_SITE_IDuses a variable interpolation that might not work as expected- The Firebase token is not properly configured in the deployment step
Required actions:
- Update
_FIREBASE_PROJECT_IDin cloudbuild.yaml with the actual Firebase project ID- Set
_FIREBASE_SITE_IDdirectly without variable interpolation- Add
FIREBASE_TOKENas a secret in Cloud Build and reference it properly in the deployment step🔗 Analysis chain
Line range hint
81-88: Verify Firebase deployment configurationThe deployment step looks correct, but let's ensure the required secrets are properly configured.
Let me verify the Firebase configuration further by checking the Cloud Build configuration.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check if Firebase token secret is configured in Cloud Build gcloud secrets list --filter="name:FIREBASE_TOKEN" --format="get(name)" # Verify Firebase project configuration if [ -f "firebase.json" ]; then echo "Current Firebase configuration:" cat firebase.json fiLength of output: 2569
Script:
#!/bin/bash # Check Cloud Build configuration for Firebase-related substitutions if [ -f "cloudbuild.yaml" ]; then echo "Cloud Build configuration:" grep -A 10 "substitutions:" cloudbuild.yaml fi # Check if Firebase token is referenced in any environment files rg -l "FIREBASE_TOKEN|_FIREBASE_PROJECT_ID|_FIREBASE_SITE_ID" --type yaml --type envLength of output: 619
🧹 Nitpick comments (2)
cloudbuild.yaml (2)
25-25: Remove trailing whitespaceThere is a trailing space after
node:20.- - name: 'node:20' + - name: 'node:20'🧰 Tools
🪛 yamllint (1.35.1)
[error] 25-25: trailing spaces
(trailing-spaces)
Line range hint
25-52: Simplify build environment handlingThe current if-else chain could be simplified, and defaulting to test environment without explicit warning might be risky.
Consider using a more concise approach:
args: - '-c' - | echo "Build env:" ${_BUILD_ENV} - if [ "${_BUILD_ENV}" = "test" ]; then - npm run build-test; - elif [ "${_BUILD_ENV}" = "release" ]; then - npm run build-release; - # ... more conditions ... - else - npm run build-test; - fi + if [ -z "${_BUILD_ENV}" ]; then + echo "Error: _BUILD_ENV is not set" + exit 1 + fi + BUILD_SCRIPT="build-${_BUILD_ENV}" + if npm run | grep -q "^ ${BUILD_SCRIPT} "; then + npm run ${BUILD_SCRIPT} + else + echo "Error: Unknown build environment: ${_BUILD_ENV}" + exit 1 + fi🧰 Tools
🪛 yamllint (1.35.1)
[error] 25-25: trailing spaces
(trailing-spaces)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
cloudbuild.yaml(3 hunks)
🧰 Additional context used
🪛 yamllint (1.35.1)
cloudbuild.yaml
[error] 25-25: trailing spaces
(trailing-spaces)
| # Step 1: Install Node.js dependencies | ||
| - name: 'node:20' | ||
| entrypoint: 'sh' | ||
| args: | ||
| - '-c' | ||
| - | | ||
| echo clean | ||
| npm cache clean --force | ||
| echo remove | ||
| rm -rf node_modules package-lock.json | ||
| echo Installing source NPM dependencies... | ||
| npm install |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Optimize dependency installation for better reliability and performance
While the current setup works, consider these improvements:
- Use
npm ciinstead ofnpm installfor more reliable builds - Consider preserving the npm cache between builds
- Commit package-lock.json to source control for deterministic builds
- npm cache clean --force
- rm -rf node_modules package-lock.json
- echo Installing source NPM dependencies...
- npm install
+ rm -rf node_modules
+ echo Installing dependencies from package-lock.json...
+ npm ci📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| # Step 1: Install Node.js dependencies | |
| - name: 'node:20' | |
| entrypoint: 'sh' | |
| args: | |
| - '-c' | |
| - | | |
| echo clean | |
| npm cache clean --force | |
| echo remove | |
| rm -rf node_modules package-lock.json | |
| echo Installing source NPM dependencies... | |
| npm install | |
| # Step 1: Install Node.js dependencies | |
| - name: 'node:20' | |
| entrypoint: 'sh' | |
| args: | |
| - '-c' | |
| - | | |
| echo clean | |
| rm -rf node_modules | |
| echo Installing dependencies from package-lock.json... | |
| npm ci |
|
A friendly reminder that this PR had no activity for 15 days. |
What do you want to achieve?
PR Checklist
How was the code tested?
What new thing you came across while writing this code?
In case you fixed a bug then please describe the root cause of it?
Screenshot
NOTE: Make sure you're comparing your branch with the correct base branch
Summary by CodeRabbit