chore(web): add dockerfiles #89
Conversation
int-tt
force-pushed
the
feature/use-nginx
branch
from
5ae85a2 to
742f60d
Compare
WalkthroughThe changes in this pull request involve modifications to GitHub Actions workflows and Docker configurations for a web application. Key updates include the removal of artifact download steps from the build and deployment workflows, the introduction of a new Changes
Possibly related PRs
Suggested reviewers
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: 7
🧹 Outside diff range and nitpick comments (10)
web/docker/nginx.conf.template (2)
1-15: Consider adding more fields to enhance observability.The JSON log format is well-structured, but could benefit from additional fields for better monitoring and debugging:
log_format json escape=json '{' '"body_bytes_sent": "$body_bytes_sent",' + '"request_length": "$request_length",' + '"upstream_response_time": "$upstream_response_time",' + '"upstream_addr": "$upstream_addr",' + '"http_x_forwarded_for": "$http_x_forwarded_for",' # ... rest of the fields ... '}';
27-52: Consider adding rate limiting for API protection.To protect your application from abuse, consider implementing rate limiting:
# Add before server block limit_req_zone $binary_remote_addr zone=one:10m rate=1r/s; # Add inside location / block limit_req zone=one burst=10 nodelay;web/Dockerfile (1)
1-3: Consider upgrading to Node.js 20 LTSWhile pinning to a specific version (18.16.1) is good practice, consider upgrading to Node.js 20 LTS for improved performance and security features. The slim variant is a good choice for the build stage.
-FROM node:18.16.1-slim AS node +FROM node:20.9.0-slim AS node.github/workflows/deploy.yml (1)
29-34: Consider adding Cloud Run best practices.The Cloud Run deployment could benefit from additional configurations for production readiness:
- Resource limits (memory/CPU)
- Environment variables
- Health checks
- Minimum instances for production stability
Consider adding these configurations:
gcloud run deploy reearth-marketplace-web \ - --image $WEB_IMAGE_GCP \ + --image "$WEB_IMAGE_GCP" \ --region $GCP_REGION \ --platform managed \ + --memory 1Gi \ + --cpu 1 \ + --min-instances 1 \ + --set-env-vars "NODE_ENV=production" \ + --port 80 \ + --health-checks "/health" \ --quiet🧰 Tools
🪛 actionlint
29-29: shellcheck reported issue in this script: SC2086:info:2:11: Double quote to prevent globbing and word splitting
(shellcheck)
29-29: shellcheck reported issue in this script: SC2086:info:3:12: Double quote to prevent globbing and word splitting
(shellcheck)
.github/workflows/web_build.yml (1)
13-22: Consider breaking down the complex job conditionThe job condition is complex and could be hard to maintain. Consider splitting it into multiple environment variables or composite conditions for better readability.
jobs: info: name: Collect information runs-on: ubuntu-latest - if: github.event.workflow_run.conclusion != 'failure' && github.event.repository.full_name == 'reearth/reearth-marketplace' && (github.event.workflow_run.head_branch == 'release' || !startsWith(github.event.head_commit.message, 'v')) + env: + IS_SUCCESS: ${{ github.event.workflow_run.conclusion != 'failure' }} + IS_CORRECT_REPO: ${{ github.event.repository.full_name == 'reearth/reearth-marketplace' }} + IS_VALID_BRANCH: ${{ github.event.workflow_run.head_branch == 'release' || !startsWith(github.event.head_commit.message, 'v') }} + if: env.IS_SUCCESS && env.IS_CORRECT_REPO && env.IS_VALID_BRANCH.github/workflows/build.yml (5)
Line range hint
29-31: Update deprecatedset-outputcommandsThe
::set-outputsyntax is deprecated. Use the$GITHUB_OUTPUTenvironment file instead.Replace the output commands with:
- echo "::set-output name=sha_short::$(git rev-parse --short HEAD)" + echo "sha_short=$(git rev-parse --short HEAD)" >> $GITHUB_OUTPUT if [[ "$BRANCH" = "release" ]]; then TAG=$(git tag --points-at HEAD) if [[ ! -z "$TAG" ]]; then - echo "::set-output name=new_tag::$TAG" - echo "::set-output name=new_tag_short::${TAG#v}" + echo "new_tag=$TAG" >> $GITHUB_OUTPUT + echo "new_tag_short=${TAG#v}" >> $GITHUB_OUTPUT else - echo "::set-output name=name::rc" + echo "name=rc" >> $GITHUB_OUTPUT fi else - echo "::set-output name=name::nightly" + echo "name=nightly" >> $GITHUB_OUTPUT fi
Line range hint
57-57: Address commented out TODO for server artifactsThere's a commented TODO about server artifacts. This should be addressed or removed if no longer relevant.
Would you like me to help create a GitHub issue to track this technical debt?
Line range hint
143-144: Consider using GITHUB_TOKEN for Docker operationsThe workflow is using a GitHub App token for Docker operations. Consider using the built-in
GITHUB_TOKENif possible, as it has automatic token rotation and limited scope.
Line range hint
171-173: Add Docker image scanning stepConsider adding a security scanning step for the Docker image before pushing it to DockerHub.
You could add Trivy scanning:
- name: Run Trivy vulnerability scanner uses: aquasecurity/trivy-action@master with: image-ref: ${{ steps.options.outputs.tags }} format: 'table' exit-code: '1' ignore-unfixed: true vuln-type: 'os,library' severity: 'CRITICAL,HIGH'
184-184: Implement test environment deployment for nightly buildsThe TODO indicates missing functionality for deploying to a test environment when building nightly versions.
Would you like me to help:
- Create a GitHub issue to track this enhancement
- Provide an implementation for the test environment deployment workflow
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (8)
.github/workflows/build.yml(1 hunks).github/workflows/deploy.yml(2 hunks).github/workflows/web_build.yml(1 hunks)web/.dockerignore(1 hunks)web/Dockerfile(1 hunks)web/docker/40-envsubst-on-reearth_config.sh(1 hunks)web/docker/nginx.conf.template(1 hunks)web/docker/reearth_config.json.template(1 hunks)
✅ Files skipped from review due to trivial changes (3)
- web/.dockerignore
- web/docker/40-envsubst-on-reearth_config.sh
- web/docker/reearth_config.json.template
🧰 Additional context used
🪛 actionlint
.github/workflows/deploy.yml
24-24: shellcheck reported issue in this script: SC2086:info:2:17: Double quote to prevent globbing and word splitting
(shellcheck)
24-24: shellcheck reported issue in this script: SC2086:info:3:13: Double quote to prevent globbing and word splitting
(shellcheck)
29-29: shellcheck reported issue in this script: SC2086:info:2:11: Double quote to prevent globbing and word splitting
(shellcheck)
29-29: shellcheck reported issue in this script: SC2086:info:3:12: Double quote to prevent globbing and word splitting
(shellcheck)
74-74: shellcheck reported issue in this script: SC2086:info:2:11: Double quote to prevent globbing and word splitting
(shellcheck)
74-74: shellcheck reported issue in this script: SC2086:info:3:12: Double quote to prevent globbing and word splitting
(shellcheck)
.github/workflows/web_build.yml
32-32: shellcheck reported issue in this script: SC2236:style:4:9: Use -n instead of ! -z
(shellcheck)
32-32: workflow command "set-output" was deprecated. use echo "{name}={value}" >> $GITHUB_OUTPUT instead: https://docs.github.com/en/actions/using-workflows/workflow-commands-for-github-actions
(deprecated-commands)
32-32: workflow command "set-output" was deprecated. use echo "{name}={value}" >> $GITHUB_OUTPUT instead: https://docs.github.com/en/actions/using-workflows/workflow-commands-for-github-actions
(deprecated-commands)
32-32: workflow command "set-output" was deprecated. use echo "{name}={value}" >> $GITHUB_OUTPUT instead: https://docs.github.com/en/actions/using-workflows/workflow-commands-for-github-actions
(deprecated-commands)
32-32: workflow command "set-output" was deprecated. use echo "{name}={value}" >> $GITHUB_OUTPUT instead: https://docs.github.com/en/actions/using-workflows/workflow-commands-for-github-actions
(deprecated-commands)
32-32: workflow command "set-output" was deprecated. use echo "{name}={value}" >> $GITHUB_OUTPUT instead: https://docs.github.com/en/actions/using-workflows/workflow-commands-for-github-actions
(deprecated-commands)
76-76: property "tag_short" is not defined in object type {name: string; new_tag: string; new_tag_short: string; sha_short: string}
(expression)
79-79: workflow command "set-output" was deprecated. use echo "{name}={value}" >> $GITHUB_OUTPUT instead: https://docs.github.com/en/actions/using-workflows/workflow-commands-for-github-actions
(deprecated-commands)
79-79: workflow command "set-output" was deprecated. use echo "{name}={value}" >> $GITHUB_OUTPUT instead: https://docs.github.com/en/actions/using-workflows/workflow-commands-for-github-actions
(deprecated-commands)
79-79: workflow command "set-output" was deprecated. use echo "{name}={value}" >> $GITHUB_OUTPUT instead: https://docs.github.com/en/actions/using-workflows/workflow-commands-for-github-actions
(deprecated-commands)
🔇 Additional comments (9)
web/docker/nginx.conf.template (1)
17-17: Verify real IP header configuration for security.
The real_ip_header directive uses an environment variable which is good for flexibility, but ensure:
- The variable is properly set in production to handle Google Cloud Load Balancer headers.
- Consider adding
set_real_ip_fromdirectives to specify trusted proxy IPs.
web/Dockerfile (4)
4-8: Well-structured dependency installation
Excellent use of:
- Build mounts for efficient layer caching
- Yarn cache mounting for faster builds
--frozen-lockfilefor deterministic installations
37-40: Well-structured file copying and permissions
Good practices observed:
- Proper ownership setting using nginx user
- Clear separation of configuration templates
- Organized environment variable substitution setup
9-20: Verify the necessity of TEMP.js
The inclusion of TEMP.js in the build process seems suspicious. Is this a temporary file that should be removed?
✅ Verification successful
Let me verify if this file is used in the build process by checking the build scripts in package.json and any references in the codebase.
Let me check the extension build configuration to understand the complete build process.
Based on the gathered information, I can now provide the final response:
TEMP.js is a necessary build script for extension optimization
The file TEMP.js is actually a post-build script that removes React and ReactDOM imports from the extension bundle, as these dependencies are marked as external in vite.config.ext.ts and are expected to be provided by the host environment. This is referenced in the build:ext script in package.json: "build:ext": "tsc && vite build -c vite.config.ext.ts && node ./TEMP".
While the name could be more descriptive (e.g., remove-external-imports.js), the file serves a legitimate purpose in the build process.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if TEMP.js is referenced in build configurations
rg -l "TEMP.js" "vite.config.*" "package.json"
# Check the content of TEMP.js if it exists
fd -t f "TEMP.js" -x cat {}
Length of output: 421
Script:
#!/bin/bash
# Check build scripts in package.json
fd -t f "package.json" -x cat {}
# Look for any references to TEMP.js in the codebase
rg -l "TEMP.js"
Length of output: 3291
Script:
#!/bin/bash
# Check the extension build configuration
fd -t f "vite.config.ext.ts" -x cat {}
# Look for any references to external dependencies in the extension
rg -l "react" "dist/reearth-marketplace-ext.js"
Length of output: 1568
34-35:
Review CORS configuration
An empty CORS_ALLOWED_ORIGINS ("") could lead to either:
- All origins being allowed (security risk)
- No origins being allowed (functionality break)
Please verify the CORS handling in the Nginx configuration template.
✅ Verification successful
CORS configuration is properly handled
The empty CORS_ALLOWED_ORIGINS is safe as the Nginx configuration implements a secure CORS handling mechanism:
- It uses a map block to validate origins against the CORS_ALLOWED_ORIGINS environment variable
- Only origins matching the exact pattern in CORS_ALLOWED_ORIGINS are allowed (using regex ^...$ anchors)
- When CORS_ALLOWED_ORIGINS is empty, the default map value is an empty string, effectively blocking all cross-origin requests
- The configuration supports multiple origins using | as delimiter (e.g. "https://example.com|http://localhost:3000")
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check CORS handling in Nginx config
cat docker/nginx.conf.template | grep -A 5 "add_header 'Access-Control-Allow-Origin'"
Length of output: 152
Script:
#!/bin/bash
# Search for nginx configuration files
fd nginx.conf
# Search for any CORS-related configuration files
fd cors
# Search for CORS-related code
rg -i "cors" -A 5 -B 5
Length of output: 5261
.github/workflows/deploy.yml (1)
11-12: LGTM! Verify image registry permissions.
The Docker image variables are well-structured and follow GCP Artifact Registry conventions.
Let's verify the repository permissions:
✅ Verification successful
Docker image variables are correctly configured and used in the workflow
The workflow file shows that:
- Both image variables are properly used in their respective deployment jobs
- The workflow has proper authentication setup for GCP Artifact Registry
- Images are built/loaded and pushed to the correct registry paths
- Cloud Run deployments use the correct image references
No issues found with the image variable definitions or their usage in the deployment process.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if the repository exists and we have proper access
gcloud artifacts repositories describe reearth \
--project=reearth-oss \
--location=us-central1 \
--format="yaml(name,format,mode)"
Length of output: 201
Script:
#!/bin/bash
# Let's check the workflow file to understand the deployment setup and verify image usage
cat .github/workflows/deploy.yml
Length of output: 3079
.github/workflows/web_build.yml (2)
1-11: LGTM: Well-configured workflow triggers and concurrency settings
The workflow configuration follows best practices with appropriate trigger conditions and concurrency settings to prevent parallel runs on the same branch.
53-73: LGTM: Well-structured Docker job setup with secure credentials handling
The Docker job configuration follows best practices with proper setup of QEMU and Buildx, and secure handling of DockerHub credentials.
.github/workflows/build.yml (1)
Line range hint 2-5: Verify workflow integration
Let's verify the integration with the removed artifact download steps mentioned in the AI summary.
✅ Verification successful
Let me check the actual content of these workflow files to understand the artifact download steps better.
Let me verify the CI workflow that's being referenced to ensure the complete workflow chain is properly integrated.
Workflow integration is properly configured
The workflow chain is correctly set up:
- CI workflow uploads the web artifact
- Build workflow downloads from CI workflow
- Deploy workflow downloads from build workflow
- Release workflow handles changelog artifacts separately
The workflow dependencies and artifact handling are consistent across all related workflows.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any remaining artifact download steps in other workflow files
# and verify the new web-build.yml workflow exists
# Check for artifact download steps in other workflows
echo "Checking for artifact download steps in other workflows..."
rg -l "action-download-artifact" .github/workflows/
# Verify web-build.yml exists and is properly configured
echo "Checking for web-build.yml..."
fd "web-build.yml" .github/workflows/
Length of output: 342
Script:
#!/bin/bash
# Check the content of workflow files that contain artifact download steps
echo "Content of deploy.yml with artifact download context:"
rg -A 5 -B 5 "action-download-artifact" .github/workflows/deploy.yml
echo -e "\nContent of build.yml with artifact download context:"
rg -A 5 -B 5 "action-download-artifact" .github/workflows/build.yml
Length of output: 1672
Script:
#!/bin/bash
# Check the CI workflow content and its artifact upload steps
echo "Checking ci.yml workflow content:"
if [ -f .github/workflows/ci.yml ]; then
rg -A 5 -B 5 "artifact" .github/workflows/ci.yml
else
echo "ci.yml not found"
fi
# Also check the release workflow for completeness
echo -e "\nChecking release.yml workflow content:"
if [ -f .github/workflows/release.yml ]; then
rg -A 5 -B 5 "artifact" .github/workflows/release.yml
else
echo "release.yml not found"
fi
Length of output: 1281
web/docker/nginx.conf.template
Outdated
| server { | ||
| listen ${PORT}; | ||
| root /usr/share/nginx/html; | ||
| server_name _; | ||
|
|
||
| access_log /dev/stdout json; | ||
| error_log /dev/stderr warn; | ||
|
|
||
| location / { | ||
| add_header 'Access-Control-Allow-Origin' "$cors"; | ||
| add_header 'Access-Control-Allow-Credentials' 'true'; | ||
| add_header 'Access-Control-Allow-Methods' 'GET, POST, PATCH, PUT, HEAD OPTION'; | ||
| add_header 'Access-Control-Allow-Headers' 'Origin, Authorization, Accept'; | ||
|
|
||
| if ($request_method = 'OPTIONS') { | ||
| return 204; | ||
| } | ||
|
|
||
| try_files $uri /index.html =404; | ||
| } | ||
|
|
||
| error_page 500 502 503 504 /50x.html; | ||
| location = /50x.html { | ||
| root /usr/share/nginx/html; | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add security headers and optimize static file serving.
- Fix the typo in CORS methods:
- add_header 'Access-Control-Allow-Methods' 'GET, POST, PATCH, PUT, HEAD OPTION';
+ add_header 'Access-Control-Allow-Methods' 'GET, POST, PATCH, PUT, HEAD, OPTIONS';- Add security headers:
# Add these inside the location / block
add_header X-Content-Type-Options "nosniff";
add_header X-Frame-Options "DENY";
add_header X-XSS-Protection "1; mode=block";- Enable gzip compression for better performance:
# Add before the server block
gzip on;
gzip_types text/plain text/css application/json application/javascript text/xml application/xml application/xml+rss text/javascript;- Add cache control for static assets:
# Add inside the location / block
expires 1h;
add_header Cache-Control "public, no-transform";
web/docker/nginx.conf.template
Outdated
| # CORS_ALLOWED_ORIGINS specifies multiple origins to which access is allowed. | ||
| # The delimiter is | and the regular expression or is used to match. | ||
| # exmaple: CORS_ALLOWED_ORIGINS=https://example.com|http://localhosts:3000 | ||
| map $http_origin $cors{ | ||
| default ''; | ||
| ~^(${CORS_ALLOWED_ORIGINS})$ $http_origin; | ||
| } |
There was a problem hiding this comment.
Fix typo and enhance CORS security configuration.
- Fix the typo in the comment ("exmaple" → "example")
- Consider adding more security-related CORS headers:
map $http_origin $cors{
default '';
~^(${CORS_ALLOWED_ORIGINS})$ $http_origin;
}
+
+# Consider adding these headers in the location block:
+# add_header 'Access-Control-Max-Age' '3600';
+# add_header 'Vary' 'Origin';- Be cautious with regex matching of origins - ensure the
CORS_ALLOWED_ORIGINSpattern is strictly validated to prevent security issues.
Committable suggestion skipped: line range outside the PR's diff.
.github/workflows/deploy.yml
Outdated
| docker tag reearth/reearth-marketplace:nightly $SERVER_GCP | ||
| docker push $SERVER_IMAGE_GCP |
There was a problem hiding this comment.
Fix variable typo and shell script security issues.
There are several issues in the server deployment section:
- Line 71:
$SERVER_GCPshould be$SERVER_IMAGE_GCP - Unquoted variables in shell commands
Apply these fixes:
- docker tag reearth/reearth-marketplace:nightly $SERVER_GCP
- docker push $SERVER_IMAGE_GCP
+ docker tag reearth/reearth-marketplace:nightly "$SERVER_IMAGE_GCP"
+ docker push "$SERVER_IMAGE_GCP"
gcloud run deploy reearth-marketplace-backend \
- --image $SERVER_IMAGE_GCP \
+ --image "$SERVER_IMAGE_GCP" \Also applies to: 76-76
| - name: checkout | ||
| uses: actions/checkout@v4 | ||
| - name: Fetch tags | ||
| run: git fetch --prune --unshallow --tags | ||
| - name: Get info | ||
| id: info | ||
| # The tag name should be retrieved lazily, as tagging may be delayed. | ||
| env: | ||
| BRANCH: ${{ github.event.workflow_run.head_branch }} | ||
| run: | | ||
| echo "::set-output name=sha_short::$(git rev-parse --short HEAD)" | ||
| if [[ "$BRANCH" = "release" ]]; then | ||
| TAG=$(git tag --points-at HEAD) | ||
| if [[ ! -z "$TAG" ]]; then | ||
| echo "::set-output name=new_tag::$TAG" | ||
| echo "::set-output name=new_tag_short::${TAG#v}" | ||
| else | ||
| echo "::set-output name=name::rc" | ||
| fi | ||
| else | ||
| echo "::set-output name=name::nightly" | ||
| fi | ||
| - name: Show info | ||
| env: | ||
| SHA_SHORT: ${{ steps.info.outputs.sha_short }} | ||
| NEW_TAG: ${{ steps.info.outputs.new_tag }} | ||
| NEW_TAG_SHORT: ${{ steps.info.outputs.new_tag_short }} | ||
| NAME: ${{ steps.info.outputs.name }} | ||
| run: echo "sha_short=$SHA_SHORT, new_tag=$NEW_TAG, new_tag_short=$NEW_TAG_SHORT, name=$NAME" |
There was a problem hiding this comment.
Update deprecated GitHub Actions commands and improve shell script
- The
set-outputcommand is deprecated. UseGITHUB_OUTPUTinstead. - Shell script style can be improved.
Apply these changes:
run: |
- echo "::set-output name=sha_short::$(git rev-parse --short HEAD)"
+ echo "sha_short=$(git rev-parse --short HEAD)" >> $GITHUB_OUTPUT
if [[ "$BRANCH" = "release" ]]; then
TAG=$(git tag --points-at HEAD)
- if [[ ! -z "$TAG" ]]; then
+ if [[ -n "$TAG" ]]; then
- echo "::set-output name=new_tag::$TAG"
- echo "::set-output name=new_tag_short::${TAG#v}"
+ echo "new_tag=$TAG" >> $GITHUB_OUTPUT
+ echo "new_tag_short=${TAG#v}" >> $GITHUB_OUTPUT
else
- echo "::set-output name=name::rc"
+ echo "name=rc" >> $GITHUB_OUTPUT
fi
else
- echo "::set-output name=name::nightly"
+ echo "name=nightly" >> $GITHUB_OUTPUT
fi📝 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.
| - name: checkout | |
| uses: actions/checkout@v4 | |
| - name: Fetch tags | |
| run: git fetch --prune --unshallow --tags | |
| - name: Get info | |
| id: info | |
| # The tag name should be retrieved lazily, as tagging may be delayed. | |
| env: | |
| BRANCH: ${{ github.event.workflow_run.head_branch }} | |
| run: | | |
| echo "::set-output name=sha_short::$(git rev-parse --short HEAD)" | |
| if [[ "$BRANCH" = "release" ]]; then | |
| TAG=$(git tag --points-at HEAD) | |
| if [[ ! -z "$TAG" ]]; then | |
| echo "::set-output name=new_tag::$TAG" | |
| echo "::set-output name=new_tag_short::${TAG#v}" | |
| else | |
| echo "::set-output name=name::rc" | |
| fi | |
| else | |
| echo "::set-output name=name::nightly" | |
| fi | |
| - name: Show info | |
| env: | |
| SHA_SHORT: ${{ steps.info.outputs.sha_short }} | |
| NEW_TAG: ${{ steps.info.outputs.new_tag }} | |
| NEW_TAG_SHORT: ${{ steps.info.outputs.new_tag_short }} | |
| NAME: ${{ steps.info.outputs.name }} | |
| run: echo "sha_short=$SHA_SHORT, new_tag=$NEW_TAG, new_tag_short=$NEW_TAG_SHORT, name=$NAME" | |
| - name: checkout | |
| uses: actions/checkout@v4 | |
| - name: Fetch tags | |
| run: git fetch --prune --unshallow --tags | |
| - name: Get info | |
| id: info | |
| # The tag name should be retrieved lazily, as tagging may be delayed. | |
| env: | |
| BRANCH: ${{ github.event.workflow_run.head_branch }} | |
| run: | | |
| echo "sha_short=$(git rev-parse --short HEAD)" >> $GITHUB_OUTPUT | |
| if [[ "$BRANCH" = "release" ]]; then | |
| TAG=$(git tag --points-at HEAD) | |
| if [[ -n "$TAG" ]]; then | |
| echo "new_tag=$TAG" >> $GITHUB_OUTPUT | |
| echo "new_tag_short=${TAG#v}" >> $GITHUB_OUTPUT | |
| else | |
| echo "name=rc" >> $GITHUB_OUTPUT | |
| fi | |
| else | |
| echo "name=nightly" >> $GITHUB_OUTPUT | |
| fi | |
| - name: Show info | |
| env: | |
| SHA_SHORT: ${{ steps.info.outputs.sha_short }} | |
| NEW_TAG: ${{ steps.info.outputs.new_tag }} | |
| NEW_TAG_SHORT: ${{ steps.info.outputs.new_tag_short }} | |
| NAME: ${{ steps.info.outputs.name }} | |
| run: echo "sha_short=$SHA_SHORT, new_tag=$NEW_TAG, new_tag_short=$NEW_TAG_SHORT, name=$NAME" |
🧰 Tools
🪛 actionlint
32-32: shellcheck reported issue in this script: SC2236:style:4:9: Use -n instead of ! -z
(shellcheck)
32-32: workflow command "set-output" was deprecated. use echo "{name}={value}" >> $GITHUB_OUTPUT instead: https://docs.github.com/en/actions/using-workflows/workflow-commands-for-github-actions
(deprecated-commands)
32-32: workflow command "set-output" was deprecated. use echo "{name}={value}" >> $GITHUB_OUTPUT instead: https://docs.github.com/en/actions/using-workflows/workflow-commands-for-github-actions
(deprecated-commands)
32-32: workflow command "set-output" was deprecated. use echo "{name}={value}" >> $GITHUB_OUTPUT instead: https://docs.github.com/en/actions/using-workflows/workflow-commands-for-github-actions
(deprecated-commands)
32-32: workflow command "set-output" was deprecated. use echo "{name}={value}" >> $GITHUB_OUTPUT instead: https://docs.github.com/en/actions/using-workflows/workflow-commands-for-github-actions
(deprecated-commands)
32-32: workflow command "set-output" was deprecated. use echo "{name}={value}" >> $GITHUB_OUTPUT instead: https://docs.github.com/en/actions/using-workflows/workflow-commands-for-github-actions
(deprecated-commands)
.github/workflows/web_build.yml
Outdated
| id: options | ||
| env: | ||
| TAG: ${{ needs.info.outputs.tag_short }} | ||
| NAME: ${{ needs.info.outputs.name }} | ||
| SHA: ${{ needs.info.outputs.sha_short }} | ||
| run: | | ||
| if [[ -n $TAG ]]; then | ||
| PLATFORMS=linux/amd64,linux/arm64 | ||
| VERSION=$TAG | ||
| TAGS=$IMAGE_NAME:$TAG | ||
| if [[ ! $TAG =~ '-' ]]; then | ||
| TAGS+=,${IMAGE_NAME}:${TAG%.*} | ||
| TAGS+=,${IMAGE_NAME}:${TAG%%.*} | ||
| TAGS+=,${IMAGE_NAME}:latest | ||
| fi | ||
| else | ||
| PLATFORMS=linux/amd64 | ||
| VERSION=$SHA | ||
| TAGS=$IMAGE_NAME:$NAME | ||
| fi | ||
| echo "::set-output name=platforms::$PLATFORMS" | ||
| echo "::set-output name=version::$VERSION" | ||
| echo "::set-output name=tags::$TAGS" | ||
| - name: Build and push docker image | ||
| uses: docker/build-push-action@v6 | ||
| with: | ||
| context: ./web | ||
| platforms: ${{ steps.options.outputs.platforms }} | ||
| push: true | ||
| tags: ${{ steps.options.outputs.tags }} | ||
| cache-from: type=gha | ||
| cache-to: type=gha,mode=max |
There was a problem hiding this comment.
Fix type error in environment variables and consider simplifying tag logic
- The
tag_shortproperty doesn't exist in the info job outputs. It should benew_tag_short. - Tag generation logic could be simplified for better maintainability.
Fix the environment variables:
env:
- TAG: ${{ needs.info.outputs.tag_short }}
+ TAG: ${{ needs.info.outputs.new_tag_short }}
NAME: ${{ needs.info.outputs.name }}
SHA: ${{ needs.info.outputs.sha_short }}Consider extracting the tag generation logic into a separate composite action for better reusability and maintainability.
📝 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.
| id: options | |
| env: | |
| TAG: ${{ needs.info.outputs.tag_short }} | |
| NAME: ${{ needs.info.outputs.name }} | |
| SHA: ${{ needs.info.outputs.sha_short }} | |
| run: | | |
| if [[ -n $TAG ]]; then | |
| PLATFORMS=linux/amd64,linux/arm64 | |
| VERSION=$TAG | |
| TAGS=$IMAGE_NAME:$TAG | |
| if [[ ! $TAG =~ '-' ]]; then | |
| TAGS+=,${IMAGE_NAME}:${TAG%.*} | |
| TAGS+=,${IMAGE_NAME}:${TAG%%.*} | |
| TAGS+=,${IMAGE_NAME}:latest | |
| fi | |
| else | |
| PLATFORMS=linux/amd64 | |
| VERSION=$SHA | |
| TAGS=$IMAGE_NAME:$NAME | |
| fi | |
| echo "::set-output name=platforms::$PLATFORMS" | |
| echo "::set-output name=version::$VERSION" | |
| echo "::set-output name=tags::$TAGS" | |
| - name: Build and push docker image | |
| uses: docker/build-push-action@v6 | |
| with: | |
| context: ./web | |
| platforms: ${{ steps.options.outputs.platforms }} | |
| push: true | |
| tags: ${{ steps.options.outputs.tags }} | |
| cache-from: type=gha | |
| cache-to: type=gha,mode=max | |
| id: options | |
| env: | |
| TAG: ${{ needs.info.outputs.new_tag_short }} | |
| NAME: ${{ needs.info.outputs.name }} | |
| SHA: ${{ needs.info.outputs.sha_short }} | |
| run: | | |
| if [[ -n $TAG ]]; then | |
| PLATFORMS=linux/amd64,linux/arm64 | |
| VERSION=$TAG | |
| TAGS=$IMAGE_NAME:$TAG | |
| if [[ ! $TAG =~ '-' ]]; then | |
| TAGS+=,${IMAGE_NAME}:${TAG%.*} | |
| TAGS+=,${IMAGE_NAME}:${TAG%%.*} | |
| TAGS+=,${IMAGE_NAME}:latest | |
| fi | |
| else | |
| PLATFORMS=linux/amd64 | |
| VERSION=$SHA | |
| TAGS=$IMAGE_NAME:$NAME | |
| fi | |
| echo "::set-output name=platforms::$PLATFORMS" | |
| echo "::set-output name=version::$VERSION" | |
| echo "::set-output name=tags::$TAGS" | |
| - name: Build and push docker image | |
| uses: docker/build-push-action@v6 | |
| with: | |
| context: ./web | |
| platforms: ${{ steps.options.outputs.platforms }} | |
| push: true | |
| tags: ${{ steps.options.outputs.tags }} | |
| cache-from: type=gha | |
| cache-to: type=gha,mode=max |
🧰 Tools
🪛 actionlint
76-76: property "tag_short" is not defined in object type {name: string; new_tag: string; new_tag_short: string; sha_short: string}
(expression)
79-79: workflow command "set-output" was deprecated. use echo "{name}={value}" >> $GITHUB_OUTPUT instead: https://docs.github.com/en/actions/using-workflows/workflow-commands-for-github-actions
(deprecated-commands)
79-79: workflow command "set-output" was deprecated. use echo "{name}={value}" >> $GITHUB_OUTPUT instead: https://docs.github.com/en/actions/using-workflows/workflow-commands-for-github-actions
(deprecated-commands)
79-79: workflow command "set-output" was deprecated. use echo "{name}={value}" >> $GITHUB_OUTPUT instead: https://docs.github.com/en/actions/using-workflows/workflow-commands-for-github-actions
(deprecated-commands)
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (3)
web/Dockerfile (3)
4-19: Optimize build stage configurationThe current build configuration can be improved for better efficiency and maintainability:
- Consider combining the two RUN steps to reduce layers
- Add build arguments for better version control
- Consider using
yarn install --frozen-lockfilefor deterministic builds+ARG NODE_ENV=production RUN --mount=type=bind,source=package.json,target=package.json \ --mount=type=bind,source=yarn.lock,target=yarn.lock \ --mount=type=bind,source=index.html,target=index.html \ --mount=type=bind,source=tsconfig.json,target=tsconfig.json \ --mount=type=bind,source=tsconfig.node.json,target=tsconfig.node.json \ --mount=type=bind,source=vite.config.ts,target=vite.config.ts \ --mount=type=bind,source=vite.config.ext.ts,target=vite.config.ext.ts \ --mount=type=bind,source=src,target=src \ --mount=type=bind,source=TEMP.js,target=TEMP.js \ --mount=type=cache,target=/root/.yarn,sharing=locked \ - yarn install - -RUN --mount=type=bind,source=package.json,target=package.json \ - --mount=type=bind,source=yarn.lock,target=yarn.lock \ yarn install --frozen-lockfile && \ yarn build && \ yarn build:ext
35-44: Enhance environment variable configurationThe current environment variable setup has several potential improvements:
- Group related variables together (e.g., Auth-related, API endpoints)
- Consider using secrets management for sensitive values
- Add validation for required environment variables
Consider implementing a startup validation script:
#!/bin/sh # Add to docker/validate-env.sh required_vars="AUTH_CLIENT_ID AUTH_DOMAIN MARKETPLACE_API" for var in $required_vars; do if [ -z "$(eval echo \$$var)" ]; then echo "Error: Required environment variable '$var' is not set" exit 1 fi doneThen add it to the entrypoint:
COPY --chown=nginx:nginx docker/nginx.conf.template /etc/nginx/templates/nginx.conf.template COPY --chown=nginx:nginx docker/40-envsubst-on-reearth_config.sh /docker-entrypoint.d/ +COPY --chown=nginx:nginx docker/validate-env.sh /docker-entrypoint.d/
45-48: Add health check and security configurationsConsider adding:
- A health check endpoint for container orchestration
- Security headers in the Nginx configuration
Add these configurations to your nginx.conf.template:
# Health check endpoint location /health { access_log off; return 200 'healthy\n'; } # Security headers add_header X-Frame-Options "SAMEORIGIN"; add_header X-XSS-Protection "1; mode=block"; add_header X-Content-Type-Options "nosniff"; add_header Referrer-Policy "strict-origin-when-cross-origin";
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
web/yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (3)
web/Dockerfile(1 hunks)web/docker/nginx.conf.template(1 hunks)web/docker/reearth_config.json.template(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- web/docker/nginx.conf.template
- web/docker/reearth_config.json.template
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (2)
.github/workflows/deploy.yml (2)
28-33: Enhance Cloud Run deployment configuration.Consider adding these recommended flags for better security and resource management:
gcloud run deploy reearth-marketplace-web \ - --image $WEB_IMAGE_GCP \ + --image "$WEB_IMAGE_GCP" \ --region $GCP_REGION \ --platform managed \ + --port 80 \ + --allow-unauthenticated \ + --min-instances 0 \ + --max-instances 10 \ --quiet🧰 Tools
🪛 actionlint (1.7.3)
28-28: shellcheck reported issue in this script: SC2086:info:2:11: Double quote to prevent globbing and word splitting
(shellcheck)
28-28: shellcheck reported issue in this script: SC2086:info:3:12: Double quote to prevent globbing and word splitting
(shellcheck)
23-33: Consider adding error handling and logging.The deployment steps could benefit from better error handling and logging:
- Add error checking after critical operations
- Include logging for debugging purposes
- Consider implementing deployment verification
Example enhancement:
run: | set -eo pipefail # Exit on error echo "Starting deployment..." # Your existing commands here # Add verification echo "Verifying deployment..." gcloud run services describe reearth-marketplace-web \ --region $GCP_REGION \ --format='get(status.url)' | \ xargs curl -s -o /dev/null -w "%{http_code}\n" | \ grep 200 || (echo "Deployment verification failed" && exit 1)Also applies to: 70-75
🧰 Tools
🪛 actionlint (1.7.3)
23-23: shellcheck reported issue in this script: SC2086:info:2:17: Double quote to prevent globbing and word splitting
(shellcheck)
23-23: shellcheck reported issue in this script: SC2086:info:3:13: Double quote to prevent globbing and word splitting
(shellcheck)
28-28: shellcheck reported issue in this script: SC2086:info:2:11: Double quote to prevent globbing and word splitting
(shellcheck)
28-28: shellcheck reported issue in this script: SC2086:info:3:12: Double quote to prevent globbing and word splitting
(shellcheck)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
.github/workflows/deploy.yml(2 hunks)
🧰 Additional context used
🪛 actionlint (1.7.3)
.github/workflows/deploy.yml
23-23: shellcheck reported issue in this script: SC2086:info:2:17: Double quote to prevent globbing and word splitting
(shellcheck)
23-23: shellcheck reported issue in this script: SC2086:info:3:13: Double quote to prevent globbing and word splitting
(shellcheck)
28-28: shellcheck reported issue in this script: SC2086:info:2:11: Double quote to prevent globbing and word splitting
(shellcheck)
28-28: shellcheck reported issue in this script: SC2086:info:3:12: Double quote to prevent globbing and word splitting
(shellcheck)
73-73: shellcheck reported issue in this script: SC2086:info:2:11: Double quote to prevent globbing and word splitting
(shellcheck)
73-73: shellcheck reported issue in this script: SC2086:info:3:12: Double quote to prevent globbing and word splitting
(shellcheck)
🔇 Additional comments (1)
.github/workflows/deploy.yml (1)
10-11: LGTM: Environment variables are well-structured.
The Docker image registry paths follow GCP conventions and are properly namespaced.
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
.github/workflows/deploy.yml(2 hunks)
🧰 Additional context used
🪛 actionlint (1.7.3)
.github/workflows/deploy.yml
23-23: shellcheck reported issue in this script: SC2086:info:2:17: Double quote to prevent globbing and word splitting
(shellcheck)
23-23: shellcheck reported issue in this script: SC2086:info:3:13: Double quote to prevent globbing and word splitting
(shellcheck)
28-28: shellcheck reported issue in this script: SC2086:info:2:11: Double quote to prevent globbing and word splitting
(shellcheck)
28-28: shellcheck reported issue in this script: SC2086:info:3:12: Double quote to prevent globbing and word splitting
(shellcheck)
73-73: shellcheck reported issue in this script: SC2086:info:2:11: Double quote to prevent globbing and word splitting
(shellcheck)
73-73: shellcheck reported issue in this script: SC2086:info:3:12: Double quote to prevent globbing and word splitting
(shellcheck)
🔇 Additional comments (1)
.github/workflows/deploy.yml (1)
10-11: LGTM: Environment variables are well-structured
The new environment variables for Docker images follow consistent naming and use correct GCP artifact registry paths.
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Outside diff range and nitpick comments (2)
.github/workflows/web_build.yml (2)
3-7: Enhance workflow security with additional checksConsider adding repository check to prevent potential security issues with workflow_run events:
on: workflow_run: workflows: [ci-web] types: [completed] branches: [main, release] + repositories: + - reearth/reearth-marketplace
16-16: Simplify complex conditionalThe current condition is hard to read. Consider breaking it down into multiple conditions or creating a composite condition:
- if: github.event.workflow_run.conclusion != 'failure' && github.event.repository.full_name == 'reearth/reearth-marketplace' && (github.event.workflow_run.head_branch == 'release' || !startsWith(github.event.head_commit.message, 'v')) + if: | + github.event.workflow_run.conclusion != 'failure' && + github.event.repository.full_name == 'reearth/reearth-marketplace' && + ( + github.event.workflow_run.head_branch == 'release' || + !startsWith(github.event.head_commit.message, 'v') + )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
.github/workflows/deploy.yml(2 hunks).github/workflows/web_build.yml(1 hunks)
🧰 Additional context used
🪛 actionlint (1.7.3)
.github/workflows/deploy.yml
26-26: shellcheck reported issue in this script: SC2086:info:2:17: Double quote to prevent globbing and word splitting
(shellcheck)
26-26: shellcheck reported issue in this script: SC2086:info:3:13: Double quote to prevent globbing and word splitting
(shellcheck)
31-31: shellcheck reported issue in this script: SC2086:info:2:11: Double quote to prevent globbing and word splitting
(shellcheck)
31-31: shellcheck reported issue in this script: SC2086:info:3:12: Double quote to prevent globbing and word splitting
(shellcheck)
76-76: shellcheck reported issue in this script: SC2086:info:2:11: Double quote to prevent globbing and word splitting
(shellcheck)
76-76: shellcheck reported issue in this script: SC2086:info:3:12: Double quote to prevent globbing and word splitting
(shellcheck)
.github/workflows/web_build.yml
32-32: shellcheck reported issue in this script: SC2236:style:4:9: Use -n instead of ! -z
(shellcheck)
32-32: workflow command "set-output" was deprecated. use echo "{name}={value}" >> $GITHUB_OUTPUT instead: https://docs.github.com/en/actions/using-workflows/workflow-commands-for-github-actions
(deprecated-commands)
32-32: workflow command "set-output" was deprecated. use echo "{name}={value}" >> $GITHUB_OUTPUT instead: https://docs.github.com/en/actions/using-workflows/workflow-commands-for-github-actions
(deprecated-commands)
32-32: workflow command "set-output" was deprecated. use echo "{name}={value}" >> $GITHUB_OUTPUT instead: https://docs.github.com/en/actions/using-workflows/workflow-commands-for-github-actions
(deprecated-commands)
32-32: workflow command "set-output" was deprecated. use echo "{name}={value}" >> $GITHUB_OUTPUT instead: https://docs.github.com/en/actions/using-workflows/workflow-commands-for-github-actions
(deprecated-commands)
32-32: workflow command "set-output" was deprecated. use echo "{name}={value}" >> $GITHUB_OUTPUT instead: https://docs.github.com/en/actions/using-workflows/workflow-commands-for-github-actions
(deprecated-commands)
76-76: property "tag_short" is not defined in object type {name: string; new_tag: string; new_tag_short: string; sha_short: string}
(expression)
79-79: workflow command "set-output" was deprecated. use echo "{name}={value}" >> $GITHUB_OUTPUT instead: https://docs.github.com/en/actions/using-workflows/workflow-commands-for-github-actions
(deprecated-commands)
79-79: workflow command "set-output" was deprecated. use echo "{name}={value}" >> $GITHUB_OUTPUT instead: https://docs.github.com/en/actions/using-workflows/workflow-commands-for-github-actions
(deprecated-commands)
79-79: workflow command "set-output" was deprecated. use echo "{name}={value}" >> $GITHUB_OUTPUT instead: https://docs.github.com/en/actions/using-workflows/workflow-commands-for-github-actions
(deprecated-commands)
🔇 Additional comments (3)
.github/workflows/deploy.yml (2)
10-11: LGTM: Environment variables are well-structured
The Docker image variables follow the correct GCP artifact registry format and maintain consistent naming conventions.
31-36: 🛠️ Refactor suggestion
Enhance Cloud Run deployment configuration
The current Cloud Run configuration lacks important production-ready parameters for resource management, scaling, and reliability.
Apply these improvements:
gcloud run deploy reearth-marketplace-web \
- --image $WEB_IMAGE_GCP \
- --region $GCP_REGION \
- --platform managed \
- --quiet
+ --image "$WEB_IMAGE_GCP" \
+ --region "$GCP_REGION" \
+ --platform managed \
+ --port 8080 \
+ --allow-unauthenticated \
+ --min-instances 1 \
+ --max-instances 10 \
+ --memory 1Gi \
+ --cpu 1 \
+ --timeout 300s \
+ --health-check-path / \
+ --quietLikely invalid or redundant comment.
🧰 Tools
🪛 actionlint (1.7.3)
31-31: shellcheck reported issue in this script: SC2086:info:2:11: Double quote to prevent globbing and word splitting
(shellcheck)
31-31: shellcheck reported issue in this script: SC2086:info:3:12: Double quote to prevent globbing and word splitting
(shellcheck)
.github/workflows/web_build.yml (1)
100-100: Verify Docker context path
Please ensure that the Docker context path './web' exists and contains the necessary Dockerfile.
✅ Verification successful
Docker context path is valid and properly configured
The verification confirms that:
- The
./webdirectory exists - A valid Dockerfile is present in the directory
- The Dockerfile contains proper configuration for a Node.js application build
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if the Docker context path exists and contains Dockerfile
if [[ -d "./web" ]]; then
echo "web directory exists"
if [[ -f "./web/Dockerfile" ]]; then
echo "Dockerfile exists"
head -n 5 "./web/Dockerfile"
else
echo "Dockerfile not found in web directory"
fi
else
echo "web directory not found"
fi
Length of output: 334
| - name: Configure docker | ||
| run: gcloud auth configure-docker us-central1-docker.pkg.dev --quiet | ||
| - name: docker build and push | ||
| run: | | ||
| cd web | ||
| docker build -t $WEB_IMAGE_GCP . | ||
| docker push $WEB_IMAGE_GCP |
There was a problem hiding this comment.
Fix shell script security issues
Unquoted variables in Docker commands could lead to word splitting or command injection issues.
Apply this fix:
run: |
cd web
- docker build -t $WEB_IMAGE_GCP .
- docker push $WEB_IMAGE_GCP
+ docker build -t "$WEB_IMAGE_GCP" .
+ docker push "$WEB_IMAGE_GCP"📝 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.
| - name: Configure docker | |
| run: gcloud auth configure-docker us-central1-docker.pkg.dev --quiet | |
| - name: docker build and push | |
| run: | | |
| cd web | |
| docker build -t $WEB_IMAGE_GCP . | |
| docker push $WEB_IMAGE_GCP | |
| - name: Configure docker | |
| run: gcloud auth configure-docker us-central1-docker.pkg.dev --quiet | |
| - name: docker build and push | |
| run: | | |
| cd web | |
| docker build -t "$WEB_IMAGE_GCP" . | |
| docker push "$WEB_IMAGE_GCP" |
🧰 Tools
🪛 actionlint (1.7.3)
26-26: shellcheck reported issue in this script: SC2086:info:2:17: Double quote to prevent globbing and word splitting
(shellcheck)
26-26: shellcheck reported issue in this script: SC2086:info:3:13: Double quote to prevent globbing and word splitting
(shellcheck)
| docker tag reearth/reearth-marketplace-api:nightly $SERVER_IMAGE_GCP | ||
| docker push $SERVER_IMAGE_GCP | ||
| - name: Deploy to Cloud Run | ||
| run: | | ||
| gcloud run deploy reearth-marketplace-backend \ | ||
| --image $IMAGE_GCP \ | ||
| gcloud run deploy reearth-marketplace-api \ | ||
| --image $SERVER_IMAGE_GCP \ | ||
| --region $GCP_REGION \ | ||
| --platform managed \ | ||
| --quiet |
There was a problem hiding this comment.
Fix variable inconsistency and enhance deployment configuration
Multiple issues need to be addressed in the server deployment section:
- Incorrect variable name in docker tag command
- Unquoted variables in shell commands
- Missing recommended Cloud Run parameters
Apply these fixes:
- docker tag reearth/reearth-marketplace-api:nightly $SERVER_GCP
- docker push $SERVER_IMAGE_GCP
+ docker tag reearth/reearth-marketplace-api:nightly "$SERVER_IMAGE_GCP"
+ docker push "$SERVER_IMAGE_GCP"
gcloud run deploy reearth-marketplace-api \
- --image $SERVER_IMAGE_GCP \
- --region $GCP_REGION \
- --platform managed \
- --quiet
+ --image "$SERVER_IMAGE_GCP" \
+ --region "$GCP_REGION" \
+ --platform managed \
+ --port 8080 \
+ --allow-unauthenticated \
+ --min-instances 1 \
+ --max-instances 10 \
+ --memory 1Gi \
+ --cpu 1 \
+ --timeout 300s \
+ --health-check-path /api/health \
+ --quietCommittable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 actionlint (1.7.3)
76-76: shellcheck reported issue in this script: SC2086:info:2:11: Double quote to prevent globbing and word splitting
(shellcheck)
76-76: shellcheck reported issue in this script: SC2086:info:3:12: Double quote to prevent globbing and word splitting
(shellcheck)
| run: | | ||
| gcloud run deploy reearth-marketplace-web \ | ||
| --image $WEB_IMAGE_GCP \ | ||
| --region $GCP_REGION \ | ||
| --platform managed \ | ||
| --quiet |
There was a problem hiding this comment.
💡 Codebase verification
Missing health check endpoint and port configuration needs attention
The verification reveals important deployment prerequisites that need to be addressed:
- No health check endpoint is implemented in the server code, which is crucial for Cloud Run to monitor the service health
- While the nginx configuration properly handles the Cloud Run port (8080), the server's Dockerfile lacks both port exposure and health endpoint configuration
Required actions:
- Implement a health check endpoint in the server code (e.g.,
/healthor/healthz) - Add
EXPOSE 8080to the server's Dockerfile to explicitly document the port configuration - Consider adding readiness/liveness probe configurations in the deployment
🔗 Analysis chain
Verify deployment prerequisites
Before deploying, please verify:
- Health check endpoints are implemented
- Docker configurations align with Cloud Run parameters
Also applies to: 77-81
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if health check endpoints and Docker configurations exist
# Check for health check endpoint in server code
echo "Checking server health endpoint..."
rg -l "health" --type go
# Check Docker port configurations
echo "Checking Docker port configurations..."
fd Dockerfile | xargs grep -l "EXPOSE"
Length of output: 229
Script:
#!/bin/bash
# Let's try a broader search for health-related endpoints and Docker configurations
# Check for any health-related routes or endpoints
echo "Checking for health endpoints in all files..."
rg -i "(/health|healthz|health-check|health_check)" -l
# Check for Docker configurations with different cases
echo "Checking Docker configurations..."
fd -i dockerfile
fd -i docker-compose
# Check for port-related configurations in Docker files
echo "Checking port configurations..."
fd -i docker | xargs cat
Length of output: 2798
Script:
#!/bin/bash
# Let's check the content of the nginx configuration and web Docker directory
# Check nginx configuration template
echo "Checking nginx configuration..."
cat web/docker/nginx.conf.template
# Check the web Docker directory for port and health check configurations
echo "Checking web Docker directory contents..."
ls -la web/docker/
cat web/docker/40-envsubst-on-reearth_config.sh
# Check for any health check related configurations in the server
echo "Checking server routes and configurations..."
rg -i "router|mux|handler.*health" --type go
Length of output: 2851
🧰 Tools
🪛 actionlint (1.7.3)
31-31: shellcheck reported issue in this script: SC2086:info:2:11: Double quote to prevent globbing and word splitting
(shellcheck)
31-31: shellcheck reported issue in this script: SC2086:info:3:12: Double quote to prevent globbing and word splitting
(shellcheck)
| context: ./web | ||
| platforms: ${{ steps.options.outputs.platforms }} | ||
| push: true | ||
| tags: ${{ steps.options.outputs.new_tag_short }} |
There was a problem hiding this comment.
Fix incorrect tags parameter in docker/build-push-action
The tags parameter is using new_tag_short instead of the tags output from the options step.
- tags: ${{ steps.options.outputs.new_tag_short }}
+ tags: ${{ steps.options.outputs.tags }}📝 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.
| tags: ${{ steps.options.outputs.new_tag_short }} | |
| tags: ${{ steps.options.outputs.tags }} |
| - name: Get options | ||
| id: options | ||
| env: | ||
| TAG: ${{ needs.info.outputs.tag_short }} |
There was a problem hiding this comment.
Fix incorrect output variable reference
The environment variable TAG is referencing a non-existent output variable 'tag_short'.
- TAG: ${{ needs.info.outputs.tag_short }}
+ TAG: ${{ needs.info.outputs.new_tag_short }}📝 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.
| TAG: ${{ needs.info.outputs.tag_short }} | |
| TAG: ${{ needs.info.outputs.new_tag_short }} |
🧰 Tools
🪛 actionlint (1.7.3)
76-76: property "tag_short" is not defined in object type {name: string; new_tag: string; new_tag_short: string; sha_short: string}
(expression)
What I've done
What I haven't done
How I tested
Which point I want you to review particularly
Notes
Summary by CodeRabbit
Release Notes
New Features
.dockerignorefile to optimize Docker build context.Bug Fixes
Chores