Add health_check_no.sh script for Docker and non-Docker health checks #3108
Conversation
WalkthroughThe pull request introduces a new Bash script Changes
Assessment against linked issues
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
Documentation and Community
|
Our Pull Request Approval ProcessThanks for contributing! Testing Your CodeRemember, your PRs won't be reviewed until these criteria are met:
Our policies make our code better. ReviewersDo not assign reviewers. Our Queue Monitors will review your PR and assign them.
Reviewing Your CodeYour reviewer(s) will have the following roles:
CONTRIBUTING.mdRead our CONTRIBUTING.md file. Most importantly:
Other
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (2)
.github/workflows/pull-request.yml (1)
343-344: Improve script permissions handlingThe current approach of setting execute permissions during workflow execution is not ideal. Consider:
- Setting permissions at repository level
- Adding error handling for chmod operations
Either:
- Set permissions in the repository:
git update-index --chmod=+x .github/workflows/scripts/health_check.sh git update-index --chmod=+x .github/workflows/scripts/health_check_no.shOr 2. Add error handling:
-chmod +x .github/workflows/scripts/health_check.sh +if ! chmod +x .github/workflows/scripts/health_check.sh; then + echo "Failed to set execute permissions on health check script" + exit 1 +fiAlso applies to: 356-357, 393-394
.github/workflows/scripts/health_check_no.sh (1)
8-14: Enhance error handling and loggingThe health check loop could benefit from improved error handling and debug logging.
+debug="${DEBUG:-false}" + while ! nc -z localhost "${port}" && [ "${timeout}" -gt 0 ]; do + if [ "${debug}" = "true" ]; then + echo "Debug: nc exit code: $?" + fi sleep 1 timeout=$((timeout-1)) if [ $((timeout % 10)) -eq 0 ]; then echo "Still waiting for app to start... ${timeout}s remaining" fi done
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.github/workflows/pull-request.yml(3 hunks).github/workflows/scripts/health_check_no.sh(1 hunks)
🧰 Additional context used
🪛 Shellcheck (0.10.0)
.github/workflows/scripts/health_check_no.sh
[error] 2-2: The shebang must be on the first line. Delete blanks and move comments.
(SC1128)
🪛 GitHub Check: Performs linting, formatting, type-checking, checking for different source and target branch
.github/workflows/scripts/health_check_no.sh
[warning] 1-1:
File ignored by default.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop-postgres #3108 +/- ##
=====================================================
+ Coverage 24.64% 89.48% +64.83%
=====================================================
Files 301 322 +21
Lines 7628 8451 +823
Branches 1668 1843 +175
=====================================================
+ Hits 1880 7562 +5682
+ Misses 5621 657 -4964
- Partials 127 232 +105 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
.github/workflows/scripts/health_check_no.sh (1)
4-6: 🛠️ Refactor suggestionAdd parameter validation
The script accepts parameters but doesn't validate them, which could lead to runtime errors.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.github/workflows/pull-request.yml(3 hunks).github/workflows/scripts/health_check_no.sh(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- .github/workflows/pull-request.yml
🧰 Additional context used
🪛 Shellcheck (0.10.0)
.github/workflows/scripts/health_check_no.sh
[error] 2-2: The shebang must be on the first line. Delete blanks and move comments.
(SC1128)
🪛 GitHub Check: Performs linting, formatting, type-checking, checking for different source and target branch
.github/workflows/scripts/health_check_no.sh
[warning] 1-1:
File ignored by default.
🔇 Additional comments (3)
.github/workflows/scripts/health_check_no.sh (3)
16-24: Consider adding HTTP health validation
The script only verifies if the port is open but doesn't validate if the service is responding correctly.
1-24: Update script permissions
The script needs executable permissions to function properly.
🧰 Tools
🪛 Shellcheck (0.10.0)
[error] 2-2: The shebang must be on the first line. Delete blanks and move comments.
(SC1128)
🪛 GitHub Check: Performs linting, formatting, type-checking, checking for different source and target branch
[warning] 1-1:
File ignored by default.
1-2:
Move shebang to first line
The shebang must be on the first line for proper script execution.
-# .github/workflows/scripts/health_check_no.sh
-#!/bin/bash
+#!/bin/bash
+# .github/workflows/scripts/health_check_no.shLikely invalid or redundant comment.
🧰 Tools
🪛 Shellcheck (0.10.0)
[error] 2-2: The shebang must be on the first line. Delete blanks and move comments.
(SC1128)
🪛 GitHub Check: Performs linting, formatting, type-checking, checking for different source and target branch
[warning] 1-1:
File ignored by default.
Cioppolo14
requested review from
noman2002 and
beingnoble03
and removed request for
palisadoes
palisadoes
added
the
ignore-sensitive-files-pr
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
.github/workflows/scripts/app_health_check.sh (1)
3-19: Enhance documentation with parameter validation and exit codesThe documentation is well-structured, but could be improved by adding:
- Valid ranges/requirements for parameters (e.g., port number constraints)
- Description of exit codes (currently only implicitly mentioned)
Add these sections to the documentation:
# Variables: # port="$1" - The port to check (passed as the first argument to the script). +# Must be a valid port number between 1-65535. # timeout="${2:-120}" - The maximum time in seconds to wait for the application to start. Defaults to 120 seconds if not provided. +# Must be a positive integer. # is_docker_test="${3:-false}" - A flag to indicate whether the script is being run in a Docker-based test. Defaults to false. +# Must be either "true" or "false". + +# Exit codes: +# 0 - Health check passed successfully +# 1 - Health check failed (timeout or invalid parameters)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.github/workflows/pull-request.yml(3 hunks).github/workflows/scripts/app_health_check.sh(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- .github/workflows/pull-request.yml
🧰 Additional context used
🪛 GitHub Check: Performs linting, formatting, type-checking, checking for different source and target branch
.github/workflows/scripts/app_health_check.sh
[warning] 1-1:
File ignored by default.
🔇 Additional comments (1)
.github/workflows/scripts/app_health_check.sh (1)
1-1: Ensure proper file permissions and line endings
The script needs to be executable in the GitHub Actions environment.
Run this script to verify and fix permissions:
✅ Verification successful
Script requires executable permissions for GitHub Actions
The script currently has read-only permissions (-rw-r--r--), which will prevent it from being executed in the GitHub Actions environment. While we attempted to fix the permissions with chmod +x and git update-index --chmod=+x, we should verify if the changes were successful.
Script permissions have been properly set
The file permissions have been successfully updated:
- File mode is now
-rwxr-xr-x(755), making it executable for all users - Git index shows the file with mode
100755, confirming the executable bit is properly tracked in the repository
The script is now correctly configured for execution in the GitHub Actions environment.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check file permissions and line endings
ls -l .github/workflows/scripts/app_health_check.sh
file .github/workflows/scripts/app_health_check.sh
# Fix permissions if needed
chmod +x .github/workflows/scripts/app_health_check.sh
git update-index --chmod=+x .github/workflows/scripts/app_health_check.sh
Length of output: 429
Script:
#!/bin/bash
# Verify if permissions were updated
ls -l .github/workflows/scripts/app_health_check.sh
# Check if the file is tracked by git and its executable bit is set
git ls-files -s .github/workflows/scripts/app_health_check.sh
Length of output: 308
🧰 Tools
🪛 GitHub Check: Performs linting, formatting, type-checking, checking for different source and target branch
[warning] 1-1:
File ignored by default.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
.github/workflows/scripts/app_health_check.sh (4)
3-19: Enhance documentation with usage examples and exit codesThe documentation is well-structured but could be more helpful with:
- Example usage:
./app_health_check.sh 4321 120 true- Exit codes: 0 for success, 1 for failures
Add these sections to the documentation:
# 4. If the port is detected as open, print a success message and exit. + +# Usage: +# ./app_health_check.sh <port> [timeout] [is_docker_test] +# Example: +# ./app_health_check.sh 4321 120 true + +# Exit codes: +# 0 - Success: Application is running on the specified port +# 1 - Error: Invalid parameters or health check failed
22-24: Improve variable initialization robustnessMake the variables readonly to prevent accidental modification and ensure proper quoting.
Apply this diff:
-port="$1" -timeout="${2:-120}" -is_docker_test="${3:-false}" +readonly port="$1" +readonly timeout="${2:-120}" +readonly is_docker_test="${3:-false}"
74-74: Maintain consistent message formatThe success message format doesn't match the error message format used elsewhere in the script.
Apply this diff:
-echo "App started successfully on port ${port}" +echo "Success: Application is responding on port ${port}"
58-58: Add check for netstat command availabilityThe script uses
netstatfor debugging but doesn't verify its availability first.Add command availability check at the beginning of the script:
readonly DOCKER_CONTAINER_NAME="${DOCKER_CONTAINER_NAME:-talawa-admin-app-container}" + +# Check if netstat is available for debugging +NETSTAT_AVAILABLE=false +if command -v netstat >/dev/null 2>&1; then + NETSTAT_AVAILABLE=true +fiThen update the netstat usage:
- netstat -an | grep "${port}" || true + if [ "${NETSTAT_AVAILABLE}" = true ]; then + netstat -an | grep "${port}" || true + else + echo "Note: netstat not available for additional debugging information" + fiAlso applies to: 65-65
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/scripts/app_health_check.sh(1 hunks)
🧰 Additional context used
🪛 GitHub Check: Performs linting, formatting, type-checking, checking for different source and target branch
.github/workflows/scripts/app_health_check.sh
[warning] 1-1:
File ignored by default.
🔇 Additional comments (1)
.github/workflows/scripts/app_health_check.sh (1)
68-70: 🛠️ Refactor suggestion
Make Docker container name configurable
The Docker container name is hardcoded, which reduces script reusability.
Add container name configuration at the beginning of the script:
readonly is_docker_test="${3:-false}"
+
+# Configuration
+readonly DOCKER_CONTAINER_NAME="${DOCKER_CONTAINER_NAME:-talawa-admin-app-container}"
# Validate required port parameterThen update the Docker logs section:
- if ! docker logs talawa-admin-app-container; then
- echo "Error: Failed to fetch logs from container talawa-admin-app-container"
+ if ! docker logs "${DOCKER_CONTAINER_NAME}"; then
+ echo "Error: Failed to fetch logs from container ${DOCKER_CONTAINER_NAME}"Likely invalid or redundant comment.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
.github/workflows/scripts/app_health_check.sh (2)
3-19: Enhance documentation with usage examples and dependenciesThe documentation is well-structured, but could be improved by adding:
- Usage examples showing different parameter combinations
- List of required system dependencies (nc, netstat)
Add the following section after the existing documentation:
# 4. If the port is detected as open, print a success message and exit. +# Dependencies: +# - netcat (nc): Required for port checking +# - netstat: Required for port status information +# +# Usage examples: +# Check port 4173 with default timeout: +# ./app_health_check.sh 4173 +# +# Check port 4321 with 60s timeout: +# ./app_health_check.sh 4321 60 +# +# Check port 4321 in Docker environment: +# ./app_health_check.sh 4321 120 true
22-24: Make Docker container name configurableThe Docker container name should be configurable via an environment variable to improve script flexibility.
Add this configuration at the beginning of the script:
port="$1" timeout="${2:-120}" is_docker_test="${3:-false}" + +# Docker container name can be configured via environment variable +DOCKER_CONTAINER=${DOCKER_CONTAINER:-talawa-admin-app-container}Then update the Docker logs command accordingly:
- if ! docker logs talawa-admin-app-container; then - echo "Error: Failed to fetch logs from container talawa-admin-app-container" + if ! docker logs "${DOCKER_CONTAINER}"; then + echo "Error: Failed to fetch logs from container ${DOCKER_CONTAINER}"
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/scripts/app_health_check.sh(1 hunks)
🧰 Additional context used
🪛 GitHub Check: Performs linting, formatting, type-checking, checking for different source and target branch
.github/workflows/scripts/app_health_check.sh
[warning] 1-1:
File ignored by default.
🔇 Additional comments (1)
.github/workflows/scripts/app_health_check.sh (1)
27-43: LGTM! Comprehensive parameter validation
The validation logic is thorough and handles all edge cases correctly:
- Port number validation ensures it's within valid range (1-65535)
- Timeout validation ensures it's a positive integer
- Docker test flag validation ensures it's a boolean value
palisadoes
merged commit 96a343e
into
PalisadoesFoundation:develop-postgres
Issue Number:
Fixes #2499
Summary by CodeRabbit