Skip to content
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

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

Closed
Closed
Changes from 14 commits
Commits
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
63 changes: 21 additions & 42 deletions .github/workflows/pull-request.yml
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

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

File ignored by default.
##############################################################################
#
# NOTE!
@@ -304,22 +304,15 @@
run: |
npm run preview &
echo $! > .pidfile_prod

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

- name: Check if Production App is running
run: |
timeout=120
echo "Starting production health check with ${timeout}s timeout"
while ! nc -z localhost 4173 && [ $timeout -gt 0 ]; do
sleep 1
timeout=$((timeout-1))
if [ $((timeout % 10)) -eq 0 ]; then
echo "Still waiting for production app to start... ${timeout}s remaining"
fi
done
if [ $timeout -eq 0 ]; then
echo "Timeout waiting for production application to start"
exit 1
fi
echo "Production app started successfully"
.github/workflows/scripts/healthcheck.sh 4321 120
echo "Production app started successfully"
- name: Stop Production App
run: |
if [ -f .pidfile_prod ]; then
@@ -329,22 +322,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: |
@@ -377,30 +364,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
73 changes: 73 additions & 0 deletions .github/workflows/scripts/healthcheck.sh
Original file line number Diff line number Diff line change
@@ -0,0 +1,73 @@
#!/bin/bash

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

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 ! [[ "$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
}

# 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

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

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.

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

# 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
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 || handle_timeout
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

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.