[CHORE] Consolidate env config into single common.env with zero duplication#1842
Open
mathumathi-zipstack wants to merge 8 commits intomainfrom
Open
[CHORE] Consolidate env config into single common.env with zero duplication#1842mathumathi-zipstack wants to merge 8 commits intomainfrom
mathumathi-zipstack wants to merge 8 commits intomainfrom
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughConsolidated infrastructure env vars into Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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 |
|
|
…cation - Create docker/sample.common.env as single source of truth for all shared env vars (DB, Redis, RabbitMQ, MinIO, Flipt, service URLs, timeouts) - Remove docker/sample.essentials.env (merged into common.env) - Eliminate credential duplication by using Docker Compose environment mappings to derive infra vars from app vars (e.g. POSTGRES_USER from DB_USER, RABBITMQ_DEFAULT_USER from CELERY_BROKER_USER) - Standardize DB env var names across services: PG_BE_* -> DB_*, DB_USERNAME -> DB_USER, REDIS_USERNAME -> REDIS_USER - Strip shared vars from per-service sample.env files (backend, workers, prompt-service, runner, platform-service, x2text-service) - Update docker-compose files to use .env (auto-read by Docker Compose for variable substitution) - Update docker/README.md with simplified setup instructions Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
mathumathi-zipstack
force-pushed
the
temp/common-env-config
branch
from
6f52884 to
0af3d94
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
docker/sample.common.env (1)
54-63: Consider templating MinIO credentials to reduce duplication.The MinIO credentials are defined in
MINIO_ROOT_USER/MINIO_ROOT_PASSWORDbut must be manually duplicated in the JSON storage credential blocks. While the comment on line 55 instructs users to update both, this creates a maintenance burden and risk of inconsistency.For a future improvement, consider using environment variable substitution within the JSON strings if your application supports it, or document this coupling more prominently.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docker/sample.common.env` around lines 54 - 63, The JSON credential blocks duplicate MINIO_ROOT_USER and MINIO_ROOT_PASSWORD; update WORKFLOW_EXECUTION_FILE_STORAGE_CREDENTIALS, API_FILE_STORAGE_CREDENTIALS, PERMANENT_REMOTE_STORAGE, and TEMPORARY_REMOTE_STORAGE to reference the single source-of-truth variables MINIO_ROOT_USER and MINIO_ROOT_PASSWORD (e.g., via environment variable substitution or your template syntax) so changes to MINIO_ROOT_USER/MINIO_ROOT_PASSWORD automatically propagate and eliminate duplication/inconsistency.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@backend/sample.env`:
- Around line 2-5: Update the header of backend/sample.env to explicitly state
this file is incomplete without docker/sample.common.env and must be combined
with it (do not treat it as standalone); call out the required shared keys that
must be copied from docker/sample.common.env — at minimum
CELERY_BROKER_BASE_URL, CELERY_BROKER_USER, CELERY_BROKER_PASS plus DB, REDIS,
RABBITMQ, FLIPT, MINIO and timeout vars — and add a short warning that failing
to include these will cause startup failures or silent fallbacks (see
backend/backend/settings/base.py and backend/utils/cache_service.py where those
defaults are used).
In `@docker/README.md`:
- Around line 20-31: The env-file copy commands are inconsistent with the
compose commands: update the paths so they use the same working-directory
convention as the rest of the README (which references docker-compose.yaml and
sample.compose.override.yaml at repo root). Either change the cp lines
(sample.common.env, backend/sample.env, platform-service/sample.env, etc.) to be
repo-root relative (remove the leading "docker/" from cp targets) or make the
compose commands docker/-relative; pick one convention and update all
occurrences (sample.common.env -> docker/.env, backend/sample.env ->
docker/backend/.env, or vice versa) to ensure the copy commands run from the
same directory as the docker-compose commands.
In `@platform-service/src/unstract/platform_service/env.py`:
- Around line 15-19: Replace the optional os.environ.get() calls for DB_HOST,
DB_USER, DB_PASSWORD, and DB_NAME with EnvManager.get_required_setting() so
these variables raise a clear error if missing; keep DB_PORT as-is (or if you
want it required too, wrap EnvManager.get_required_setting("DB_PORT") with
int()) and ensure any returned values are cast to the correct types where
needed; update the assignments for the symbols DB_HOST, DB_USER, DB_PASSWORD,
DB_NAME to call
EnvManager.get_required_setting("DB_HOST"/"DB_USER"/"DB_PASSWORD"/"DB_NAME") to
match how REDIS_HOST/REDIS_PORT are handled.
In `@workers/sample.env`:
- Around line 4-9: The runtime validation message (referencing CELERY_BROKER_URL
and CELERY_RESULT_BACKEND in worker_config.py) points users to
workers/sample.env but that file no longer contains the DB_* / CELERY_BROKER_*
examples; update the project so the guidance is consistent by either restoring a
small commented block of required shared vars (e.g., DB_*, CELERY_BROKER_*,
CELERY_RESULT_BACKEND, CELERY_BROKER_URL) into workers/sample.env, or change the
validation text in the code that builds/validates CELERY_BROKER_URL and
CELERY_RESULT_BACKEND to point to docker/sample.common.env instead; edit the
unique symbols CELERY_BROKER_URL, CELERY_RESULT_BACKEND and the validation
message in worker_config.py to match whichever option you choose.
---
Nitpick comments:
In `@docker/sample.common.env`:
- Around line 54-63: The JSON credential blocks duplicate MINIO_ROOT_USER and
MINIO_ROOT_PASSWORD; update WORKFLOW_EXECUTION_FILE_STORAGE_CREDENTIALS,
API_FILE_STORAGE_CREDENTIALS, PERMANENT_REMOTE_STORAGE, and
TEMPORARY_REMOTE_STORAGE to reference the single source-of-truth variables
MINIO_ROOT_USER and MINIO_ROOT_PASSWORD (e.g., via environment variable
substitution or your template syntax) so changes to
MINIO_ROOT_USER/MINIO_ROOT_PASSWORD automatically propagate and eliminate
duplication/inconsistency.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 14ff15a1-7def-44d6-a6f3-1dcbab713447
📒 Files selected for processing (17)
backend/sample.envdocker/README.mddocker/docker-compose-dev-essentials.yamldocker/docker-compose.yamldocker/sample.common.envdocker/sample.essentials.envplatform-service/sample.envplatform-service/src/unstract/platform_service/config.pyplatform-service/src/unstract/platform_service/env.pyplatform-service/src/unstract/platform_service/extensions.pyprompt-service/sample.envprompt-service/src/unstract/prompt_service/extensions.pyrunner/sample.envworkers/sample.envx2text-service/app/env.pyx2text-service/app/models.pyx2text-service/sample.env
💤 Files with no reviewable changes (1)
- docker/sample.essentials.env
for more information, see https://pre-commit.ci
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
platform-service/sample.env (1)
21-23:⚠️ Potential issue | 🟡 MinorRename
REMOTE_MODEL_PRICES_FILE_PATHtoMODEL_PRICES_FILE_PATHin sample.env.The environment variable in sample.env uses the wrong name. The code expects
MODEL_PRICES_FILE_PATH(defined inplatform-service/src/unstract/platform_service/env.pyline 25 and consumed incost_calculation.pyline 19), but sample.env definesREMOTE_MODEL_PRICES_FILE_PATH, which is never used anywhere in the codebase.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@platform-service/sample.env` around lines 21 - 23, The sample.env contains REMOTE_MODEL_PRICES_FILE_PATH but the code reads MODEL_PRICES_FILE_PATH (referenced in env.py and consumed in cost_calculation.py), so update sample.env by renaming REMOTE_MODEL_PRICES_FILE_PATH to MODEL_PRICES_FILE_PATH (preserve the current value "unstract/cost/model_prices.json" and quoting style) so the runtime loads the correct environment variable.
🧹 Nitpick comments (2)
workers/sample.env (1)
54-65: Clarify the relationship between REDIS_ and CACHE_REDIS_ variables.**The file defines both
REDIS_DB=0(line 55) and a separateCACHE_REDIS_*block (lines 58-65). Perworker_config.py, theCACHE_REDIS_*variables are used for the worker's Redis cache connection, whileREDIS_HOST/REDIS_PORTfromdocker/sample.common.envmay be used elsewhere. Consider adding a brief comment clarifying thatCACHE_REDIS_HOSTdefaults tounstract-redisto match the common config, and when to use each set.💡 Suggested clarification
+# Cache-Specific Redis Configuration +# These override the base REDIS_* vars from common.env for worker cache operations. +# For Docker: use unstract-redis. For local dev: use localhost. CACHE_REDIS_ENABLED=true CACHE_REDIS_HOST=unstract-redis🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@workers/sample.env` around lines 54 - 65, Add a short comment above the REDIS_DB and CACHE_REDIS_* block clarifying that REDIS_* variables (e.g., REDIS_DB) are the general/shared Redis settings used elsewhere (from docker/sample.common.env), while CACHE_REDIS_* (CACHE_REDIS_HOST, CACHE_REDIS_PORT, CACHE_REDIS_DB, etc.) are the worker-specific cache settings consumed by worker_config.py; note that CACHE_REDIS_HOST defaults to "unstract-redis" to match the common config and that if CACHE_REDIS_* are unset the worker will fall back to the shared REDIS_* values.docker/sample.common.env (1)
57-63: Ensure JSON credential blocks stay in sync with MinIO root credentials.The hardcoded
"key": "minio", "secret": "minio123"in the JSON blocks must matchMINIO_ROOT_USERandMINIO_ROOT_PASSWORD. Consider adding a comment reminding users to update all four JSON blocks if they change the MinIO credentials.💡 Suggested comment addition
# MinIO (Object Storage) # MINIO_ROOT_USER/PASSWORD are used by the MinIO container on init. -# Update the JSON blocks below if you change these credentials. +# IMPORTANT: Update ALL FOUR JSON blocks below if you change these credentials. +# The "key" and "secret" values must match MINIO_ROOT_USER and MINIO_ROOT_PASSWORD. # =============================================================================🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docker/sample.common.env` around lines 57 - 63, The JSON credential blocks (WORKFLOW_EXECUTION_FILE_STORAGE_CREDENTIALS, API_FILE_STORAGE_CREDENTIALS, PERMANENT_REMOTE_STORAGE, TEMPORARY_REMOTE_STORAGE) hardcode "key" and "secret" which must match MINIO_ROOT_USER and MINIO_ROOT_PASSWORD; add a clear single-line comment above these entries reminding maintainers to keep the JSON blocks in sync with MINIO_ROOT_USER and MINIO_ROOT_PASSWORD (or update all four JSON envs) when changing the MinIO credentials; reference MINIO_ROOT_USER, MINIO_ROOT_PASSWORD, and the four JSON env var names in that comment so it’s obvious which values must be updated.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@platform-service/sample.env`:
- Around line 21-23: The sample.env contains REMOTE_MODEL_PRICES_FILE_PATH but
the code reads MODEL_PRICES_FILE_PATH (referenced in env.py and consumed in
cost_calculation.py), so update sample.env by renaming
REMOTE_MODEL_PRICES_FILE_PATH to MODEL_PRICES_FILE_PATH (preserve the current
value "unstract/cost/model_prices.json" and quoting style) so the runtime loads
the correct environment variable.
---
Nitpick comments:
In `@docker/sample.common.env`:
- Around line 57-63: The JSON credential blocks
(WORKFLOW_EXECUTION_FILE_STORAGE_CREDENTIALS, API_FILE_STORAGE_CREDENTIALS,
PERMANENT_REMOTE_STORAGE, TEMPORARY_REMOTE_STORAGE) hardcode "key" and "secret"
which must match MINIO_ROOT_USER and MINIO_ROOT_PASSWORD; add a clear
single-line comment above these entries reminding maintainers to keep the JSON
blocks in sync with MINIO_ROOT_USER and MINIO_ROOT_PASSWORD (or update all four
JSON envs) when changing the MinIO credentials; reference MINIO_ROOT_USER,
MINIO_ROOT_PASSWORD, and the four JSON env var names in that comment so it’s
obvious which values must be updated.
In `@workers/sample.env`:
- Around line 54-65: Add a short comment above the REDIS_DB and CACHE_REDIS_*
block clarifying that REDIS_* variables (e.g., REDIS_DB) are the general/shared
Redis settings used elsewhere (from docker/sample.common.env), while
CACHE_REDIS_* (CACHE_REDIS_HOST, CACHE_REDIS_PORT, CACHE_REDIS_DB, etc.) are the
worker-specific cache settings consumed by worker_config.py; note that
CACHE_REDIS_HOST defaults to "unstract-redis" to match the common config and
that if CACHE_REDIS_* are unset the worker will fall back to the shared REDIS_*
values.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: b695fecd-37f1-4f3c-8f91-4c4bd6fd8cd0
📒 Files selected for processing (17)
backend/sample.envdocker/README.mddocker/docker-compose-dev-essentials.yamldocker/docker-compose.yamldocker/sample.common.envdocker/sample.essentials.envplatform-service/sample.envplatform-service/src/unstract/platform_service/config.pyplatform-service/src/unstract/platform_service/env.pyplatform-service/src/unstract/platform_service/extensions.pyprompt-service/sample.envprompt-service/src/unstract/prompt_service/extensions.pyrunner/sample.envworkers/sample.envx2text-service/app/env.pyx2text-service/app/models.pyx2text-service/sample.env
💤 Files with no reviewable changes (1)
- docker/sample.essentials.env
🚧 Files skipped from review as they are similar to previous changes (8)
- x2text-service/app/models.py
- docker/README.md
- platform-service/src/unstract/platform_service/config.py
- runner/sample.env
- docker/docker-compose-dev-essentials.yaml
- prompt-service/src/unstract/prompt_service/extensions.py
- platform-service/src/unstract/platform_service/extensions.py
- platform-service/src/unstract/platform_service/env.py
- Make backend/sample.env and workers/sample.env headers explicit about dependency on docker/sample.common.env - Fix docker/README.md copy commands to be docker/-relative - Make DB_* vars required in platform-service env.py (use EnvManager.get_required_setting instead of os.environ.get) - Remove unused os import from platform-service env.py - Update worker_config.py validation messages to reference docker/sample.common.env for shared vars - Remove unused REMOTE_MODEL_PRICES_FILE_PATH from platform-service sample.env - Improve MinIO credential sync warning in sample.common.env - Add clarifying comments for REDIS_* vs CACHE_REDIS_* in workers sample.env Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
platform-service/src/unstract/platform_service/env.py (1)
1-12:⚠️ Potential issue | 🔴 CriticalMissing
osimport causes NameError.Lines 11-12 use
os.environ.get()but theosmodule is not imported. This will cause aNameErrorat runtime when theEnvclass is loaded.🐛 Proposed fix
+import os + from unstract.platform_service.constants import LogLevel from unstract.platform_service.utils import EnvManager🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@platform-service/src/unstract/platform_service/env.py` around lines 1 - 12, The module references os.environ in the Env class (REDIS_USER and REDIS_PASSWORD) but never imports os; add an import for the os module near the top with the other imports so REDIS_USER = os.environ.get("REDIS_USER") and REDIS_PASSWORD = os.environ.get("REDIS_PASSWORD") do not raise a NameError when the Env class is loaded.
🧹 Nitpick comments (2)
platform-service/sample.env (1)
1-6: Consider making the header warning more explicit.The header says "LOCAL DEVELOPMENT NOTE" but doesn't explicitly warn that this file is incomplete without
docker/sample.common.env. Compare withbackend/sample.envwhich uses "WARNING: This file is NOT runnable by itself."For consistency and to prevent setup issues, consider aligning the header style:
💡 Suggested header alignment
# ----------------------------------------------------------------------------- -# LOCAL DEVELOPMENT NOTE: -# Shared env vars (DB, Redis, RabbitMQ, Flipt, service URLs, MinIO, timeouts) -# live in docker/sample.common.env. When running outside Docker, copy those vars -# here and replace container hostnames with localhost (see common.env header). +# WARNING: This file is NOT runnable by itself. +# It must be combined with docker/sample.common.env (copied as docker/.env), +# which provides required shared vars: DB_*, REDIS_*, CELERY_BROKER_*, +# and other infrastructure settings. Missing these will cause startup failures. +# +# For local dev outside Docker, copy shared vars here and replace +# container hostnames with localhost (see docker/sample.common.env header). # -----------------------------------------------------------------------------🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@platform-service/sample.env` around lines 1 - 6, Update the header in platform-service/sample.env to be an explicit warning that the file is incomplete and not runnable by itself: replace or augment the "LOCAL DEVELOPMENT NOTE" heading with wording similar to "WARNING: This file is NOT runnable by itself — it requires docker/sample.common.env" and clearly instruct developers to copy vars from docker/sample.common.env and swap container hostnames for localhost; reference the existing phrasing in backend/sample.env as a model and ensure the header mentions docker/sample.common.env and the non-runnable status.docker/sample.common.env (1)
54-65: MinIO credentials duplication is fragile but acceptable with warning.The warning on lines 55-56 about keeping credentials in sync across the JSON blocks is good. However, the manual synchronization of
key/secretvalues in four separate JSON strings (lines 61-64) withMINIO_ROOT_USER/MINIO_ROOT_PASSWORD(lines 58-59) is error-prone.Consider documenting a pattern or script for users to validate that these values match, or note that Docker Compose environment variable substitution doesn't work inside JSON strings within env files.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docker/sample.common.env` around lines 54 - 65, The env file duplicates MinIO credentials across MINIO_ROOT_USER / MINIO_ROOT_PASSWORD and four JSON env values (WORKFLOW_EXECUTION_FILE_STORAGE_CREDENTIALS, API_FILE_STORAGE_CREDENTIALS, PERMANENT_REMOTE_STORAGE, TEMPORARY_REMOTE_STORAGE), which is fragile; update the file to: 1) add a clear comment near MINIO_ROOT_USER/MINIO_ROOT_PASSWORD stating that Docker .env variable substitution does not work inside JSON strings and so those JSON blocks must be kept in sync manually, and 2) include or reference a small validation step (e.g., a script name like validate-minio-credentials or a one-liner command) that checks the JSON "key"/"secret" values match MINIO_ROOT_USER/MINIO_ROOT_PASSWORD before starting services, mentioning the exact env names (MINIO_ROOT_USER, MINIO_ROOT_PASSWORD, WORKFLOW_EXECUTION_FILE_STORAGE_CREDENTIALS, API_FILE_STORAGE_CREDENTIALS, PERMANENT_REMOTE_STORAGE, TEMPORARY_REMOTE_STORAGE) so users can locate and fix mismatches.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@platform-service/src/unstract/platform_service/env.py`:
- Around line 1-12: The module references os.environ in the Env class
(REDIS_USER and REDIS_PASSWORD) but never imports os; add an import for the os
module near the top with the other imports so REDIS_USER =
os.environ.get("REDIS_USER") and REDIS_PASSWORD =
os.environ.get("REDIS_PASSWORD") do not raise a NameError when the Env class is
loaded.
---
Nitpick comments:
In `@docker/sample.common.env`:
- Around line 54-65: The env file duplicates MinIO credentials across
MINIO_ROOT_USER / MINIO_ROOT_PASSWORD and four JSON env values
(WORKFLOW_EXECUTION_FILE_STORAGE_CREDENTIALS, API_FILE_STORAGE_CREDENTIALS,
PERMANENT_REMOTE_STORAGE, TEMPORARY_REMOTE_STORAGE), which is fragile; update
the file to: 1) add a clear comment near MINIO_ROOT_USER/MINIO_ROOT_PASSWORD
stating that Docker .env variable substitution does not work inside JSON strings
and so those JSON blocks must be kept in sync manually, and 2) include or
reference a small validation step (e.g., a script name like
validate-minio-credentials or a one-liner command) that checks the JSON
"key"/"secret" values match MINIO_ROOT_USER/MINIO_ROOT_PASSWORD before starting
services, mentioning the exact env names (MINIO_ROOT_USER, MINIO_ROOT_PASSWORD,
WORKFLOW_EXECUTION_FILE_STORAGE_CREDENTIALS, API_FILE_STORAGE_CREDENTIALS,
PERMANENT_REMOTE_STORAGE, TEMPORARY_REMOTE_STORAGE) so users can locate and fix
mismatches.
In `@platform-service/sample.env`:
- Around line 1-6: Update the header in platform-service/sample.env to be an
explicit warning that the file is incomplete and not runnable by itself: replace
or augment the "LOCAL DEVELOPMENT NOTE" heading with wording similar to
"WARNING: This file is NOT runnable by itself — it requires
docker/sample.common.env" and clearly instruct developers to copy vars from
docker/sample.common.env and swap container hostnames for localhost; reference
the existing phrasing in backend/sample.env as a model and ensure the header
mentions docker/sample.common.env and the non-runnable status.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 44be609f-dbde-4c18-a427-091728103bc3
📒 Files selected for processing (7)
backend/sample.envdocker/README.mddocker/sample.common.envplatform-service/sample.envplatform-service/src/unstract/platform_service/env.pyworkers/sample.envworkers/shared/infrastructure/config/worker_config.py
- Replace sample.essentials.env reference with sample.common.env in run-platform.sh (fixes CI build failure) - Keep sample.env merge for compose-level vars (worker settings, etc.) - Update db-setup README to reference common.env and explain env mappings Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docker/scripts/db-setup/README.md`:
- Line 9: The sentence uses "setup" incorrectly as a verb; change "helps setup
the DB user and creates a new schema as well." to use the phrasal verb "set up"
(e.g., "helps set up the DB user and creates a new schema as well.") and scan
the same README text for any other occurrences of "setup" used as a verb and
replace them with "set up" while keeping "setup" for the noun form where
appropriate.
- Around line 5-7: Docs omit the POSTGRES_PASSWORD mapping; add an entry to the
environment mappings list showing POSTGRES_PASSWORD (mapped from DB_PASSWORD) so
the README matches the docker-compose env mappings; update the same list where
POSTGRES_USER, POSTGRES_DB and POSTGRES_SCHEMA are listed (ensure the new
POSTGRES_PASSWORD line sits with those and uses the same phrasing).
In `@run-platform.sh`:
- Around line 241-242: The current sequence calling copy_or_merge_envs for
"sample.common.env" and "sample.env" writes sample.common.env into docker/.env
on first run and then skips merging sample.env because copy_or_merge_envs only
merges when opt_only_env or opt_update are set; update copy_or_merge_envs so
that if the destination file already exists it performs a merge unconditionally
(regardless of opt_only_env/opt_update), ensuring that the second call
copy_or_merge_envs "$script_dir/docker/sample.env" "$script_dir/docker/.env"
"docker compose" will merge its values into docker/.env on first-setup, and keep
existing flag logic only for the case when the destination does not yet exist.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 4436a53c-2a4f-48dc-9d07-3c14dc13bc5c
📒 Files selected for processing (2)
docker/scripts/db-setup/README.mdrun-platform.sh
…vice env.py These optional vars use os.environ.get() (not EnvManager) since they can be empty/unset. The import was accidentally removed in the previous commit. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
platform-service/src/unstract/platform_service/env.py (1)
13-14: Consider keeping a temporaryREDIS_USERNAMEfallback.This rename is fine for the new config, but
platform-servicenow ignores the legacy key entirely while other Redis env readers in the repo still accept both names. That makes mixed-rollout/manual deployments easier to break than necessary.Suggested compatibility shim
- REDIS_USER = os.environ.get("REDIS_USER") + REDIS_USER = os.environ.get("REDIS_USER") or os.environ.get("REDIS_USERNAME") REDIS_PASSWORD = os.environ.get("REDIS_PASSWORD")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@platform-service/src/unstract/platform_service/env.py` around lines 13 - 14, Update the Redis env var handling in env.py to accept the legacy name as a fallback: keep the new REDIS_USER variable but if os.environ.get("REDIS_USER") is None, read from "REDIS_USERNAME" instead so mixed rollouts don't break; adjust the REDIS_USER assignment (and leave REDIS_PASSWORD unchanged) so callers still use REDIS_USER while the code tolerates the older REDIS_USERNAME key.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@platform-service/src/unstract/platform_service/env.py`:
- Around line 13-14: Update the Redis env var handling in env.py to accept the
legacy name as a fallback: keep the new REDIS_USER variable but if
os.environ.get("REDIS_USER") is None, read from "REDIS_USERNAME" instead so
mixed rollouts don't break; adjust the REDIS_USER assignment (and leave
REDIS_PASSWORD unchanged) so callers still use REDIS_USER while the code
tolerates the older REDIS_USERNAME key.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 0ed35965-9dc7-4fe5-a2e0-251ece1bc8ae
📒 Files selected for processing (1)
platform-service/src/unstract/platform_service/env.py
- Add missing POSTGRES_PASSWORD mapping to db-setup README - Fix grammar: "helps setup" → "helps set up" - Fix copy_or_merge_envs to always merge when dest exists, ensuring sample.env gets merged into .env on first setup Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
mathumathi-zipstack
requested review from
chandrasekharan-zipstack,
gaya3-zipstack and
ritwik-g
docker/sample.common.env
Outdated
Comment on lines
+61
to
+69
| WORKFLOW_EXECUTION_FILE_STORAGE_CREDENTIALS='{"provider": "minio", "credentials": {"endpoint_url": "http://unstract-minio:9000", "key": "minio", "secret": "minio123"}}' | ||
| API_FILE_STORAGE_CREDENTIALS='{"provider": "minio", "credentials": {"endpoint_url": "http://unstract-minio:9000", "key": "minio", "secret": "minio123"}}' | ||
| PERMANENT_REMOTE_STORAGE='{"provider": "minio", "credentials": {"endpoint_url": "http://unstract-minio:9000", "key": "minio", "secret": "minio123"}}' | ||
| TEMPORARY_REMOTE_STORAGE='{"provider": "minio", "credentials": {"endpoint_url": "http://unstract-minio:9000", "key": "minio", "secret": "minio123"}}' | ||
| REMOTE_PROMPT_STUDIO_FILE_PATH="unstract/prompt-studio-data" | ||
|
|
||
| # File execution directory prefixes | ||
| WORKFLOW_EXECUTION_DIR_PREFIX="unstract/execution" | ||
| API_EXECUTION_DIR_PREFIX="unstract/api" |
Contributor
There was a problem hiding this comment.
Not sure if these envs are needed everywhere. It's explicitly needed for backend and prompt-service (maybe workers too) but its not needed for others I believe. Can you recheck the necessity for this to be common?
Author
There was a problem hiding this comment.
These are used by backend, workers, runner, and prompt-service (4 out of 6 services). Moving them to per-service files would duplicate the same MinIO credentials across 4 sample.env files — which is what this PR is trying to eliminate. Extra unused vars in platform-service/x2text-service containers are harmless since they just don't read them. Will move them out if you'd prefer though.
…allbacks Address PR review comments: - Merge docker/sample.common.env + docker/sample.env into single docker/sample.env for naming consistency with other folders - Remove unused Qdrant env vars (not read by any Python code) - Remove unused ADAPTER_LLMW_STATUS_RETRIES - Move LLMWhisperer vars to backend and workers sample.env (only services that use them via workflow-execution) - Add backward-compatible fallbacks for renamed env vars (PG_BE_* -> DB_*, DB_USERNAME -> DB_USER, REDIS_USERNAME -> REDIS_USER) to support rolling deployments - Simplify run-platform.sh to single copy_or_merge_envs call - Update all references across READMEs, sample.env headers, and worker_config.py validation messages Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
for more information, see https://pre-commit.ci
Contributor
Test ResultsSummary
Runner Tests - Full Report
|
|
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
3 participants
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.
What
docker/sample.envfile with zero credential duplicationWhy
sample.essentials.env, per-servicesample.envfiles, and within the env file itself (e.g.,POSTGRES_USERandDB_USERboth set to the same value)How
Env consolidation
docker/sample.essentials.envand olddocker/sample.env(compose/worker config) into a singledocker/sample.env-- consistent naming with other service foldersenvironment:mappings that derive infra-specific vars from app vars at compose time:POSTGRES_USER: ${DB_USER}-- Postgres container gets its required var from the singleDB_USERRABBITMQ_DEFAULT_USER: ${CELERY_BROKER_USER}-- RabbitMQ gets its var fromCELERY_BROKER_USERFLIPT_DB_URL-- composed fromDB_USER,DB_PASSWORD,DB_NAMEin the compose filecommon.envto.envso Docker Compose auto-reads it for variable substitutionPG_BE_*toDB_*,DB_USERNAMEtoDB_USER,REDIS_USERNAMEtoREDIS_USERsample.envfilesCleanup of unused/misplaced vars
QDRANT_USER/PASS/DB-- not read by any Python code (Qdrant adapter uses URL + API key from settings, not env vars)ADAPTER_LLMW_STATUS_RETRIES-- defined in env files but unused in codeADAPTER_LLMW_POLL_INTERVALandADAPTER_LLMW_MAX_POLLStobackend/sample.envandworkers/sample.env(only services that use them via workflow-execution)Rolling deployment safety
DB_HOSTfalls back toPG_BE_HOST,REDIS_USERfalls back toREDIS_USERNAME, etc.DB_HOSTfalls back toPG_BE_HOST, etc.DB_USERfalls back toDB_USERNAMECan this PR break any existing features
PG_BE_*,DB_USERNAME,REDIS_USERNAME), so existing env files continue to work without changes.docker/sample.env. The oldessentials.envandcommon.envfile names are no longer referenced.Database Migrations
Env Config
docker/sample.env-- Single source of truth (merged from oldsample.essentials.env+ oldsample.env)docker/sample.essentials.env-- DELETEDdocker/sample.common.env-- DELETED (was intermediate, merged intosample.env)sample.envfiles -- stripped of vars now in docker/sample.envenv_filechanged to./.envPG_BE_HOSTtoDB_HOST,PG_BE_PORTtoDB_PORT,PG_BE_USERNAMEtoDB_USER,PG_BE_PASSWORDtoDB_PASSWORD,PG_BE_DATABASEtoDB_NAMEDB_USERNAMEtoDB_USER,REDIS_USERNAMEtoREDIS_USERNotes on Testing
docker compose configresolves all env vars correctly for both compose filesPOSTGRES_USER=unstract_devderived fromDB_USERRABBITMQ_DEFAULT_USER=adminderived fromCELERY_BROKER_USERFLIPT_DB_URLcomposed from DB_* varsDB_HOST) and legacy fallbacks (PG_BE_HOST)Checklist
I have read and understood the Contribution Guidelines.
Generated with Claude Code