-
-
Notifications
You must be signed in to change notification settings - Fork 677
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
Add health check to docker compose #546
Conversation
👋 Hi! It looks like this PR has not had any changes for a week now. Would you like someone to review this PR? If so - please remove the "[WIP]" prefix from the PR title. That will let the community know that this PR is open for a review. |
WalkthroughThe pull request introduces a health check configuration for the 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 (
|
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: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
upcoming-release-notes/546.md
is excluded by!**/*.md
📒 Files selected for processing (1)
docker-compose.yml
(1 hunks)
🔇 Additional comments (1)
docker-compose.yml (1)
25-28
: LGTM! Health check parameters are well configured.The health check parameters are reasonably configured:
- 60s interval provides a good balance between responsiveness and overhead
- 10s timeout is sufficient for a health check
- 3 retries help avoid false positives
- 20s start period gives enough time for the service to initialize
@@ -19,4 +19,11 @@ services: | |||
# Change './actual-data' below to the path to the folder you want Actual to store its data in on your server. | |||
# '/data' is the path Actual will look for its files in by default, so leave that as-is. | |||
- ./actual-data:/data | |||
healthcheck: | |||
# Enable health check for the instance | |||
test: ["CMD-SHELL", "node src/scripts/health-check.js"] |
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.
🛠️ Refactor suggestion
Consider using an absolute path or mounting the health check script.
The relative path src/scripts/health-check.js
might not be accessible in the container. Consider either:
- Using an absolute path if the script is already included in the container image
- Mounting the script via a volume if it needs to be provided externally
Example of mounting the script:
volumes:
# Change './actual-data' below to the path to the folder you want Actual to store its data in on your server.
# '/data' is the path Actual will look for its files in by default, so leave that as-is.
- ./actual-data:/data
+ # Mount the health check script
+ - ./scripts/health-check.js:/app/health-check.js
healthcheck:
# Enable health check for the instance
- test: ["CMD-SHELL", "node src/scripts/health-check.js"]
+ test: ["CMD-SHELL", "node /app/health-check.js"]
Committable suggestion skipped: line range outside the PR's diff.
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.
LGTM, thank you
Added health check in the included docker-compose.yml file