ci: fix lint on push/schedule and bake orchestrator test runtime image#96
Conversation
Two unrelated CI breakages were piling up failures: - main.yml's lint job uses golangci-lint with `only-new-issues: true`, which needs a PR base to diff against. On `push` and `schedule` there is no base, so every pre-existing issue is reported as "new" and the job fails. Gate the job to `pull_request` / `workflow_dispatch`. - Functional tests were flaking with "Orchestrator not ready" because each orchestrator container ran `apt-get update && apt-get install curl sqlite3 > /dev/null 2>&1` at startup, inside the 120s readiness window, with output silenced. Move the install into a pre-baked `orchestrator-runtime` image (ubuntu:24.04 + curl + sqlite3 + ca-certs) built once per job, and drop the apt step from every orchestrator service's command. Locally, readiness drops from a 120s timeout to ~1s and smoke tests pass 26/26.
📝 WalkthroughWalkthroughThe changes introduce a custom Docker runtime image ( Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a custom Docker image, orchestrator-runtime, for functional testing, moving the installation of dependencies like curl and sqlite3 from the container startup command to a dedicated Dockerfile. This change streamlines the docker-compose.yml configuration for several orchestrator services. Feedback suggests adding a build section to the orchestrator service in the Compose file to automatically build the runtime image if it is missing locally, improving the developer experience.
|
|
||
| orchestrator: | ||
| image: ubuntu:24.04 | ||
| image: orchestrator-runtime:latest |
There was a problem hiding this comment.
To improve the local development experience, consider adding a build section to the orchestrator service. This allows docker-compose up to automatically build the orchestrator-runtime image using the provided Dockerfile if it doesn't exist locally, rather than failing with an image not found error. Since other services in this file share the same image name, defining the build context once here is sufficient for a full stack startup.
build:
context: .
dockerfile: orchestrator-runtime.Dockerfile
image: orchestrator-runtime:latest![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/functional/orchestrator-runtime.Dockerfile (1)
1-5: Harden the runtime image by dropping root privileges.This image currently runs as root. Adding a dedicated non-root user improves baseline container security with minimal cost.
🔧 Proposed hardening diff
FROM ubuntu:24.04 RUN apt-get update \ && apt-get install -y --no-install-recommends curl sqlite3 ca-certificates \ && rm -rf /var/lib/apt/lists/* + +RUN useradd --create-home --shell /usr/sbin/nologin orchestrator +USER orchestrator🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/functional/orchestrator-runtime.Dockerfile` around lines 1 - 5, Add a non-root user and switch to it in the Dockerfile: create a dedicated user/group (e.g., "orchestrator") and home directory, chown any runtime-required directories/files (for example those created or used by the apt installs or later runtime artifacts), and add a USER instruction before the image final layer so the container no longer runs as root; reference the existing FROM ubuntu:24.04 and the RUN apt-get ... lines to locate where to insert the user/group creation, chown, and USER orchestrator statements and ensure the new user has a non-login shell and limited privileges.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@tests/functional/orchestrator-runtime.Dockerfile`:
- Around line 1-5: Add a non-root user and switch to it in the Dockerfile:
create a dedicated user/group (e.g., "orchestrator") and home directory, chown
any runtime-required directories/files (for example those created or used by the
apt installs or later runtime artifacts), and add a USER instruction before the
image final layer so the container no longer runs as root; reference the
existing FROM ubuntu:24.04 and the RUN apt-get ... lines to locate where to
insert the user/group creation, chown, and USER orchestrator statements and
ensure the new user has a non-login shell and limited privileges.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 22c86548-cfea-4b89-95c0-6c6605aea8e6
📒 Files selected for processing (4)
.github/workflows/functional.yml.github/workflows/main.ymltests/functional/docker-compose.ymltests/functional/orchestrator-runtime.Dockerfile
1 participant
Summary
main.yml): gated topull_request/workflow_dispatch. Withonly-new-issues: true, golangci-lint needs a PR base to diff against; onpush/scheduleevery pre-existing issue is flagged as new and the job fails. Every scheduled run since April 9 has been red for this reason.apt-get update && apt-get install curl sqlite3 > /dev/null 2>&1inside the 120s readiness window with output silenced, causing flaky "Orchestrator not ready" timeouts across the matrix (mysql 9.2/9.5/9.6, postgresql 15/17, raft). Moved the apt step into a pre-bakedorchestrator-runtime:latestimage built once per job; the orchestrator/orchestrator-pg/orchestrator-raft1..3 services now just run the binary.Locally, orchestrator readiness drops from a 120s timeout to ~1s and the smoke suite passes 26/26.
Test plan
CIworkflow green on this PR (lint compares against master base and passes)Functional Testsworkflow green on this PR across the full matrix (mysql 5.7/8.0/8.4/9.0/9.2/9.5/9.6, postgresql 15/16/17, raft)CIruns stop failing on the lint jobSummary by CodeRabbit