Update PR GitHub Action to correct duplicated health check logic. Fixes #2499 #2673
Conversation
WalkthroughThe pull request introduces modifications to the GitHub Actions workflow in Changes
Assessment against linked issues
Possibly related PRs
Suggested labels
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 (1)
.github/workflows/scripts/healthcheck.sh (1)
16-37: Improve Docker container logs handlingThe script attempts to show Docker logs on failure but doesn't handle potential Docker command failures.
Add Docker command error handling:
if [ -f /.dockerenv ]; then echo "Running inside Docker container" check_health if [ "${timeout}" -eq 0 ]; then echo "Timeout waiting for application to start" echo "Container logs:" - docker logs talawa-admin-app-container + if ! docker logs talawa-admin-app-container 2>/dev/null; then + echo "Error: Failed to retrieve container logs" + fi exit 1 fi echo "Port check passed, verifying health endpoint..."
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.github/workflows/pull-request.yml(2 hunks).github/workflows/scripts/healthcheck.sh(1 hunks)
🔇 Additional comments (1)
.github/workflows/pull-request.yml (1)
332-341: Verify script implementation across environments
While the PR successfully addresses duplicate Healthcheck logic by introducing a common script, there are some considerations:
- The script is used differently in Docker vs non-Docker environments
- The implementation paths are inconsistent
- Error handling could be improved
Please ensure these issues are addressed before merging.
Let's verify the script implementation:
Also applies to: 374-382
| check_health() { | ||
| while ! nc -z localhost "${port}" && [ "${timeout}" -gt 0 ]; do | ||
| sleep 1 | ||
| timeout=$((timeout-1)) | ||
| if [ $((timeout % 10)) -eq 0 ]; then | ||
| echo "Still waiting for app to start... ${timeout}s remaining" | ||
| fi | ||
| done | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling for netcat command
The health check function should handle potential netcat command failures.
Add error handling:
check_health() {
+ if ! command -v nc >/dev/null 2>&1; then
+ echo "Error: netcat (nc) is not installed"
+ exit 1
+ fi
+
while ! nc -z localhost "${port}" && [ "${timeout}" -gt 0 ]; do
sleep 1
timeout=$((timeout-1))
if [ $((timeout % 10)) -eq 0 ]; then
echo "Still waiting for app to start... ${timeout}s remaining"
fi
done
}📝 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.
| check_health() { | |
| while ! nc -z localhost "${port}" && [ "${timeout}" -gt 0 ]; do | |
| sleep 1 | |
| timeout=$((timeout-1)) | |
| if [ $((timeout % 10)) -eq 0 ]; then | |
| echo "Still waiting for app to start... ${timeout}s remaining" | |
| fi | |
| done | |
| } | |
| check_health() { | |
| if ! command -v nc >/dev/null 2>&1; then | |
| echo "Error: netcat (nc) is not installed" | |
| exit 1 | |
| fi | |
| while ! nc -z localhost "${port}" && [ "${timeout}" -gt 0 ]; do | |
| sleep 1 | |
| timeout=$((timeout-1)) | |
| if [ $((timeout % 10)) -eq 0 ]; then | |
| echo "Still waiting for app to start... ${timeout}s remaining" | |
| fi | |
| done | |
| } |
| #!/bin/bash | ||
| port="$1" | ||
| timeout="${2:-120}" | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider adding input validation for script parameters
The script accepts port and timeout parameters but doesn't validate their values.
Add parameter validation:
#!/bin/bash
+
+# Validate input parameters
+if [ -z "$1" ]; then
+ echo "Error: Port number is required"
+ echo "Usage: $0 <port> [timeout]"
+ exit 1
+fi
+
+if ! [[ "$1" =~ ^[0-9]+$ ]] || [ "$1" -lt 1 ] || [ "$1" -gt 65535 ]; then
+ echo "Error: Invalid port number. Must be between 1 and 65535"
+ exit 1
+fi
+
+if [ ! -z "$2" ] && ! [[ "$2" =~ ^[0-9]+$ ]]; then
+ echo "Error: Timeout must be a positive number"
+ exit 1
+fi
+
port="$1"
timeout="${2:-120}"📝 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.
| #!/bin/bash | |
| port="$1" | |
| timeout="${2:-120}" | |
| #!/bin/bash | |
| # Validate input parameters | |
| if [ -z "$1" ]; then | |
| echo "Error: Port number is required" | |
| echo "Usage: $0 <port> [timeout]" | |
| exit 1 | |
| fi | |
| if ! [[ "$1" =~ ^[0-9]+$ ]] || [ "$1" -lt 1 ] || [ "$1" -gt 65535 ]; then | |
| echo "Error: Invalid port number. Must be between 1 and 65535" | |
| exit 1 | |
| fi | |
| if [ ! -z "$2" ] && ! [[ "$2" =~ ^[0-9]+$ ]]; then | |
| echo "Error: Timeout must be a positive number" | |
| exit 1 | |
| fi | |
| port="$1" | |
| timeout="${2:-120}" |
.github/workflows/pull-request.yml
Outdated
| - name: Make health-check.sh executable | ||
| run: | | ||
| chmod +x ./github/workflows/scripts/health-check.sh | ||
|
|
There was a problem hiding this comment.
Fix inconsistent script paths
The healthcheck script paths are inconsistent between jobs:
- Start-App-Without-Docker uses
./github/workflows/scripts/health-check.sh - Docker-Start-Check uses
.github/scripts/healthcheck.sh
This inconsistency could lead to failures.
Standardize the script paths:
- - name: Make health-check.sh executable
+ - name: Make healthcheck.sh executable
run: |
- chmod +x ./github/workflows/scripts/health-check.sh
+ chmod +x .github/workflows/scripts/healthcheck.sh
# In Docker-Start-Check job
- name: Make healthcheck.sh executable
run: |
- chmod +x .github/scripts/healthcheck.sh
+ chmod +x .github/workflows/scripts/healthcheck.shAlso applies to: 374-377
.github/workflows/pull-request.yml
Outdated
| echo "Timeout waiting for development application to start" | ||
| exit 1 | ||
| fi | ||
| ./github/workflows/scripts/health-check.sh 4321 120 |
There was a problem hiding this comment.
Fix inconsistent script execution paths
The script execution paths are also inconsistent between jobs.
Standardize the execution paths:
# In Start-App-Without-Docker job
- ./github/workflows/scripts/health-check.sh 4321 120
+ .github/workflows/scripts/healthcheck.sh 4321 120
# In Docker-Start-Check job
- ./github/scripts/healthcheck.sh 4321 120
+ .github/workflows/scripts/healthcheck.sh 4321 120Also applies to: 380-380
.github/workflows/pull-request.yml
Outdated
| ./github/workflows/scripts/health-check.sh 4321 120 | ||
| echo "Development app started successfully" | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve error handling in workflow steps
The workflow steps don't properly handle script execution failures.
Add error handling:
# In both jobs
- name: Check if Development App is running
run: |
+ set -e
.github/workflows/scripts/healthcheck.sh 4321 120
+ if [ $? -ne 0 ]; then
+ echo "Health check failed"
+ exit 1
+ fi
echo "Development app started successfully"Also applies to: 380-382
palisadoes
changed the title
|
@Akshat76845 Please fix the failing tests. |
|
@palisadoes @Cioppolo14 I think now it is working correctly. |
|
Please fix:
Make sure CodeRabbit.ai approves your changes |
|
When you are done, and this PR is merged, please create an issue to move all the scripts to your newly created |
|
@palisadoes Okay |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
.github/workflows/scripts/healthcheck.sh (2)
15-18: Enhance timeout validationThe timeout validation should also check for positive values to prevent potential issues with zero or negative timeouts.
-if [ ! -z "$2" ] && ! [[ "$2" =~ ^[0-9]+$ ]]; then +if [ ! -z "$2" ] && (! [[ "$2" =~ ^[0-9]+$ ]] || [ "$2" -lt 1 ]); then echo "Error: Timeout must be a positive number" exit 1 fi
1-59: Consider enhancing script reusabilityThe script's location in
.github/workflows/scriptsaligns with the planned directory structure mentioned in the PR comments. To further improve its reusability across different workflows:
- Consider adding configuration options for:
- Custom health check endpoints
- Different connection protocols (not just TCP)
- Configurable retry intervals
- Add script documentation with usage examples
- Consider creating a shared library of common workflow functions
Would you like assistance in implementing any of these improvements?
🧰 Tools
🪛 GitHub Check: Performs linting, formatting, type-checking, checking for different source and target branch
[warning] 1-1:
File ignored by default.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/scripts/healthcheck.sh(1 hunks)
🧰 Additional context used
🪛 GitHub Check: Performs linting, formatting, type-checking, checking for different source and target branch
.github/workflows/scripts/healthcheck.sh
[warning] 1-1:
File ignored by default.
🔇 Additional comments (1)
.github/workflows/scripts/healthcheck.sh (1)
20-21: LGTM!
Parameter assignment is clean and uses proper shell parameter expansion for default timeout value.
| check_health() { | ||
| if ! command -v nc >/dev/null 2>&1; then | ||
| echo "Error: netcat (nc) is not installed" | ||
| exit 1 | ||
| fi | ||
| while ! nc -z localhost "${port}" && [ "${timeout}" -gt 0 ]; do | ||
| sleep 1 | ||
| timeout=$((timeout-1)) | ||
| if [ $((timeout % 10)) -eq 0 ]; then | ||
| echo "Still waiting for app to start... ${timeout}s remaining" | ||
| fi | ||
| done | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve health check robustness and error handling
The health check function needs additional error handling and improved feedback:
- Add explicit success/failure status for netcat connection
- Handle netcat command failures
- Enhance progress messages with port information
check_health() {
if ! command -v nc >/dev/null 2>&1; then
echo "Error: netcat (nc) is not installed"
exit 1
fi
+ local status=1
while ! nc -z localhost "${port}" && [ "${timeout}" -gt 0 ]; do
sleep 1
timeout=$((timeout-1))
if [ $((timeout % 10)) -eq 0 ]; then
- echo "Still waiting for app to start... ${timeout}s remaining"
+ echo "Still waiting for app to start on port ${port}... ${timeout}s remaining"
fi
done
+ if nc -z localhost "${port}" >/dev/null 2>&1; then
+ status=0
+ fi
+ return $status
}📝 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.
| check_health() { | |
| if ! command -v nc >/dev/null 2>&1; then | |
| echo "Error: netcat (nc) is not installed" | |
| exit 1 | |
| fi | |
| while ! nc -z localhost "${port}" && [ "${timeout}" -gt 0 ]; do | |
| sleep 1 | |
| timeout=$((timeout-1)) | |
| if [ $((timeout % 10)) -eq 0 ]; then | |
| echo "Still waiting for app to start... ${timeout}s remaining" | |
| fi | |
| done | |
| } | |
| check_health() { | |
| if ! command -v nc >/dev/null 2>&1; then | |
| echo "Error: netcat (nc) is not installed" | |
| exit 1 | |
| fi | |
| local status=1 | |
| while ! nc -z localhost "${port}" && [ "${timeout}" -gt 0 ]; do | |
| sleep 1 | |
| timeout=$((timeout-1)) | |
| if [ $((timeout % 10)) -eq 0 ]; then | |
| echo "Still waiting for app to start on port ${port}... ${timeout}s remaining" | |
| fi | |
| done | |
| if nc -z localhost "${port}" >/dev/null 2>&1; then | |
| status=0 | |
| fi | |
| return $status | |
| } |
| # Check if the script is running inside Docker container | ||
| if [ -f /.dockerenv ]; then | ||
| echo "Running inside Docker container" | ||
| check_health | ||
|
|
||
| if [ "${timeout}" -eq 0 ]; then | ||
| echo "Timeout waiting for application to start" | ||
| echo "Container logs:" | ||
| docker logs talawa-admin-app-container | ||
| exit 1 | ||
| fi | ||
| echo "Port check passed, verifying health endpoint..." | ||
| else | ||
| echo "Running outside Docker container" | ||
| check_health | ||
|
|
||
| if [ "${timeout}" -eq 0 ]; then | ||
| echo "Timeout waiting for application to start" | ||
| exit 1 | ||
| fi | ||
| echo "App started successfully on port ${port}" | ||
| fi |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Refactor environment-specific handling for better consistency
The environment-specific handling needs improvements:
- Add error handling for docker logs command
- Consolidate duplicate timeout logic
- Make health verification consistent across environments
+# Function to handle timeout
+handle_timeout() {
+ echo "Timeout waiting for application to start"
+ if [ -f /.dockerenv ]; then
+ echo "Container logs:"
+ docker logs talawa-admin-app-container 2>/dev/null || echo "Failed to retrieve container logs"
+ fi
+ exit 1
+}
+
# Check if the script is running inside Docker container
if [ -f /.dockerenv ]; then
echo "Running inside Docker container"
- check_health
-
- if [ "${timeout}" -eq 0 ]; then
- echo "Timeout waiting for application to start"
- echo "Container logs:"
- docker logs talawa-admin-app-container
- exit 1
- fi
- echo "Port check passed, verifying health endpoint..."
else
echo "Running outside Docker container"
- check_health
-
- if [ "${timeout}" -eq 0 ]; then
- echo "Timeout waiting for application to start"
- exit 1
- fi
- echo "App started successfully on port ${port}"
fi
+
+check_health || handle_timeout
+echo "App started successfully on port ${port}"📝 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.
| # Check if the script is running inside Docker container | |
| if [ -f /.dockerenv ]; then | |
| echo "Running inside Docker container" | |
| check_health | |
| if [ "${timeout}" -eq 0 ]; then | |
| echo "Timeout waiting for application to start" | |
| echo "Container logs:" | |
| docker logs talawa-admin-app-container | |
| exit 1 | |
| fi | |
| echo "Port check passed, verifying health endpoint..." | |
| else | |
| echo "Running outside Docker container" | |
| check_health | |
| if [ "${timeout}" -eq 0 ]; then | |
| echo "Timeout waiting for application to start" | |
| exit 1 | |
| fi | |
| echo "App started successfully on port ${port}" | |
| fi | |
| # Function to handle timeout | |
| handle_timeout() { | |
| echo "Timeout waiting for application to start" | |
| if [ -f /.dockerenv ]; then | |
| echo "Container logs:" | |
| docker logs talawa-admin-app-container 2>/dev/null || echo "Failed to retrieve container logs" | |
| fi | |
| exit 1 | |
| } | |
| # Check if the script is running inside Docker container | |
| if [ -f /.dockerenv ]; then | |
| echo "Running inside Docker container" | |
| else | |
| echo "Running outside Docker container" | |
| fi | |
| check_health || handle_timeout | |
| echo "App started successfully on port ${port}" |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop-postgres #2673 +/- ##
====================================================
+ Coverage 83.81% 86.35% +2.53%
====================================================
Files 295 312 +17
Lines 7274 8134 +860
Branches 1592 1841 +249
====================================================
+ Hits 6097 7024 +927
+ Misses 940 933 -7
+ Partials 237 177 -60 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
.github/workflows/scripts/healthcheck.sh (4)
20-21: Document the default timeout valueAdd a comment explaining why 120 seconds was chosen as the default timeout value.
+# Default timeout of 120 seconds should be sufficient for most application startups port="$1" timeout="${2:-120}"
24-43: Enhance health check robustnessConsider these improvements for better reliability:
- Add trap handler for SIGTERM
- Use printf instead of echo for progress updates (more efficient)
check_health() { + # Setup trap for clean exit + trap 'echo "Received SIGTERM"; exit 1' TERM + if ! command -v nc >/dev/null 2>&1; then echo "Error: netcat (nc) is not installed" exit 1 fi local status=1 while ! nc -z localhost "${port}" && [ "${timeout}" -gt 0 ]; do sleep 1 timeout=$((timeout - 1)) if [ $((timeout % 10)) -eq 0 ]; then - echo "Still waiting for app to start on port ${port}... ${timeout}s remaining" + printf "\rWaiting for app on port %s... %ds remaining" "${port}" "${timeout}" fi done + printf "\n" if nc -z localhost "${port}" >/dev/null 2>&1; then status=0 fi return $status }
55-65: Remove duplicated timeout messageThe timeout message is duplicated between
handle_timeoutand the non-Docker case. Consider usinghandle_timeoutconsistently.# Check if the script is running inside Docker container if [ -f /.dockerenv ]; then echo "Running inside Docker container" check_health || handle_timeout else echo "Running outside Docker container" - check_health || { - echo "Timeout waiting for application to start" - exit 1 - } + check_health || handle_timeout fi
1-1: Ensure script permissions and portable shebang
- Make the script executable:
chmod +x healthcheck.sh- Consider using a more portable shebang
-#!/bin/bash +#!/usr/bin/env bash🧰 Tools
🪛 GitHub Check: Performs linting, formatting, type-checking, checking for different source and target branch
[warning] 1-1:
File ignored by default.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/scripts/healthcheck.sh(1 hunks)
🧰 Additional context used
🪛 GitHub Check: Performs linting, formatting, type-checking, checking for different source and target branch
.github/workflows/scripts/healthcheck.sh
[warning] 1-1:
File ignored by default.
| handle_timeout() { | ||
| echo "Timeout waiting for application to start" | ||
| if [ -f /.dockerenv ]; then | ||
| echo "Container logs:" | ||
| docker logs talawa-admin-app-container 2>/dev/null || echo "Failed to retrieve container logs" | ||
| fi | ||
| exit 1 | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Parameterize Docker container name
The Docker container name 'talawa-admin-app-container' is hardcoded. Consider making it configurable.
+# Default container name, can be overridden by environment variable
+CONTAINER_NAME="${CONTAINER_NAME:-talawa-admin-app-container}"
+
handle_timeout() {
echo "Timeout waiting for application to start"
if [ -f /.dockerenv ]; then
echo "Container logs:"
- docker logs talawa-admin-app-container 2>/dev/null || echo "Failed to retrieve container logs"
+ docker logs "${CONTAINER_NAME}" 2>/dev/null || echo "Failed to retrieve container logs"
fi
exit 1
}📝 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.
| handle_timeout() { | |
| echo "Timeout waiting for application to start" | |
| if [ -f /.dockerenv ]; then | |
| echo "Container logs:" | |
| docker logs talawa-admin-app-container 2>/dev/null || echo "Failed to retrieve container logs" | |
| fi | |
| exit 1 | |
| } | |
| # Default container name, can be overridden by environment variable | |
| CONTAINER_NAME="${CONTAINER_NAME:-talawa-admin-app-container}" | |
| handle_timeout() { | |
| echo "Timeout waiting for application to start" | |
| if [ -f /.dockerenv ]; then | |
| echo "Container logs:" | |
| docker logs "${CONTAINER_NAME}" 2>/dev/null || echo "Failed to retrieve container logs" | |
| fi | |
| exit 1 | |
| } |
| if [ -z "$1" ]; then | ||
| echo "Error: Port number is required" | ||
| echo "Usage: $0 <port> [timeout]" | ||
| exit 1 | ||
| fi | ||
|
|
||
| if [ ! -z "$2" ] && (! [[ "$2" =~ ^[0-9]+$ ]] || [ "$2" -lt 1 ]); then | ||
| echo "Error: Invalid port number. Must be between 1 and 65535" | ||
| exit 1 | ||
| fi | ||
|
|
||
| if [ ! -z "$2" ] && ! [[ "$2" =~ ^[0-9]+$ ]]; then | ||
| echo "Error: Timeout must be a positive number" | ||
| exit 1 | ||
| fi |
There was a problem hiding this comment.
Fix parameter validation logic
There are several issues with the parameter validation:
- Port number range validation is incorrectly checking $2 instead of $1
- Port validation is missing the upper bound check (65535)
- The order of validation checks could be improved
Apply this fix:
# Validate input parameters
if [ -z "$1" ]; then
echo "Error: Port number is required"
echo "Usage: $0 <port> [timeout]"
exit 1
fi
-if [ ! -z "$2" ] && (! [[ "$2" =~ ^[0-9]+$ ]] || [ "$2" -lt 1 ]); then
+if ! [[ "$1" =~ ^[0-9]+$ ]] || [ "$1" -lt 1 ] || [ "$1" -gt 65535 ]; then
echo "Error: Invalid port number. Must be between 1 and 65535"
exit 1
fi
if [ ! -z "$2" ] && ! [[ "$2" =~ ^[0-9]+$ ]]; then
echo "Error: Timeout must be a positive number"
exit 1
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.
| if [ -z "$1" ]; then | |
| echo "Error: Port number is required" | |
| echo "Usage: $0 <port> [timeout]" | |
| exit 1 | |
| fi | |
| if [ ! -z "$2" ] && (! [[ "$2" =~ ^[0-9]+$ ]] || [ "$2" -lt 1 ]); then | |
| echo "Error: Invalid port number. Must be between 1 and 65535" | |
| exit 1 | |
| fi | |
| if [ ! -z "$2" ] && ! [[ "$2" =~ ^[0-9]+$ ]]; then | |
| echo "Error: Timeout must be a positive number" | |
| exit 1 | |
| fi | |
| if [ -z "$1" ]; then | |
| echo "Error: Port number is required" | |
| echo "Usage: $0 <port> [timeout]" | |
| exit 1 | |
| fi | |
| if ! [[ "$1" =~ ^[0-9]+$ ]] || [ "$1" -lt 1 ] || [ "$1" -gt 65535 ]; then | |
| echo "Error: Invalid port number. Must be between 1 and 65535" | |
| exit 1 | |
| fi | |
| if [ ! -z "$2" ] && ! [[ "$2" =~ ^[0-9]+$ ]]; then | |
| echo "Error: Timeout must be a positive number" | |
| exit 1 | |
| fi |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
.github/workflows/scripts/healthcheck.sh (3)
15-18: Enhance timeout parameter validationThe timeout validation should also check for positive values to prevent potential issues with zero or negative timeouts.
-if [ ! -z "$2" ] && ! [[ "$2" =~ ^[0-9]+$ ]]; then +if [ ! -z "$2" ] && (! [[ "$2" =~ ^[0-9]+$ ]] || [ "$2" -lt 1 ]); then echo "Error: Timeout must be a positive number" exit 1 fi
30-43: Enhance error handling in health checkWhile the function handles basic connectivity checks, it could benefit from more robust error handling and informative messages.
local status=1 + local nc_error="" while ! nc -z localhost "${port}" && [ "${timeout}" -gt 0 ]; do + nc_error=$(nc -z localhost "${port}" 2>&1) sleep 1 timeout=$((timeout - 1)) if [ $((timeout % 10)) -eq 0 ]; then - echo "Still waiting for app to start on port ${port}... ${timeout}s remaining" + echo "Still waiting for app to start on port ${port}... ${timeout}s remaining. Last error: ${nc_error}" fi done if nc -z localhost "${port}" >/dev/null 2>&1; then status=0 + else + echo "Failed to connect to port ${port}. Last error: ${nc_error}" fi return $status
44-45: Add documentation for environment variablesConsider adding a comment block explaining the available environment variables and their purpose.
+# Environment Variables: +# CONTAINER_NAME: Override the default Docker container name +# Default: talawa-admin-app-container + # Default container name, can be overridden by environment variable CONTAINER_NAME="${CONTAINER_NAME:-talawa-admin-app-container}"
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/scripts/healthcheck.sh(1 hunks)
🧰 Additional context used
🪛 GitHub Check: Performs linting, formatting, type-checking, checking for different source and target branch
.github/workflows/scripts/healthcheck.sh
[warning] 1-1:
File ignored by default.
🔇 Additional comments (1)
.github/workflows/scripts/healthcheck.sh (1)
20-21: LGTM! Clear variable initialization with sensible defaults.
| #!/bin/bash | ||
|
|
||
| # Validate input parameters | ||
| if [ -z "$1" ]; then | ||
| echo "Error: Port number is required" | ||
| echo "Usage: $0 <port> [timeout]" | ||
| exit 1 | ||
| fi | ||
|
|
||
| if ! [[ "$1" =~ ^[0-9]+$ ]] || [ "$1" -lt 1 ] || [ "$1" -gt 65535 ]; then | ||
| echo "Error: Invalid port number. Must be between 1 and 65535" | ||
| exit 1 | ||
| fi | ||
|
|
||
| if [ ! -z "$2" ] && ! [[ "$2" =~ ^[0-9]+$ ]]; then | ||
| echo "Error: Timeout must be a positive number" | ||
| exit 1 | ||
| fi | ||
|
|
||
| port="$1" | ||
| timeout="${2:-120}" | ||
|
|
||
| # Function to check if the app is running | ||
| check_health() { | ||
| if ! command -v nc >/dev/null 2>&1; then | ||
| echo "Error: netcat (nc) is not installed" | ||
| exit 1 | ||
| fi | ||
|
|
||
| local status=1 | ||
| while ! nc -z localhost "${port}" && [ "${timeout}" -gt 0 ]; do | ||
| sleep 1 | ||
| timeout=$((timeout - 1)) | ||
| if [ $((timeout % 10)) -eq 0 ]; then | ||
| echo "Still waiting for app to start on port ${port}... ${timeout}s remaining" | ||
| fi | ||
| done | ||
|
|
||
| if nc -z localhost "${port}" >/dev/null 2>&1; then | ||
| status=0 | ||
| fi | ||
| return $status | ||
| } | ||
| # Default container name, can be overridden by environment variable | ||
| CONTAINER_NAME="${CONTAINER_NAME:-talawa-admin-app-container}" | ||
| # Function to handle timeout | ||
| handle_timeout() { | ||
| echo "Timeout waiting for application to start" | ||
| if [ -f /.dockerenv ]; then | ||
| echo "Container logs:" | ||
| docker logs "${CONTAINER_NAME}" 2>/dev/null || echo "Failed to retrieve container logs" | ||
| fi | ||
| exit 1 | ||
| } | ||
|
|
||
| # Check if the script is running inside Docker container | ||
| if [ -f /.dockerenv ]; then | ||
| echo "Running inside Docker container" | ||
| check_health || handle_timeout | ||
| else | ||
| echo "Running outside Docker container" | ||
| check_health || { | ||
| echo "Timeout waiting for application to start" | ||
| exit 1 | ||
| } | ||
| fi |
There was a problem hiding this comment.
💡 Codebase verification
Duplicate health check logic still exists in pull-request.yml
The healthcheck script is properly integrated in .github/workflows/pull-request.yml, but there's still duplicate health check logic in the workflow file itself. This defeats the purpose of centralizing the health check logic.
.github/workflows/pull-request.ymlcontains a duplicate implementation of the port checking logic that should be replaced with a call to the healthcheck script- The script permissions (644) need to be updated to make it executable (755)
🔗 Analysis chain
Verify script integration with GitHub workflow
The script successfully addresses the duplicate health check logic. Let's verify its integration with the workflow.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the script's integration in GitHub workflows
# Check if the script is referenced in workflow files
echo "Checking workflow references:"
rg -l "healthcheck.sh" .github/workflows/
# Verify the script permissions
echo -e "\nChecking script permissions:"
ls -l .github/workflows/scripts/healthcheck.sh
# Look for any remaining duplicate health check logic
echo -e "\nChecking for remaining duplicate health check logic:"
rg -A 5 "nc -z localhost" .github/workflows/
Length of output: 1937
🧰 Tools
🪛 GitHub Check: Performs linting, formatting, type-checking, checking for different source and target branch
[warning] 1-1:
File ignored by default.
| # Check if the script is running inside Docker container | ||
| if [ -f /.dockerenv ]; then | ||
| echo "Running inside Docker container" | ||
| check_health || handle_timeout | ||
| else | ||
| echo "Running outside Docker container" | ||
| check_health || { | ||
| echo "Timeout waiting for application to start" | ||
| exit 1 | ||
| } | ||
| fi |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Standardize error handling across environments
The error handling differs between Docker and non-Docker environments. Consider using the handle_timeout function consistently.
if [ -f /.dockerenv ]; then
echo "Running inside Docker container"
check_health || handle_timeout
else
echo "Running outside Docker container"
- check_health || {
- echo "Timeout waiting for application to start"
- exit 1
- }
+ check_health || handle_timeout
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.
| # Check if the script is running inside Docker container | |
| if [ -f /.dockerenv ]; then | |
| echo "Running inside Docker container" | |
| check_health || handle_timeout | |
| else | |
| echo "Running outside Docker container" | |
| check_health || { | |
| echo "Timeout waiting for application to start" | |
| exit 1 | |
| } | |
| fi | |
| # Check if the script is running inside Docker container | |
| if [ -f /.dockerenv ]; then | |
| echo "Running inside Docker container" | |
| check_health || handle_timeout | |
| else | |
| echo "Running outside Docker container" | |
| check_health || handle_timeout | |
| fi |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
.github/workflows/scripts/healthcheck.sh (2)
24-43: Enhance connection testing reliabilityThe health check could be more robust by:
- Adding multiple connection attempts before failing
- Including a small delay between connection attempts
- Adding debug logging for connection failures
check_health() { if ! command -v nc >/dev/null 2>&1; then echo "Error: netcat (nc) is not installed" exit 1 fi local status=1 + local retry_count=3 + local retry_delay=2 + while ! nc -z localhost "${port}" && [ "${timeout}" -gt 0 ]; do sleep 1 timeout=$((timeout - 1)) if [ $((timeout % 10)) -eq 0 ]; then echo "Still waiting for app to start on port ${port}... ${timeout}s remaining" fi done - if nc -z localhost "${port}" >/dev/null 2>&1; then - status=0 - fi + # Verify connection with multiple attempts + while [ "${retry_count}" -gt 0 ]; do + if nc -z localhost "${port}" >/dev/null 2>&1; then + status=0 + break + fi + echo "Connection attempt failed, retrying in ${retry_delay}s... (${retry_count} attempts remaining)" + sleep "${retry_delay}" + retry_count=$((retry_count - 1)) + done return $status }
57-63: Enhance environment detection and loggingAdd more detailed environment information to help with debugging.
+# Function to log environment details +log_environment() { + echo "Environment details:" + echo "- Hostname: $(hostname)" + echo "- User: $(whoami)" + if [ -f /.dockerenv ]; then + echo "- Container ID: $(hostname)" + echo "- Container Name: ${CONTAINER_NAME}" + fi + echo "- Port: ${port}" + echo "- Timeout: ${timeout}s" +} + if [ -f /.dockerenv ]; then echo "Running inside Docker container" + log_environment check_health || handle_timeout else echo "Running outside Docker container" + log_environment check_health || handle_timeout fi
📜 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/healthcheck.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/healthcheck.sh
[warning] 1-1:
File ignored by default.
| handle_timeout() { | ||
| echo "Timeout waiting for application to start" | ||
| if [ -f /.dockerenv ]; then | ||
| echo "Container logs:" | ||
| docker logs "${CONTAINER_NAME}" 2>/dev/null || echo "Failed to retrieve container logs" | ||
| fi | ||
| exit 1 | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add signal handling for graceful cleanup
The script should handle interruption signals (SIGINT, SIGTERM) to ensure proper cleanup.
+# Cleanup function
+cleanup() {
+ echo "Cleaning up..."
+ # Add any necessary cleanup commands here
+ exit "${1:-1}"
+}
+
+# Set up signal handling
+trap 'cleanup' SIGINT SIGTERM
+
handle_timeout() {
echo "Timeout waiting for application to start"
if [ -f /.dockerenv ]; then
echo "Container logs:"
docker logs "${CONTAINER_NAME}" 2>/dev/null || echo "Failed to retrieve container logs"
fi
- exit 1
+ cleanup 1
}📝 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.
| handle_timeout() { | |
| echo "Timeout waiting for application to start" | |
| if [ -f /.dockerenv ]; then | |
| echo "Container logs:" | |
| docker logs "${CONTAINER_NAME}" 2>/dev/null || echo "Failed to retrieve container logs" | |
| fi | |
| exit 1 | |
| } | |
| # Cleanup function | |
| cleanup() { | |
| echo "Cleaning up..." | |
| # Add any necessary cleanup commands here | |
| exit "${1:-1}" | |
| } | |
| # Set up signal handling | |
| trap 'cleanup' SIGINT SIGTERM | |
| handle_timeout() { | |
| echo "Timeout waiting for application to start" | |
| if [ -f /.dockerenv ]; then | |
| echo "Container logs:" | |
| docker logs "${CONTAINER_NAME}" 2>/dev/null || echo "Failed to retrieve container logs" | |
| fi | |
| cleanup 1 | |
| } |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
.github/workflows/scripts/healthcheck.sh (4)
1-1: Specify bash version for better compatibilityConsider updating the shebang line to ensure compatibility across different environments.
-#!/bin/bash +#!/usr/bin/env bash🧰 Tools
🪛 GitHub Check: Performs linting, formatting, type-checking, checking for different source and target branch
[warning] 1-1:
File ignored by default.
23-43: Add hostname resolution check and improve documentationThe health check function should verify that localhost can be resolved and document the return status codes.
# Function to check if the app is running +# Returns: +# 0 if the app is running and accessible +# 1 if the app failed to start or is not accessible check_health() { if ! command -v nc >/dev/null 2>&1; then echo "Error: netcat (nc) is not installed" exit 1 fi + # Verify localhost resolution + if ! getent hosts localhost >/dev/null 2>&1; then + echo "Error: Unable to resolve localhost" + exit 1 + fi + local status=1 while ! nc -z localhost "${port}" && [ "${timeout}" -gt 0 ]; do
56-64: Improve timeout handling and loggingEnhance the timeout handler with more informative messages and logging.
handle_timeout() { - echo "Timeout waiting for application to start" + echo "$(date '+%Y-%m-%d %H:%M:%S') Timeout waiting for application to start on port ${port} after ${2:-120} seconds" if [ -f /.dockerenv ]; then echo "Container logs:" - docker logs "${CONTAINER_NAME}" 2>/dev/null || echo "Failed to retrieve container logs" + docker logs --tail 50 "${CONTAINER_NAME}" 2>/dev/null || echo "Failed to retrieve container logs" + echo "Container status:" + docker inspect --format '{{.State.Status}}' "${CONTAINER_NAME}" 2>/dev/null || echo "Failed to retrieve container status" fi cleanup 1 }
66-73: Consolidate environment-specific executionThe duplicate health check calls can be consolidated for better maintainability.
-# Check if the script is running inside Docker container -if [ -f /.dockerenv ]; then - echo "Running inside Docker container" - check_health || handle_timeout -else - echo "Running outside Docker container" - check_health || handle_timeout -fi +# Log environment type +echo "Running ${[ -f /.dockerenv ] && echo "inside" || echo "outside"} Docker container" + +# Perform health check +check_health || handle_timeout
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/scripts/healthcheck.sh(1 hunks)
🧰 Additional context used
🪛 GitHub Check: Performs linting, formatting, type-checking, checking for different source and target branch
.github/workflows/scripts/healthcheck.sh
[warning] 1-1:
File ignored by default.
| #!/bin/bash | ||
|
|
||
| # Validate input parameters | ||
| if [ -z "$1" ]; then | ||
| echo "Error: Port number is required" | ||
| echo "Usage: $0 <port> [timeout]" | ||
| exit 1 | ||
| fi | ||
|
|
||
| if ! [[ "$1" =~ ^[0-9]+$ ]] || [ "$1" -lt 1 ] || [ "$1" -gt 65535 ]; then | ||
| echo "Error: Invalid port number. Must be between 1 and 65535" | ||
| exit 1 | ||
| fi | ||
|
|
||
| if [ ! -z "$2" ] && ! [[ "$2" =~ ^[0-9]+$ ]]; then | ||
| echo "Error: Timeout must be a positive number" | ||
| exit 1 | ||
| fi | ||
|
|
||
| port="$1" | ||
| timeout="${2:-120}" | ||
|
|
||
| # Function to check if the app is running | ||
| check_health() { | ||
| if ! command -v nc >/dev/null 2>&1; then | ||
| echo "Error: netcat (nc) is not installed" | ||
| exit 1 | ||
| fi | ||
|
|
||
| local status=1 | ||
| while ! nc -z localhost "${port}" && [ "${timeout}" -gt 0 ]; do | ||
| sleep 1 | ||
| timeout=$((timeout - 1)) | ||
| if [ $((timeout % 10)) -eq 0 ]; then | ||
| echo "Still waiting for app to start on port ${port}... ${timeout}s remaining" | ||
| fi | ||
| done | ||
|
|
||
| if nc -z localhost "${port}" >/dev/null 2>&1; then | ||
| status=0 | ||
| fi | ||
| return $status | ||
| } | ||
| # Default container name, can be overridden by environment variable | ||
| CONTAINER_NAME="${CONTAINER_NAME:-talawa-admin-app-container}" | ||
| # Cleanup function | ||
| cleanup() { | ||
| echo "Cleaning up..." | ||
| # Add any necessary cleanup commands here | ||
| exit "${1:-1}" | ||
| } | ||
|
|
||
| # Set up signal handling | ||
| trap 'cleanup' SIGINT SIGTERM | ||
|
|
||
| # Function to handle timeout | ||
| handle_timeout() { | ||
| echo "Timeout waiting for application to start" | ||
| if [ -f /.dockerenv ]; then | ||
| echo "Container logs:" | ||
| docker logs "${CONTAINER_NAME}" 2>/dev/null || echo "Failed to retrieve container logs" | ||
| fi | ||
| cleanup 1 | ||
| } | ||
|
|
||
| # Check if the script is running inside Docker container | ||
| if [ -f /.dockerenv ]; then | ||
| echo "Running inside Docker container" | ||
| check_health || handle_timeout | ||
| else | ||
| echo "Running outside Docker container" | ||
| check_health || handle_timeout | ||
| fi |
There was a problem hiding this comment.
Make script executable
The script needs to be made executable to function in the GitHub Actions workflow.
Run the following command:
git update-index --chmod=+x .github/workflows/scripts/healthcheck.sh🧰 Tools
🪛 GitHub Check: Performs linting, formatting, type-checking, checking for different source and target branch
[warning] 1-1:
File ignored by default.
| # Cleanup function | ||
| cleanup() { | ||
| echo "Cleaning up..." | ||
| # Add any necessary cleanup commands here | ||
| exit "${1:-1}" | ||
| } | ||
|
|
||
| # Set up signal handling | ||
| trap 'cleanup' SIGINT SIGTERM | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance cleanup and signal handling
The cleanup function should include actual cleanup operations and handle more signals.
# Cleanup function
cleanup() {
echo "Cleaning up..."
- # Add any necessary cleanup commands here
+ # Kill any background processes started by this script
+ jobs -p | xargs -r kill
+ # Remove any temporary files if created
+ [ -n "${TEMP_FILES}" ] && rm -f ${TEMP_FILES}
exit "${1:-1}"
}
# Set up signal handling
-trap 'cleanup' SIGINT SIGTERM
+trap 'cleanup' SIGINT SIGTERM SIGHUP EXIT📝 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.
| # Cleanup function | |
| cleanup() { | |
| echo "Cleaning up..." | |
| # Add any necessary cleanup commands here | |
| exit "${1:-1}" | |
| } | |
| # Set up signal handling | |
| trap 'cleanup' SIGINT SIGTERM | |
| # Cleanup function | |
| cleanup() { | |
| echo "Cleaning up..." | |
| # Kill any background processes started by this script | |
| jobs -p | xargs -r kill | |
| # Remove any temporary files if created | |
| [ -n "${TEMP_FILES}" ] && rm -f ${TEMP_FILES} | |
| exit "${1:-1}" | |
| } | |
| # Set up signal handling | |
| trap 'cleanup' SIGINT SIGTERM SIGHUP EXIT |
|
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
.github/workflows/scripts/healthcheck.sh (4)
1-21: Enhance script initialization and input validationConsider adding shell options for better error handling and using more descriptive variable names.
#!/bin/bash +set -euo pipefail + +# Constants +readonly DEFAULT_TIMEOUT=120 # Validate input parameters if [ -z "$1" ]; then echo "Error: Port number is required" echo "Usage: $0 <port> [timeout]" exit 1 fi if ! [[ "$1" =~ ^[0-9]+$ ]] || [ "$1" -lt 1 ] || [ "$1" -gt 65535 ]; then echo "Error: Invalid port number. Must be between 1 and 65535" exit 1 fi if [ ! -z "$2" ] && ! [[ "$2" =~ ^[0-9]+$ ]]; then echo "Error: Timeout must be a positive number" exit 1 fi -port="$1" -timeout="${2:-120}" +port_number="$1" +wait_timeout="${2:-$DEFAULT_TIMEOUT}"🧰 Tools
🪛 GitHub Check: Performs linting, formatting, type-checking, checking for different source and target branch
[warning] 1-1:
File ignored by default.
22-22: Improve temporary files handlingUse an array to safely handle multiple filenames with spaces.
-TEMP_FILES="" +declare -a TEMP_FILES=()
46-47: Add documentation for environment variablesDocument the environment variables at the top of the script.
#!/bin/bash set -euo pipefail +# Environment variables: +# CONTAINER_NAME: Override the default Docker container name (default: talawa-admin-app-container) + # Constants readonly DEFAULT_TIMEOUT=120
62-70: Improve timeout handling and loggingAdd timestamps and preserve health check exit codes.
# Function to handle timeout handle_timeout() { - echo "Timeout waiting for application to start" + local exit_code=$1 + echo "[$(date '+%Y-%m-%d %H:%M:%S')] Timeout waiting for application to start" if [ -f /.dockerenv ]; then - echo "Container logs:" + echo "[$(date '+%Y-%m-%d %H:%M:%S')] Container logs:" docker logs "${CONTAINER_NAME}" 2>/dev/null || echo "Failed to retrieve container logs" fi - cleanup 1 + cleanup $exit_code }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/scripts/healthcheck.sh(1 hunks)
🧰 Additional context used
🪛 GitHub Check: Performs linting, formatting, type-checking, checking for different source and target branch
.github/workflows/scripts/healthcheck.sh
[warning] 1-1:
File ignored by default.
🔇 Additional comments (2)
.github/workflows/scripts/healthcheck.sh (2)
1-79: Verify removal of duplicate health check logic
The script successfully centralizes the health check logic, but we should verify that all duplicate implementations have been removed from the workflow files.
✅ Verification successful
Health check logic has been successfully centralized
The verification confirms that:
- The
nc -zhealth check logic only exists in the centralizedhealthcheck.shscript - The script is properly integrated in the workflow, being used consistently for all health checks:
- Production app check (port 4173)
- Development app check (port 4321)
- Docker container check (port 4321)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any remaining health check implementations in workflow files
echo "Checking for remaining health check implementations:"
rg -A 5 "nc -z" .github/workflows/
echo -e "\nVerifying script integration:"
rg -A 5 "healthcheck.sh" .github/workflows/
Length of output: 3324
🧰 Tools
🪛 GitHub Check: Performs linting, formatting, type-checking, checking for different source and target branch
[warning] 1-1:
File ignored by default.
72-79: 🛠️ Refactor suggestion
Update main execution logic
Pass parameters explicitly and handle exit codes properly.
# Check if the script is running inside Docker container
if [ -f /.dockerenv ]; then
echo "Running inside Docker container"
- check_health || handle_timeout
+ check_health "$port_number" "$wait_timeout" || handle_timeout $?
else
echo "Running outside Docker container"
- check_health || handle_timeout
+ check_health "$port_number" "$wait_timeout" || handle_timeout $?
fiLikely invalid or redundant comment.
| # Function to check if the app is running | ||
| check_health() { | ||
| if ! command -v nc >/dev/null 2>&1; then | ||
| echo "Error: netcat (nc) is not installed" | ||
| exit 1 | ||
| fi | ||
|
|
||
| local status=1 | ||
| while ! nc -z localhost "${port}" && [ "${timeout}" -gt 0 ]; do | ||
| sleep 1 | ||
| timeout=$((timeout - 1)) | ||
| if [ $((timeout % 10)) -eq 0 ]; then | ||
| echo "Still waiting for app to start on port ${port}... ${timeout}s remaining" | ||
| fi | ||
| done | ||
|
|
||
| if nc -z localhost "${port}" >/dev/null 2>&1; then | ||
| status=0 | ||
| fi | ||
| return $status | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve health check function encapsulation and error handling
The function should receive parameters instead of using global variables, and use more descriptive status codes.
# Function to check if the app is running
-check_health() {
+check_health() {
+ local port=$1
+ local timeout=$2
+
if ! command -v nc >/dev/null 2>&1; then
echo "Error: netcat (nc) is not installed"
- exit 1
+ return 2 # Return distinct error code for missing dependency
fi
- local status=1
+ local status=3 # Initial state: timeout
while ! nc -z localhost "${port}" && [ "${timeout}" -gt 0 ]; do
sleep 1
timeout=$((timeout - 1))
if [ $((timeout % 10)) -eq 0 ]; then
echo "Still waiting for app to start on port ${port}... ${timeout}s remaining"
fi
done
if nc -z localhost "${port}" >/dev/null 2>&1; then
- status=0
+ status=0 # Success
fi
return $status
}📝 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.
| # Function to check if the app is running | |
| check_health() { | |
| if ! command -v nc >/dev/null 2>&1; then | |
| echo "Error: netcat (nc) is not installed" | |
| exit 1 | |
| fi | |
| local status=1 | |
| while ! nc -z localhost "${port}" && [ "${timeout}" -gt 0 ]; do | |
| sleep 1 | |
| timeout=$((timeout - 1)) | |
| if [ $((timeout % 10)) -eq 0 ]; then | |
| echo "Still waiting for app to start on port ${port}... ${timeout}s remaining" | |
| fi | |
| done | |
| if nc -z localhost "${port}" >/dev/null 2>&1; then | |
| status=0 | |
| fi | |
| return $status | |
| } | |
| # Function to check if the app is running | |
| check_health() { | |
| local port=$1 | |
| local timeout=$2 | |
| if ! command -v nc >/dev/null 2>&1; then | |
| echo "Error: netcat (nc) is not installed" | |
| return 2 # Return distinct error code for missing dependency | |
| fi | |
| local status=3 # Initial state: timeout | |
| while ! nc -z localhost "${port}" && [ "${timeout}" -gt 0 ]; do | |
| sleep 1 | |
| timeout=$((timeout - 1)) | |
| if [ $((timeout % 10)) -eq 0 ]; then | |
| echo "Still waiting for app to start on port ${port}... ${timeout}s remaining" | |
| fi | |
| done | |
| if nc -z localhost "${port}" >/dev/null 2>&1; then | |
| status=0 # Success | |
| fi | |
| return $status | |
| } |
| # Cleanup function | ||
| cleanup() { | ||
| echo "Cleaning up..." | ||
| # Kill any background processes started by this script | ||
| jobs -p | xargs -r kill | ||
| # Remove any temporary files if created | ||
| [ -n "${TEMP_FILES}" ] && rm -f ${TEMP_FILES} | ||
| exit "${1:-1}" | ||
| } | ||
|
|
||
| # Set up signal handling | ||
| trap 'cleanup' SIGINT SIGTERM SIGHUP EXIT | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance cleanup handling
Update cleanup function to handle arrays and add error reporting.
# Cleanup function
cleanup() {
echo "Cleaning up..."
+ local exit_code="${1:-1}"
+ local cleanup_failed=0
+
# Kill any background processes started by this script
- jobs -p | xargs -r kill
+ if ! jobs -p | xargs -r kill 2>/dev/null; then
+ echo "Warning: Failed to kill some background processes"
+ cleanup_failed=1
+ fi
+
# Remove any temporary files if created
- [ -n "${TEMP_FILES}" ] && rm -f ${TEMP_FILES}
- exit "${1:-1}"
+ if [ ${#TEMP_FILES[@]} -gt 0 ]; then
+ if ! rm -f "${TEMP_FILES[@]}" 2>/dev/null; then
+ echo "Warning: Failed to remove some temporary files"
+ cleanup_failed=1
+ fi
+ fi
+
+ [ $cleanup_failed -eq 1 ] && exit_code=1
+ exit $exit_code
}📝 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.
| # Cleanup function | |
| cleanup() { | |
| echo "Cleaning up..." | |
| # Kill any background processes started by this script | |
| jobs -p | xargs -r kill | |
| # Remove any temporary files if created | |
| [ -n "${TEMP_FILES}" ] && rm -f ${TEMP_FILES} | |
| exit "${1:-1}" | |
| } | |
| # Set up signal handling | |
| trap 'cleanup' SIGINT SIGTERM SIGHUP EXIT | |
| # Cleanup function | |
| cleanup() { | |
| echo "Cleaning up..." | |
| local exit_code="${1:-1}" | |
| local cleanup_failed=0 | |
| # Kill any background processes started by this script | |
| if ! jobs -p | xargs -r kill 2>/dev/null; then | |
| echo "Warning: Failed to kill some background processes" | |
| cleanup_failed=1 | |
| fi | |
| # Remove any temporary files if created | |
| if [ ${#TEMP_FILES[@]} -gt 0 ]; then | |
| if ! rm -f "${TEMP_FILES[@]}" 2>/dev/null; then | |
| echo "Warning: Failed to remove some temporary files" | |
| cleanup_failed=1 | |
| fi | |
| fi | |
| [ $cleanup_failed -eq 1 ] && exit_code=1 | |
| exit $exit_code | |
| } | |
| # Set up signal handling | |
| trap 'cleanup' SIGINT SIGTERM SIGHUP EXIT |
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
.github/workflows/scripts/healthcheck.sh (1)
86-93: Simplify environment check logicThe Docker and non-Docker paths execute identical commands. This can be simplified.
-# Check if the script is running inside Docker container -if [ -f /.dockerenv ]; then - echo "Running inside Docker container" - check_health || handle_timeout -else - echo "Running outside Docker container" - check_health || handle_timeout -fi +# Log environment type +echo "Running ${[ -f /.dockerenv ] && echo "inside" || echo "outside"} Docker container" +check_health "${port}" "${timeout}" || handle_timeout
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/scripts/healthcheck.sh(1 hunks)
🧰 Additional context used
🪛 Shellcheck (0.10.0)
.github/workflows/scripts/healthcheck.sh
[warning] 25-46: check_health references arguments, but none are ever passed.
(SC2120)
[error] 57-57: You are missing a required space after the !.
(SC1035)
🪛 GitHub Check: Performs linting, formatting, type-checking, checking for different source and target branch
.github/workflows/scripts/healthcheck.sh
[warning] 1-1:
File ignored by default.
| # Check if the script is running inside Docker container | ||
| if [ -f /.dockerenv ]; then | ||
| echo "Running inside Docker container" | ||
| check_health || handle_timeout |
There was a problem hiding this comment.
Fix health check function calls
The check_health function expects port and timeout parameters, but they are not passed in the function calls.
- check_health || handle_timeout
+ check_health "${port}" "${timeout}" || handle_timeoutAlso applies to: 92-92
| @@ -0,0 +1,93 @@ | |||
| #!/bin/bash | |||
There was a problem hiding this comment.
Make script executable
The script needs to be executable to function in GitHub Actions workflow.
Run this command:
git update-index --chmod=+x .github/workflows/scripts/healthcheck.sh🧰 Tools
🪛 GitHub Check: Performs linting, formatting, type-checking, checking for different source and target branch
[warning] 1-1:
File ignored by default.
|
|
||
| port="$1" | ||
| timeout="${2:-120}" | ||
| TEMP_FILES="" |
There was a problem hiding this comment.
Fix TEMP_FILES initialization
TEMP_FILES is initialized as a string but used as an array in the cleanup function. This mismatch could cause issues.
-TEMP_FILES=""
+declare -a TEMP_FILES=()📝 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.
| TEMP_FILES="" | |
| declare -a TEMP_FILES=() |
| if !jobs -p | xargs -r kill 2>/dev/null; then | ||
| echo "Warning: Failed to kill some background processes" | ||
| cleanup_failed=1 | ||
| fi | ||
| # Remove any temporary files if created | ||
| if [ ${#TEMP_FILES[@]} -gt 0 ]; then | ||
| if ! rm -f "${TEMP_FILES[@]}" 2>/dev/null; then | ||
| echo "Warning: Failed to remove some temporary files" | ||
| cleanup_failed=1 | ||
| fi | ||
| fi |
There was a problem hiding this comment.
Fix syntax and formatting issues in cleanup function
There are syntax and formatting issues in the cleanup function:
- Missing space after ! in the jobs command
- Inconsistent indentation in the temporary files cleanup block
- if !jobs -p | xargs -r kill 2>/dev/null; then
+ if ! jobs -p | xargs -r kill 2>/dev/null; then
echo "Warning: Failed to kill some background processes"
cleanup_failed=1
fi
# Remove any temporary files if created
- if [ ${#TEMP_FILES[@]} -gt 0 ]; then
- if ! rm -f "${TEMP_FILES[@]}" 2>/dev/null; then
- echo "Warning: Failed to remove some temporary files"
- cleanup_failed=1
- fi
- fi
+ if [ ${#TEMP_FILES[@]} -gt 0 ]; then
+ if ! rm -f "${TEMP_FILES[@]}" 2>/dev/null; then
+ echo "Warning: Failed to remove some temporary files"
+ cleanup_failed=1
+ fi
+ 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.
| if !jobs -p | xargs -r kill 2>/dev/null; then | |
| echo "Warning: Failed to kill some background processes" | |
| cleanup_failed=1 | |
| fi | |
| # Remove any temporary files if created | |
| if [ ${#TEMP_FILES[@]} -gt 0 ]; then | |
| if ! rm -f "${TEMP_FILES[@]}" 2>/dev/null; then | |
| echo "Warning: Failed to remove some temporary files" | |
| cleanup_failed=1 | |
| fi | |
| fi | |
| if ! jobs -p | xargs -r kill 2>/dev/null; then | |
| echo "Warning: Failed to kill some background processes" | |
| cleanup_failed=1 | |
| fi | |
| # Remove any temporary files if created | |
| if [ ${#TEMP_FILES[@]} -gt 0 ]; then | |
| if ! rm -f "${TEMP_FILES[@]}" 2>/dev/null; then | |
| echo "Warning: Failed to remove some temporary files" | |
| cleanup_failed=1 | |
| fi | |
| fi |
🧰 Tools
🪛 Shellcheck (0.10.0)
[error] 57-57: You are missing a required space after the !.
(SC1035)
|
Please fix the failing tests. Our test code coverage system will fail if any of these conditions occur:
Here are some steps to take:
We do this to ensure the overall reliability of the code base is maintained. This is a prerequisite for getting your PR approved. Assigned reviewers regularly review the PR queue and tend to focus on PRs that are passing. Additional notes:
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
.github/workflows/scripts/healthcheck.sh (2)
15-18: Enhance timeout validationThe timeout validation should also check for zero or negative values to prevent infinite waits.
-if [ ! -z "$2" ] && ! [[ "$2" =~ ^[0-9]+$ ]]; then +if [ ! -z "$2" ] && (! [[ "$2" =~ ^[0-9]+$ ]] || [ "$2" -le 0 ]); then echo "Error: Timeout must be a positive number" exit 1 fi
62-67: Fix indentation in cleanup functionThe temporary files cleanup block has incorrect indentation.
# Remove any temporary files if created - if [ ${#TEMP_FILES[@]} -gt 0 ]; then - if ! rm -f "${TEMP_FILES[@]}" 2>/dev/null; then - echo "Warning: Failed to remove some temporary files" - cleanup_failed=1 - fi - fi + if [ ${#TEMP_FILES[@]} -gt 0 ]; then + if ! rm -f "${TEMP_FILES[@]}" 2>/dev/null; then + echo "Warning: Failed to remove some temporary files" + cleanup_failed=1 + fi + fi.github/workflows/pull-request.yml (1)
317-318: Add error handling for health check script executionThe health check script execution should handle potential failures explicitly.
.github/workflows/scripts/healthcheck.sh 4173 120 +if [ $? -ne 0 ]; then + echo "Health check failed for production app" + exit 1 +fi echo "Production app started successfully"Apply similar changes to the development app check.
Also applies to: 333-334
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.github/workflows/pull-request.yml(4 hunks).github/workflows/scripts/healthcheck.sh(1 hunks)
🧰 Additional context used
🪛 yamllint (1.35.1)
.github/workflows/pull-request.yml
[error] 42-42: trailing spaces
(trailing-spaces)
[error] 329-329: trailing spaces
(trailing-spaces)
[error] 368-368: trailing spaces
(trailing-spaces)
🪛 GitHub Check: Performs linting, formatting, type-checking, checking for different source and target branch
.github/workflows/scripts/healthcheck.sh
[warning] 1-1:
File ignored by default.
🔇 Additional comments (4)
.github/workflows/scripts/healthcheck.sh (2)
77-84: LGTM! Well-structured timeout handling
The timeout handling is robust with appropriate Docker-specific logging and cleanup.
92-92:
Fix missing parameters in check_health call
The check_health function expects port and timeout parameters, but they're not provided in this call.
- check_health || handle_timeout
+ check_health "${port}" "${timeout}" || handle_timeoutLikely invalid or redundant comment.
.github/workflows/pull-request.yml (2)
44-46: LGTM! Correct approach for setting script permissions
Using git update-index --chmod=+x is the proper way to make scripts executable in git.
368-374: LGTM! Consistent health check integration
The Docker health check integration is consistent with the non-Docker implementation and uses the correct port.
🧰 Tools
🪛 yamllint (1.35.1)
[error] 368-368: trailing spaces
(trailing-spaces)
|
This pull request did not get any activity in the past 10 days and will be closed in 180 days if no update occurs. Please verify it has no conflicts with the develop branch and rebase if needed. Mention it now if you need help or give permission to other people to finish your work. |
|
This issue isn't signed to you. Closing |
What kind of change does this PR introduce?
Added a script for the pull_request workflow
Issue Number:
Fixes #2499
Did you add tests for your changes?
No
If relevant, did you update the documentation?
Summary
This PR addresses the following issues:
Correct duplicate Healthcheck logic
Does this PR introduce a breaking change?
No
Have you read the contributing guide?
Yes
Summary by CodeRabbit
New Features
Bug Fixes
Chores