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 41 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
41 commits
Select commit Hold shift + click to select a range
69a4e4c
convert circle ci config to github actions
hotzenklotz Oct 24, 2024
4a9811e
use new docker compose syntax
hotzenklotz Oct 24, 2024
d0eed33
only run github actions on PRs and master branch
hotzenklotz Oct 24, 2024
53e35c4
CI
hotzenklotz Oct 28, 2024
53d1c23
fix yarn lockfile
hotzenklotz Oct 28, 2024
eb8af0b
CI
hotzenklotz Oct 28, 2024
1d77e3c
CI
hotzenklotz Oct 28, 2024
03c1cbf
Ci
hotzenklotz Oct 28, 2024
7acf3a9
CI
hotzenklotz Oct 28, 2024
9521869
Merge branch 'master' of github.com:scalableminds/webknossos into ci
hotzenklotz Oct 28, 2024
633b9bf
CI
hotzenklotz Oct 28, 2024
267d646
sudo!
hotzenklotz Oct 28, 2024
188753d
sudo
hotzenklotz Oct 28, 2024
47e66d6
bump ubuntu version
hotzenklotz Oct 28, 2024
f69e1c9
CI
hotzenklotz Oct 28, 2024
98337a4
use different package names for ubuntu
hotzenklotz Oct 28, 2024
da87a2a
CI
hotzenklotz Oct 28, 2024
a15f95c
enable ssh debug
hotzenklotz Oct 28, 2024
5a2766f
CI
hotzenklotz Oct 28, 2024
35e7381
CI
hotzenklotz Oct 28, 2024
e81a452
add sbt action
hotzenklotz Oct 28, 2024
e3bd804
ci
hotzenklotz Oct 28, 2024
d12c110
ci
hotzenklotz Oct 28, 2024
46aafa2
Merge branch 'master' of github.com:scalableminds/webknossos into ci
hotzenklotz Oct 30, 2024
b91701d
CI
hotzenklotz Oct 30, 2024
c15cc6b
CI
hotzenklotz Oct 30, 2024
e3c6b4d
CI
hotzenklotz Oct 30, 2024
543bba1
CI
hotzenklotz Oct 30, 2024
f25cca0
enable ssh debugging
hotzenklotz Nov 11, 2024
e6de5cb
set postgres url
hotzenklotz Nov 11, 2024
816989d
CI
hotzenklotz Nov 11, 2024
495296e
CI
hotzenklotz Nov 11, 2024
d9322ce
ci
hotzenklotz Nov 11, 2024
035f5a3
ci
hotzenklotz Nov 11, 2024
b162be2
ci
hotzenklotz Nov 11, 2024
6c06ca3
CI
hotzenklotz Nov 11, 2024
18b7cb4
ci
hotzenklotz Nov 11, 2024
77d54be
ci
hotzenklotz Nov 12, 2024
46f6696
ci
hotzenklotz Nov 12, 2024
e864dbb
ci
hotzenklotz Nov 12, 2024
c9f8868
ci
hotzenklotz Nov 12, 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
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.

🛠️ Refactor suggestion

Add error handling to frontend checks

The frontend check commands should fail fast if any check fails. Add proper error handling:

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

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

    - name: Check for cyclic dependencies in frontend
      run: |
+       set -euo pipefail
        yarn check-cyclic-dependencies
📝 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: 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
- name: Lint frontend code and check formatting
run: |
set -euo pipefail
yarn run check-frontend
- name: Typecheck frontend code
run: |
set -euo pipefail
yarn typecheck
- name: Check for cyclic dependencies in frontend
run: |
set -euo pipefail
yarn check-cyclic-dependencies

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 +80 to +90
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)
Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Fix shell script quoting in retry logic

The retry logic needs proper quoting and error handling:

  run: |
+   set -euo pipefail
    for i in {1..3}; do # retry
-     .github/not-on-master.sh docker compose run e2e-tests && s=0 && break || s=$?
+     if .github/not-on-master.sh docker compose run e2e-tests; then
+       s=0
+       break
+     else
+       s=$?
+     fi
    done
-   (exit $s)
+   (exit "${s}")
📝 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
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)
run: |
set -euo pipefail
for i in {1..3}; do # retry
if .github/not-on-master.sh docker compose run e2e-tests; then
s=0
break
else
s=$?
fi
done
(exit "${s}")
🧰 Tools
🪛 actionlint

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

(shellcheck)


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

(shellcheck)


- 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
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
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 issues in Docker operations

The Docker operations script has multiple quoting and error handling issues.

 function retry() {
+  local cmd=("$@")
   for i in {1..5}; do
-    "$@" && s=0 && break || s=$?
+    if "${cmd[@]}"; then
+      return 0
+    fi
     sleep 10
   done
-  return $s
+  return 1
 }
-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}"
   # Similar fixes for other docker commands
 fi

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

🧰 Tools
🪛 actionlint

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

(shellcheck)


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

(shellcheck)


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

(shellcheck)


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

(shellcheck)


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

(shellcheck)


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

(shellcheck)


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

(shellcheck)


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

(shellcheck)


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

(shellcheck)


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

(shellcheck)


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

(shellcheck)


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

(shellcheck)


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

(shellcheck)


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

(shellcheck)


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

(shellcheck)

docker logout
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
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 +235 to +236
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 GitHub Actions native Slack integration.

Instead of using the CircleCI script, implement Slack notifications using GitHub Actions:

- name: Notify Slack
  if: github.ref == 'refs/heads/master'
  uses: slackapi/[email protected]
  with:
    channel-id: 'CHANNEL_ID'
    slack-message: "GitHub Action build result: ${{ job.status }}\n${{ github.event.pull_request.html_url || github.event.head_commit.url }}"
  env:
    SLACK_BOT_TOKEN: ${{ secrets.SLACK_BOT_TOKEN }}

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