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 46 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 33 commits
Commits
Show all changes
46 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
219a68f
Merge branch 'master' of github.com:scalableminds/webknossos into ci
hotzenklotz Dec 4, 2024
0be3a3b
Update .github/workflows/build_test_deploy.yml
hotzenklotz Dec 4, 2024
afb35b6
Update .github/workflows/build_test_deploy.yml
hotzenklotz Dec 4, 2024
d22dade
Update .github/workflows/build_test_deploy.yml
hotzenklotz Dec 4, 2024
c0c3db5
fix directory permissions
robert-oleynik Dec 4, 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
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
Contributor

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
238 changes: 235 additions & 3 deletions .github/workflows/build_test_deploy.yml
Original file line number Diff line number Diff line change
@@ -1,13 +1,245 @@
name: CI Pipeline

on:
push:
branches:
- master
pull_request:
branches:
- '*'
workflow_dispatch:
inputs:
debug_enabled:
type: boolean
description: 'Run the build with tmate debugging enabled (https://github.com/marketplace/actions/debugging-with-tmate)'
required: false
default: false
Comment on lines +11 to +16
Copy link
Contributor

Choose a reason for hiding this comment

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

💡 Codebase verification

Restrict debug access and protect sensitive data

The workflow contains several security concerns that need to be addressed:

  1. The workflow_dispatch trigger has no permission restrictions, allowing any user with repository access to enable debugging
  2. The tmate debugging session has no timeout configured
  3. The workflow exposes sensitive environment variables and credentials:
    • Database credentials (POSTGRES_PASSWORD)
    • Docker credentials (DOCKER_USER, DOCKER_PASS)
    • These are visible in logs and to anyone with debug access

Recommendations:

  • Add permissions field to restrict who can trigger workflow_dispatch
  • Configure tmate timeout using timeout-minutes
  • Move sensitive values to GitHub Actions secrets
  • Use secret masking for all credential environment variables
  • Add job-level timeout to limit maximum debug session duration
🔗 Analysis chain

Review debug access controls

The tmate debugging option could potentially expose sensitive information. Consider:

  1. Limiting the workflow_dispatch trigger to specific user roles
  2. Adding a timeout for debug sessions
  3. Documenting security implications in the workflow

Based on the search results, there are numerous files containing sensitive information like passwords, tokens, secrets, and credentials. Let me analyze the workflow file further to provide specific recommendations.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Check if there are any sensitive files that might be exposed during debug
rg -g '!.git' -l '(?i)(password|secret|key|token|credential)' || echo "No sensitive files found"

Length of output: 36548


Script:

#!/bin/bash
# Check if there are any environment variables or secrets used in the workflow
rg -g '.github/workflows/build_test_deploy.yml' '(?i)(secret|token|password|credential|key)'

# Check if there are any security controls on the workflow_dispatch trigger
cat .github/workflows/build_test_deploy.yml

Length of output: 8697

Comment on lines +4 to +16
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

Add security controls for workflow triggers

The workflow needs additional security controls:

  1. Add permissions to restrict access
  2. Add concurrency controls to prevent parallel runs
  3. Configure timeout for debug sessions

Add these security configurations:

 name: CI Pipeline

 on:
   push:
     branches:
       - master
   pull_request:
     branches:
       - '*'
   workflow_dispatch:
     inputs:
       debug_enabled:
         type: boolean
         description: 'Run the build with tmate debugging enabled'
         required: false
         default: false

+permissions:
+  contents: read
+  actions: write
+  pull-requests: write
+
+concurrency:
+  group: ${{ github.workflow }}-${{ github.ref }}
+  cancel-in-progress: true
+
+defaults:
+  run:
+    shell: bash
📝 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
push:
branches:
- master
pull_request:
branches:
- '*'
workflow_dispatch:
inputs:
debug_enabled:
type: boolean
description: 'Run the build with tmate debugging enabled (https://github.com/marketplace/actions/debugging-with-tmate)'
required: false
default: false
push:
branches:
- master
pull_request:
branches:
- '*'
workflow_dispatch:
inputs:
debug_enabled:
type: boolean
description: 'Run the build with tmate debugging enabled'
required: false
default: false
permissions:
contents: read
actions: write
pull-requests: write
concurrency:
group: ${{ github.workflow }}-${{ github.ref }}
cancel-in-progress: true
defaults:
run:
shell: bash


env:
USER_NAME: root
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

Security: Avoid running as root user

Running containers as root user is a security risk. Consider using a non-root user with minimal required permissions.

-  USER_NAME: root
+  USER_NAME: webknossos
📝 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
USER_NAME: root
USER_NAME: webknossos

USER_UID: 1000
USER_GID: 1000
DOCKER_USER: ${{ secrets.DOCKER_USER }}
DOCKER_PASS: ${{ secrets.DOCKER_PASS }}
Comment on lines +19 to +23
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

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.

Comment on lines +18 to +23
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

Security: Critical configuration issues

Several security concerns need to be addressed:

  1. Running as root user is a security risk
  2. Consider using GitHub's OIDC for Docker Hub authentication instead of storing credentials
 env:
-  USER_NAME: root
+  USER_NAME: webknossos
   USER_UID: 1000
   USER_GID: 1000
-  DOCKER_USER: ${{ secrets.DOCKER_USER }}
-  DOCKER_PASS: ${{ secrets.DOCKER_PASS }}

Consider using OIDC for Docker Hub:

permissions:
  id-token: write
  contents: read

jobs:
  build:
    steps:
      - uses: docker/login-action@v2
        with:
          registry: docker.io
          username: ${{ github.actor }}
          password: ${{ secrets.GITHUB_TOKEN }}


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

Choose a reason for hiding this comment

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

⚠️ Potential issue

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
Contributor

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
Contributor

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-24.04
env:
# The hostname used to communicate with the PostgreSQL service container
# POSTGRES_HOST: localhost
# POSTGRES_PORT: 5432
PGPASSWORD: secret_password
services:
postgres:
image: postgres:10-alpine
env:
POSTGRES_URL: jdbc:postgresql://postgres/webknossos
POSTGRES_USER: webknossos_user
Comment on lines +62 to +64
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

Security: Use secrets for database credentials

Database credentials should not be hardcoded in the workflow file.

 env:
-  POSTGRES_URL: jdbc:postgresql://postgres/webknossos
-  POSTGRES_USER: webknossos_user
-  POSTGRES_PASSWORD: secret_password
+  POSTGRES_URL: ${{ secrets.POSTGRES_URL }}
+  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_URL: jdbc:postgresql://postgres/webknossos
POSTGRES_USER: webknossos_user
POSTGRES_PASSWORD: secret_password
POSTGRES_URL: ${{ secrets.POSTGRES_URL }}
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
Contributor

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:
# Enable tmate debugging of manually-triggered workflows if the input option was provided

- 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
Contributor

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
uses: actions/setup-java@v4
with:
distribution: 'temurin' # See 'Supported distributions' for available options
java-version: '21'
cache: 'sbt'
- name: Set up sbt
uses: sbt/setup-sbt@v1

- 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: node tools/postgres/dbtool.js assert-unique-evolution-numbers

# - name: Assert schema.sql and evolutions are equal
# run: |
# docker compose up -d postgres
# sleep 3
Comment on lines +119 to +126
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

Uncomment and fix database validation steps

The commented-out database validation steps are critical for ensuring schema integrity.

-# - name: Assert unique evolution numbers
-#   run: node tools/postgres/dbtool.js assert-unique-evolution-numbers
+- name: Assert unique evolution numbers
+  run: node 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
+- 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
📝 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 unique evolution numbers
# run: node 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
- name: Assert unique evolution numbers
run: node tools/postgres/dbtool.js assert-unique-evolution-numbers
- 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

# docker compose run compile tools/postgres/dbtool.js check-evolutions-schema
- name: Setup tmate session
uses: mxschmitt/action-tmate@v3
if: ${{ github.event_name == 'workflow_dispatch' && inputs.debug_enabled }}

- 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
Copy link
Contributor

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

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
env:
POSTGRES_URL: jdbc:postgresql://webknossos_user:secret_password@localhost:5432/webknossos
POSTGRES_USER: webknossos_user
POSTGRES_PASSWORD: secret_password

- 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
env:
POSTGRES_URL: jdbc:postgresql://webknossos_user@localhost:5432/webknossos
POSTGRES_USER: webknossos_user
PGUSER: webknossos_user
POSTGRES_PASSWORD: secret_password
PGPASSWORD: secret_password

- 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: 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
Contributor

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: Start webknossos, datastore, and tracingstore
hotzenklotz marked this conversation as resolved.
Show resolved Hide resolved
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
Copy link
Contributor

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

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
Contributor

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
Contributor

@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
Contributor

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' }}
Copy link
Contributor

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Improve 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
Copy link
Contributor

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

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
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

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 +237 to +238
Copy link
Contributor

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

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
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
Contributor

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
Contributor

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