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.

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

reverting workflow changes #2498

Merged
merged 3 commits into from
Nov 28, 2024
Merged

reverting workflow changes #2498

Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
f8b910f
reverting workflow changes
prayanshchh Nov 28, 2024
e07a558
changes 1
prayanshchh Nov 28, 2024
7af0a38
identation fix
prayanshchh Nov 28, 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
141 changes: 134 additions & 7 deletions .github/workflows/pull-request.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,7 @@ jobs:

- name: Get changed TypeScript files
id: changed-files
uses: tj-actions/changed-files@v40
uses: tj-actions/changed-files@v45
- name: Check formatting
if: steps.changed-files.outputs.only_changed != 'true'
run: npm run format:check
Expand All @@ -58,7 +58,7 @@ jobs:
- name: Check for linting errors in modified files
if: steps.changed-files.outputs.only_changed != 'true'
env:
CHANGED_FILES: ${{ steps.changed_files.outputs.all_changed_files }}
CHANGED_FILES: ${{ steps.changed-files.outputs.all_changed_files }}
run: npx eslint ${CHANGED_FILES} && python .github/workflows/eslint_disable_check.py

- name: Check for TSDoc comments
Expand Down Expand Up @@ -92,9 +92,10 @@ jobs:

- name: Get Changed Unauthorized files
id: changed-unauth-files
uses: tj-actions/changed-files@v40
uses: tj-actions/changed-files@v45
with:
files: |
.env*
.github/**
env.example
.node-version
Expand Down Expand Up @@ -123,6 +124,12 @@ jobs:
ISSUE_GUIDELINES.md
PR_GUIDELINES.md
README.md
*.pem
*.key
palisadoes marked this conversation as resolved.
Show resolved Hide resolved
*.cert
*.password
*.secret
*.credentials

- name: List all changed unauthorized files
if: steps.changed-unauth-files.outputs.any_changed == 'true' || steps.changed-unauth-files.outputs.any_deleted == 'true'
Expand All @@ -144,7 +151,7 @@ jobs:

- name: Get changed files
id: changed-files
uses: tj-actions/changed-files@v40
uses: tj-actions/changed-files@v45

- name: Echo number of changed files
env:
Expand Down Expand Up @@ -197,7 +204,7 @@ jobs:

- name: Get changed TypeScript files
id: changed-files
uses: tj-actions/changed-files@v40
uses: tj-actions/changed-files@v45

- name: Run Jest Tests
if: steps.changed-files.outputs.only_changed != 'true'
Expand Down Expand Up @@ -230,14 +237,14 @@ jobs:
fi
done

- name: Upload Coverage to Codecov
- name: Present and Upload coverage to Codecov as ${{env.CODECOV_UNIQUE_NAME}}
uses: codecov/codecov-action@v4
with:
token: ${{ secrets.CODECOV_TOKEN }}
verbose: true
fail_ci_if_error: false
files: './coverage/lcov.info'
name: '${{env.CODECOV_UNIQUE_NAME}}'
fail_ci_if_error: false

- name: Test acceptable level of code coverage
uses: VeryGoodOpenSource/very_good_coverage@v3
Expand Down Expand Up @@ -272,6 +279,126 @@ jobs:

- name: Validate Documents
run: graphql-inspector validate './src/GraphQl/**/*.ts' './talawa-api/schema.graphql'
Start-App-Without-Docker:
name: Check if Talawa Admin app starts (No Docker)
Copy link
Contributor

Choose a reason for hiding this comment

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

Incorrect indentation

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

runs-on: ubuntu-latest
needs: [Code-Quality-Checks, Test-Application]
if: github.actor != 'dependabot'
steps:
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 YAML structure for new jobs

The Start-App-Without-Docker job is incorrectly indented and duplicates keys from the previous job.

- Start-App-Without-Docker:
+ start-app-without-docker:
    name: Check if Talawa Admin app starts (No Docker)
    runs-on: ubuntu-latest
    needs: [Code-Quality-Checks, Test-Application]
    if: github.actor != 'dependabot'
    steps:
📝 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
Start-App-Without-Docker:
name: Check if Talawa Admin app starts (No Docker)
runs-on: ubuntu-latest
needs: [Code-Quality-Checks, Test-Application]
if: github.actor != 'dependabot'
steps:
start-app-without-docker:
name: Check if Talawa Admin app starts (No Docker)
runs-on: ubuntu-latest
needs: [Code-Quality-Checks, Test-Application]
if: github.actor != 'dependabot'
steps:
🧰 Tools
🪛 actionlint (1.7.4)

283-283: unexpected key "Start-App-Without-Docker" for "job" section. expected one of "concurrency", "container", "continue-on-error", "defaults", "env", "environment", "if", "name", "needs", "outputs", "permissions", "runs-on", "secrets", "services", "steps", "strategy", "timeout-minutes", "uses", "with"

(syntax-check)

284-284: key "name" is duplicated in "Graphql-Inspector" job. previously defined at line:258,col:5

(syntax-check)

285-285: key "runs-on" is duplicated in "Graphql-Inspector" job. previously defined at line:259,col:5

(syntax-check)

287-287: key "if" is duplicated in "Graphql-Inspector" job. previously defined at line:257,col:5

(syntax-check)

288-288: key "steps" is duplicated in "Graphql-Inspector" job. previously defined at line:260,col:5

(syntax-check)

🪛 yamllint (1.35.1)

[error] 284-284: duplication of key "name" in mapping

(key-duplicates)

[error] 285-285: duplication of key "runs-on" in mapping

(key-duplicates)

[error] 287-287: duplication of key "if" in mapping

(key-duplicates)

[error] 288-288: duplication of key "steps" in mapping

(key-duplicates)

- name: Checkout the Repository
uses: actions/checkout@v4

- name: Set up Node.js
uses: actions/setup-node@v4
with:
node-version: '20.x'

- name: Install Dependencies
run: npm install

- name: Build Production App
run: npm run build

- name: Start Production App
run: |
npm run preview &
echo $! > .pidfile_prod
- 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
Comment on lines +308 to +320
Copy link
Contributor

@coderabbitai coderabbitai bot Nov 28, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Improve shell script safety and reusability

  1. The health check logic is duplicated across multiple places.

  2. Shell variables should be properly quoted to prevent word splitting.

  3. Create a reusable health check script:

+ # .github/workflows/scripts/health_check.sh
+ #!/bin/bash
+ 
+ port="$1"
+ timeout="${2:-120}"
+ 
+ echo "Starting health check with ${timeout}s 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... ${timeout}s remaining"
+   fi
+ done
+ 
+ if [ "${timeout}" -eq 0 ]; then
+   echo "Timeout waiting for application to start"
+   exit 1
+ fi
+ echo "App started successfully on port ${port}"
  1. Update the jobs to use the script:
- 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"
-   exit 1
- fi
+ chmod +x .github/workflows/scripts/health_check.sh
+ .github/workflows/scripts/health_check.sh 4321 120

Also applies to: 333-346, 381-395

Copy link
Contributor Author

@prayanshchh prayanshchh Nov 28, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

@coderabbitai all the workflows aren't getting trigerred, check pull-request.yml file

Copy link
Contributor

Choose a reason for hiding this comment

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

@prayanshchh, it seems that the workflows aren't getting triggered due to indentation issues in your pull-request.yml file. The new job definitions for Start-App-Without-Docker and Docker-Start-Check need to be properly indented under the jobs: section.

Here's how you can adjust the indentation:

jobs:
  Code-Quality-Checks:
    # existing job definitions

  Graphql-Inspector:
    # existing job definitions

  Start-App-Without-Docker:
    name: Check if Talawa Admin app starts (No Docker)
    runs-on: ubuntu-latest
    needs: [Code-Quality-Checks, Test-Application]
    if: github.actor != 'dependabot'
    steps:
      # steps here...

  Docker-Start-Check:
    name: Check if Talawa Admin app starts in Docker
    runs-on: ubuntu-latest
    needs: [Code-Quality-Checks, Test-Application]
    if: github.actor != 'dependabot'
    steps:
      # steps here...

  Check-Target-Branch:
    # existing job definitions

Ensure that all jobs are correctly indented under the jobs: key. Incorrect indentation can prevent GitHub Actions from recognizing and triggering the workflows.

You might also want to validate your YAML file using a linter or an online YAML validator to catch any syntax errors.

Let me know if this helps resolve the issue.

Comment on lines +308 to +320
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 shell script safety and reduce duplication

The health check logic is duplicated across multiple places and has shell script safety issues:

  1. Shell variables should be properly quoted

  2. Health check logic should be extracted into a reusable script

  3. Create a reusable health check script:

+ # .github/workflows/scripts/health_check.sh
+ #!/bin/bash
+ 
+ port="$1"
+ timeout="${2:-120}"
+ 
+ echo "Starting health check with ${timeout}s 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... ${timeout}s remaining"
+   fi
+ done
+ 
+ if [ "${timeout}" -eq 0 ]; then
+   echo "Timeout waiting for application to start"
+   exit 1
+ fi
+ echo "App started successfully on port ${port}"
  1. Update the jobs to use the script:
- 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"
-   exit 1
- fi
+ chmod +x .github/workflows/scripts/health_check.sh
+ .github/workflows/scripts/health_check.sh 4321 120

Also applies to: 333-346, 381-395

echo "Production app started successfully"
- name: Stop Production App
run: |
if [ -f .pidfile_prod ]; then
kill "$(cat .pidfile_prod)"
fi
- name: Start Development App
run: |
npm run serve &
echo $! > .pidfile_dev
- 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
echo "Development app started successfully"
- name: Stop Development App
if: always()
run: |
if [ -f .pidfile_dev ]; then
kill "$(cat .pidfile_dev)"
fi
Docker-Start-Check:
name: Check if Talawa Admin app starts in Docker
runs-on: ubuntu-latest
needs: [Code-Quality-Checks, Test-Application]
if: github.actor != 'dependabot'
steps:
- name: Checkout the Repository
uses: actions/checkout@v4

- name: Set up Docker
uses: docker/setup-buildx-action@v3
with:
driver-opts: |
image=moby/buildkit:latest
- name: Build Docker image
run: |
set -e
echo "Building Docker image..."
docker build -t talawa-admin-app .
echo "Docker image built successfully"
- name: Run Docker Container
run: |
set -e
echo "Started Docker container..."
docker run -d --name talawa-admin-app-container -p 4321:4321 talawa-admin-app
echo "Docker container started successfully"
- 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
echo "Port check passed, verifying health endpoint..."
- name: Stop Docker Container
Comment on lines +380 to +397
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

Double quote variables in shell scripts

Shell variables should be properly quoted to prevent word splitting and globbing issues.

- timeout="${HEALTH_CHECK_TIMEOUT:-120}"
+ timeout="${HEALTH_CHECK_TIMEOUT:-120}"
  echo "Starting health check with ${timeout}s timeout"
- while ! nc -z localhost 4321 && [ $timeout -gt 0 ]; do
+ while ! nc -z localhost 4321 && [ "${timeout}" -gt 0 ]; do
    sleep 1
-   timeout=$((timeout-1))
+   timeout=$((timeout-1))
-   if [ $((timeout % 10)) -eq 0 ]; then
+   if [ $((timeout % 10)) -eq 0 ]; then
      echo "Still waiting for app to start... ${timeout}s remaining"
    fi
  done
- if [ $timeout -eq 0 ]; then
+ if [ "${timeout}" -eq 0 ]; then

Committable suggestion skipped: line range outside the PR's diff.

🧰 Tools
🪛 actionlint (1.7.4)

381-381: shellcheck reported issue in this script: SC2086:info:3:35: Double quote to prevent globbing and word splitting

(shellcheck)

381-381: shellcheck reported issue in this script: SC2086:info:10:6: Double quote to prevent globbing and word splitting

(shellcheck)

if: always()
run: |
docker stop talawa-admin-app-container
docker rm talawa-admin-app-container

Check-Target-Branch:
if: ${{ github.actor != 'dependabot[bot]' }}
Expand Down