chore(infra): use per-service container WORKDIR#4258
Open
mmabrouk wants to merge 1 commit intochore/sdk/python-reorgfrom
Open
chore(infra): use per-service container WORKDIR#4258mmabrouk wants to merge 1 commit intochore/sdk/python-reorgfrom
mmabrouk wants to merge 1 commit intochore/sdk/python-reorgfrom
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
dosubot
Bot
added
the
size:L
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThis PR systematically restructures the application's filesystem layout by migrating from a single ChangesDirectory Structure Migration
🎯 4 (Complex) | ⏱️ ~60 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
mmabrouk
force-pushed
the
chore/sdk/python-reorg
branch
from
0e2da39 to
5c30ece
Compare
mmabrouk
force-pushed
the
chore/infra/workdir-rename
branch
2 times, most recently
from
fd16b98 to
7ff2745
Compare
mmabrouk
force-pushed
the
chore/sdk/python-reorg
branch
from
5c30ece to
627bf63
Compare
mmabrouk
force-pushed
the
chore/infra/workdir-rename
branch
from
7ff2745 to
c2d79d1
Compare
dosubot
Bot
added
size:XXL
mmabrouk
force-pushed
the
chore/infra/workdir-rename
branch
from
c2d79d1 to
2ab5165
Compare
Contributor
Railway Preview Environment
|
mmabrouk
force-pushed
the
chore/infra/workdir-rename
branch
from
b090167 to
a125124
Compare
dosubot
Bot
added
size:L
mmabrouk
force-pushed
the
chore/infra/workdir-rename
branch
from
a125124 to
7b68615
Compare
mmabrouk
force-pushed
the
chore/sdk/python-reorg
branch
from
627bf63 to
df5606a
Compare
Containers were uniformly using `/app` as `WORKDIR` regardless of which
service they hosted, which made multi-service compose stacks ambiguous
and produced confusing log paths. This aligns container `WORKDIR` with
the service that owns it:
| Service | WORKDIR |
|-------------------------------|-------------|
| `api/oss`, `api/ee` | `/api` |
| `web/oss`, `web/ee` | `/web` |
| `services/oss`, `services/ee` | `/services` |
The shared SDK and client install locations also move out from under
the per-container `WORKDIR` and become top-level paths in every image:
- `/app/sdks/python` -> `/sdks/python`
- `/app/clients/python` -> `/clients/python`
This is stacked on `chore/sdk/python-reorg`. That PR introduces
`sdks/python/` and `clients/python/` as build-context paths and adds
them to `PYTHONPATH`. Without that prerequisite, Docker builds would
fail with `failed to compute cache key: "/sdks/python": not found`
because the source directories don't exist on `main` yet.
## What changes
- 12 Dockerfiles (api/{oss,ee}, web/{oss,ee}, services/{oss,ee} -
dev + gh)
- 7 docker-compose files (mount targets, watchmedo dirs, cron command)
- Helm: _helpers.tpl alembic defaults, web-deployment, cron-deployment
- Railway: all worker/cron/web/alembic Dockerfiles + scripts +
railway.json
- 4 alembic.ini script_location, 4 migration READMEs
- 4 env.example files (ALEMBIC_CFG_PATH_* defaults)
- 3 self-host docs (docker exec command examples)
- api/oss/src/utils/env.py (alembic fallback paths)
- api/oss/src/crons/queries.sh (crontab path)
- web/entrypoint.sh (ENTRYPOINT_DIR default)
## Risks
- Largest blast radius: every container in the stack has its WORKDIR
changed. Any custom Dockerfile or compose override held outside this
repo will break.
- Helm values overrides: if any deployment overrides alembicCfgPath
in their values.yaml, those overrides reference the old /app/... path
and need to be updated.
- Cron command in compose gh.yml: pinned older AGENTA_API_IMAGE_TAG
values still emit /app/crontab and would fail to start. Operators
must bump the image tag alongside this compose update.
- No application logic changes. Only paths in Docker, compose, helm,
alembic, env defaults move.
## QA
- [ ] CI green (now that /sdks/python and /clients/python exist via
the parent PR)
- [ ] docker compose dev stack (oss + ee) starts cleanly
- [ ] helm template shows the new /api path in env vars
- [ ] Run a test alembic migration in a dev container
- [ ] Cron container: queries.sh runs from /api/
- [ ] Self-host docs render correctly
Originally part of #4239 - split out for independent review.
mmabrouk
force-pushed
the
chore/infra/workdir-rename
branch
from
7b68615 to
fe5fe8a
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
hosting/railway/oss/cron/Dockerfile (1)
1-14:⚠️ Potential issue | 🟠 MajorPin the Railway base image to a specific version.
All Railway Dockerfiles using
ghcr.io/agenta-ai/agenta-api:latest(cron, alembic, api, worker-evaluations, worker-events, worker-tracing, worker-webhooks) depend on/api/...paths that may not exist if the base image layout differs or changes. The floating:latesttag creates a deployment risk where the cron service could fail at runtime if the base image no longer contains the expected paths.
🧹 Nitpick comments (1)
web/oss/docker/Dockerfile.dev (1)
8-8: Reduce Docker image size by hardening apt install flags.Line 8 should add
--no-install-recommendsand clear apt lists to eliminate unnecessary dependencies.Proposed patch
-RUN apt-get update && apt-get install -y jq +RUN apt-get update \ + && apt-get install -y --no-install-recommends jq \ + && rm -rf /var/lib/apt/lists/*
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 1c9c36a1-06a4-45d9-911c-4d9c1c03eb51
📒 Files selected for processing (50)
api/ee/databases/postgres/migrations/core/README.mdapi/ee/databases/postgres/migrations/core/alembic.iniapi/ee/databases/postgres/migrations/tracing/README copy.mdapi/ee/databases/postgres/migrations/tracing/alembic.iniapi/ee/docker/Dockerfile.devapi/ee/docker/Dockerfile.ghapi/oss/databases/postgres/migrations/core/README.mdapi/oss/databases/postgres/migrations/core/alembic.iniapi/oss/databases/postgres/migrations/tracing/README.mdapi/oss/databases/postgres/migrations/tracing/alembic.iniapi/oss/docker/Dockerfile.devapi/oss/docker/Dockerfile.ghapi/oss/src/crons/queries.shapi/oss/src/utils/env.pydocs/docs/self-host/01-quick-start.mdxdocs/docs/self-host/02-configuration.mdxdocs/docs/self-host/03-upgrading.mdxhosting/docker-compose/ee/docker-compose.dev.ymlhosting/docker-compose/ee/docker-compose.gh.local.ymlhosting/docker-compose/ee/docker-compose.gh.ymlhosting/docker-compose/ee/env.ee.dev.examplehosting/docker-compose/ee/env.ee.gh.examplehosting/docker-compose/oss/docker-compose.dev.ymlhosting/docker-compose/oss/docker-compose.gh.local.ymlhosting/docker-compose/oss/docker-compose.gh.ssl.ymlhosting/docker-compose/oss/docker-compose.gh.ymlhosting/docker-compose/oss/env.oss.dev.examplehosting/docker-compose/oss/env.oss.gh.examplehosting/helm/agenta-oss/templates/_helpers.tplhosting/helm/agenta-oss/templates/cron-deployment.yamlhosting/helm/agenta-oss/templates/web-deployment.yamlhosting/railway/oss/alembic/Dockerfilehosting/railway/oss/cron/Dockerfilehosting/railway/oss/scripts/build-and-push-images.shhosting/railway/oss/scripts/deploy-from-images.shhosting/railway/oss/web/Dockerfilehosting/railway/oss/web/railway.jsonhosting/railway/oss/worker-evaluations/Dockerfilehosting/railway/oss/worker-events/Dockerfilehosting/railway/oss/worker-tracing/Dockerfilehosting/railway/oss/worker-webhooks/Dockerfileservices/ee/docker/Dockerfile.devservices/ee/docker/Dockerfile.ghservices/oss/docker/Dockerfile.devservices/oss/docker/Dockerfile.ghweb/ee/docker/Dockerfile.devweb/ee/docker/Dockerfile.ghweb/entrypoint.shweb/oss/docker/Dockerfile.devweb/oss/docker/Dockerfile.gh
|
|
||
| ```bash | ||
| docker exec -e PYTHONPATH=/app -w /app/ee/databases/postgres/migrations/core agenta-ee-dev-api-1 alembic -c alembic.ini downgrade head | ||
| docker exec -e PYTHONPATH=/api -w /api/ee/databases/postgres/migrations/core agenta-ee-dev-api-1 alembic -c alembic.ini downgrade head |
There was a problem hiding this comment.
Fix rollback command to actually downgrade.
Line 34 uses alembic ... downgrade head, which does not revert to a previous revision. For “revert the migration,” use downgrade -1 (or a concrete target revision).
📝 Suggested docs fix
-docker exec -e PYTHONPATH=/api -w /api/ee/databases/postgres/migrations/core agenta-ee-dev-api-1 alembic -c alembic.ini downgrade head
+docker exec -e PYTHONPATH=/api -w /api/ee/databases/postgres/migrations/core agenta-ee-dev-api-1 alembic -c alembic.ini downgrade -1📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| docker exec -e PYTHONPATH=/api -w /api/ee/databases/postgres/migrations/core agenta-ee-dev-api-1 alembic -c alembic.ini downgrade head | |
| docker exec -e PYTHONPATH=/api -w /api/ee/databases/postgres/migrations/core agenta-ee-dev-api-1 alembic -c alembic.ini downgrade -1 |
|
|
||
| ```bash | ||
| docker exec -e PYTHONPATH=/app -w /app/ee/databases/postgres/migrations/core agenta-ee-dev-api-1 alembic -c alembic.ini revision --autogenerate -m "migration message" | ||
| docker exec -e PYTHONPATH=/api -w /api/ee/databases/postgres/migrations/core agenta-ee-dev-api-1 alembic -c alembic.ini revision --autogenerate -m "migration message" |
There was a problem hiding this comment.
Fix tracing README commands: wrong migration directory and ineffective rollback target.
Line 16, Line 28, and Line 34 still point to .../migrations/core in a tracing README, and downgrade head won’t rollback the latest revision as described.
Proposed doc fix
-docker exec -e PYTHONPATH=/api -w /api/ee/databases/postgres/migrations/core agenta-ee-dev-api-1 alembic -c alembic.ini revision --autogenerate -m "migration message"
+docker exec -e PYTHONPATH=/api -w /api/ee/databases/postgres/migrations/tracing agenta-ee-dev-api-1 alembic -c alembic.ini revision --autogenerate -m "migration message"
-docker exec -e PYTHONPATH=/api -w /api/ee/databases/postgres/migrations/core agenta-ee-dev-api-1 alembic -c alembic.ini upgrade head
+docker exec -e PYTHONPATH=/api -w /api/ee/databases/postgres/migrations/tracing agenta-ee-dev-api-1 alembic -c alembic.ini upgrade head
-docker exec -e PYTHONPATH=/api -w /api/ee/databases/postgres/migrations/core agenta-ee-dev-api-1 alembic -c alembic.ini downgrade head
+docker exec -e PYTHONPATH=/api -w /api/ee/databases/postgres/migrations/tracing agenta-ee-dev-api-1 alembic -c alembic.ini downgrade -1Also applies to: 28-28, 34-34
| # === EXECUTION ============================================ # | ||
| init: true | ||
| command: supercronic /app/crontab | ||
| command: supercronic /api/crontab |
There was a problem hiding this comment.
Add a temporary fallback for crontab path compatibility.
Line 253 hard-switches to /api/crontab, but this GH compose variant uses tagged remote images; older tags that still write /app/crontab will fail cron startup and miss scheduled jobs.
💡 Suggested compatibility-safe command
- command: supercronic /api/crontab
+ command: >
+ sh -c 'if [ -f /api/crontab ]; then
+ exec supercronic /api/crontab;
+ elif [ -f /app/crontab ]; then
+ exec supercronic /app/crontab;
+ else
+ echo "crontab file not found at /api/crontab or /app/crontab" >&2;
+ exit 1;
+ fi'
Comment on lines
+35
to
36
| command: ["/web/entrypoint.sh"] | ||
| args: ["node", {{ include "agenta.webServerPath" . | quote }}] |
There was a problem hiding this comment.
Avoid hard-coupling the chart to a single image layout.
web.image is still user-overridable, but this now assumes every compatible image exposes /web/entrypoint.sh. Any deployment pinned to an older or custom web image with /app/entrypoint.sh will fail at startup as soon as the chart is upgraded.
💡 Suggested fix
- command: ["/web/entrypoint.sh"]
+ command: [{{ .Values.web.entrypointPath | default "/web/entrypoint.sh" | quote }}]Add a matching default in values.yaml so the new layout stays the default while preserving an escape hatch for pinned/custom images.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| command: ["/web/entrypoint.sh"] | |
| args: ["node", {{ include "agenta.webServerPath" . | quote }}] | |
| command: [{{ .Values.web.entrypointPath | default "/web/entrypoint.sh" | quote }}] | |
| args: ["node", {{ include "agenta.webServerPath" . | quote }}] |
Comment on lines
+23
to
+29
| SDKS_SOURCE_DIR="$ROOT_DIR/sdks" | ||
| API_SDKS_DIR="$ROOT_DIR/api/sdks" | ||
| SERVICES_SDKS_DIR="$ROOT_DIR/services/sdks" | ||
|
|
||
| if [ ! -d "$SDK_SOURCE_DIR" ]; then | ||
| printf "Missing SDK directory: %s\n" "$SDK_SOURCE_DIR" >&2 | ||
| CLIENTS_SOURCE_DIR="$ROOT_DIR/clients" | ||
| API_CLIENTS_DIR="$ROOT_DIR/api/clients" | ||
| SERVICES_CLIENTS_DIR="$ROOT_DIR/services/clients" |
There was a problem hiding this comment.
Validate the exact python subpaths that the Dockerfiles consume.
This script only checks sdks/ and clients/, but both downstream Dockerfiles copy ./sdks/python and ./clients/python. If the parent directories exist but those subpaths do not, this passes and then fails later inside docker build with a much less actionable error.
💡 Suggested fix
-SDKS_SOURCE_DIR="$ROOT_DIR/sdks"
+SDKS_SOURCE_DIR="$ROOT_DIR/sdks"
+SDKS_PYTHON_SOURCE_DIR="$SDKS_SOURCE_DIR/python"
API_SDKS_DIR="$ROOT_DIR/api/sdks"
SERVICES_SDKS_DIR="$ROOT_DIR/services/sdks"
-CLIENTS_SOURCE_DIR="$ROOT_DIR/clients"
+CLIENTS_SOURCE_DIR="$ROOT_DIR/clients"
+CLIENTS_PYTHON_SOURCE_DIR="$CLIENTS_SOURCE_DIR/python"
API_CLIENTS_DIR="$ROOT_DIR/api/clients"
SERVICES_CLIENTS_DIR="$ROOT_DIR/services/clients"
-if [ ! -d "$SDKS_SOURCE_DIR" ]; then
- printf "Missing SDKs directory: %s\n" "$SDKS_SOURCE_DIR" >&2
+if [ ! -d "$SDKS_PYTHON_SOURCE_DIR" ]; then
+ printf "Missing SDK Python directory: %s\n" "$SDKS_PYTHON_SOURCE_DIR" >&2
exit 1
fi
-if [ ! -d "$CLIENTS_SOURCE_DIR" ]; then
- printf "Missing clients directory: %s\n" "$CLIENTS_SOURCE_DIR" >&2
+if [ ! -d "$CLIENTS_PYTHON_SOURCE_DIR" ]; then
+ printf "Missing clients Python directory: %s\n" "$CLIENTS_PYTHON_SOURCE_DIR" >&2
exit 1
fiAlso applies to: 31-39
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
chore(infra): use per-service container WORKDIR
Containers were uniformly using
/appasWORKDIRregardless of whichservice they hosted, which made multi-service compose stacks ambiguous
and produced confusing log paths. This aligns container
WORKDIRwiththe service that owns it:
api/oss,api/ee/apiweb/oss,web/ee/webservices/oss,services/ee/servicesThe shared SDK and client install locations also move out from under
the per-container
WORKDIRand become top-level paths in every image:/app/sdks/python->/sdks/python/app/clients/python->/clients/pythonThis is stacked on
chore/sdk/python-reorg. That PR introducessdks/python/andclients/python/as build-context paths and addsthem to
PYTHONPATH. Without that prerequisite, Docker builds wouldfail with
failed to compute cache key: "/sdks/python": not foundbecause the source directories don't exist on
mainyet.What changes
dev + gh)
railway.json
Risks
changed. Any custom Dockerfile or compose override held outside this
repo will break.
in their values.yaml, those overrides reference the old /app/... path
and need to be updated.
values still emit /app/crontab and would fail to start. Operators
must bump the image tag alongside this compose update.
alembic, env defaults move.
QA
the parent PR)
Originally part of #4239 - split out for independent review.