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

Jump to bottom

WIP Convert CircleCI config to Github Actions #8147

Open
wants to merge 23 commits into
base: master
Choose a base branch
from
Open

WIP Convert CircleCI config to Github Actions #8147

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
23 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
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
16 changes: 16 additions & 0 deletions .github/actions/health_check_action.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
name: 'URL Health Check'
description: 'Performs URL 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
Comment on lines +12 to +16
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

Enhance security and reliability of the health check implementation.

Consider the following improvements:

  1. Add timeout to prevent hanging requests
  2. Validate the URL input
  3. Remove verbose flag to prevent potential sensitive information exposure
  4. Make retry count and delay configurable

Apply this diff to implement the suggested improvements:

+inputs:
+  url:
+    description: 'Health check URL'
+    required: true
+  max_attempts:
+    description: 'Maximum number of retry attempts'
+    required: false
+    default: '20'
+  retry_delay:
+    description: 'Delay between retries in seconds'
+    required: false
+    default: '5'
 runs:
   using: 'composite'
   steps:
     - shell: bash
       run: |
-        for i in {1..20}; do
-          curl --fail -v "${{ inputs.url }}" && exit 0
-          sleep 5
+        # Validate URL format
+        if ! [[ "${{ inputs.url }}" =~ ^https?:// ]]; then
+          echo "Error: Invalid URL format. Must start with http:// or https://"
+          exit 1
+        fi
+        
+        for i in $(seq 1 ${{ inputs.max_attempts }}); do
+          echo "Attempt $i of ${{ inputs.max_attempts }}"
+          # Add timeout and remove verbose flag
+          if curl --fail --max-time 30 "${{ inputs.url }}"; then
+            echo "Health check succeeded"
+            exit 0
+          fi
+          echo "Attempt failed, retrying in ${{ inputs.retry_delay }} seconds..."
+          sleep ${{ inputs.retry_delay }}
         done
+        echo "Health check failed after ${{ inputs.max_attempts }} attempts"
         exit 1

Committable suggestion was skipped due to low confidence.

🧰 Tools
🪛 yamllint

[error] 16-16: no new line character at the end of file

(new-line-at-end-of-file)

8 changes: 8 additions & 0 deletions .github/not-on-master.sh
View file
Open in desktop
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
190 changes: 188 additions & 2 deletions .github/workflows/build_test_deploy.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,13 +1,199 @@
name: CI Pipeline

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

env:
# USER_NAME: root
# USER_UID: 1000
# USER_GID: 1000
DOCKER_USER: ${{ secrets.DOCKER_USER }}
DOCKER_PASS: ${{ secrets.DOCKER_PASS }}

jobs:
foo:
static_frontend_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 && yarn cache clean

- 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
Comment on lines +151 to +160
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
uses: ./.github/actions/health_check_action
with:
url: http://localhost:9000/api/health


- name: Run webknossos-datastore smoke test
uses: ./.github/actions/health_check_action
with:
url: http://localhost:9090/data/health

- name: Run webknossos-tracingstore smoke test
uses: ./.github/actions/health_check_action
with:
url: http://localhost:9050/tracings/health

- 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 +201 to +207
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 +200 to +222
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 +199 to +223
Copy link

@coderabbitai coderabbitai bot Oct 24, 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

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 +230 to +231
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 +233 to +234
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
6 changes: 2 additions & 4 deletions docker-compose.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,3 @@
version: "2.2"

services:
# Production
webknossos:
Expand Down Expand Up @@ -105,8 +103,8 @@ services:
- POSTGRES_USER=webknossos_user
- POSTGRES_PASSWORD=secret_password
- HOME=/root
- CIRCLE_TAG=${CIRCLE_TAG}
- CIRCLE_BUILD_NUM=${CIRCLE_BUILD_NUM}
- CI_TAG=${GITHUB_REF}
- CI_BUILD_NUM=${GITHUB_JOB}
- NODE_OPTIONS=--max_old_space_size=2048
Comment on lines +106 to +107
Copy link

Choose a reason for hiding this comment

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

💡 Codebase verification

⚠️ Potential issue

Environment variables need adjustment for GitHub Actions

Based on the usage in project/BuildInfoSettings.scala, these variables are used to populate build information. The current mappings need to be corrected:

  1. CI_TAG=${GITHUB_REF} - This will include the full ref path, making ciTag contain unwanted prefixes like "refs/tags/" or "refs/heads/"
  2. CI_BUILD_NUM=${GITHUB_JOB} - This will store the job name instead of a build number in ciBuild, which differs from the previous behavior

Suggested fix:

-      - CI_TAG=${GITHUB_REF}
-      - CI_BUILD_NUM=${GITHUB_JOB}
+      - CI_TAG=${GITHUB_REF_NAME}
+      - CI_BUILD_NUM=${GITHUB_RUN_NUMBER}
  • GITHUB_REF_NAME provides just the branch/tag name without the "refs/*/" prefix
  • GITHUB_RUN_NUMBER provides a sequential number for the workflow runs, similar to the previous CI build number
🔗 Analysis chain

Review environment variable mappings for GitHub Actions.

The current mappings might not provide equivalent values:

  1. GITHUB_REF includes the full reference path (e.g., 'refs/heads/main', 'refs/tags/v1.0.0') unlike CIRCLE_TAG which only contained the tag name.
  2. GITHUB_JOB provides the job name, not a sequential build number like CIRCLE_BUILD_NUM.

Consider these alternatives:

-      - CI_TAG=${GITHUB_REF}
-      - CI_BUILD_NUM=${GITHUB_JOB}
+      - CI_TAG=${GITHUB_REF#refs/tags/}
+      - CI_BUILD_NUM=${GITHUB_RUN_NUMBER}

Let's verify the usage of these variables:

Let me gather more context about how these variables might be used in the application code.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Search for CI_TAG and CI_BUILD_NUM usage in the codebase
rg "CI_(TAG|BUILD_NUM)" --type-add 'config:*.{yml,yaml,json,conf,properties}' -t config

Length of output: 191

Script:

#!/bin/bash
# Search for CI_TAG and CI_BUILD_NUM usage in all files
rg "CI_(TAG|BUILD_NUM)" -A 3 -B 3

# Also check for any shell scripts that might use these variables
fd ".sh$" --exec grep -l "CI_TAG\|CI_BUILD_NUM" {} \;

Length of output: 1350

# This will be picked up by ava so that the tests fail if a snapshot doesn't exist during CI, see https://github.com/avajs/ava/issues/1585
- CI
Expand Down
4 changes: 2 additions & 2 deletions project/BuildInfoSettings.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,8 +13,8 @@ object BuildInfoSettings {
}

val ciBuild: String =
if (System.getenv().containsKey("CIRCLE_BUILD_NUM")) System.getenv().get("CIRCLE_BUILD_NUM") else ""
val ciTag: String = if (System.getenv().containsKey("CIRCLE_TAG")) System.getenv().get("CIRCLE_TAG") else ""
if (System.getenv().containsKey("CI_BUILD_NUM")) System.getenv().get("CI_BUILD_NUM") else ""
val ciTag: String = if (System.getenv().containsKey("CI_TAG")) System.getenv().get("CI_TAG") else ""

def commitHash: String = getStdoutFromCommand("git rev-parse HEAD", "<getting commit hash failed>")
def commitDate: String = getStdoutFromCommand("git log -1 --format=%cd ", "<getting git date failed>")
Expand Down
22 changes: 6 additions & 16 deletions yarn.lock
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8109,7 +8109,7 @@ __metadata:
languageName: node
linkType: hard

"lodash-es@npm:^4.17.21, lodash-es@npm:^4.2.1":
"lodash-es@npm:^4.17.21":
version: 4.17.21
resolution: "lodash-es@npm:4.17.21"
checksum: 10c0/fb407355f7e6cd523a9383e76e6b455321f0f153a6c9625e21a8827d10c54c2a2341bd2ae8d034358b60e07325e1330c14c224ff582d04612a46a4f0479ff2f2
Expand Down Expand Up @@ -8144,7 +8144,7 @@ __metadata:
languageName: node
linkType: hard

"lodash@npm:^4.17.14, lodash@npm:^4.17.15, lodash@npm:^4.17.21, lodash@npm:^4.2.1":
"lodash@npm:^4.17.14, lodash@npm:^4.17.15, lodash@npm:^4.17.21":
version: 4.17.21
resolution: "lodash@npm:4.17.21"
checksum: 10c0/d8cbea072bb08655bb4c989da418994b073a608dffa608b09ac04b43a791b12aeae7cd7ad919aa4c925f33b48490b5cfe6c1f71d827956071dae2e7bb3a6b74c
Expand Down Expand Up @@ -11587,14 +11587,11 @@ __metadata:
linkType: hard

"redux@npm:^4.2.0":
version: 3.7.2
resolution: "redux@npm:3.7.2"
version: 4.2.1
resolution: "redux@npm:4.2.1"
dependencies:
lodash: "npm:^4.2.1"
lodash-es: "npm:^4.2.1"
loose-envify: "npm:^1.1.0"
symbol-observable: "npm:^1.0.3"
checksum: 10c0/544456f95734de33326637b370894addb57d9de2524edf36a20e4a326d0a36a0e223979d027545c5aa8a8d7a2859363981f63d1146401b72df0d16f373dd09cb
"@babel/runtime": "npm:^7.9.2"
checksum: 10c0/136d98b3d5dbed1cd6279c8c18a6a74c416db98b8a432a46836bdd668475de6279a2d4fd9d1363f63904e00f0678a8a3e7fa532c897163340baf1e71bb42c742
languageName: node
linkType: hard

Expand Down Expand Up @@ -12835,13 +12832,6 @@ __metadata:
languageName: node
linkType: hard

"symbol-observable@npm:^1.0.3":
version: 1.2.0
resolution: "symbol-observable@npm:1.2.0"
checksum: 10c0/009fee50798ef80ed4b8195048288f108b03de162db07493f2e1fd993b33fafa72d659e832b584da5a2427daa78e5a738fb2a9ab027ee9454252e0bedbcd1fdc
languageName: node
linkType: hard

"symbol-tree@npm:^3.2.4":
version: 3.2.4
resolution: "symbol-tree@npm:3.2.4"
Expand Down
Loading