Fix BASEHREF handling in Docker Compose #249
Conversation
- envsubst would overwrite existing skosmos config on every startup - basehref now written to config.ttl with bash script
|
Warning Rate limit exceeded@FelixFrizzy has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 12 minutes and 55 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (2)
WalkthroughThis pull request introduces changes to the Docker Compose configuration and Skosmos configuration files. The primary modifications involve removing quotation marks from environment variables in Changes
Sequence DiagramsequenceDiagram
participant DC as Docker Compose
participant HS as Helper Script
participant CF as Config File
DC->>HS: Execute replace-basehref.sh
HS->>CF: Read original config
HS->>CF: Create backup
HS->>CF: Replace baseHref value
HS->>CF: Validate and save changes
HS-->>DC: Return success/failure status
Possibly Related PRs
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: 0
🧹 Nitpick comments (2)
helperscripts/replace-basehref.sh (1)
1-77: Well-structured script with comprehensive error handling!The script follows best practices with clear documentation, error handling, and backup creation. A few suggestions for improvement:
- Declare and assign variables separately to avoid masking return values:
- local timestamp=$(date +"%Y%m%d_%H%M%S") + local timestamp + timestamp=$(date +"%Y%m%d_%H%M%S")
- Add input validation for the basehref format:
basehref="$2" +# Validate basehref format +if [[ ! "$basehref" =~ ^https?:// ]]; then + echo "Error: basehref must start with http:// or https://" + exit 1 +fi🧰 Tools
🪛 Shellcheck (0.10.0)
[warning] 17-17: Declare and assign separately to avoid masking return values.
(SC2155)
skosmos-live/config.ttl (1)
8-8: Consider environment-specific configuration files.The hardcoded baseHref values in both config files (
http://localhost:9000/skosmos-dev/andhttp://localhost:9000/skosmos-live/) might limit deployment flexibility. Consider maintaining separate configuration files for different environments (development, staging, production).This would allow for:
- Environment-specific settings without runtime modifications
- Better version control of configurations
- Simplified deployment process
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
docker-compose.yml(4 hunks)helperscripts/replace-basehref.sh(1 hunks)skosmos-dev/config-template.ttl(0 hunks)skosmos-dev/config.ttl(1 hunks)skosmos-live/config-template.ttl(0 hunks)skosmos-live/config.ttl(1 hunks)
💤 Files with no reviewable changes (2)
- skosmos-dev/config-template.ttl
- skosmos-live/config-template.ttl
🧰 Additional context used
🪛 Shellcheck (0.10.0)
helperscripts/replace-basehref.sh
[warning] 17-17: Declare and assign separately to avoid masking return values.
(SC2155)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: test-all
- GitHub Check: test-all
🔇 Additional comments (5)
helperscripts/replace-basehref.sh (1)
46-46: Consider file permissions for backup and temporary files.The script creates backup and temporary files but doesn't set their permissions. This could cause issues if the files need to be accessed by other users/processes.
Add permission handling:
echo "$content" > "$backup_path" +chmod --reference="$file_path" "$backup_path" echo "$updated_content" > "$temp_file_path" +chmod --reference="$file_path" "$temp_file_path"Also applies to: 50-50
docker-compose.yml (1)
165-165: Ensure script execution permissions in containers.The helper script is mounted but might not have execution permissions in the containers.
Add a step to make the script executable:
command: > /bin/bash -c "export BASEHREFDEV && + chmod +x /home/replace-basehref.sh && /home/replace-basehref.sh /var/www/html/config.ttl $$BASEHREFDEV &&Also applies to: 170-171, 195-195, 200-201
✅ Verification successful
Review comment is valid and the suggested fix is recommended.
The script has execution permissions on the host, but Docker bind mounts may not preserve these permissions. Adding
chmod +xensures reliable script execution during container startup, following the same pattern used for other critical scripts in the codebase.🏁 Scripts executed
The following scripts were executed for the analysis:
Error: No shell scripts found
Script:
#!/bin/bash # Check current permissions of the script ls -l helperscripts/replace-basehref.sh # Search for existing chmod commands in docker-compose and Dockerfile echo "=== Searching for chmod in docker-compose.yml ===" rg "chmod" docker-compose.yml -A 2 -B 2 echo "=== Searching for chmod in Dockerfile ===" fd Dockerfile --exec rg "chmod" {} -A 2 -B 2 # Find all usages of replace-basehref.sh echo "=== Finding all usages of replace-basehref.sh ===" rg "replace-basehref.sh" -A 2 -B 2Length of output: 2806
skosmos-dev/config.ttl (3)
1-7: LGTM! Removed unused prefixes.The removal of unused
dc:andvoid:prefixes helps maintain a cleaner configuration file.
Line range hint
9-52: LGTM! Configuration follows best practices.The remaining Skosmos configuration is well-structured and includes all necessary settings.
8-8: Verify the hardcoded base URL approach.While hardcoding
http://localhost:9000/skosmos-dev/fixes theenvsubstoverwriting issue, please verify:
- If this URL is appropriate for all development environments
- If there's a more flexible solution that doesn't require hardcoding
Let's check if this base URL is referenced elsewhere:
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## development #249 +/- ##
============================================
Coverage 93.85% 93.85%
============================================
Files 49 49
Lines 1774 1774
============================================
Hits 1665 1665
Misses 109 109 ☔ View full report in Codecov by Sentry. |
Summary by CodeRabbit
New Features
Chores
Bug Fixes