1312 move text to cypher to a dedicated directory #1314
Conversation
…her to a dedicated directory
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Static Code Review 📊 ✅ All quality checks passed! |
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughThe changes reorganize the text-to-cypher binary into a dedicated Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes
Poem
Pre-merge checks and finishing touches✅ Passed checks (5 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Pull request overview
This PR refactors the text-to-cypher service to use a dedicated directory structure, improving separation of concerns and Docker image organization. The changes also include security enhancements and environment variable renaming for clarity.
Key changes:
- Relocated text-to-cypher service from
/appto/text-to-cypherdirectory - Renamed
CYPHERenvironment variable toTEXT_TO_CYPHERfor clarity - Added Trivy vulnerability scanning to CI/CD workflows
Reviewed changes
Copilot reviewed 6 out of 7 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| supervisord.conf | Updated text-to-cypher service directory and simplified command |
| package.json | Changed Next.js version constraint from exact to caret |
| entrypoint.sh | Updated environment variable check from CYPHER to TEXT_TO_CYPHER |
| Dockerfile | Reorganized file structure, added npm removal for security, and created dedicated text-to-cypher directory |
| .github/workflows/release-image.yml | Added Trivy security scanning step and upgraded build-push-action version |
| .github/workflows/nextjs.yml | Added comprehensive security-scan job with Trivy integration and PR comments |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
entrypoint.sh
Outdated
| case "${TEXT_TO_CYPHER:-}" in | ||
| 1|true|True|TRUE) | ||
| export CYPHER="true" |
There was a problem hiding this comment.
The environment variable was renamed from CYPHER to TEXT_TO_CYPHER in the case statement (line 4), but the export statement (line 6) still uses the old CYPHER variable name. This inconsistency means supervisord won't recognize the new variable. Change 'export CYPHER' to 'export TEXT_TO_CYPHER' to maintain consistency.
supervisord.conf
Outdated
| [program:text-to-cypher] | ||
| command=sh -c '/app/text-to-cypher 2>&1 | while IFS= read -r line; do echo "[$(date "+%%Y-%%m-%%d %%H:%%M:%%S")] [TEXT-TO-CYPHER] $line"; done' | ||
| directory=/app | ||
| command=sh -c './text-to-cypher' |
There was a problem hiding this comment.
The simplified command removes timestamp logging that was present in the original implementation. This makes debugging and monitoring more difficult. Consider restoring the logging wrapper or ensure that the text-to-cypher binary itself handles timestamped logging internally.
| command=sh -c './text-to-cypher' | |
| command=sh -c './text-to-cypher 2>&1 | awk '\''{ cmd="date -u +\"%Y-%m-%dT%H:%M:%SZ\""; cmd | getline ts; close(cmd); print ts, $0; fflush(); }'\''' |
Co-authored-by: Copilot <[email protected]>
typo-app
bot
added
Manual Review Time 30-60 min
and removed
Manual Review Time 30-60 min
labels
typo-app
bot
added
Manual Review Time 30-60 min
and removed
Manual Review Time 30-60 min
labels
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
supervisord.conf (1)
22-24: Directory and environment variable updates look correct.The changes properly align with the Dockerfile's
/text-to-cypherdirectory and the renamedTEXT_TO_CYPHERvariable.Regarding the simplified command: the timestamp logging wrapper was removed. If timestamped logs are needed for debugging, consider relying on the container runtime's logging driver (e.g., Docker's
--log-opt t=trueor fluentd/json-file with timestamps) rather than inline awk..github/workflows/release-image.yml (1)
63-63: Trivy scan will fail whenTAGScontains multiple values.When pushing tagged releases,
env.TAGSwill befalkordb/falkordb-browser:latest,falkordb/falkordb-browser:${{ github.ref_name }}. Trivy'simage-refexpects a single reference.Proposed fix: use the first tag
One approach is to set a dedicated scan tag:
- name: Set tags run: | if ${{ github.event_name == 'push' && startsWith(github.ref, 'refs/tags') }}; then echo "TAGS=falkordb/falkordb-browser:latest,falkordb/falkordb-browser:${{ github.ref_name }}" >> $GITHUB_ENV + echo "SCAN_TAG=falkordb/falkordb-browser:latest" >> $GITHUB_ENV else echo "TAGS=falkordb/falkordb-browser:edge" >> $GITHUB_ENV + echo "SCAN_TAG=falkordb/falkordb-browser:edge" >> $GITHUB_ENV fi - name: Run Trivy vulnerability scanner uses: aquasecurity/[email protected] with: - image-ref: ${{ env.TAGS }} + image-ref: ${{ env.SCAN_TAG }}
🧹 Nitpick comments (3)
.github/workflows/nextjs.yml (2)
16-19: Consider scoping permissions to only the jobs that need them.The
buildjob doesn't appear to usepull-requests: writeorsecurity-events: write. These permissions are only needed bysecurity-scan. You could move this permissions block to just thesecurity-scanjob, or use workflow-level defaults with job-level overrides.
21-22: Updateactions/checkoutto v4.Both checkout steps use v3, which the static analyzer flags as outdated.
Proposed fix
- name: Checkout - uses: actions/checkout@v3 + uses: actions/checkout@v4Apply to both line 22 and line 71.
Also applies to: 70-71
.github/workflows/release-image.yml (1)
70-78: Note: This rebuilds the image.The push step rebuilds rather than pushing the previously scanned image. Consider using Docker's
docker pushcommand directly on the loaded image, or enable Docker layer caching to avoid redundant builds.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (5)
.github/workflows/nextjs.yml(2 hunks).github/workflows/release-image.yml(2 hunks)Dockerfile(3 hunks)entrypoint.sh(1 hunks)supervisord.conf(1 hunks)
🧰 Additional context used
🪛 actionlint (1.7.9)
.github/workflows/nextjs.yml
71-71: the runner of "actions/checkout@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Setup and cache dependencies
- GitHub Check: security-scan
🔇 Additional comments (7)
entrypoint.sh (1)
4-12: LGTM!The environment variable rename from
CYPHERtoTEXT_TO_CYPHERis now consistent throughout the script. The boolean normalization for supervisord is correct.Dockerfile (3)
48-50: Good security hardening.Removing npm from the production image reduces the attack surface since Next.js standalone mode doesn't require it at runtime.
62-72: LGTM!The
/text-to-cypherdirectory setup with proper ownership and the updated copy target correctly isolate the text-to-cypher component.
86-86: Environment variable renamed correctly.Consistent with
entrypoint.shandsupervisord.conf..github/workflows/nextjs.yml (2)
87-99: Solid Trivy configuration.The dual-output approach (SARIF for Security tab, table for PR comments) with
continue-on-error: trueand a separate failure step is a well-designed pattern.
118-161: PR comment handling looks good.The logic correctly finds and updates existing bot comments to avoid spam, with proper fallback to create new comments.
.github/workflows/release-image.yml (1)
49-58: Good pattern: build locally, scan, then push.Using
load: truewithpush: falsemakes the image available for Trivy scanning before pushing to the registry.
🔒 Trivy Security Scan Results |
typo-app
bot
added
Manual Review Time 5-10 min
and removed
Manual Review Time 30-60 min
labels
typo-app
bot
added
Manual Review Time 5-10 min
and removed
Manual Review Time 5-10 min
labels
![typo-app[bot]](https://avatars.githubusercontent.com/in/360763?s=60&v=4){kind=small}
| command=sh -c '/app/text-to-cypher 2>&1 | while IFS= read -r line; do echo "[$(date "+%%Y-%%m-%%d %%H:%%M:%%S")] [TEXT-TO-CYPHER] $line"; done' | ||
| directory=/app | ||
| autostart=%(ENV_CYPHER)s | ||
| command=sh -c './text-to-cypher 2>&1 | awk '\''{cmd="date -u +\"%%Y-%%m-%%dT%%H:%%M:%%SZ\""; cmd | getline ts; close(cmd); print ts, $0; fflush()}'\''' |
There was a problem hiding this comment.
performance: Inefficient logging command forks a process per log line. Use gawk's built-in strftime to avoid high CPU usage.
| command=sh -c './text-to-cypher 2>&1 | awk '\''{cmd="date -u +\"%%Y-%%m-%%dT%%H:%%M:%%SZ\""; cmd | getline ts; close(cmd); print ts, $0; fflush()}'\''' | |
| command=sh -c './text-to-cypher 2>&1 | gawk -W interactive '\''{print strftime("%%Y-%%m-%%dT%%H:%%M:%%SZ", systime(), 1), $0; fflush()}'\'\'' |
typo-app
bot
added
Manual Review Time 5-10 min
and removed
Manual Review Time 5-10 min
labels
fix #1312
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.
PR Summary by Typo
Overview
This PR refactors the Docker image and associated scripts to move the
text-to-cypherapplication into its own dedicated directory (/text-to-cypher) for better organization and isolation.Key Changes
Dockerfileto create and set permissions for the new/text-to-cypherdirectory and copy the application content into it.CYPHERenvironment variable toTEXT_TO_CYPHERacross theDockerfile,entrypoint.sh, andsupervisord.conffor clarity.supervisord.confto reflect the new directory structure and environment variable for starting thetext-to-cypherprogram.Work Breakdown
To turn off PR summary, please visit Notification settings.