Skip to content

Update PR GitHub Action to correct duplicated health check logic. Fixes #2499 #2673

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.

Sign up for GitHub

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

Closed
wants to merge 22 commits into from
Closed

Update PR GitHub Action to correct duplicated health check logic. Fixes #2499 #2673

Show file tree
Hide file tree
Changes from 8 commits
Commits
Show all changes
22 commits
Select commit Hold shift + click to select a range
0979eb6
Created a script for the pull_request
Dec 15, 2024
6f43573
Created a script for the pull_request
Dec 16, 2024
e1334ec
Added a script for pull_request
Dec 16, 2024
d37f1c6
Merge branch 'develop-postgres' into scripts
Akshat76845 Dec 20, 2024
bdebff2
Update pull-request.yml
Akshat76845 Dec 20, 2024
9c19ce9
Update pull-request.yml
Akshat76845 Dec 20, 2024
87d3bf4
Update healthcheck.sh
Akshat76845 Dec 21, 2024
27f5d15
Update healthcheck.sh
Akshat76845 Dec 21, 2024
6cf8875
Update healthcheck.sh
Akshat76845 Dec 21, 2024
291f7b5
Changed some mistake in healthcheck.sh
Dec 20, 2024
4fead38
Changed some code
Dec 21, 2024
92c2bd7
Merge branch 'develop-postgres' into scripts
Akshat76845 Dec 21, 2024
f760394
Done some changes for health check
Dec 21, 2024
7e38edf
Corrected the healthcheck.sh
Dec 21, 2024
bd722a2
Corrected the code of pull_request for production healthcheck
Dec 21, 2024
82c6438
Merge branch 'develop-postgres' into scripts
Akshat76845 Dec 21, 2024
43e3fb2
Modified some healthcheck logic code
Dec 21, 2024
3771c8c
Merge remote-tracking branch 'origin/scripts' into scripts
Dec 21, 2024
4a01efa
Changed the health-check logic
Dec 21, 2024
aebab6f
Updated the healthcheck logic
Dec 21, 2024
f6e5294
Make healthcheck executable
Dec 21, 2024
2896445
Update pull-request.yml
Akshat76845 Dec 22, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
42 changes: 14 additions & 28 deletions .github/workflows/pull-request.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
##############################################################################

Check warning on line 1 in .github/workflows/pull-request.yml

View workflow job for this annotation

GitHub Actions / Performs linting, formatting, type-checking, checking for different source and target branch 

File ignored by default.
##############################################################################
#
# NOTE!
Expand Down Expand Up @@ -329,22 +329,16 @@
run: |
npm run serve &
echo $! > .pidfile_dev

- name: Make health-check.sh executable
run: |
chmod +x .github/workflows/scripts/healthcheck.sh

- name: Check if Development App is running
run: |
timeout=120
echo "Starting development health check with ${timeout}s timeout"
while ! nc -z localhost 4321 && [ $timeout -gt 0 ]; do
sleep 1
timeout=$((timeout-1))
if [ $((timeout % 10)) -eq 0 ]; then
echo "Still waiting for development app to start... ${timeout}s remaining"
fi
done
if [ $timeout -eq 0 ]; then
echo "Timeout waiting for development application to start"
exit 1
fi
.github/workflows/scripts/healthcheck.sh 4321 120
echo "Development app started successfully"

- name: Stop Development App
if: always()
run: |
Expand Down Expand Up @@ -377,30 +371,22 @@
echo "Started Docker container..."
docker run -d --name talawa-admin-app-container -p 4321:4321 talawa-admin-app
echo "Docker container started successfully"
- name: Make healthcheck.sh executable
run: |
chmod +x .github/workflows/scripts/healthcheck.sh

- name: Check if Talawa Admin App is running
run: |
timeout="${HEALTH_CHECK_TIMEOUT:-120}"
echo "Starting health check with ${timeout}s timeout"
while ! nc -z localhost 4321 && [ $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
if [ $timeout -eq 0 ]; then
echo "Timeout waiting for application to start"
echo "Container logs:"
docker logs talawa-admin-app-container
exit 1
fi
.github/workflows/scripts/healthcheck.sh 4321 120
echo "Port check passed, verifying health endpoint..."

- name: Stop Docker Container
if: always()
run: |
docker stop talawa-admin-app-container
docker rm talawa-admin-app-container


Check-Target-Branch:
if: ${{ github.actor != 'dependabot[bot]' }}
name: Check Target Branch
Expand Down
65 changes: 65 additions & 0 deletions .github/workflows/scripts/healthcheck.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,65 @@
#!/bin/bash

Check warning on line 1 in .github/workflows/scripts/healthcheck.sh

View workflow job for this annotation

GitHub Actions / Performs linting, formatting, type-checking, checking for different source and target branch 

File ignored by default.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

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.


# 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
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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Fix parameter validation logic

There are several issues with the parameter validation:

  1. Port number range validation is incorrectly checking $2 instead of $1
  2. Port validation is missing the upper bound check (65535)
  3. 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.

Suggested change
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


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
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ 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.

Suggested change
# 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
}


# 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
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ 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.

Suggested change
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
}


# Check if the script is running inside Docker container
if [ -f /.dockerenv ]; then
echo "Running inside Docker container"
check_health || handle_timeout
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

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_timeout

Also applies to: 92-92

else
echo "Running outside Docker container"
check_health || {
echo "Timeout waiting for application to start"
exit 1
}
fi
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ 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.

Suggested change
# 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

Loading