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 17 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
16 changes: 16 additions & 0 deletions .github/actions/health_check_action.yml
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
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
216 changes: 213 additions & 3 deletions .github/workflows/build_test_deploy.yml
Original file line number Diff line number Diff line change
@@ -1,13 +1,223 @@
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 }}
Comment on lines +19 to +23
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

Security: Avoid running containers as root user

Running containers as root user (USER_NAME: root) is a security anti-pattern. Consider:

  1. Using a non-root user for better security
  2. Following the principle of least privilege
-  USER_NAME: root
+  USER_NAME: webknossos

Committable suggestion was skipped due to low confidence.


jobs:
foo:
runs-on: ubuntu-20.04
static_frontend_code_checks:
runs-on: ubuntu-24.04
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

Align Ubuntu versions across jobs

The static_frontend_code_checks job uses ubuntu-24.04 while build_test_deploy uses ubuntu-20.04. This inconsistency could lead to environment differences. Consider using the same Ubuntu version for both jobs.

Also applies to: 46-46

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-24.04
env:
# The hostname used to communicate with the PostgreSQL service container
POSTGRES_HOST: localhost
POSTGRES_PORT: 5432
services:
postgres:
image: postgres:10-alpine
env:
POSTGRES_DB: webknossos
POSTGRES_USER: webknossos_user
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

Use secrets for database credentials

Database credentials are currently hardcoded in the workflow file. Consider using GitHub secrets:

-            POSTGRES_DB: webknossos
-            POSTGRES_USER: webknossos_user
-            POSTGRES_PASSWORD: secret_password
+            POSTGRES_DB: ${{ secrets.POSTGRES_DB }}
+            POSTGRES_USER: ${{ secrets.POSTGRES_USER }}
+            POSTGRES_PASSWORD: ${{ secrets.POSTGRES_PASSWORD }}
📝 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
POSTGRES_DB: webknossos
POSTGRES_USER: webknossos_user
POSTGRES_PASSWORD: secret_password
POSTGRES_DB: ${{ secrets.POSTGRES_DB }}
POSTGRES_USER: ${{ secrets.POSTGRES_USER }}
POSTGRES_PASSWORD: ${{ secrets.POSTGRES_PASSWORD }}

POSTGRES_PASSWORD: secret_password
# Set health checks to wait until postgres has started
options: >-
--health-cmd pg_isready
--health-interval 2s
--health-timeout 5s
--health-retries 30
ports:
Comment on lines +71 to +72
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

Restrict PostgreSQL port exposure

The PostgreSQL port is exposed on all interfaces. For better security, restrict it to localhost:

    ports:
-     - 5432:5432
+     - 127.0.0.1:5432:5432
📝 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
ports:
- 5432:5432
ports:
- 127.0.0.1:5432:5432

- 5432:5432
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
- uses: actions/setup-node@v4
with:
node-version: 18

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

- name: Build webknossos (webpack)
run: yarn build

- name: Set up Java / SBT
uses: actions/setup-java@v4
with:
distribution: 'temurin' # See 'Supported distributions' for available options
java-version: '21'
cache: 'sbt'

- name: Install OS dependencies / libs
run: sudo apt-get update && sudo apt-get install -y \
libdraco-dev \
libblosc-dev

# build-essential \
# cmake \

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

# - 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
Comment on lines +119 to +130
Copy link

Choose a reason for hiding this comment

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

💡 Codebase verification

These database validation steps should be uncommented and fixed

The commented-out steps are critical database schema validation checks that:

  1. Ensure evolution numbers are unique (assert-unique-evolution-numbers)
  2. Verify schema.sql matches the evolutions (check-evolutions-schema)

These checks are important as evidenced by:

  • The dbtool.js script contains active implementations of these commands
  • The script is actively used in other parts of the codebase for database validation
  • No alternative validation steps exist in the workflow file
🔗 Analysis chain

Clean up commented code blocks

Several steps are commented out but seem important for database schema validation. Either:

  1. Remove these comments if they're no longer needed
  2. Uncomment and fix them if they're still required

Let me check how these database tools are actually used to determine if the commented steps are still needed.


Let me check if these database validation steps are replaced by other mechanisms in the workflow.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Check if these database tools are used elsewhere
rg -l "dbtool.js"

Length of output: 217


Script:

#!/bin/bash
# Check the content of files that use dbtool.js
rg "dbtool\.js" -C 3

# Also check if there are any other similar database schema validation tasks
rg "schema" .github/workflows/ -C 3

Length of output: 6445


Script:

#!/bin/bash
# Check the entire workflow file for other database validation steps
cat .github/workflows/build_test_deploy.yml

# Also check if dbtool.js exists and what commands it supports
cat tools/postgres/dbtool.js 2>/dev/null || echo "File not found"

Length of output: 21423


- name: Build webknossos (sbt)
run: |
if [ "${{ github.ref }}" == "refs/heads/master" ]; then
sbt -no-colors clean compile stage
else
sbt -no-colors -DfailOnWarning compile stage
Comment on lines +131 to +137
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 SBT build configuration

The SBT build could be optimized:

  1. Use .sbtopts file for common options
  2. Enable parallel execution
  3. Configure memory settings
    - name: Build webknossos (sbt)
      run: |
+       echo "-J-Xmx4G" > .sbtopts
+       echo "-J-Xms2G" >> .sbtopts
+       echo "-Dsbt.parallel=true" >> .sbtopts
        if [ "${{ github.ref }}" == "refs/heads/master" ]; then
-         sbt -no-colors clean compile stage
+         sbt clean compile stage
        else
-         sbt -no-colors -DfailOnWarning compile stage
+         sbt -DfailOnWarning compile stage
        fi
📝 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 (sbt)
run: |
if [ "${{ github.ref }}" == "refs/heads/master" ]; then
sbt -no-colors clean compile stage
else
sbt -no-colors -DfailOnWarning compile stage
fi
- name: Build webknossos (sbt)
run: |
echo "-J-Xmx4G" > .sbtopts
echo "-J-Xms2G" >> .sbtopts
echo "-Dsbt.parallel=true" >> .sbtopts
if [ "${{ github.ref }}" == "refs/heads/master" ]; then
sbt clean compile stage
else
sbt -DfailOnWarning compile stage
fi

fi

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

- name: Build webknossos-tracingstore (sbt)
run: sbt -no-colors -DfailOnWarning "project webknossosTracingstore" copyMessages compile stage

- 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: Set up Docker
uses: docker/setup-buildx-action@v2

- 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: 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: 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 +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: Build frontend documentation
run: |
WK_VERSION=${{ github.event.release.tag_name || github.run_number || 'dev' }}
Comment on lines +230 to +233
Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Improve documentation version handling

The current version fallback chain could be more explicit:

-          WK_VERSION=${{ github.event.release.tag_name || github.run_number || 'dev' }}
+          WK_VERSION=$(
+            if [ -n "${{ github.event.release.tag_name }}" ]; then
+              echo "${{ github.event.release.tag_name }}"
+            elif [ "${{ github.ref }}" = "refs/heads/master" ]; then
+              echo "${{ github.run_number }}"
+            else
+              echo "dev"
+            fi
+          )
📝 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 frontend documentation
run: |
WK_VERSION=${{ github.event.release.tag_name || github.run_number || 'dev' }}
yarn run docs --project-version $WK_VERSION
- name: Build frontend documentation
run: |
WK_VERSION=$(
if [ -n "${{ github.event.release.tag_name }}" ]; then
echo "${{ github.event.release.tag_name }}"
elif [ "${{ github.ref }}" = "refs/heads/master" ]; then
echo "${{ github.run_number }}"
else
echo "dev"
fi
)
yarn run docs --project-version $WK_VERSION

yarn run docs --project-version $WK_VERSION
Comment on lines +230 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

Add error handling to documentation build

The documentation build step should fail the workflow if there are any errors.

      - name: Build frontend documentation
        run: |
+         set -eo pipefail
          WK_VERSION=${{ github.event.release.tag_name || github.run_number || 'dev' }}
-         yarn run docs --project-version $WK_VERSION
+         yarn run docs --project-version "${WK_VERSION}"
📝 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 frontend documentation
run: |
WK_VERSION=${{ github.event.release.tag_name || github.run_number || 'dev' }}
yarn run docs --project-version $WK_VERSION
- name: Build frontend documentation
run: |
set -eo pipefail
WK_VERSION=${{ github.event.release.tag_name || github.run_number || 'dev' }}
yarn run docs --project-version "${WK_VERSION}"



- 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
1 change: 1 addition & 0 deletions Dockerfile
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ RUN curl -sL "https://deb.nodesource.com/setup_${VERSION_NODE}" | bash - \
RUN mkdir -p /webknossos
WORKDIR /webknossos

# Copy compiled Scala output from a previous build step, e.g. output of the Docker-dev image
COPY target/universal/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 validating the copied Scala artifacts.

While the COPY command is correct, consider adding validation to ensure the required Scala artifacts are present and complete.

Consider adding a validation step:

 # Copy compiled Scala output from a previous build step, e.g. output of the Docker-dev image
 COPY target/universal/stage .
+RUN test -f bin/webknossos && \
+    test -d lib || \
+    (echo "Error: Required Scala artifacts are missing" && exit 1)
📝 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
COPY target/universal/stage .
# Copy compiled Scala output from a previous build step, e.g. output of the Docker-dev image
COPY target/universal/stage .
RUN test -f bin/webknossos && \
test -d lib || \
(echo "Error: Required Scala artifacts are missing" && exit 1)


RUN addgroup --system --gid 999 webknossos \
Expand Down
6 changes: 2 additions & 4 deletions docker-compose.yml
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
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
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