-
Notifications
You must be signed in to change notification settings - Fork 24
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
base: master
Are you sure you want to change the base?
Changes from 17 commits
69a4e4c
4a9811e
d0eed33
53e35c4
53d1c23
eb8af0b
1d77e3c
03c1cbf
7acf3a9
9521869
633b9bf
267d646
188753d
47e66d6
f69e1c9
98337a4
da87a2a
a15f95c
5a2766f
35e7381
e81a452
e3bd804
d12c110
46aafa2
b91701d
c15cc6b
e3c6b4d
543bba1
f25cca0
e6de5cb
816989d
495296e
d9322ce
035f5a3
b162be2
6c06ca3
18b7cb4
77d54be
46f6696
e864dbb
c9f8868
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
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 | ||
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 |
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 | ||||||||||||||||||||||||||||||||||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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
Suggested change
|
||||||||||||||||||||||||||||||||||||||||||||||||||
USER_UID: 1000 | ||||||||||||||||||||||||||||||||||||||||||||||||||
USER_GID: 1000 | ||||||||||||||||||||||||||||||||||||||||||||||||||
DOCKER_USER: ${{ secrets.DOCKER_USER }} | ||||||||||||||||||||||||||||||||||||||||||||||||||
DOCKER_PASS: ${{ secrets.DOCKER_PASS }} | ||||||||||||||||||||||||||||||||||||||||||||||||||
Comment on lines
+19
to
+23
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Security: Avoid running containers as root user Running containers as root user (
- USER_NAME: root
+ USER_NAME: webknossos
Comment on lines
+18
to
+23
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Security: Critical configuration issues Several security concerns need to be addressed:
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 | ||||||||||||||||||||||||||||||||||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Align Ubuntu versions across jobs The 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
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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
Suggested change
|
||||||||||||||||||||||||||||||||||||||||||||||||||
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
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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
Suggested change
|
||||||||||||||||||||||||||||||||||||||||||||||||||
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 | ||||||||||||||||||||||||||||||||||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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
Suggested change
|
||||||||||||||||||||||||||||||||||||||||||||||||||
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
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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
Suggested change
|
||||||||||||||||||||||||||||||||||||||||||||||||||
- 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
+80
to
+90
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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
Suggested change
🧰 Tools🪛 actionlint
|
||||||||||||||||||||||||||||||||||||||||||||||||||
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 | ||||||||||||||||||||||||||||||||||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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:
These checks are important as evidenced by:
🔗 Analysis chainClean up commented code blocks Several steps are commented out but seem important for database schema validation. Either:
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 executedThe 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 | ||||||||||||||||||||||||||||||||||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🛠️ Refactor suggestion Optimize SBT build configuration The SBT build could be optimized:
- 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
Suggested change
|
||||||||||||||||||||||||||||||||||||||||||||||||||
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 | ||||||||||||||||||||||||||||||||||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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
Suggested change
🧰 Tools🪛 actionlint
|
||||||||||||||||||||||||||||||||||||||||||||||||||
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) | ||||||||||||||||||||||||||||||||||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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
Suggested change
🧰 Tools🪛 actionlint180-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 | ||||||||||||||||||||||||||||||||||||||||||||||||||
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 | ||||||||||||||||||||||||||||||||||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 - 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 | ||||||||||||||||||||||||||||||||||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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
🧰 Tools🪛 actionlint211-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 | ||||||||||||||||||||||||||||||||||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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:
- 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
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That sound great. Please do it! There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 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' }} | ||||||||||||||||||||||||||||||||||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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
Suggested change
|
||||||||||||||||||||||||||||||||||||||||||||||||||
yarn run docs --project-version $WK_VERSION | ||||||||||||||||||||||||||||||||||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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
Suggested change
|
||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||
- name: Report coverage | ||||||||||||||||||||||||||||||||||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Don't silently ignore coverage failures The coverage command ignores failures with Consider:
- 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
|
||||||||||||||||||||||||||||||||||||||||||||||||||
run: .github/not-on-master.sh docker compose run base yarn coverage || true | ||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||
# - name: Send Slack notification (master only) | ||||||||||||||||||||||||||||||||||||||||||||||||||
Comment on lines
+235
to
+236
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
Original file line number | Diff line number | Diff line change | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -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 . | ||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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
Suggested change
|
||||||||||||||
|
||||||||||||||
RUN addgroup --system --gid 999 webknossos \ | ||||||||||||||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,5 +1,3 @@ | ||
version: "2.2" | ||
|
||
services: | ||
# Production | ||
webknossos: | ||
|
@@ -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
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 💡 Codebase verification Environment variables need adjustment for GitHub Actions Based on the usage in
Suggested fix: - - CI_TAG=${GITHUB_REF}
- - CI_BUILD_NUM=${GITHUB_JOB}
+ - CI_TAG=${GITHUB_REF_NAME}
+ - CI_BUILD_NUM=${GITHUB_RUN_NUMBER}
🔗 Analysis chainReview environment variable mappings for GitHub Actions. The current mappings might not provide equivalent values:
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 executedThe 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 | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Enhance security and reliability of the health check implementation.
Consider the following improvements:
Apply this diff to implement the suggested improvements:
🧰 Tools
🪛 yamllint