Convert CircleCI config to Github Actions #8147
Conversation
📝 WalkthroughWalkthroughThe changes update the CI/CD pipeline and Docker configurations. The GitHub workflow file now triggers on push and pull requests, adds environment variables, and splits tests into separate jobs. New Dockerfiles and a Docker Compose file defining multiple services have been introduced, while obsolete CircleCI configurations, development Dockerfiles, and Docker startup scripts were removed. Minor code improvements and updates to CI build information are also included. Changes
Poem
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (2)
.github/workflows/build_test_deploy.yml (2)
4-9: Consider restricting branch patternsThe current trigger configuration runs on all branches (
'*'). This might lead to unnecessary workflow runs. Consider:
- Limiting to specific branch patterns (e.g.,
main,develop,feature/*)- Using path filters to run only when relevant files change
push: branches: - - '*' + - main + - develop + - 'feature/**' + paths-ignore: + - '**.md' + - 'docs/**' pull_request: branches: - - '*' + - main + - develop
12-18: Enhance security of Docker credentialsThe Docker credentials are exposed as environment variables. While they are using secrets, consider:
- Limiting the scope of these credentials to only the job that needs them
- Using GITHUB_TOKEN where possible for container registry authentication
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- .github/not-on-master.sh (1 hunks)
- .github/workflows/build_test_deploy.yml (1 hunks)
🧰 Additional context used
🪛 actionlint
.github/workflows/build_test_deploy.yml
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)
72-72: shellcheck reported issue in this script: SC2086:info:1:42: Double quote to prevent globbing and word splitting
(shellcheck)
117-117: shellcheck reported issue in this script: SC2086:info:1:38: Double quote to prevent globbing and word splitting
(shellcheck)
137-137: shellcheck reported issue in this script: SC2034:warning:1:1: i appears unused. Verify use (or export if used externally)
(shellcheck)
137-137: shellcheck reported issue in this script: SC2086:info:4:7: Double quote to prevent globbing and word splitting
(shellcheck)
153-153: shellcheck reported issue in this script: SC2034:warning:1:1: i appears unused. Verify use (or export if used externally)
(shellcheck)
153-153: shellcheck reported issue in this script: SC2086:info:5:7: Double quote to prevent globbing and word splitting
(shellcheck)
161-161: shellcheck reported issue in this script: SC2034:warning:1:1: i appears unused. Verify use (or export if used externally)
(shellcheck)
161-161: shellcheck reported issue in this script: SC2086:info:5:7: Double quote to prevent globbing and word splitting
(shellcheck)
169-169: shellcheck reported issue in this script: SC2034:warning:1:1: i appears unused. Verify use (or export if used externally)
(shellcheck)
169-169: shellcheck reported issue in this script: SC2086:info:5:7: Double quote to prevent globbing and word splitting
(shellcheck)
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)
🔇 Additional comments (4)
.github/not-on-master.sh (3)
1-2: LGTM: Robust shell script configuration.The script follows shell scripting best practices:
- Uses proper shebang line for portability
- Sets appropriate safety flags:
-E: ERR trap inheritance-e: Exit on error-u: Error on unbound variables-o pipefail: Propagate pipe failures
7-7: Consider adding command validation.The
exec "$@"executes arbitrary commands passed as arguments without validation.Consider:
- Adding command whitelisting
- Implementing logging for audit purposes
- Adding error handling for failed commands
4-8:⚠️ Potential issueFix branch reference comparison.
The current branch comparison has potential issues:
GITHUB_REFtypically includes the full ref path (e.g., "refs/heads/master")- String comparison should use
=instead of==for POSIX compatibilityApply this fix:
-if [ "${GITHUB_REF}" == "master" ]; then +if [ "${GITHUB_REF}" = "refs/heads/master" ]; thenAdditionally, consider adding input validation:
.github/workflows/build_test_deploy.yml (1)
21-43: LGTM! Well-structured frontend checksThe frontend code checks job is well-organized with:
- Proper Node.js setup
- Comprehensive checks (lint, types, cyclic deps)
- Clear step names
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Outside diff range and nitpick comments (4)
.github/actions/health_check_action.yml (1)
16-16: Add newline at end of file.Following YAML best practices, ensure there's a newline character at the end of the file.
🧰 Tools
🪛 yamllint
[error] 16-16: no new line character at the end of file
(new-line-at-end-of-file)
project/BuildInfoSettings.scala (1)
16-17: Consider documenting CI environment variables.Since these environment variables are crucial for versioning and build information, consider adding documentation about their expected values and usage.
Add a comment block above the variable declarations:
+ // CI environment variables used for versioning: + // CI_BUILD_NUM: Build number from CI system (e.g., GitHub Actions run number) + // CI_TAG: Git tag from CI system, if building a tagged commit val ciBuild: String = 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 "".github/workflows/build_test_deploy.yml (1)
28-30: Fix YAML indentationThe indentation is inconsistent with the rest of the file.
- uses: actions/setup-node@v4 with: - node-version: 18 + node-version: 18🧰 Tools
🪛 yamllint
[warning] 30-30: wrong indentation: expected 8 but found 10
(indentation)
docker-compose.yml (1)
Line range hint
249-257: Fix syntax error in dev service command.There's an extra quote at the end of the
-Ddatastore.redis.address=redisline which will cause the command to fail.Apply this fix:
command: - bash - -c - > sbt -v -d -jvm-debug 5005 "run -Djava.net.preferIPv4Stack=true -Dtracingstore.fossildb.address=fossildb -Dtracingstore.redis.address=redis" - -Ddatastore.redis.address=redis" + -Ddatastore.redis.address=redis
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
- .github/actions/health_check_action.yml (1 hunks)
- .github/workflows/build_test_deploy.yml (1 hunks)
- docker-compose.yml (1 hunks)
- project/BuildInfoSettings.scala (1 hunks)
🧰 Additional context used
🪛 yamllint
.github/actions/health_check_action.yml
[error] 16-16: no new line character at the end of file
(new-line-at-end-of-file)
.github/workflows/build_test_deploy.yml
[warning] 23-23: wrong indentation: expected 6 but found 4
(indentation)
[warning] 30-30: wrong indentation: expected 8 but found 10
(indentation)
[warning] 46-46: too many blank lines
(4 > 2) (empty-lines)
[error] 153-153: trailing spaces
(trailing-spaces)
[error] 155-155: trailing spaces
(trailing-spaces)
[error] 159-159: trailing spaces
(trailing-spaces)
[error] 164-164: trailing spaces
(trailing-spaces)
🪛 actionlint
.github/workflows/build_test_deploy.yml
56-56: shellcheck reported issue in this script: SC2086:info:3:50: Double quote to prevent globbing and word splitting
(shellcheck)
56-56: shellcheck reported issue in this script: SC2086:info:5:36: Double quote to prevent globbing and word splitting
(shellcheck)
56-56: shellcheck reported issue in this script: SC2086:info:7:38: Double quote to prevent globbing and word splitting
(shellcheck)
56-56: shellcheck reported issue in this script: SC2086:info:8:47: Double quote to prevent globbing and word splitting
(shellcheck)
71-71: shellcheck reported issue in this script: SC2086:info:1:42: Double quote to prevent globbing and word splitting
(shellcheck)
116-116: shellcheck reported issue in this script: SC2086:info:1:38: Double quote to prevent globbing and word splitting
(shellcheck)
136-136: shellcheck reported issue in this script: SC2034:warning:1:1: i appears unused. Verify use (or export if used externally)
(shellcheck)
136-136: shellcheck reported issue in this script: SC2086:info:4:7: Double quote to prevent globbing and word splitting
(shellcheck)
171-171: shellcheck reported issue in this script: SC2034:warning:2:3: i appears unused. Verify use (or export if used externally)
(shellcheck)
171-171: shellcheck reported issue in this script: SC2086:info:6:10: Double quote to prevent globbing and word splitting
(shellcheck)
171-171: shellcheck reported issue in this script: SC2086:info:8:23: Double quote to prevent globbing and word splitting
(shellcheck)
171-171: shellcheck reported issue in this script: SC2086:info:8:39: Double quote to prevent globbing and word splitting
(shellcheck)
171-171: shellcheck reported issue in this script: SC2086:info:13:39: Double quote to prevent globbing and word splitting
(shellcheck)
171-171: shellcheck reported issue in this script: SC2086:info:13:78: Double quote to prevent globbing and word splitting
(shellcheck)
171-171: shellcheck reported issue in this script: SC2086:info:14:46: Double quote to prevent globbing and word splitting
(shellcheck)
171-171: shellcheck reported issue in this script: SC2086:info:15:49: Double quote to prevent globbing and word splitting
(shellcheck)
171-171: shellcheck reported issue in this script: SC2086:info:15:98: Double quote to prevent globbing and word splitting
(shellcheck)
171-171: shellcheck reported issue in this script: SC2086:info:16:56: Double quote to prevent globbing and word splitting
(shellcheck)
171-171: shellcheck reported issue in this script: SC2086:info:17:52: Double quote to prevent globbing and word splitting
(shellcheck)
171-171: shellcheck reported issue in this script: SC2086:info:17:104: Double quote to prevent globbing and word splitting
(shellcheck)
171-171: shellcheck reported issue in this script: SC2086:info:18:59: Double quote to prevent globbing and word splitting
(shellcheck)
171-171: shellcheck reported issue in this script: SC2086:info:19:72: Double quote to prevent globbing and word splitting
(shellcheck)
171-171: shellcheck reported issue in this script: SC2086:info:20:50: Double quote to prevent globbing and word splitting
(shellcheck)
🔇 Additional comments (2)
.github/actions/health_check_action.yml (1)
1-6: LGTM! Clear and well-structured action metadata.The action name, description, and input parameters are well-defined and follow GitHub Actions best practices.
project/BuildInfoSettings.scala (1)
16-17: LGTM! Verify environment variables in GitHub Actions workflow.The change from CircleCI-specific environment variables to generic CI variables is a good practice for portability.
Let's verify that these environment variables are properly set in the GitHub Actions workflow:
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (3)
.github/workflows/build_test_deploy.yml (3)
23-23: Fix YAML indentation issuesThere are inconsistent indentation levels in the workflow file:
- Line 23: Should be 6 spaces instead of 4
- Line 30: Should be 8 spaces instead of 10
steps: - - name: Checkout code + - name: Checkout code uses: actions/checkout@v3 with: fetch-depth: 5 - uses: actions/setup-node@v4 with: - node-version: 18 + node-version: 18Also applies to: 30-30
🧰 Tools
🪛 yamllint
[warning] 23-23: wrong indentation: expected 6 but found 4
(indentation)
44-46: Remove excessive blank linesThere are too many consecutive blank lines (4 > 2) between the jobs.
- name: Check for cyclic dependencies in frontend run: yarn check-cyclic-dependencies - - build_test_deploy:🧰 Tools
🪛 yamllint
[warning] 46-46: too many blank lines
(4 > 2) (empty-lines)
153-165: Fix trailing spaces in YAMLRemove trailing spaces from the following lines:
- Line 153:
with:- Line 155: Empty line with spaces
- Line 159:
with:- Line 164:
with:- name: Run webknossos smoke test uses: ./.github/actions/health_check_action - with: + with: url: http://localhost:9000/api/health - name: Run webknossos-datastore smoke test uses: ./.github/actions/health_check_action - with: + with: url: http://localhost:9090/data/health - name: Run webknossos-tracingstore smoke test uses: ./.github/actions/health_check_action - with: + with: url: http://localhost:9050/tracings/health🧰 Tools
🪛 yamllint
[error] 153-153: trailing spaces
(trailing-spaces)
[error] 155-155: trailing spaces
(trailing-spaces)
[error] 159-159: trailing spaces
(trailing-spaces)
[error] 164-164: trailing spaces
(trailing-spaces)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (2)
- .github/workflows/build_test_deploy.yml (1 hunks)
- docker-compose.yml (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- docker-compose.yml
🧰 Additional context used
🪛 actionlint
.github/workflows/build_test_deploy.yml
56-56: shellcheck reported issue in this script: SC2086:info:3:50: Double quote to prevent globbing and word splitting
(shellcheck)
56-56: shellcheck reported issue in this script: SC2086:info:5:36: Double quote to prevent globbing and word splitting
(shellcheck)
56-56: shellcheck reported issue in this script: SC2086:info:7:38: Double quote to prevent globbing and word splitting
(shellcheck)
56-56: shellcheck reported issue in this script: SC2086:info:8:47: Double quote to prevent globbing and word splitting
(shellcheck)
71-71: shellcheck reported issue in this script: SC2086:info:1:42: Double quote to prevent globbing and word splitting
(shellcheck)
116-116: shellcheck reported issue in this script: SC2086:info:1:38: Double quote to prevent globbing and word splitting
(shellcheck)
136-136: shellcheck reported issue in this script: SC2034:warning:1:1: i appears unused. Verify use (or export if used externally)
(shellcheck)
136-136: shellcheck reported issue in this script: SC2086:info:4:7: Double quote to prevent globbing and word splitting
(shellcheck)
171-171: shellcheck reported issue in this script: SC2034:warning:2:3: i appears unused. Verify use (or export if used externally)
(shellcheck)
171-171: shellcheck reported issue in this script: SC2086:info:6:10: Double quote to prevent globbing and word splitting
(shellcheck)
171-171: shellcheck reported issue in this script: SC2086:info:8:23: Double quote to prevent globbing and word splitting
(shellcheck)
171-171: shellcheck reported issue in this script: SC2086:info:8:39: Double quote to prevent globbing and word splitting
(shellcheck)
171-171: shellcheck reported issue in this script: SC2086:info:13:39: Double quote to prevent globbing and word splitting
(shellcheck)
171-171: shellcheck reported issue in this script: SC2086:info:13:78: Double quote to prevent globbing and word splitting
(shellcheck)
171-171: shellcheck reported issue in this script: SC2086:info:14:46: Double quote to prevent globbing and word splitting
(shellcheck)
171-171: shellcheck reported issue in this script: SC2086:info:15:49: Double quote to prevent globbing and word splitting
(shellcheck)
171-171: shellcheck reported issue in this script: SC2086:info:15:98: Double quote to prevent globbing and word splitting
(shellcheck)
171-171: shellcheck reported issue in this script: SC2086:info:16:56: Double quote to prevent globbing and word splitting
(shellcheck)
171-171: shellcheck reported issue in this script: SC2086:info:17:52: Double quote to prevent globbing and word splitting
(shellcheck)
171-171: shellcheck reported issue in this script: SC2086:info:17:104: Double quote to prevent globbing and word splitting
(shellcheck)
171-171: shellcheck reported issue in this script: SC2086:info:18:59: Double quote to prevent globbing and word splitting
(shellcheck)
171-171: shellcheck reported issue in this script: SC2086:info:19:72: Double quote to prevent globbing and word splitting
(shellcheck)
171-171: shellcheck reported issue in this script: SC2086:info:20:50: Double quote to prevent globbing and word splitting
(shellcheck)
🪛 yamllint
.github/workflows/build_test_deploy.yml
[warning] 23-23: wrong indentation: expected 6 but found 4
(indentation)
[warning] 30-30: wrong indentation: expected 8 but found 10
(indentation)
[warning] 46-46: too many blank lines
(4 > 2) (empty-lines)
[error] 153-153: trailing spaces
(trailing-spaces)
[error] 155-155: trailing spaces
(trailing-spaces)
[error] 159-159: trailing spaces
(trailing-spaces)
[error] 164-164: trailing spaces
(trailing-spaces)
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (1)
.github/workflows/build_test_deploy.yml (1)
13-15: Remove or document commented environment variablesThe commented user-related environment variables (
USER_NAME,USER_UID,USER_GID) should either be removed if they're no longer needed or documented if they serve a specific purpose.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- .github/workflows/build_test_deploy.yml (1 hunks)
🧰 Additional context used
🪛 actionlint
.github/workflows/build_test_deploy.yml
56-56: shellcheck reported issue in this script: SC2086:info:3:50: Double quote to prevent globbing and word splitting
(shellcheck)
56-56: shellcheck reported issue in this script: SC2086:info:5:36: Double quote to prevent globbing and word splitting
(shellcheck)
56-56: shellcheck reported issue in this script: SC2086:info:7:38: Double quote to prevent globbing and word splitting
(shellcheck)
56-56: shellcheck reported issue in this script: SC2086:info:8:47: Double quote to prevent globbing and word splitting
(shellcheck)
71-71: shellcheck reported issue in this script: SC2086:info:1:42: Double quote to prevent globbing and word splitting
(shellcheck)
116-116: shellcheck reported issue in this script: SC2086:info:1:38: Double quote to prevent globbing and word splitting
(shellcheck)
136-136: shellcheck reported issue in this script: SC2034:warning:1:1: i appears unused. Verify use (or export if used externally)
(shellcheck)
136-136: shellcheck reported issue in this script: SC2086:info:4:7: Double quote to prevent globbing and word splitting
(shellcheck)
171-171: shellcheck reported issue in this script: SC2034:warning:2:3: i appears unused. Verify use (or export if used externally)
(shellcheck)
171-171: shellcheck reported issue in this script: SC2086:info:6:10: Double quote to prevent globbing and word splitting
(shellcheck)
171-171: shellcheck reported issue in this script: SC2086:info:8:23: Double quote to prevent globbing and word splitting
(shellcheck)
171-171: shellcheck reported issue in this script: SC2086:info:8:39: Double quote to prevent globbing and word splitting
(shellcheck)
171-171: shellcheck reported issue in this script: SC2086:info:13:39: Double quote to prevent globbing and word splitting
(shellcheck)
171-171: shellcheck reported issue in this script: SC2086:info:13:78: Double quote to prevent globbing and word splitting
(shellcheck)
171-171: shellcheck reported issue in this script: SC2086:info:14:46: Double quote to prevent globbing and word splitting
(shellcheck)
171-171: shellcheck reported issue in this script: SC2086:info:15:49: Double quote to prevent globbing and word splitting
(shellcheck)
171-171: shellcheck reported issue in this script: SC2086:info:15:98: Double quote to prevent globbing and word splitting
(shellcheck)
171-171: shellcheck reported issue in this script: SC2086:info:16:56: Double quote to prevent globbing and word splitting
(shellcheck)
171-171: shellcheck reported issue in this script: SC2086:info:17:52: Double quote to prevent globbing and word splitting
(shellcheck)
171-171: shellcheck reported issue in this script: SC2086:info:17:104: Double quote to prevent globbing and word splitting
(shellcheck)
171-171: shellcheck reported issue in this script: SC2086:info:18:59: Double quote to prevent globbing and word splitting
(shellcheck)
171-171: shellcheck reported issue in this script: SC2086:info:19:72: Double quote to prevent globbing and word splitting
(shellcheck)
171-171: shellcheck reported issue in this script: SC2086:info:20:50: Double quote to prevent globbing and word splitting
(shellcheck)
🪛 yamllint
.github/workflows/build_test_deploy.yml
[warning] 23-23: wrong indentation: expected 6 but found 4
(indentation)
[warning] 30-30: wrong indentation: expected 8 but found 10
(indentation)
[warning] 46-46: too many blank lines
(4 > 2) (empty-lines)
[error] 153-153: trailing spaces
(trailing-spaces)
[error] 155-155: trailing spaces
(trailing-spaces)
[error] 159-159: trailing spaces
(trailing-spaces)
[error] 164-164: trailing spaces
(trailing-spaces)
🔇 Additional comments (1)
.github/workflows/build_test_deploy.yml (1)
20-42: LGTM: Frontend checks are well-structuredThe frontend checks job is well-organized with appropriate steps for dependency installation, linting, type checking, and cyclic dependency validation.
🧰 Tools
🪛 yamllint
[warning] 23-23: wrong indentation: expected 6 but found 4
(indentation)
[warning] 30-30: wrong indentation: expected 8 but found 10
(indentation)
There was a problem hiding this comment.
Actionable comments posted: 4
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- .github/workflows/build_test_deploy.yml (1 hunks)
- Dockerfile (1 hunks)
🧰 Additional context used
🪛 actionlint
.github/workflows/build_test_deploy.yml
54-54: shellcheck reported issue in this script: SC2086:info:3:50: Double quote to prevent globbing and word splitting
(shellcheck)
54-54: shellcheck reported issue in this script: SC2086:info:5:36: Double quote to prevent globbing and word splitting
(shellcheck)
54-54: shellcheck reported issue in this script: SC2086:info:7:38: Double quote to prevent globbing and word splitting
(shellcheck)
54-54: shellcheck reported issue in this script: SC2086:info:8:47: Double quote to prevent globbing and word splitting
(shellcheck)
131-131: shellcheck reported issue in this script: SC2086:info:1:38: Double quote to prevent globbing and word splitting
(shellcheck)
151-151: shellcheck reported issue in this script: SC2034:warning:1:1: i appears unused. Verify use (or export if used externally)
(shellcheck)
151-151: shellcheck reported issue in this script: SC2086:info:4:7: Double quote to prevent globbing and word splitting
(shellcheck)
183-183: shellcheck reported issue in this script: SC2034:warning:2:3: i appears unused. Verify use (or export if used externally)
(shellcheck)
183-183: shellcheck reported issue in this script: SC2086:info:6:10: Double quote to prevent globbing and word splitting
(shellcheck)
183-183: shellcheck reported issue in this script: SC2086:info:8:23: Double quote to prevent globbing and word splitting
(shellcheck)
183-183: shellcheck reported issue in this script: SC2086:info:8:39: Double quote to prevent globbing and word splitting
(shellcheck)
183-183: shellcheck reported issue in this script: SC2086:info:13:39: Double quote to prevent globbing and word splitting
(shellcheck)
183-183: shellcheck reported issue in this script: SC2086:info:13:78: Double quote to prevent globbing and word splitting
(shellcheck)
183-183: shellcheck reported issue in this script: SC2086:info:14:46: Double quote to prevent globbing and word splitting
(shellcheck)
183-183: shellcheck reported issue in this script: SC2086:info:15:49: Double quote to prevent globbing and word splitting
(shellcheck)
183-183: shellcheck reported issue in this script: SC2086:info:15:98: Double quote to prevent globbing and word splitting
(shellcheck)
183-183: shellcheck reported issue in this script: SC2086:info:16:56: Double quote to prevent globbing and word splitting
(shellcheck)
183-183: shellcheck reported issue in this script: SC2086:info:17:52: Double quote to prevent globbing and word splitting
(shellcheck)
183-183: shellcheck reported issue in this script: SC2086:info:17:104: Double quote to prevent globbing and word splitting
(shellcheck)
183-183: shellcheck reported issue in this script: SC2086:info:18:59: Double quote to prevent globbing and word splitting
(shellcheck)
183-183: shellcheck reported issue in this script: SC2086:info:19:72: Double quote to prevent globbing and word splitting
(shellcheck)
183-183: shellcheck reported issue in this script: SC2086:info:20:50: Double quote to prevent globbing and word splitting
(shellcheck)
🪛 yamllint
.github/workflows/build_test_deploy.yml
[warning] 23-23: wrong indentation: expected 6 but found 4
(indentation)
[error] 70-70: trailing spaces
(trailing-spaces)
[error] 80-80: trailing spaces
(trailing-spaces)
[error] 165-165: trailing spaces
(trailing-spaces)
[error] 167-167: trailing spaces
(trailing-spaces)
[error] 171-171: trailing spaces
(trailing-spaces)
[error] 176-176: trailing spaces
(trailing-spaces)
🔇 Additional comments (2)
Dockerfile (1)
11-11: Please clarify the Docker-dev image reference in the comment.The comment mentions "Docker-dev image" but this dependency isn't clearly documented. This could make it difficult for other developers to understand the build prerequisites.
Let's check for any Docker-dev image references:
✅ Verification successful
Let me check the docker-compose files to understand if there's a dev image defined:
Let me check the Dockerfile.dev since I found a reference to webknossos-dev image in docker-compose.yml:
Based on the docker-compose.yml and Dockerfile.dev analysis, I can now generate the final response:
The Docker-dev image reference is accurate and well-integrated
The comment refers to the
webknossos-devimage which is properly defined in docker-compose.yml under thebaseservice. This dev image is built from Dockerfile.dev and is used for development and compilation tasks. The compiled Scala output from this dev environment is indeed used in the production Dockerfile, making the comment accurate and helpful.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for Docker-dev image references in the repository rg -i "docker-dev.*image" # Check docker-compose files for related services fd -e yml -e yaml | xargs rg -l "docker"Length of output: 329
Script:
#!/bin/bash # Check contents of docker-compose files for dev image definitions echo "=== Main docker-compose.yml ===" cat docker-compose.yml echo -e "\n=== tools/hosting/docker-compose.yml ===" cat tools/hosting/docker-compose.yml echo -e "\n=== webknossos-tracingstore/deployment/docker-compose.yml ===" cat webknossos-tracingstore/deployment/docker-compose.yml echo -e "\n=== webknossos-datastore/deployment/docker-compose.yml ===" cat webknossos-datastore/deployment/docker-compose.ymlLength of output: 14631
Script:
#!/bin/bash # Check Dockerfile.dev contents cat Dockerfile.devLength of output: 1079
.github/workflows/build_test_deploy.yml (1)
4-6: Consider including additional protected branches in push triggersThe push trigger is currently limited to the
masterbranch. Consider including other protected branches (e.g.,main,develop,release/*) to ensure the pipeline runs on all important branches.
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Outside diff range and nitpick comments (1)
.github/workflows/build_test_deploy.yml (1)
12-17: Add timezone environment variableConsider adding
TZ: UTC(or your preferred timezone) to ensure consistent timestamps across the pipeline execution.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- .github/workflows/build_test_deploy.yml (1 hunks)
🧰 Additional context used
🪛 actionlint
.github/workflows/build_test_deploy.yml
73-73: shellcheck reported issue in this script: SC2086:info:3:50: Double quote to prevent globbing and word splitting
(shellcheck)
73-73: shellcheck reported issue in this script: SC2086:info:5:36: Double quote to prevent globbing and word splitting
(shellcheck)
73-73: shellcheck reported issue in this script: SC2086:info:7:38: Double quote to prevent globbing and word splitting
(shellcheck)
73-73: shellcheck reported issue in this script: SC2086:info:8:47: Double quote to prevent globbing and word splitting
(shellcheck)
141-141: shellcheck reported issue in this script: SC2086:info:1:38: Double quote to prevent globbing and word splitting
(shellcheck)
158-158: shellcheck reported issue in this script: SC2034:warning:1:1: i appears unused. Verify use (or export if used externally)
(shellcheck)
158-158: shellcheck reported issue in this script: SC2086:info:4:7: Double quote to prevent globbing and word splitting
(shellcheck)
189-189: shellcheck reported issue in this script: SC2034:warning:2:3: i appears unused. Verify use (or export if used externally)
(shellcheck)
189-189: shellcheck reported issue in this script: SC2086:info:6:10: Double quote to prevent globbing and word splitting
(shellcheck)
189-189: shellcheck reported issue in this script: SC2086:info:8:23: Double quote to prevent globbing and word splitting
(shellcheck)
189-189: shellcheck reported issue in this script: SC2086:info:8:39: Double quote to prevent globbing and word splitting
(shellcheck)
189-189: shellcheck reported issue in this script: SC2086:info:13:39: Double quote to prevent globbing and word splitting
(shellcheck)
189-189: shellcheck reported issue in this script: SC2086:info:13:78: Double quote to prevent globbing and word splitting
(shellcheck)
189-189: shellcheck reported issue in this script: SC2086:info:14:46: Double quote to prevent globbing and word splitting
(shellcheck)
189-189: shellcheck reported issue in this script: SC2086:info:15:49: Double quote to prevent globbing and word splitting
(shellcheck)
189-189: shellcheck reported issue in this script: SC2086:info:15:98: Double quote to prevent globbing and word splitting
(shellcheck)
189-189: shellcheck reported issue in this script: SC2086:info:16:56: Double quote to prevent globbing and word splitting
(shellcheck)
189-189: shellcheck reported issue in this script: SC2086:info:17:52: Double quote to prevent globbing and word splitting
(shellcheck)
189-189: shellcheck reported issue in this script: SC2086:info:17:104: Double quote to prevent globbing and word splitting
(shellcheck)
189-189: shellcheck reported issue in this script: SC2086:info:18:59: Double quote to prevent globbing and word splitting
(shellcheck)
189-189: shellcheck reported issue in this script: SC2086:info:19:72: Double quote to prevent globbing and word splitting
(shellcheck)
189-189: shellcheck reported issue in this script: SC2086:info:20:50: Double quote to prevent globbing and word splitting
(shellcheck)
🪛 yamllint
.github/workflows/build_test_deploy.yml
[warning] 23-23: wrong indentation: expected 6 but found 4
(indentation)
[warning] 52-52: wrong indentation: expected 6 but found 8
(indentation)
[error] 89-89: trailing spaces
(trailing-spaces)
[error] 99-99: trailing spaces
(trailing-spaces)
[error] 172-172: trailing spaces
(trailing-spaces)
[error] 174-174: trailing spaces
(trailing-spaces)
[error] 177-177: trailing spaces
(trailing-spaces)
[error] 182-182: trailing spaces
(trailing-spaces)
normanrz
changed the title
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
docker-compose.yml (1)
1-2: 💡 Verification agent🧩 Analysis chain
Mismatch in New Docker Compose File Location
The comment instructs users to use
tools/hosting/docker-compose.yml, but the PR documentation and related changes indicate that the new Docker Compose file is located at.github/docker-compose.yml. Please verify and update the comment to point to the correct file location.
Below is a shell script to help verify which Docker Compose file actually exists in the repository:
Action Required: Update Docker Compose File Reference
- The note in
docker-compose.ymlcurrently instructs users to usetools/hosting/docker-compose.yml.- Verification based on the current PR documentation shows that the new Docker Compose file is located at
.github/docker-compose.yml.- Please update the comment in
docker-compose.yml(lines 1-2) to reference the correct file location:.github/docker-compose.yml.
🏁 Script executed:
#!/bin/bash # Check for the new Docker Compose file location if [ -f ".github/docker-compose.yml" ]; then echo "New docker-compose file exists at .github/docker-compose.yml" else echo "New docker-compose file NOT found at .github/docker-compose.yml" fi # Check for the old Docker Compose file location mentioned in the comment if [ -f "tools/hosting/docker-compose.yml" ]; then echo "Old docker-compose file exists at tools/hosting/docker-compose.yml" else echo "Old docker-compose file NOT found at tools/hosting/docker-compose.yml" fiLength of output: 481
Update Docker Compose Reference to Reflect New File Location
- In docker-compose.yml (lines 1-2), the comment currently directs users to use
tools/hosting/docker-compose.yml.- Verification shows that the new Docker Compose file is located at
.github/docker-compose.yml.- Action: Update the comment in docker-compose.yml to instruct users to use
.github/docker-compose.ymlinstead.- # NOTE: This file is no longer used. Please, use `tools/hosting/docker-compose.yml` instead. + # NOTE: This file is no longer used. Please, use `.github/docker-compose.yml` instead.
♻️ Duplicate comments (1)
.github/workflows/build_test_deploy.yml (1)
189-190: 🛠️ Refactor suggestion
⚠️ Potential issueRestrict Filesystem Permissions for binaryData Folder
The command to prepare the binaryData folder useschmod 777, which is overly permissive and may expose the folder to unintended write access. It is advisable to use a more restricted permission mode, such as755.- mkdir -p binaryData/Organization_X && chmod 777 binaryData/Organization_X + mkdir -p binaryData/Organization_X && chmod 755 binaryData/Organization_X
🧹 Nitpick comments (3)
.github/workflows/build_test_deploy.yml (3)
105-116: Custom Environment Variables: Verify DOCKER_TAG Generation Logic
The custom environment variables block correctly normalizes the branch name and constructs aDOCKER_TAGusing${{ github.run_id }}. Consider verifying that usinggithub.run_id(a unique run identifier) meets your sequential build numbering requirements. In some cases, you might prefer${{ github.run_number }}for a simpler sequential build number.
139-144: SBT Build Step: Verify Branch-Dependent Commands
The SBT build step conditionally runs a clean compile and stage on master versus a warning-sensitive compile on other branches. Please verify that usingif [ "${{ github.ref_name }}" == 'master' ]; then sbt clean compile stage else sbt -DfailOnWarning compile stage fimeets your intended CI strategy for production versus development builds.
1-1: Overall Workflow Configuration
The revised GitHub Actions configuration demonstrates a significant improvement over the previous CircleCI setup. The jobs are logically segmented for frontend tests, backend tests, and build–test–e2e–deploy. In addition to the comments above, consider reviewing the following optional improvements:
- Caching: You might add caching for Node.js dependencies (e.g. using
actions/cache@v3) to further improve build performance.- Permissions: Although the workflow currently triggers only on push and pull_request (which is secure), if you plan to add manual triggers (workflow_dispatch) in the future, consider a top‑level
permissionsblock to limit who can trigger debug sessions.
Overall, excellent progress on integrating GitHub Actions into your CI/CD pipeline.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
.circleci/config.yml(0 hunks).circleci/not-on-master.sh(0 hunks).circleci/slack-notification.sh(0 hunks).github/workflows/build_test_deploy.yml(1 hunks)Dockerfile.dev(0 hunks)docker-compose.yml(1 hunks)start-docker.sh(0 hunks)
💤 Files with no reviewable changes (5)
- .circleci/not-on-master.sh
- Dockerfile.dev
- start-docker.sh
- .circleci/config.yml
- .circleci/slack-notification.sh
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: backend-tests
- GitHub Check: build-test-e2e-deploy
🔇 Additional comments (5)
.github/workflows/build_test_deploy.yml (5)
21-24: Verify Branch Filter for Frontend Tests Job
The frontend-tests job is conditioned to run only when the branch is not master (if: ${{ github.ref_name != 'master' }}). Please confirm that you intend to skip frontend tests on the master branch.
45-47: Confirm Backend Tests Branch Condition
Similarly, the backend-tests job is gated withif: ${{ github.ref_name != 'master' }}. Verify that backend tests are not intended to run on master—or update the condition if tests should be run for every commit regardless of branch.
153-184: Docker Images Build Steps Look Good
The docker build steps for webknossos, datastore, and tracingstore correctly usedocker/build-push-action@v6with proper context and tagging (using both${{ env.NORMALIZED_BRANCH }}and${{ env.DOCKER_TAG }}). No immediate issues were observed here.
234-269: Docker Push Commands Explicitly Quote Variables
The steps pushing docker images (for the three services) already use explicit quoting (e.g."scalableminds/webknosos:${{ env.DOCKER_TAG }}"). This is a good practice that helps prevent word‐splitting issues.
270-275: Slack Notification Step for Master Branch
The Slack Notification step is conditioned to run only when the branch is master. This is appropriate for production notifications. Just ensure that the Slack secret has the proper access restrictions.
|
Looks good. Just some nitpicking from my side. Please add a changelog entry and revise the PR summary. |
Co-authored-by: Norman Rzepka <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
.github/docker-compose.yml (1)
1-3: CI Identification and Naming
The header comment and the name declaration (name: webknossos-ci) clearly indicate that this configuration is dedicated to CI. This naming follows the suggestion provided in previous reviews.
🧹 Nitpick comments (5)
.github/workflows/build_test_deploy.yml (2)
45-76: Backend Tests Job Setup
Thebackend-testsjob appears to be correctly configured with steps to check out code, set up Node.js, install dependencies, and even install Java and system packages. One minor note: the step title “Install frontend dependencies” (line 57) in the backend job might be a misnomer if these are actually backend or shared dependencies. Please confirm that the naming is intentional.
105-115: Custom Environment Variables Script
In the “Custom environment variables” step, you set upNORMALIZED_BRANCHandDOCKER_TAGbased on the value of${{ github.ref_type }}. The logic is clean and usessedto normalize branch names. One point to verify: for non‑branch events (e.g. when the ref is a tag), the script defaultsDOCKER_TAGto “master” and setsNORMALIZED_BRANCHto${{ github.ref_name }}. Please confirm that this is the intended behavior for your deployment pipeline..github/slack-notification.sh (2)
5-14: Mapping GitHub Actor to Slack Mentions
The series of parameter substitutions mapping the GitHub actor to a Slack username is clear and works as expected. For maintainability and scalability, you might consider storing these mappings in an associative array (if Bash version ≥ 4) instead of hardcoding each substitution. This refactor could simplify future updates if additional mappings are needed.
23-24: Slack Message Formatting – Replace Unicode Quotes
Line 24 uses Unicode “curly quotes” around the commit message. These can sometimes cause issues in shells or when parsing logs. It is recommended to replace them with standard ASCII quotes.- mesg="${author} WEBKNOSSOS docker image \`master__${GITHUB_RUN_ID}\` for “${commitmsg}” is ${buildlink}." + mesg="${author} WEBKNOSSOS docker image \`master__${GITHUB_RUN_ID}\` for \"${commitmsg}\" is ${buildlink}."🧰 Tools
🪛 Shellcheck (0.10.0)
[warning] 24-24: This is a unicode quote. Delete and retype it (or ignore/singlequote for literal).
(SC1111)
[warning] 24-24: This is a unicode quote. Delete and retype it (or ignore/singlequote for literal).
(SC1111)
.github/docker-compose.yml (1)
4-113: Overall Docker Compose Configuration Consistency
All service definitions properly use environment variable expansion, volume mappings, and dependency conditions. A small note: the use of the non-standard fieldpull_policy: neverin multiple services should be verified against your compose file version or any custom tooling in your CI/CD pipeline. If this field is a custom extension, ensure that its behavior is well-documented for future maintainers.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
.github/docker-compose.yml(1 hunks).github/slack-notification.sh(1 hunks).github/workflows/build_test_deploy.yml(1 hunks)webknossos-jni/src/bucketScanner.cpp(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- webknossos-jni/src/bucketScanner.cpp
🧰 Additional context used
🪛 Shellcheck (0.10.0)
.github/slack-notification.sh
[warning] 24-24: This is a unicode quote. Delete and retype it (or ignore/singlequote for literal).
(SC1111)
[warning] 24-24: This is a unicode quote. Delete and retype it (or ignore/singlequote for literal).
(SC1111)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: frontend-tests
- GitHub Check: build-test-e2e-deploy
- GitHub Check: backend-tests
🔇 Additional comments (16)
.github/workflows/build_test_deploy.yml (6)
3-10: Workflow Trigger Configuration
The workflow now triggers on push events to the master branch and on all pull requests (branches: '*'). This configuration is clear and focused. Please double‑check that you intentionally want tests (or builds) to run only on master for push events and on every pull request.
21-44: Frontend Tests Job Setup
Thefrontend-testsjob is well configured on Ubuntu 24.04. Using checkout@v4 and setup-node@v4 is great; note its condition (if: ${{ github.ref_name != 'master' }}) restricts this job to non‑master branches. Please verify that you indeed want to skip these frontend tests on master (perhaps because master may use a different deployment strategy).
190-197: End-to-End Tests Execution
The e2e test step uses your custom retry action to run the sbt command with specific-Dflags targeting local endpoints. This approach is helpful for robustness. Please verify that usinglocalhostfor all service addresses (9000, 9090, 9050) correctly reflects your test environment.
229-232: Docker Login Action
Using the docker/login-action@v3 with credentials retrieved from GitHub Secrets is implemented correctly here.
234-269: Docker Push Commands with Retry Mechanism
The push steps for webknosos, datastore, and tracingstore images are wrapped within your retry action and all variable interpolations are correctly quoted. This approach enhances robustness and prevents word‐splitting issues.
270-274: Slack Notification Step
The Slack notification step is conditioned to run only on the master branch and uses a dedicated script with the Slack token supplied via secrets. This integration looks good and aligns with the overall move to GitHub Actions..github/slack-notification.sh (4)
1-4: Script Header and Strict Mode Settings
The shebang and strict error-handling options (set -Eeuo pipefail) are set correctly, ensuring the script fails fast on errors.
16-21: Commit Message Processing and PR Link Conversion
The extraction of the latest commit message and its transformation (by replacing pull request references with clickable links) is implemented correctly using the while-loop and regex. Just ensure that the modified commit message does not unintentionally match the regex again, which could lead to an infinite loop for commit messages with multiple PR references.
27-35: Sending the Slack Notification via cURL
The cURL command to post the message to Slack is well-formed, and the use of a multi-line command with appropriate data fields makes it readable. The retrieval of the Slack token from an environment variable (with a default empty value) is acceptable given that the CI environment should supply it.
36-40: Response Handling and Logging
The script correctly processes the Slack API response usingjqand logs a warning if the message was not sent successfully. The final informational log confirms successful notification..github/docker-compose.yml (6)
4-31:webknossosService Configuration
The configuration for thewebknossosservice is well-defined:
- The service image uses a dynamic tag via
${DOCKER_TAG}.- Port mapping, command options, and volume mounts are clear and appropriately set.
- Environment variables for database connectivity (e.g.,
POSTGRES_URL,POSTGRES_USER, andPOSTGRES_PASSWORD) are correctly injected.- The
depends_oncondition ensuring dependent services are healthy adds robustness.
Overall, this segment aligns well with CI requirements.
32-50:webknossos-datastoreService Setup
The datastore service configuration is consistent:
- The service is tagged with
${DOCKER_TAG}and exposes port 9090.- The commands correctly set memory parameters and HTTP configurations.
- The URL settings (for example,
-Dhttp.uri=http://webknossos-datastore:9090) appear valid within the internal network context.
This segment is clear and follows the intended design.
51-70:webknossos-tracingstoreService Adjustments
The tracing service now correctly sets itshttp.urito reference itself (http://webknossos-tracingstore:9050), which resolves previous inconsistencies. This change ensures that notifications related to tracing use the correct endpoint.
71-82:fossildbService Command and Mounts
The FossilDB service is configured with a specific image version (master__510) and a command that passes a comma-separated list of configuration parameters. The resolved concatenation (with commas between each entry) is now correct. Volume mappings for data and backup directories are clearly defined.
83-98:postgresService with Healthcheck
The Postgres service is pinned to a specific image tag (postgres:17-bookworm) with environment variables provided via the shell environment. The healthcheck that usespg_isreadyis well-configured to delay service startup until the DB is ready.
99-113:redisService Healthcheck and Configuration
The Redis service uses a recent version (redis:7.4) and includes a robust healthcheck using a TCP connection test. The command and healthcheck configuration are clear.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
.github/workflows/build_test_deploy.yml (1)
191-192: 🛠️ Refactor suggestion
⚠️ Potential issueOverly Permissive File Permissions
The “Prepare binaryData folder” step uses
chmod 777on thebinaryData/Organization_Xdirectory. Using mode 777 is overly permissive and can expose your system to modification risks. It is recommended to use a more restrictive permission (for example,chmod 755) to enhance security.- mkdir -p binaryData/Organization_X && chmod 777 binaryData/Organization_X + mkdir -p binaryData/Organization_X && chmod 755 binaryData/Organization_X
🧹 Nitpick comments (1)
.github/workflows/build_test_deploy.yml (1)
3-10: Workflow Triggers ConfigurationThe workflow is now triggered on push to the master branch and pull requests on all branches. This setup is clear and meets the PR’s objective of migrating from CircleCI.
Note: Consider adding a manual trigger (workflow_dispatch) if you want on‑demand debugging or ad hoc runs in the future.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/build_test_deploy.yml(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: build-test-e2e-deploy
- GitHub Check: frontend-tests
- GitHub Check: backend-tests
🔇 Additional comments (6)
.github/workflows/build_test_deploy.yml (6)
21-24: Frontend Tests Job ConditionThe
frontend-testsjob is conditioned withif: github.ref != 'refs/heads/master', which means it does not run on the master branch. Please verify if this is the intended behavior. In many CI pipelines, you may want to run tests on master too to catch regressions.
45-48: Backend Tests Job ConditionSimilarly, the
backend-testsjob is also skipped on master via the conditionif: github.ref != 'refs/heads/master'. Please double-check that you want to exclude master from running these tests, or adjust the condition if tests should run on master as well.
105-118: Custom Environment Variables CalculationThe custom environment script correctly computes the branch-based variables (
NORMALIZED_BRANCHandDOCKER_TAG) and exports them to the GitHub Actions environment. The implementation is clear and the use of parameter expansion (to replace slashes and hyphens) is appropriate.
193-199: End-to-End Tests with RetryThe end-to-end tests step uses a custom retry action with
retries: 3which is a good approach to mitigate transient failures. Ensure that the test command correctly propagates errors if a failure occurs beyond the allowed retries.
237-272: Docker Image Push CommandsThe Docker push steps correctly use retry actions and the image tags are appropriately quoted (e.g.
"scalableminds/webknosos:${{ env.DOCKER_TAG }}"). This minimizes issues due to word splitting in shell commands. The consistent use of retries and delays improves robustness.
273-278: Slack Notification StepThe Slack notification step is configured to run only when not on the master branch, and it uses a secret for
SLACK_NOTIFY_TOKEN. Ensure that the referenced script.github/slack-notification.shimplements proper error handling. Overall, this step is well integrated.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
.github/workflows/build_test_deploy.yml (1)
11-16: 🛠️ Refactor suggestion
⚠️ Potential issueCritical Security Issue: Hardcoded Sensitive Credentials
The global environment block currently hardcodes sensitive values (e.g., POSTGRES_URL, POSTGRES_USER, and POSTGRES_PASSWORD). Hardcoding credentials is a major security risk. It is strongly recommended to source these values from GitHub Secrets. For example, update the declarations as follows:- POSTGRES_URL: "jdbc:postgresql://localhost:5432/webknossos" - POSTGRES_USER: "webknossos_user" - POSTGRES_PASSWORD: "secret_password" + POSTGRES_URL: ${{ secrets.POSTGRES_URL }} + POSTGRES_USER: ${{ secrets.POSTGRES_USER }} + POSTGRES_PASSWORD: ${{ secrets.POSTGRES_PASSWORD }}
🧹 Nitpick comments (2)
.github/workflows/build_test_deploy.yml (2)
119-121: Debug Session Setup: Consider Restricting Debug Access
The workflow sets up a tmate session viamxschmitt/action-tmate@v3which can be very useful for on‑demand debugging. However, because debug sessions may expose sensitive runtime information, consider adding a top‑level permissions field (e.g.permissions: { contents: read }) or other access restrictions to limit who can trigger these sessions.
1-3: Consider Adding Workflow-Level Permissions
Although not tied to a specific change segment, note that the workflow currently does not specify a top‑levelpermissionsfield. Adding explicit permissions (e.g.permissions: { contents: read }) can help restrict who may trigger manual dispatch events (especially with debug inputs enabled). This is a recommended security enhancement for the overall CI pipeline.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/build_test_deploy.yml(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: frontend-tests
- GitHub Check: backend-tests
- GitHub Check: build-test-e2e-deploy
🔇 Additional comments (4)
.github/workflows/build_test_deploy.yml (4)
21-44: Frontend Tests Job – Looks Good
The “frontend-tests” job is well configured: it checks out the code with actions/checkout@v4, sets up Node.js correctly, installs dependencies, performs linting, type checking, and runs tests.
45-76: Backend Tests Job – Implementation Appears Correct
The “backend-tests” job follows best practices with updated actions for checkout, Node.js, Java, and SBT. The steps such as installing system dependencies, starting PostgreSQL via Docker Compose, linting, and running tests are logically organized.
105-118: Custom Environment Variables – Clear but Verify Conditional Logic
The custom environment variables step uses a bash snippet to set DOCKER_TAG and NORMALIZED_BRANCH based on the branch type. The use of bash parameter expansion is correct. Just ensure that the condition based on${{ github.ref_type }}fits all your cases.
275-280: Slack Notification Step – Looks Good
The Slack notification step is correctly set up with a conditional run (skipping on master) and securely references the SLACK_NOTIFY_TOKEN from secrets.
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
.github/workflows/build_test_deploy.yml (1)
1-277: 🛠️ Refactor suggestionOverall Workflow Security Enhancement: Add Explicit Permissions
The workflow does not define a top‑levelpermissionsblock. Adding explicit permissions (for example:permissions: contents: read) can help restrict who can trigger manual (
workflow_dispatch) runs—especially given that debug-related inputs are available. This is an important security best practice for CI/CD pipelines.
♻️ Duplicate comments (1)
.github/workflows/build_test_deploy.yml (1)
190-192: 🛠️ Refactor suggestion
⚠️ Potential issueFilesystem Permissions: Overly Permissive Directory Mode
The "Prepare binaryData folder" step creates the directorybinaryData/Organization_Xwith permissions set viachmod 777, which is overly permissive and can expose your system to security risks. It is recommended to use more restrictive permissions (for example,755) as follows:- mkdir -p binaryData/Organization_X && chmod 777 binaryData/Organization_X + mkdir -p binaryData/Organization_X && chmod 755 binaryData/Organization_X
🧹 Nitpick comments (3)
.github/workflows/build_test_deploy.yml (3)
105-117: Custom Environment Variable Setup
The "Custom environment variables" step buildsNORMALIZED_BRANCHandDOCKER_TAGbased on the branch name. The approach using bash substitution (e.g."${GITHUB_HEAD_REF//[\/-]/_}") is effective; however, note that for non–pull-request events,GITHUB_HEAD_REFmay be empty. Please verify that the current logic correctly handles all scenarios (push vs. pull_request).
210-216: Webknossos Smoke Test Timing
The smoke test for webknossos uses curl with 20 retries and a 5‑second delay. These parameters appear robust but consider whether they align with the typical startup time of your service. Adjust if necessary to avoid false negatives.
217-227: Datastore & Tracingstore Smoke Tests
The subsequent smoke tests for the datastore and tracingstore endpoints follow a similar retry strategy. Ensure that their endpoints (http://localhost:9090/data/healthandhttp://localhost:9050/tracings/health) are correct and that the retry settings are optimal for your container startup times.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/build_test_deploy.yml(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: build-test-e2e-deploy
- GitHub Check: frontend-tests
- GitHub Check: backend-tests
🔇 Additional comments (9)
.github/workflows/build_test_deploy.yml (9)
3-10: CI Trigger Configuration Verification
The workflow is now triggered by a push to the master branch and by all pull_request events. Please confirm that limiting pushes to only the master branch is intentional and that these trigger settings align with your deployment and testing strategy.
21-44: Clarification on Frontend Test Execution
Thefrontend-testsjob is set to run only when the reference is not master (if: github.ref != 'refs/heads/master'). Confirm that you intend to skip these tests on the master branch (e.g. if master represents a production build where tests have already been run) or if they should run on all branches.
45-77: Clarification on Backend Test Execution
Similarly, thebackend-testsjob condition (if: github.ref != 'refs/heads/master') excludes the master branch from test execution. Please verify that this conditional is intentional so that branch-specific tests run only on pull requests or feature branches.
192-199: End-to-End Tests with Retry Logic
The e2e tests are executed via a custom retry action that runs an sbt command with specific system properties. Please verify that a retry count of 3 is sufficient for your environment and that any failures from your e2e suite are correctly propagated.
203-209: Service Startup Verification
The step that starts the webknossos, datastore, and tracingstore services uses docker compose with a set of service names. Please confirm that the service names in the compose file match these references and that no additional environment-specific configuration is required at startup.
236-242: Docker Image Push for Webknossos
The workflow pushes two tags for the webknossos image – one using${{ env.DOCKER_TAG }}and another using${{ env.NORMALIZED_BRANCH }}. Please verify that the repository name is spelled correctly and that the tagging strategy meets your deployment and rollback requirements. The use of quotes in the push commands is appropriate.
248-254: Docker Image Push for Datastore
The push steps for the webknossos-datastore image are consistent with those for the main image. Confirm that both tag variants are intentional and that your downstream deployment processes correctly pull the required tag.
260-266: Docker Image Push for Tracingstore
Similarly, the push steps for the webknossos-tracingstore image use the same retry parameters and tagging scheme. Ensure that these tags are being set correctly during the build phase and that the push commands run without errors.
272-276: Slack Notification Step Verification
The final step sends a Slack notification via an external script. Please ensure that.github/slack-notification.shexists, is executable, and securely handles any sensitive output. Also, note that this step runs only if the branch isn’t master—confirm that this conditional is intentional for your notification workflow.
|
Summary of all Changes: Removed
Changed
Added
|
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
.github/workflows/build_test_deploy.yml (3)
45-76: Optional: Wait for Postgres Readiness in Backend Tests
In thebackend-testsjob you start Postgres with a Docker Compose command but do not explicitly wait for it to become ready before running tests. It’s advisable to add a health‐check loop (or use a dedicated action) to ensure the database is accepting connections. For example:- - name: "Start postgres" - run: docker compose --file .github/docker-compose.yml up -d postgres + - name: "Start postgres" + run: | + docker compose --file .github/docker-compose.yml up -d postgres + until docker compose --file .github/docker-compose.yml exec postgres pg_isready; do + echo "Waiting for Postgres to be ready..." + sleep 2 + done
105-117: Review: Custom Environment Variables Script
The custom script correctly setsDOCKER_TAGandNORMALIZED_BRANCHusing the GitHub context. Please verify that for non‑branch refs (e.g. tags) the values are as intended. You might also consider adding a check or comment explaining the rationale if tags aren’t expected.
231-242: Docker Image Push: Proper Quoting and Consistency Check
The push steps correctly use quotes around variable interpolations (e.g."scalableminds/webknossos:${{ env.DOCKER_TAG }}"), which prevents issues with word splitting. Verify that the repository name is consistent with what’s defined earlier in the Docker build steps.CHANGELOG.unreleased.md (1)
22-25: Clarity Improvement: Reword Docker Image Numbering Entry
The changelog entry under "Changed" on line 24 is ambiguous:
- Numbered docker image no use different and larger numbers. [#8147](...)Consider rephrasing it to more clearly communicate the change. For example:
Revised: "Docker image tags are now generated using a normalized and larger number scheme for improved versioning consistency. #8147"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.github/workflows/build_test_deploy.yml(1 hunks)CHANGELOG.unreleased.md(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: backend-tests
- GitHub Check: frontend-tests
- GitHub Check: build-test-e2e-deploy
🔇 Additional comments (3)
.github/workflows/build_test_deploy.yml (3)
21-44: Double‑Check Branch Testing Conditions for Frontend Tests
Thefrontend-testsjob runs only if the ref is not master (if: github.ref != 'refs/heads/master'). Confirm that this exclusion is intentional—if you want all branches (including master) to run these tests, the condition may need adjustment.
192-199: Good Use of Retry for End-to-End Tests
The e2e tests step leverages a custom retry action with a configured retry count. Ensure that the service addresses (e.g.localhostfor redis and fossildb) match your Docker network configuration. Overall, this approach improves robustness.
272-277: Slack Notification Step Looks Correct
The Slack notification step uses a separate script with the token pulled from secrets. This is a good practice for exposing notifications only on non‑master branches.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
CHANGELOG.unreleased.md (1)
27-28: Grammar Note: Pluralize "docker image" to "docker images".The current entry reads:
Numbered docker image now use different and larger numbers.
Since the verb “use” is plural, it should refer to “docker images”. Please update this to maintain correct subject–verb agreement.- Numbered docker image now use different and larger numbers. + Numbered docker images now use different and larger numbers.🧰 Tools
🪛 LanguageTool
[uncategorized] ~27-~27: The grammatical number of this noun doesn’t look right. Consider replacing it.
Context: ...ebknossos/pull/8106) - Numbered docker image now use different and larger numbers. [...(AI_EN_LECTOR_REPLACEMENT_NOUN_NUMBER)
[grammar] ~27-~27: Possible subject-verb agreement error.
Context: ...pull/8106) - Numbered docker image now use different and larger numbers. [#8147](h...(SINGULAR_NOUN_ADV_AGREEMENT)
🧹 Nitpick comments (1)
CHANGELOG.unreleased.md (1)
47-48: Breaking Change Documentation: Confirm Related Updates.The breaking change entry clearly documents that
docker-compose.ymlhas been removed in favor oftools/hosting/docker-compose.yml. Please verify that all associated documentation (e.g., migration guides, internal references) has been updated to reflect this change.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
CHANGELOG.unreleased.md(2 hunks)
🧰 Additional context used
🪛 LanguageTool
CHANGELOG.unreleased.md
[uncategorized] ~27-~27: The grammatical number of this noun doesn’t look right. Consider replacing it.
Context: ...ebknossos/pull/8106) - Numbered docker image now use different and larger numbers. [...
(AI_EN_LECTOR_REPLACEMENT_NOUN_NUMBER)
[grammar] ~27-~27: Possible subject-verb agreement error.
Context: ...pull/8106) - Numbered docker image now use different and larger numbers. [#8147](h...
(SINGULAR_NOUN_ADV_AGREEMENT)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: frontend-tests
- GitHub Check: build-test-e2e-deploy
- GitHub Check: backend-tests
Removed
Changed
Added
- backend-tests: Runs Scapegoat, Scalafmt and sbt tests
- frontend-tests: Static tests and yarn test-verbose
- build-test-e2e-deploy: Full build of webknossos including docker images followed by e2e tests and smoking tests. All docker images get pushed if all tests in this job pass.
Steps to test:
(Please delete unneeded items, merge only when none are left open)