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

WIP Convert CircleCI config to Github Actions #8147

Open
wants to merge 28 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 2 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
8 changes: 8 additions & 0 deletions .github/not-on-master.sh
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
#!/usr/bin/env bash
set -Eeuo pipefail

if [ "${GITHUB_REF}" == "master" ]; then
echo "Skipping this step on master..."
else
exec "$@"
fi
199 changes: 197 additions & 2 deletions .github/workflows/build_test_deploy.yml
Original file line number Diff line number Diff line change
@@ -1,13 +1,208 @@
name: CI Pipeline

on:
push:
branches:
- '*'
pull_request:
branches:
- '*'
workflow_dispatch:

env:
USER_NAME: circleci
USER_UID: 1000
USER_GID: 1000
TZ: Europe/Berlin
DOCKER_USER: ${{ secrets.DOCKER_USER }}
DOCKER_PASS: ${{ secrets.DOCKER_PASS }}

jobs:
foo:
static_frontent_code_checks:
runs-on: ubuntu-20.04
steps:
- name: Checkout code
uses: actions/checkout@v3
with:
fetch-depth: 5
fetch-depth: 5

- uses: actions/setup-node@v4
with:
node-version: 18

- name: Install frontend dependencies
Comment on lines +38 to +39
Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Add yarn cache to improve CI performance

Consider using GitHub's cache action for yarn dependencies to speed up the CI pipeline.

    - uses: actions/setup-node@v4
      with:
        node-version: 18
+       cache: 'yarn'
+
+   - name: Get yarn cache directory path
+     id: yarn-cache-dir-path
+     run: echo "dir=$(yarn config get cacheFolder)" >> $GITHUB_OUTPUT
+
+   - uses: actions/cache@v3
+     id: yarn-cache
+     with:
+       path: ${{ steps.yarn-cache-dir-path.outputs.dir }}
+       key: ${{ runner.os }}-yarn-${{ hashFiles('**/yarn.lock') }}
+       restore-keys: |
+         ${{ runner.os }}-yarn-

    - name: Install frontend dependencies
      run: corepack enable && yarn install --immutable
📝 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
- name: Install frontend dependencies
run: corepack enable && yarn install --immutable
- uses: actions/setup-node@v4
with:
node-version: 18
cache: 'yarn'
- name: Get yarn cache directory path
id: yarn-cache-dir-path
run: echo "dir=$(yarn config get cacheFolder)" >> $GITHUB_OUTPUT
- uses: actions/cache@v3
id: yarn-cache
with:
path: ${{ steps.yarn-cache-dir-path.outputs.dir }}
key: ${{ runner.os }}-yarn-${{ hashFiles('**/yarn.lock') }}
restore-keys: |
${{ runner.os }}-yarn-
- name: Install frontend dependencies
run: corepack enable && yarn install --immutable

run: corepack enable && yarn install --immutable

- name: Lint frontend code and check formatting
run: yarn run check-frontend

- name: Typecheck frontend code
run: yarn typecheck

- name: Check for cyclic dependencies in frontend
Comment on lines +41 to +48
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

Add error handling to frontend checks

The frontend check commands should fail fast if any check fails. Add set -e to ensure the job fails immediately on any error:

    - name: Lint frontend code and check formatting
      run: |
+       set -e
        yarn run check-frontend

    - name: Typecheck frontend code
      run: |
+       set -e
        yarn typecheck

    - name: Check for cyclic dependencies in frontend
      run: |
+       set -e
        yarn check-cyclic-dependencies

Committable suggestion was skipped due to low confidence.

run: yarn check-cyclic-dependencies




build_test_deploy:
runs-on: ubuntu-20.04
steps:
- name: Checkout code
uses: actions/checkout@v3
with:
fetch-depth: 5

- name: "Custom environment variables"
run: |
if [[ ${{ github.ref_type }} == "branch" ]]; then
NORMALIZED_BRANCH=$(echo ${{ github.ref_name }} | sed 's/[\/-]/_/g')
echo "NORMALIZED_BRANCH=$NORMALIZED_BRANCH" >> $GITHUB_ENV
DOCKER_TAG="${NORMALIZED_BRANCH}__${{ github.run_number }}"
echo "DOCKER_TAG=$DOCKER_TAG" >> $GITHUB_ENV
else
echo "NORMALIZED_BRANCH=master" >> $GITHUB_ENV
echo "DOCKER_TAG=${{ github.ref_name }}" >> $GITHUB_ENV
Comment on lines +83 to +93
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

Fix shell script quoting issues

The branch normalization script has potential issues with word splitting. Apply proper quoting:

  if [[ ${{ github.ref_type }} == "branch" ]]; then
-   NORMALIZED_BRANCH=$(echo ${{ github.ref_name }} | sed 's/[\/-]/_/g')
+   NORMALIZED_BRANCH=$(echo "${{ github.ref_name }}" | sed 's/[\/-]/_/g')
-   echo "NORMALIZED_BRANCH=$NORMALIZED_BRANCH" >> $GITHUB_ENV
+   echo "NORMALIZED_BRANCH=${NORMALIZED_BRANCH}" >> "${GITHUB_ENV}"
-   DOCKER_TAG="${NORMALIZED_BRANCH}__${{ github.run_number }}"
+   DOCKER_TAG="${NORMALIZED_BRANCH}__${{ github.run_number }}"
-   echo "DOCKER_TAG=$DOCKER_TAG" >> $GITHUB_ENV
+   echo "DOCKER_TAG=${DOCKER_TAG}" >> "${GITHUB_ENV}"
📝 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
- name: "Custom environment variables"
run: |
if [[ ${{ github.ref_type }} == "branch" ]]; then
NORMALIZED_BRANCH=$(echo ${{ github.ref_name }} | sed 's/[\/-]/_/g')
echo "NORMALIZED_BRANCH=$NORMALIZED_BRANCH" >> $GITHUB_ENV
DOCKER_TAG="${NORMALIZED_BRANCH}__${{ github.run_number }}"
echo "DOCKER_TAG=$DOCKER_TAG" >> $GITHUB_ENV
else
echo "NORMALIZED_BRANCH=master" >> $GITHUB_ENV
echo "DOCKER_TAG=${{ github.ref_name }}" >> $GITHUB_ENV
fi
- name: "Custom environment variables"
run: |
if [[ ${{ github.ref_type }} == "branch" ]]; then
NORMALIZED_BRANCH=$(echo "${{ github.ref_name }}" | sed 's/[\/-]/_/g')
echo "NORMALIZED_BRANCH=${NORMALIZED_BRANCH}" >> "${GITHUB_ENV}"
DOCKER_TAG="${NORMALIZED_BRANCH}__${{ github.run_number }}"
echo "DOCKER_TAG=${DOCKER_TAG}" >> "${GITHUB_ENV}"
else
echo "NORMALIZED_BRANCH=master" >> $GITHUB_ENV
echo "DOCKER_TAG=${{ github.ref_name }}" >> $GITHUB_ENV
fi
🧰 Tools
🪛 actionlint

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

(shellcheck)


57-57: shellcheck reported issue in this script: SC2086:info:5:36: Double quote to prevent globbing and word splitting

(shellcheck)


57-57: shellcheck reported issue in this script: SC2086:info:7:38: Double quote to prevent globbing and word splitting

(shellcheck)


57-57: shellcheck reported issue in this script: SC2086:info:8:47: Double quote to prevent globbing and word splitting

(shellcheck)

fi

- name: Set up Docker
uses: docker/setup-buildx-action@v2

- name: Build webknossos-dev docker image
run: |
docker pull scalableminds/webknossos-dev:$NORMALIZED_BRANCH || true
DEV_CACHE=$NORMALIZED_BRANCH docker compose build base

- name: Prepare dependency folders
run: mkdir -p project/target target ~/.ivy2 ~/.cache/coursier

- name: Install frontend dependencies
run: docker compose run -e PUPPETEER_SKIP_CHROMIUM_DOWNLOAD=true base yarn install --immutable

- name: Assert unique evolution numbers
run: docker compose run base tools/postgres/dbtool.js assert-unique-evolution-numbers

- name: Assert schema.sql and evolutions are equal
run: |
docker compose up -d postgres
sleep 3
docker compose run compile tools/postgres/dbtool.js check-evolutions-schema
Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Replace sleep with proper postgres readiness check

Using sleep is not reliable for ensuring postgres is ready. Consider using a proper health check.

-          sleep 3
+          until docker compose exec -T postgres pg_isready; do
+            echo "Waiting for postgres..."
+            sleep 1
+          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.

Suggested change
- name: Assert schema.sql and evolutions are equal
run: |
docker compose up -d postgres
sleep 3
docker compose run compile tools/postgres/dbtool.js check-evolutions-schema
- name: Assert schema.sql and evolutions are equal
run: |
docker compose up -d postgres
until docker compose exec -T postgres pg_isready; do
echo "Waiting for postgres..."
sleep 1
done
docker compose run compile tools/postgres/dbtool.js check-evolutions-schema


- name: Build frontend documentation
run: |
WK_VERSION=${{ github.event.release.tag_name || github.run_number || 'dev' }}
docker compose run base yarn run docs --project-version $WK_VERSION

- name: Build webknossos (webpack)
run: docker compose run base yarn build

- name: Build webknossos (sbt)
run: |
if [ "${{ github.ref }}" == "refs/heads/master" ]; then
docker compose run compile sbt -no-colors clean compile stage
else
docker compose run compile sbt -no-colors -DfailOnWarning compile stage
fi

- name: Build webknossos-datastore (sbt)
run: docker compose run base sbt -no-colors -DfailOnWarning "project webknossosDatastore" copyMessages compile stage

- name: Build webknossos-tracingstore (sbt)
run: docker compose run base sbt -no-colors -DfailOnWarning "project webknossosTracingstore" copyMessages compile stage
Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Consider parallelizing build steps

The build steps for webknossos, datastore, and tracingstore are running sequentially. Consider using GitHub Actions' job parallelization to speed up the pipeline:

  1. Split the builds into separate jobs
  2. Use job outputs to share build artifacts
  3. Use job dependencies to maintain the correct order

Example structure:

jobs:
  build-webknossos:
    # ... build webknossos
    outputs:
      checksum: ${{ steps.checksum.outputs.value }}

  build-datastore:
    needs: build-webknossos
    # ... build datastore

  build-tracingstore:
    needs: build-webknossos
    # ... build tracingstore

  tests:
    needs: [build-webknossos, build-datastore, build-tracingstore]
    # ... run tests


- name: Checksum App Dirs
run: find app webknossos-datastore/app webknossos-tracingstore/app -type f -exec md5sum {} \; | sort -k 2 | md5sum > app_checksum.txt

- name: Build webknossos docker image
run: |
docker pull scalableminds/webknossos:$NORMALIZED_BRANCH || true
DEV_CACHE=$NORMALIZED_BRANCH docker compose build --pull webknossos

- name: Build webknossos-datastore docker image
run: docker compose build --pull webknossos-datastore

- name: Build webknossos-tracingstore docker image
Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Optimize Docker builds with layer caching

Consider using BuildKit's cache features to speed up builds:

-        run: docker compose build --pull webknossos-datastore
+        run: |
+          DOCKER_BUILDKIT=1 docker compose build \
+            --pull \
+            --build-arg BUILDKIT_INLINE_CACHE=1 \
+            webknossos-datastore
📝 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
- name: Build webknossos docker image
run: |
docker pull scalableminds/webknossos:$NORMALIZED_BRANCH || true
DEV_CACHE=$NORMALIZED_BRANCH docker compose build --pull webknossos
- name: Build webknossos-datastore docker image
run: docker compose build --pull webknossos-datastore
- name: Build webknossos-tracingstore docker image
run: docker compose build --pull webknossos-tracingstore
- name: Build webknossos docker image
run: |
docker pull scalableminds/webknossos:$NORMALIZED_BRANCH || true
DEV_CACHE=$NORMALIZED_BRANCH docker compose build --pull webknossos
- name: Build webknossos-datastore docker image
run: |
DOCKER_BUILDKIT=1 docker compose build \
--pull \
--build-arg BUILDKIT_INLINE_CACHE=1 \
webknossos-datastore
- name: Build webknossos-tracingstore docker image
run: docker compose build --pull webknossos-tracingstore
🧰 Tools
🪛 actionlint

141-141: shellcheck reported issue in this script: SC2086:info:1:38: Double quote to prevent globbing and word splitting

(shellcheck)

run: docker compose build --pull webknossos-tracingstore

- name: Run frontend tests
run: .github/not-on-master.sh docker compose run base yarn test-verbose

- name: Lint backend code and check formatting
run: .github/not-on-master.sh docker compose run backend-lint-format

- name: Run backend tests
run: .github/not-on-master.sh docker compose run backend-tests

- name: Run end-to-end tests
run: |
for i in {1..3}; do # retry
.github/not-on-master.sh docker compose run e2e-tests && s=0 && break || s=$?
done
(exit $s)

- name: Validate frontend types
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

Remove duplicate type checking

The frontend type checking is already performed in the static_frontend_code_checks job (line 38-39).

Consider removing this duplicate step to improve pipeline performance.

run: .github/not-on-master.sh docker compose run base yarn typecheck

- name: Start webknossos, datastore, and tracingstore
run: |
docker compose up -d webknossos
docker compose up -d webknossos-datastore
docker compose up -d webknossos-tracingstore

- name: Run webknossos smoke test
run: |
for i in {1..20}; do # retry
curl --fail -v http://localhost:9000/api/health && s=0 && break || s=$?
sleep 5
done
(exit $s)

- name: Run webknossos-datastore smoke test
run: |
for i in {1..20}; do # retry
curl --fail -v http://localhost:9090/data/health && s=0 && break || s=$?
sleep 5
done
(exit $s)

- name: Run webknossos-tracingstore smoke test
run: |
for i in {1..20}; do # retry
curl --fail -v http://localhost:9050/tracings/health && s=0 && break || s=$?
sleep 5
done
Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Refactor duplicated health check logic

The health check implementation is duplicated across three services. Consider extracting this into a reusable composite action:

  1. Create .github/actions/health-check/action.yml:
name: 'Health Check'
description: 'Performs health check with retries'
inputs:
  url:
    description: 'Health check URL'
    required: true
runs:
  using: 'composite'
  steps:
    - shell: bash
      run: |
        for i in {1..20}; do
          curl --fail -v "${{ inputs.url }}" && exit 0
          sleep 5
        done
        exit 1
  1. Use it in the workflow:
- uses: ./.github/actions/health-check
  with:
    url: http://localhost:9000/api/health
🧰 Tools
🪛 actionlint

153-153: shellcheck reported issue in this script: SC2034:warning:1:1: i appears unused. Verify use (or export if used externally)

(shellcheck)


153-153: shellcheck reported issue in this script: SC2086:info:5:7: Double quote to prevent globbing and word splitting

(shellcheck)


161-161: shellcheck reported issue in this script: SC2034:warning:1:1: i appears unused. Verify use (or export if used externally)

(shellcheck)


161-161: shellcheck reported issue in this script: SC2086:info:5:7: Double quote to prevent globbing and word splitting

(shellcheck)


169-169: shellcheck reported issue in this script: SC2034:warning:1:1: i appears unused. Verify use (or export if used externally)

(shellcheck)


169-169: shellcheck reported issue in this script: SC2086:info:5:7: Double quote to prevent globbing and word splitting

(shellcheck)

(exit $s)

- name: Stop webknossos, datastore, and tracingstore
run: docker compose down --volumes --remove-orphans

- name: Push docker images
run: |
function retry() {
for i in {1..5}; do
"$@" && s=0 && break || s=$?
sleep 10
done
return $s
Comment on lines +207 to +213
Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Consider using GitHub Actions' built-in retry functionality

Instead of implementing a custom retry function, consider using GitHub Actions' built-in retry functionality with the nick-invision/retry@v2 action:

- uses: nick-invision/retry@v2
  with:
    timeout_minutes: 10
    max_attempts: 5
    command: docker login -u "${DOCKER_USER}" -p "${DOCKER_PASS}"

This approach provides better logging and integration with GitHub Actions.

}
retry docker login -u $DOCKER_USER -p $DOCKER_PASS
retry docker compose push webknossos
retry docker compose push webknossos-datastore
retry docker compose push webknossos-tracingstore
if [[ ${{ github.ref_type }} == "branch" ]]; then
docker tag scalableminds/webknossos:${DOCKER_TAG} scalableminds/webknossos:${NORMALIZED_BRANCH}
retry docker push scalableminds/webknossos:${NORMALIZED_BRANCH}
docker tag scalableminds/webknossos-datastore:${DOCKER_TAG} scalableminds/webknossos-datastore:${NORMALIZED_BRANCH}
retry docker push scalableminds/webknossos-datastore:${NORMALIZED_BRANCH}
docker tag scalableminds/webknossos-tracingstore:${DOCKER_TAG} scalableminds/webknossos-tracingstore:${NORMALIZED_BRANCH}
retry docker push scalableminds/webknossos-tracingstore:${NORMALIZED_BRANCH}
docker tag scalableminds/webknossos-dev scalableminds/webknossos-dev:${NORMALIZED_BRANCH}
retry docker push scalableminds/webknossos-dev:${NORMALIZED_BRANCH}
fi
Comment on lines +206 to +228
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

Fix shell script quoting in Docker operations

The Docker operations contain several shell script issues that should be addressed:

  1. Use proper quoting for variables to prevent word splitting
  2. Use "${variable}" syntax consistently

Apply this diff to fix the issues:

  function retry() {
    for i in {1..5}; do
      "$@" && s=0 && break || s=$?
      sleep 10
    done
    return $s
  }
- retry docker login -u $DOCKER_USER -p $DOCKER_PASS
+ retry docker login -u "${DOCKER_USER}" -p "${DOCKER_PASS}"
  retry docker compose push webknossos
  retry docker compose push webknossos-datastore
  retry docker compose push webknossos-tracingstore
  if [[ ${{ github.ref_type }} == "branch" ]]; then
-   docker tag scalableminds/webknossos:${DOCKER_TAG} scalableminds/webknossos:${NORMALIZED_BRANCH}
+   docker tag "scalableminds/webknossos:${DOCKER_TAG}" "scalableminds/webknossos:${NORMALIZED_BRANCH}"
-   retry docker push scalableminds/webknossos:${NORMALIZED_BRANCH}
+   retry docker push "scalableminds/webknossos:${NORMALIZED_BRANCH}"
    # ... similar fixes for other docker commands ...
  fi

Committable suggestion was skipped due to low confidence.

🧰 Tools
🪛 actionlint

171-171: shellcheck reported issue in this script: SC2034:warning:2:3: i appears unused. Verify use (or export if used externally)

(shellcheck)


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

(shellcheck)


171-171: shellcheck reported issue in this script: SC2086:info:8:23: Double quote to prevent globbing and word splitting

(shellcheck)


171-171: shellcheck reported issue in this script: SC2086:info:8:39: Double quote to prevent globbing and word splitting

(shellcheck)


171-171: shellcheck reported issue in this script: SC2086:info:13:39: Double quote to prevent globbing and word splitting

(shellcheck)


171-171: shellcheck reported issue in this script: SC2086:info:13:78: Double quote to prevent globbing and word splitting

(shellcheck)


171-171: shellcheck reported issue in this script: SC2086:info:14:46: Double quote to prevent globbing and word splitting

(shellcheck)


171-171: shellcheck reported issue in this script: SC2086:info:15:49: Double quote to prevent globbing and word splitting

(shellcheck)


171-171: shellcheck reported issue in this script: SC2086:info:15:98: Double quote to prevent globbing and word splitting

(shellcheck)


171-171: shellcheck reported issue in this script: SC2086:info:16:56: Double quote to prevent globbing and word splitting

(shellcheck)


171-171: shellcheck reported issue in this script: SC2086:info:17:52: Double quote to prevent globbing and word splitting

(shellcheck)


171-171: shellcheck reported issue in this script: SC2086:info:17:104: Double quote to prevent globbing and word splitting

(shellcheck)


171-171: shellcheck reported issue in this script: SC2086:info:18:59: Double quote to prevent globbing and word splitting

(shellcheck)


171-171: shellcheck reported issue in this script: SC2086:info:19:72: Double quote to prevent globbing and word splitting

(shellcheck)


171-171: shellcheck reported issue in this script: SC2086:info:20:50: Double quote to prevent globbing and word splitting

(shellcheck)

docker logout
Comment on lines +205 to +229
Copy link

@coderabbitai coderabbitai bot Oct 24, 2024

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Consolidate Docker push retry logic

The Docker push steps have complex retry logic. Consider:

  1. Using GitHub's built-in retry functionality
  2. Creating a reusable function for Docker operations
- name: Login to Docker Hub
  uses: docker/login-action@v2
  with:
    username: ${{ env.DOCKER_USER }}
    password: ${{ env.DOCKER_PASS }}

- name: Push images
  uses: nick-invision/retry@v2
  with:
    timeout_minutes: 10
    max_attempts: 3
    command: |
      docker compose push webknossos
      docker compose push webknossos-datastore
      docker compose push webknossos-tracingstore
      if [[ ${{ github.ref_type }} == "branch" ]]; then
        # ... tag and push branch images
      fi
🧰 Tools
🪛 actionlint

180-180: shellcheck reported issue in this script: SC2034:warning:2:3: i appears unused. Verify use (or export if used externally)

(shellcheck)


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

(shellcheck)


180-180: shellcheck reported issue in this script: SC2086:info:8:23: Double quote to prevent globbing and word splitting

(shellcheck)


180-180: shellcheck reported issue in this script: SC2086:info:8:39: Double quote to prevent globbing and word splitting

(shellcheck)


180-180: shellcheck reported issue in this script: SC2086:info:13:39: Double quote to prevent globbing and word splitting

(shellcheck)


180-180: shellcheck reported issue in this script: SC2086:info:13:78: Double quote to prevent globbing and word splitting

(shellcheck)


180-180: shellcheck reported issue in this script: SC2086:info:14:46: Double quote to prevent globbing and word splitting

(shellcheck)


180-180: shellcheck reported issue in this script: SC2086:info:15:49: Double quote to prevent globbing and word splitting

(shellcheck)


180-180: shellcheck reported issue in this script: SC2086:info:15:98: Double quote to prevent globbing and word splitting

(shellcheck)


180-180: shellcheck reported issue in this script: SC2086:info:16:56: Double quote to prevent globbing and word splitting

(shellcheck)


180-180: shellcheck reported issue in this script: SC2086:info:17:52: Double quote to prevent globbing and word splitting

(shellcheck)


180-180: shellcheck reported issue in this script: SC2086:info:17:104: Double quote to prevent globbing and word splitting

(shellcheck)


180-180: shellcheck reported issue in this script: SC2086:info:18:59: Double quote to prevent globbing and word splitting

(shellcheck)


180-180: shellcheck reported issue in this script: SC2086:info:19:72: Double quote to prevent globbing and word splitting

(shellcheck)


180-180: shellcheck reported issue in this script: SC2086:info:20:50: Double quote to prevent globbing and word splitting

(shellcheck)

Copy link
Member Author

Choose a reason for hiding this comment

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

That sound great. Please do it!

Copy link

Choose a reason for hiding this comment

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

@hotzenklotz, glad you like the suggestion! Here's the updated workflow with the consolidated Docker push retry logic:

name: CI Pipeline

on:
  push:
    branches:
      - '*'
  pull_request:
    branches:
      - '*'
  workflow_dispatch:

env:
  USER_NAME: circleci
  USER_UID: 1000
  USER_GID: 1000
  TZ: Europe/Berlin
  DOCKER_USER: ${{ secrets.DOCKER_USER }}
  DOCKER_PASS: ${{ secrets.DOCKER_PASS }}

jobs:
  static_frontent_code_checks:
    runs-on: ubuntu-20.04
    steps:
      - name: Checkout code
        uses: actions/checkout@v3
        with:
          fetch-depth: 5

      - uses: actions/setup-node@v4
        with:
          node-version: 18

      - name: Install frontend dependencies
        run: corepack enable && yarn install --immutable

      - name: Lint frontend code and check formatting
        run: yarn run check-frontend

      - name: Typecheck frontend code
        run: yarn typecheck

      - name: Check for cyclic dependencies in frontend
        run: yarn check-cyclic-dependencies

  build_test_deploy:
    runs-on: ubuntu-20.04
    steps:
      - name: Checkout code
        uses: actions/checkout@v3
        with:
          fetch-depth: 5

      - name: "Custom environment variables"
        run: |
          if [[ ${{ github.ref_type }} == "branch" ]]; then
            NORMALIZED_BRANCH=$(echo ${{ github.ref_name }} | sed 's/[\/-]/_/g')
            echo "NORMALIZED_BRANCH=$NORMALIZED_BRANCH" >> $GITHUB_ENV
            DOCKER_TAG="${NORMALIZED_BRANCH}__${{ github.run_number }}"
            echo "DOCKER_TAG=$DOCKER_TAG" >> $GITHUB_ENV
          else
            echo "NORMALIZED_BRANCH=master" >> $GITHUB_ENV
            echo "DOCKER_TAG=${{ github.ref_name }}" >> $GITHUB_ENV
          fi

      - name: Set up Docker
        uses: docker/setup-buildx-action@v2

      # ... other build and test steps ...

      - name: Login to Docker Hub
        uses: docker/login-action@v2
        with:
          username: ${{ env.DOCKER_USER }}
          password: ${{ env.DOCKER_PASS }}

      - name: Push docker images
        uses: nick-invision/retry@v2
        with:
          timeout_minutes: 10
          max_attempts: 3
          command: |
            docker compose push webknossos
            docker compose push webknossos-datastore
            docker compose push webknossos-tracingstore
            if [[ ${{ github.ref_type }} == "branch" ]]; then
              docker tag scalableminds/webknossos:${{ env.DOCKER_TAG }} scalableminds/webknossos:${{ env.NORMALIZED_BRANCH }}
              docker push scalableminds/webknossos:${{ env.NORMALIZED_BRANCH }}
              docker tag scalableminds/webknossos-datastore:${{ env.DOCKER_TAG }} scalableminds/webknossos-datastore:${{ env.NORMALIZED_BRANCH }}
              docker push scalableminds/webknossos-datastore:${{ env.NORMALIZED_BRANCH }}
              docker tag scalableminds/webknossos-tracingstore:${{ env.DOCKER_TAG }} scalableminds/webknossos-tracingstore:${{ env.NORMALIZED_BRANCH }}
              docker push scalableminds/webknossos-tracingstore:${{ env.NORMALIZED_BRANCH }}
              docker tag scalableminds/webknossos-dev scalableminds/webknossos-dev:${{ env.NORMALIZED_BRANCH }}
              docker push scalableminds/webknossos-dev:${{ env.NORMALIZED_BRANCH }}
            fi

      - name: Logout from Docker Hub
        run: docker logout

      # ... remaining steps ...

This update simplifies the retry logic by utilizing the nick-invision/retry@v2 action and the docker/login-action@v2 for authentication with Docker Hub.

Let me know if you have any questions or need further assistance!

(^‿^)/


- name: Report coverage
Comment on lines +236 to +237
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

Don't silently ignore coverage failures

The coverage command ignores failures with || true, which could hide important coverage regressions.

Consider:

  1. Setting a minimum coverage threshold
  2. Making the job fail if coverage drops below the threshold
  3. Using a coverage reporting action to track trends
-        run: .github/not-on-master.sh docker compose run base yarn coverage || true
+        run: |
+          .github/not-on-master.sh docker compose run base yarn coverage
+        continue-on-error: true

Committable suggestion was skipped due to low confidence.

run: .github/not-on-master.sh docker compose run base yarn coverage || true

# - name: Send Slack notification (master only)
Comment on lines +239 to +240
Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Consider implementing Slack notifications using GitHub Actions

Instead of using the CircleCI script, consider using a GitHub Actions marketplace action for Slack notifications:

- name: Notify Slack
  if: github.ref == 'refs/heads/main'
  uses: 8398a7/action-slack@v3
  with:
    status: ${{ job.status }}
    fields: repo,message,commit,author,action,eventName,ref,workflow,job,took
  env:
    SLACK_WEBHOOK_URL: ${{ secrets.SLACK_WEBHOOK_URL }}

# run: .circleci/slack-notification.sh
Loading