Merged
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Greptile Summary
This PR introduces an 'Easy Install' feature that enables one-command deployment of Onyx through significant infrastructure changes and comprehensive rebranding. The changes span three main areas:
Docker Compose Consolidation: Multiple docker-compose files (gpu-dev, dev, prod variants) have been consolidated into a single unified docker-compose.yml with comprehensive documentation and production guidance. A new installation script (install.sh) provides automated setup with colorized output, health checks, and resource validation. The new env.template centralizes all configuration variables with sensible defaults, replacing scattered environment variable declarations.
Systematic Rebranding: All configuration variables, environment settings, and internal references have been updated from 'DANSWER' to 'ONYX' prefixes. This includes Slack bot configurations (DANSWER_BOT_* → ONYX_BOT_*), model interaction logging (LOG_DANSWER_MODEL_INTERACTIONS → LOG_ONYX_MODEL_INTERACTIONS), and related imports across backend services. Some emoji configurations retain the old naming, creating minor inconsistencies.
Configuration Simplification: Several frontend build arguments have been removed (NEXT_PUBLIC_DISABLE_STREAMING, NEXT_PUBLIC_NEW_CHAT_DIRECTS_TO_SAME_PERSONA) to reduce deployment complexity. Nginx configurations have been streamlined with production-ready templates (app.conf.template.prod) that include SSL/TLS support, header mapping, and security considerations. The QA_PROMPT_OVERRIDE configuration has been deprecated and removed.
The changes integrate by creating a cohesive deployment experience where users can run a single installation script that downloads the unified docker-compose configuration, sets up the environment file with working defaults, and launches all services with proper dependencies and health monitoring.
Confidence score: 2/5
- This PR has several critical issues that will prevent successful deployment, particularly malformed YAML syntax and broken file references
- Score reflects serious deployment-breaking issues in CloudFormation templates and potential missing nginx configuration files
- Pay close attention to AWS CloudFormation templates, nginx template references, and the installation script's hardcoded branch references
28 files reviewed, 12 comments
Comment on lines
128
to
129
| Environment: | ||
| - Name: NEXT_PUBLIC_DISABLE_STREAMING | ||
| Value: "false" | ||
| - Name: NEXT_PUBLIC_NEW_CHAT_DIRECTS_TO_SAME_PERSONA | ||
| Value: "false" |
Contributor
There was a problem hiding this comment.
syntax: Syntax error: orphaned 'Value: "false"' without corresponding 'Name' field. This will cause CloudFormation deployment to fail.
Suggested change
| Environment: | |
| - Name: NEXT_PUBLIC_DISABLE_STREAMING | |
| Value: "false" | |
| - Name: NEXT_PUBLIC_NEW_CHAT_DIRECTS_TO_SAME_PERSONA | |
| Value: "false" | |
| Environment: | |
| - Name: INTERNAL_URL |
deployment/docker_compose/install.sh
Outdated
| print_step "Setting up nginx configuration" | ||
|
|
||
| # Base URL for nginx files | ||
| NGINX_BASE_URL="https://raw.githubusercontent.com/onyx-dot-app/onyx/docker-compose-easy/deployment/data/nginx" |
Contributor
There was a problem hiding this comment.
logic: Same hardcoded branch issue here. Consider using a variable for branch name.
Suggested change
| NGINX_BASE_URL="https://raw.githubusercontent.com/onyx-dot-app/onyx/docker-compose-easy/deployment/data/nginx" | |
| NGINX_BASE_URL="https://raw.githubusercontent.com/onyx-dot-app/onyx/main/deployment/data/nginx" |
deployment/docker_compose/install.sh
Outdated
Comment on lines
253
to
259
| cd onyx_data/deployment && $COMPOSE_CMD -f docker-compose.yml pull && cd ../.. | ||
|
|
||
| # Start services | ||
| print_step "Starting Onyx services" | ||
| print_info "Launching containers..." | ||
| echo "" | ||
| cd onyx_data/deployment && $COMPOSE_CMD -f docker-compose.yml up -d && cd ../.. |
Contributor
There was a problem hiding this comment.
logic: Directory changes without proper error handling. If cd fails, subsequent commands run in wrong location.
| max-file: "6" | ||
| # The specified script waits for the api_server to start up. | ||
| # Without this we've seen issues where nginx shows no error logs but | ||
| # does not recieve any traffic |
Contributor
There was a problem hiding this comment.
syntax: Typo in comment: 'recieve' should be 'receive'
Suggested change
| # does not recieve any traffic | |
| # does not receive any traffic |
Comment on lines
36
to
37
|
|
||
| ARG NEXT_PUBLIC_NEW_CHAT_DIRECTS_TO_SAME_PERSONA | ||
| ENV NEXT_PUBLIC_NEW_CHAT_DIRECTS_TO_SAME_PERSONA=${NEXT_PUBLIC_NEW_CHAT_DIRECTS_TO_SAME_PERSONA} | ||
|
|
Contributor
There was a problem hiding this comment.
style: Unnecessary empty lines left from removed environment variables. Clean up whitespace.
Comment on lines
118
to
119
|
|
||
| ARG NEXT_PUBLIC_NEW_CHAT_DIRECTS_TO_SAME_PERSONA | ||
| ENV NEXT_PUBLIC_NEW_CHAT_DIRECTS_TO_SAME_PERSONA=${NEXT_PUBLIC_NEW_CHAT_DIRECTS_TO_SAME_PERSONA} | ||
|
|
Contributor
There was a problem hiding this comment.
style: Unnecessary empty lines left from removed environment variables. Clean up whitespace.
| ################################################################################ | ||
| ## Database Configuration | ||
| POSTGRES_USER=postgres | ||
| POSTGRES_PASSWORD=password |
Contributor
There was a problem hiding this comment.
style: Using 'password' as default for production template poses security risk
Comment on lines
155
to
156
| MODEL_SERVER_HOST=inference_model_server | ||
| INDEXING_MODEL_SERVER_HOST=indexing_model_server |
Contributor
There was a problem hiding this comment.
logic: Duplicate MODEL_SERVER_HOST assignment - line 214 also sets this variable
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
21 issues found across 30 files
Prompt for AI agents (all 21 issues)
Understand the root cause of the following 21 issues and fix them.
<file name="deployment/docker_compose/docker-compose.prod-no-letsencrypt.yml">
<violation number="1" location="deployment/docker_compose/docker-compose.prod-no-letsencrypt.yml:233">
Nonexistent nginx template referenced; container will fail to generate config</violation>
</file>
<file name="backend/onyx/onyxbot/slack/handlers/handle_message.py">
<violation number="1" location="backend/onyx/onyxbot/slack/handlers/handle_message.py:55">
Guarding with `not` allows negative values; a negative reminder offset results in a past timestamp. Use `<= 0` to disable on non-positive values, or clamp at parse time for safety.
(Based on your team's feedback about validating env-derived timeout multipliers at parse time and providing a safe default.)</violation>
</file>
<file name="backend/onyx/configs/app_configs.py">
<violation number="1" location="backend/onyx/configs/app_configs.py:670">
Breaking change: env var renamed without backward-compatible fallback; existing LOG_DANSWER_MODEL_INTERACTIONS will be ignored.</violation>
</file>
<file name="backend/onyx/onyxbot/slack/utils.py">
<violation number="1" location="backend/onyx/onyxbot/slack/utils.py:92">
Negative response-limit values will block all messages; treat <= 0 as unlimited instead of only == 0.</violation>
<violation number="2" location="backend/onyx/onyxbot/slack/utils.py:107">
Global message counter is updated without synchronization, leading to race conditions under concurrent requests.</violation>
<violation number="3" location="backend/onyx/onyxbot/slack/utils.py:211">
Guard retry tries to be >= 1 so a misconfigured env (0/negative) doesn’t disable retries.</violation>
<violation number="4" location="backend/onyx/onyxbot/slack/utils.py:648">
Normalize max_qpm: treat negatives as unlimited (None) or clamp to a positive value to avoid permanently blocking requests.</violation>
<violation number="5" location="backend/onyx/onyxbot/slack/utils.py:649">
Clamp max_wait_time to non-negative to avoid immediate TimeoutError with a misconfigured negative value.</violation>
<violation number="6" location="backend/onyx/onyxbot/slack/utils.py:709">
Strip whitespace before lowercasing to handle env values with accidental spaces.</violation>
</file>
<file name="deployment/aws_ecs_fargate/cloudformation/services/onyx_nginx_service_template.yaml">
<violation number="1" location="deployment/aws_ecs_fargate/cloudformation/services/onyx_nginx_service_template.yaml:160">
curl download lacks --fail; HTTP 4xx/5xx won't break the chain, risking invalid config being used.</violation>
</file>
<file name="deployment/docker_compose/docker-compose.yml">
<violation number="1" location="deployment/docker_compose/docker-compose.yml:159">
Case-sensitive check prevents disabling model server when DISABLE_MODEL_SERVER=true; compare to "true" (lowercase) to match env defaults.</violation>
<violation number="2" location="deployment/docker_compose/docker-compose.yml:295">
Avoid floating image tag for MinIO; pin to a specific release for reproducibility and safer upgrades.</violation>
</file>
<file name="deployment/docker_compose/install.sh">
<violation number="1" location="deployment/docker_compose/install.sh:58">
Hard-coding a feature branch (docker-compose-easy) makes installs fragile and non-reproducible; use a stable ref such as main or a pinned tag/commit.</violation>
<violation number="2" location="deployment/docker_compose/install.sh:202">
Downloading from a moving feature branch risks breakage; point to a stable ref like main or a pinned tag/commit.</violation>
<violation number="3" location="deployment/docker_compose/install.sh:253">
Using cd ... && ... && cd ../.. with set -e can leave the script in the wrong directory if the middle command fails; wrap the operation in a subshell to avoid side effects and ensure proper failure behavior.</violation>
</file>
<file name="deployment/helm/charts/onyx/values.yaml">
<violation number="1" location="deployment/helm/charts/onyx/values.yaml:859">
Added env var ONYX_BOT_DISABLE_COT appears unused; consider removing it or wiring it into the app to avoid configuration drift.</violation>
</file>
<file name="deployment/data/nginx/app.conf.template">
<violation number="1" location="deployment/data/nginx/app.conf.template:38">
Overwrites X-Forwarded-* with $scheme/$host/$server_port, dropping upstream values; behind a TLS-terminating proxy this can misreport https→http (and 443→80), causing incorrect redirects/cookie/security behavior. Reintroduce fallback to $http_x_forwarded_* (via map) or otherwise preserve upstream headers.</violation>
</file>
<file name="deployment/data/nginx/app.conf.template.prod">
<violation number="1" location="deployment/data/nginx/app.conf.template.prod:56">
Do not trust client-supplied X-Forwarded-Proto on public :80; set it from $scheme or restrict to trusted proxies.</violation>
<violation number="2" location="deployment/data/nginx/app.conf.template.prod:57">
Avoid propagating client X-Forwarded-Host on :80; set from $host instead.</violation>
<violation number="3" location="deployment/data/nginx/app.conf.template.prod:58">
Do not trust client X-Forwarded-Port on :80; set from $server_port.</violation>
</file>
<file name="deployment/docker_compose/env.template">
<violation number="1" location="deployment/docker_compose/env.template:155">
Duplicate environment variable definition for MODEL_SERVER_HOST; later entry overrides the earlier one. Remove the duplicate to avoid confusion.</violation>
</file>
Since this is your first cubic review, here's how it works:
- cubic automatically reviews your code and comments on bugs and improvements
- Teach cubic by replying to its comments. cubic learns from your replies and gets better over time
- Ask questions if you need clarification on any suggestion
React with 👍 or 👎 to teach cubic. Mention @cubic-dev-ai to give feedback, ask questions, or re-run the review.
| command: > | ||
| /bin/sh -c "dos2unix /etc/nginx/conf.d/run-nginx.sh | ||
| && /etc/nginx/conf.d/run-nginx.sh app.conf.template.no-letsencrypt" | ||
| && /etc/nginx/conf.d/run-nginx.sh app.conf.template.prod.no-letsencrypt" |
Contributor
There was a problem hiding this comment.
Nonexistent nginx template referenced; container will fail to generate config
Prompt for AI agents
Address the following comment on deployment/docker_compose/docker-compose.prod-no-letsencrypt.yml at line 233:
<comment>Nonexistent nginx template referenced; container will fail to generate config</comment>
<file context>
@@ -232,7 +230,7 @@ services:
command: >
/bin/sh -c "dos2unix /etc/nginx/conf.d/run-nginx.sh
- && /etc/nginx/conf.d/run-nginx.sh app.conf.template.no-letsencrypt"
+ && /etc/nginx/conf.d/run-nginx.sh app.conf.template.prod.no-letsencrypt"
env_file:
- .env.nginx
</file context>
Suggested change
| && /etc/nginx/conf.d/run-nginx.sh app.conf.template.prod.no-letsencrypt" | |
| && /etc/nginx/conf.d/run-nginx.sh app.conf.template.no-letsencrypt" |
| logger = setup_logger(extra={SLACK_CHANNEL_ID: details.channel_to_respond}) | ||
|
|
||
| if not DANSWER_BOT_FEEDBACK_REMINDER: | ||
| if not ONYX_BOT_FEEDBACK_REMINDER: |
Contributor
There was a problem hiding this comment.
Guarding with not allows negative values; a negative reminder offset results in a past timestamp. Use <= 0 to disable on non-positive values, or clamp at parse time for safety.
(Based on your team's feedback about validating env-derived timeout multipliers at parse time and providing a safe default.)
Prompt for AI agents
Address the following comment on backend/onyx/onyxbot/slack/handlers/handle_message.py at line 55:
<comment>Guarding with `not` allows negative values; a negative reminder offset results in a past timestamp. Use `<= 0` to disable on non-positive values, or clamp at parse time for safety.
(Based on your team's feedback about validating env-derived timeout multipliers at parse time and providing a safe default.)</comment>
<file context>
@@ -52,7 +52,7 @@ def schedule_feedback_reminder(
logger = setup_logger(extra={SLACK_CHANNEL_ID: details.channel_to_respond})
- if not DANSWER_BOT_FEEDBACK_REMINDER:
+ if not ONYX_BOT_FEEDBACK_REMINDER:
logger.info("Scheduled feedback reminder disabled...")
return None
</file context>
| def get_feedback_visibility() -> FeedbackVisibility: | ||
| try: | ||
| return FeedbackVisibility(DANSWER_BOT_FEEDBACK_VISIBILITY.lower()) | ||
| return FeedbackVisibility(ONYX_BOT_FEEDBACK_VISIBILITY.lower()) |
Contributor
There was a problem hiding this comment.
Strip whitespace before lowercasing to handle env values with accidental spaces.
Prompt for AI agents
Address the following comment on backend/onyx/onyxbot/slack/utils.py at line 709:
<comment>Strip whitespace before lowercasing to handle env values with accidental spaces.</comment>
<file context>
@@ -706,7 +706,7 @@ def waiter(self, func_randid: int) -> None:
def get_feedback_visibility() -> FeedbackVisibility:
try:
- return FeedbackVisibility(DANSWER_BOT_FEEDBACK_VISIBILITY.lower())
+ return FeedbackVisibility(ONYX_BOT_FEEDBACK_VISIBILITY.lower())
except ValueError:
return FeedbackVisibility.PRIVATE
</file context>
Suggested change
| return FeedbackVisibility(ONYX_BOT_FEEDBACK_VISIBILITY.lower()) | |
| return FeedbackVisibility(ONYX_BOT_FEEDBACK_VISIBILITY.strip().lower()) |
| self.max_qpm: int | None = DANSWER_BOT_MAX_QPM | ||
| self.max_wait_time = DANSWER_BOT_MAX_WAIT_TIME | ||
| self.max_qpm: int | None = ONYX_BOT_MAX_QPM | ||
| self.max_wait_time = ONYX_BOT_MAX_WAIT_TIME |
Contributor
There was a problem hiding this comment.
Clamp max_wait_time to non-negative to avoid immediate TimeoutError with a misconfigured negative value.
Prompt for AI agents
Address the following comment on backend/onyx/onyxbot/slack/utils.py at line 649:
<comment>Clamp max_wait_time to non-negative to avoid immediate TimeoutError with a misconfigured negative value.</comment>
<file context>
@@ -645,8 +645,8 @@ def slack_usage_report(action: str, sender_id: str | None, client: WebClient) ->
- self.max_qpm: int | None = DANSWER_BOT_MAX_QPM
- self.max_wait_time = DANSWER_BOT_MAX_WAIT_TIME
+ self.max_qpm: int | None = ONYX_BOT_MAX_QPM
+ self.max_wait_time = ONYX_BOT_MAX_WAIT_TIME
self.active_question = 0
self.last_reset_time = time.time()
</file context>
Suggested change
| self.max_wait_time = ONYX_BOT_MAX_WAIT_TIME | |
| self.max_wait_time = max(0, ONYX_BOT_MAX_WAIT_TIME) |
| proxy_set_header X-Forwarded-Proto $forwarded_proto; | ||
| proxy_set_header X-Forwarded-Host $forwarded_host; | ||
| proxy_set_header X-Forwarded-Port $forwarded_port; | ||
| proxy_set_header X-Forwarded-Proto $scheme; |
Contributor
There was a problem hiding this comment.
Overwrites X-Forwarded-* with $scheme/$host/$server_port, dropping upstream values; behind a TLS-terminating proxy this can misreport https→http (and 443→80), causing incorrect redirects/cookie/security behavior. Reintroduce fallback to $http_x_forwarded_* (via map) or otherwise preserve upstream headers.
Prompt for AI agents
Address the following comment on deployment/data/nginx/app.conf.template at line 38:
<comment>Overwrites X-Forwarded-* with $scheme/$host/$server_port, dropping upstream values; behind a TLS-terminating proxy this can misreport https→http (and 443→80), causing incorrect redirects/cookie/security behavior. Reintroduce fallback to $http_x_forwarded_* (via map) or otherwise preserve upstream headers.</comment>
<file context>
@@ -53,9 +35,9 @@ server {
- proxy_set_header X-Forwarded-Proto $forwarded_proto;
- proxy_set_header X-Forwarded-Host $forwarded_host;
- proxy_set_header X-Forwarded-Port $forwarded_port;
+ proxy_set_header X-Forwarded-Proto $scheme;
+ proxy_set_header X-Forwarded-Host $host;
+ proxy_set_header X-Forwarded-Port $server_port;
</file context>
| proxy_set_header X-Forwarded-Host $host; | ||
| proxy_set_header X-Forwarded-Port $server_port; | ||
| proxy_set_header X-Forwarded-Proto $forwarded_proto; | ||
| proxy_set_header X-Forwarded-Host $forwarded_host; |
Contributor
There was a problem hiding this comment.
Avoid propagating client X-Forwarded-Host on :80; set from $host instead.
Prompt for AI agents
Address the following comment on deployment/data/nginx/app.conf.template.prod at line 57:
<comment>Avoid propagating client X-Forwarded-Host on :80; set from $host instead.</comment>
<file context>
@@ -35,9 +53,9 @@ server {
- proxy_set_header X-Forwarded-Host $host;
- proxy_set_header X-Forwarded-Port $server_port;
+ proxy_set_header X-Forwarded-Proto $forwarded_proto;
+ proxy_set_header X-Forwarded-Host $forwarded_host;
+ proxy_set_header X-Forwarded-Port $forwarded_port;
proxy_set_header Host $host;
</file context>
Suggested change
| proxy_set_header X-Forwarded-Host $forwarded_host; | |
| proxy_set_header X-Forwarded-Host $host; |
| proxy_set_header X-Forwarded-Proto $scheme; | ||
| proxy_set_header X-Forwarded-Host $host; | ||
| proxy_set_header X-Forwarded-Port $server_port; | ||
| proxy_set_header X-Forwarded-Proto $forwarded_proto; |
Contributor
There was a problem hiding this comment.
Do not trust client-supplied X-Forwarded-Proto on public :80; set it from $scheme or restrict to trusted proxies.
Prompt for AI agents
Address the following comment on deployment/data/nginx/app.conf.template.prod at line 56:
<comment>Do not trust client-supplied X-Forwarded-Proto on public :80; set it from $scheme or restrict to trusted proxies.</comment>
<file context>
@@ -35,9 +53,9 @@ server {
- proxy_set_header X-Forwarded-Proto $scheme;
- proxy_set_header X-Forwarded-Host $host;
- proxy_set_header X-Forwarded-Port $server_port;
+ proxy_set_header X-Forwarded-Proto $forwarded_proto;
+ proxy_set_header X-Forwarded-Host $forwarded_host;
+ proxy_set_header X-Forwarded-Port $forwarded_port;
</file context>
Suggested change
| proxy_set_header X-Forwarded-Proto $forwarded_proto; | |
| proxy_set_header X-Forwarded-Proto $scheme; |
| proxy_set_header X-Forwarded-Port $server_port; | ||
| proxy_set_header X-Forwarded-Proto $forwarded_proto; | ||
| proxy_set_header X-Forwarded-Host $forwarded_host; | ||
| proxy_set_header X-Forwarded-Port $forwarded_port; |
Contributor
There was a problem hiding this comment.
Do not trust client X-Forwarded-Port on :80; set from $server_port.
Prompt for AI agents
Address the following comment on deployment/data/nginx/app.conf.template.prod at line 58:
<comment>Do not trust client X-Forwarded-Port on :80; set from $server_port.</comment>
<file context>
@@ -35,9 +53,9 @@ server {
- proxy_set_header X-Forwarded-Port $server_port;
+ proxy_set_header X-Forwarded-Proto $forwarded_proto;
+ proxy_set_header X-Forwarded-Host $forwarded_host;
+ proxy_set_header X-Forwarded-Port $forwarded_port;
proxy_set_header Host $host;
</file context>
Suggested change
| proxy_set_header X-Forwarded-Port $forwarded_port; | |
| proxy_set_header X-Forwarded-Port $server_port; |
| # USE_SEMANTIC_KEYWORD_EXPANSIONS_BASIC_SEARCH= | ||
|
|
||
| ## Model Configuration | ||
| MODEL_SERVER_HOST=inference_model_server |
Contributor
There was a problem hiding this comment.
Duplicate environment variable definition for MODEL_SERVER_HOST; later entry overrides the earlier one. Remove the duplicate to avoid confusion.
Prompt for AI agents
Address the following comment on deployment/docker_compose/env.template at line 155:
<comment>Duplicate environment variable definition for MODEL_SERVER_HOST; later entry overrides the earlier one. Remove the duplicate to avoid confusion.</comment>
<file context>
@@ -0,0 +1,215 @@
+# USE_SEMANTIC_KEYWORD_EXPANSIONS_BASIC_SEARCH=
+
+## Model Configuration
+MODEL_SERVER_HOST=inference_model_server
+INDEXING_MODEL_SERVER_HOST=indexing_model_server
+# EMBEDDING_BATCH_SIZE=
</file context>
Suggested change
| MODEL_SERVER_HOST=inference_model_server | |
| # MODEL_SERVER_HOST=inference_model_server |
✅ Addressed in e44a636
yuhongsun96
force-pushed
the
docker-compose-easy
branch
from
1bb7402 to
11a0728
Compare
yuhongsun96
commented
Sep 20, 2025
| # Onyx Slack Bot Configs | ||
| ##### | ||
| DANSWER_BOT_NUM_RETRIES = int(os.environ.get("DANSWER_BOT_NUM_RETRIES", "5")) | ||
| ONYX_BOT_NUM_RETRIES = int(os.environ.get("ONYX_BOT_NUM_RETRIES", "5")) |
Contributor
Author
There was a problem hiding this comment.
These slackbot changes are minimally risky imo. Since they are default off riskier behaviors like responding in every channel, it shouldn't cause any issues if they no longer apply. Also some of these I suspect don't work anyway.
Contributor
There was a problem hiding this comment.
This also implies that the new env variable is set properly and can be retrieved. I think it's a matter of making sure the new one is there not so much that the old was stays or not.
Do feel like this is quite risky.
Contributor
Author
There was a problem hiding this comment.
The ones that don't work (I removed some), don't feel super critical. It's outside the scope of this PR. My only concern is with variable renaming when some people might still be deploying with old vars and this becoming a "regression" even though it's not.
But reading through what they do, I'm not too concerned about that case.
yuhongsun96
commented
Sep 20, 2025
| HostPort: 3000 | ||
| Protocol: tcp | ||
| Environment: | ||
| - Name: NEXT_PUBLIC_DISABLE_STREAMING |
Contributor
Author
There was a problem hiding this comment.
Not used I believe, might want to double check
yuhongsun96
commented
Sep 20, 2025
| @@ -4,24 +4,6 @@ log_format custom_main '$remote_addr - $remote_user [$time_local] "$request" ' | |||
| '"$http_user_agent" "$http_x_forwarded_for" ' | |||
Contributor
Author
There was a problem hiding this comment.
Following same convention as docker compose file naming, there's a default version and a prod version, just renamed the files, the diffs here are just from git not understanding the file renamings
yuhongsun96
commented
Sep 20, 2025
| '"$http_user_agent" "$http_x_forwarded_for" ' | ||
| 'rt=$request_time'; | ||
|
|
||
| # Map X-Forwarded-Proto or fallback to $scheme |
Contributor
Author
There was a problem hiding this comment.
May want to quickly sanity check that nothing is broken from the renaming. I did not test the prod setup
Contributor
justin-tahara
left a comment
justin-tahara
left a comment
There was a problem hiding this comment.
Overall I think this PR is a bit too dangerous to just merge.
My opinion is to break this up into smaller PR's that are easier to digest and easier to read through.
Ideally the Slack bot changes are all one PR, then the compose unification is another. Helm can be it's own PR and lastly the script comes in once all of the individual workflows have been tested.
From personal experience, I do prefer Python Scripts for maintainability and ease of understanding but if this is truly a one line command approach to onyx then I can be convinced to do bash instead.
Maybe we can even have both options be available but again two things to maintain is not as easy as one.
The last thing for this is that many workflows will most likely break across the board. Many customers that have hardcoded workflows will definitely complain about this upgrade and thus we should think a bit more about the migration path and how we would introduce this to existing users of Onyx in a non-invasive way such that they are able to also upgrade easily to this new workflow.
Also if we are going to introduce the bash script or any script in general, we should have a integration or test suite to ensure that it never breaks and is the source of truth to upgrading or installing onyx.
| - name: Start Docker containers | ||
| run: | | ||
| cd deployment/docker_compose | ||
| cp env.template .env |
Contributor
There was a problem hiding this comment.
Is this required to get the test to pass? Or are there other dependencies that require this to be set.
Contributor
Author
There was a problem hiding this comment.
Let me try to make it not required
| run: | | ||
| echo "Starting wait-for-service script..." | ||
|
|
||
| docker logs -f danswer-stack-api_server-1 & |
Contributor
There was a problem hiding this comment.
As long as the onyx name is correctly populated I think this is fine.
Contributor
Author
There was a problem hiding this comment.
Ya it's the default now in the compose script, a lot of things are simplified now
Comment on lines
-42
to
+43
| #DANSWER_BOT_SLACK_APP_TOKEN=<REPLACE THIS> | ||
| #DANSWER_BOT_SLACK_BOT_TOKEN=<REPLACE THIS> | ||
| #ONYX_BOT_SLACK_APP_TOKEN=<REPLACE THIS> | ||
| #ONYX_BOT_SLACK_BOT_TOKEN=<REPLACE THIS> |
| if cc_pair.access_type != AccessType.SYNC: | ||
| task_logger.error( | ||
| f"Recieved non-sync CC Pair {cc_pair.id} for external " | ||
| f"Received non-sync CC Pair {cc_pair.id} for external " |
Contributor
Author
There was a problem hiding this comment.
lmao I wanted to fix a bot comment but I realized this same typo appeared a bunch of times so just fixed it across the board
| # Onyx Slack Bot Configs | ||
| ##### | ||
| DANSWER_BOT_NUM_RETRIES = int(os.environ.get("DANSWER_BOT_NUM_RETRIES", "5")) | ||
| ONYX_BOT_NUM_RETRIES = int(os.environ.get("ONYX_BOT_NUM_RETRIES", "5")) |
Contributor
There was a problem hiding this comment.
This also implies that the new env variable is set properly and can be retrieved. I think it's a matter of making sure the new one is there not so much that the old was stays or not.
Do feel like this is quite risky.
| from slack_sdk.errors import SlackApiError | ||
|
|
||
| from onyx.configs.onyxbot_configs import DANSWER_BOT_FEEDBACK_REMINDER | ||
| from onyx.configs.onyxbot_configs import DANSWER_REACT_EMOJI |
Contributor
There was a problem hiding this comment.
Should we not change this one as well?
Contributor
Author
There was a problem hiding this comment.
Ah it's not even a thing I think people should change to be honest... ya we should change it
| from onyx.configs.onyxbot_configs import DANSWER_BOT_DISABLE_DOCS_ONLY_ANSWER | ||
| from onyx.configs.onyxbot_configs import DANSWER_BOT_DISPLAY_ERROR_MSGS | ||
| from onyx.configs.onyxbot_configs import DANSWER_BOT_NUM_RETRIES | ||
| from onyx.configs.onyxbot_configs import DANSWER_REACT_EMOJI |
backend/onyx/onyxbot/slack/utils.py
Outdated
| the limit to be exceeded. | ||
| """ | ||
| if DANSWER_BOT_RESPONSE_LIMIT_PER_TIME_PERIOD == 0: | ||
| if ONYX_BOT_RESPONSE_LIMIT_PER_TIME_PERIOD == 0: |
Contributor
There was a problem hiding this comment.
I think this is a safe change that we should probably just do to be safe
Contributor
There was a problem hiding this comment.
Is the understanding that everything should be routed through the .prod template instead of this one? Should we even keep this one?
Contributor
Author
There was a problem hiding this comment.
This is the default non SSL one. I think it's good to keep, the default deployment uses this. HTTPS setup is kind of a pain in the ass, I still don't fully understand it and we don't want to make the base deployment too painful.
Contributor
There was a problem hiding this comment.
What was the reasoning behind the Bash script? Was this more along the lines of with Bash you don't need to download anything but with Python you would need a working version of python?
I think as someone who is using docker, it probably can be assumed that they have python and so writing a python script would be nice to maintain and easier to debug in my opinion.
Open to hearing what your thoughts are
Contributor
Author
There was a problem hiding this comment.
pip is the python package manager, you can reuse it to manage other packages but it's less intuitive, it's like installing Onyx with npm, probably there's some way to get that working but it's not the right tool.
The other thing is with requiring a python installation which doesn't come out of the box with most operating systems whereas bash typically does. Maybe on Windows most people have pip instead of bash but meh...
yuhongsun96
force-pushed
the
docker-compose-easy
branch
from
a3f02f3 to
06ec9f8
Compare
yuhongsun96
force-pushed
the
docker-compose-easy
branch
from
ebd0814 to
0eba516
Compare
Contributor
There was a problem hiding this comment.
Greptile Overview
Summary
This PR introduces a "one-command install" feature for Onyx that significantly simplifies deployment. The core changes include:
- New install script (
deployment/docker_compose/install.sh) - A comprehensive guided installer that downloads configurations, checks system requirements, and manages Docker Compose deployments with start/stop/delete functionality - Unified configuration - Consolidated into a single
docker-compose.ymlandenv.templatefile, replacing multiple environment-specific configs - Environment variable migration - Complete renaming from
DANSWER_BOT_*toONYX_BOT_*andLOG_DANSWER_*toLOG_ONYX_*across the codebase - Enhanced nginx configs - New production template (
app.conf.template.prod) with proper SSL/TLS support and forwarded header handling - Simplified deployment structure - Removed complexity by standardizing on a single deployment path while maintaining production deployment options
Key issues identified:
- Install script has several directory change operations without proper error handling that could lead to commands executing in wrong directories
- Hardcoded GitHub branch references make the script fragile for non-main branch deployments
- Minor style issue with hardcoded boolean environment variable that should be removed per coding standards
The changes successfully implement the Easy Install goal while maintaining backward compatibility through production deployment variants.
Confidence Score: 3/5
- This PR has useful functionality but contains several directory handling issues that could cause deployment failures
- Score reflects the install script's directory change operations without proper error handling, which could lead to commands running in wrong directories and deployment failures. The hardcoded branch references also make the script fragile.
- The install script (deployment/docker_compose/install.sh) needs attention for directory handling and error management
Important Files Changed
File Analysis
| Filename | Score | Overview |
|---|---|---|
| deployment/docker_compose/install.sh | 3/5 | New one-command install script with several directory handling issues and hardcoded branch references |
| deployment/docker_compose/docker-compose.yml | 4/5 | Unified Docker Compose config with good structure, minor hardcoded boolean variable issue |
| deployment/docker_compose/env.template | 5/5 | Well-structured environment template with DANSWER→ONYX renaming completed properly |
| deployment/data/nginx/app.conf.template.prod | 5/5 | New production nginx template with proper SSL support and forwarded headers handling |
| backend/onyx/configs/onyxbot_configs.py | 5/5 | Environment variable renaming from DANSWER_BOT_* to ONYX_BOT_* completed successfully |
| deployment/aws_ecs_fargate/cloudformation/services/onyx_web_server_service_template.yaml | 4/5 | CloudFormation template updates for service names, no major issues beyond existing syntax error |
Sequence Diagram
sequenceDiagram
participant User
participant InstallScript as install.sh
participant GitHub as GitHub Raw
participant Docker as Docker Engine
participant Compose as Docker Compose
participant Services as Onyx Services
User->>InstallScript: ./install.sh
InstallScript->>InstallScript: Check Docker & resources
InstallScript->>GitHub: Download docker-compose.yml
InstallScript->>GitHub: Download env.template
InstallScript->>GitHub: Download nginx configs
InstallScript->>InstallScript: Create .env from template
InstallScript->>Compose: docker compose pull
Compose->>Docker: Pull Onyx images
InstallScript->>Compose: docker compose up -d
Compose->>Services: Start all containers
InstallScript->>Services: Health check (HTTP 200)
Services-->>User: Onyx ready at localhost:3000
90 files reviewed, 6 comments
| fi | ||
|
|
||
| # GitHub repo base URL - using docker-compose-easy branch | ||
| GITHUB_RAW_URL="https://raw.githubusercontent.com/onyx-dot-app/onyx/main/deployment/docker_compose" |
Contributor
There was a problem hiding this comment.
logic: Hardcoded branch main makes the script fragile if users run it from different branches or tagged releases
Suggested change
| GITHUB_RAW_URL="https://raw.githubusercontent.com/onyx-dot-app/onyx/main/deployment/docker_compose" | |
| GITHUB_RAW_URL="https://raw.githubusercontent.com/onyx-dot-app/onyx/${GIT_BRANCH:-main}/deployment/docker_compose" |
| fi | ||
|
|
||
| # Download nginx config files | ||
| NGINX_BASE_URL="https://raw.githubusercontent.com/onyx-dot-app/onyx/main/deployment/data/nginx" |
Contributor
There was a problem hiding this comment.
logic: Same hardcoded branch issue for nginx files
Suggested change
| NGINX_BASE_URL="https://raw.githubusercontent.com/onyx-dot-app/onyx/main/deployment/data/nginx" | |
| NGINX_BASE_URL="https://raw.githubusercontent.com/onyx-dot-app/onyx/${GIT_BRANCH:-main}/deployment/data/nginx" |
| print_info "This may take several minutes depending on your internet connection..." | ||
| echo "" | ||
| print_info "Downloading Docker images (this may take a while)..." | ||
| cd onyx_data/deployment && $COMPOSE_CMD -f docker-compose.yml pull --quiet && cd ../.. |
Contributor
There was a problem hiding this comment.
logic: Directory change without error handling could lead to commands running in wrong directory
Suggested change
| cd onyx_data/deployment && $COMPOSE_CMD -f docker-compose.yml pull --quiet && cd ../.. | |
| cd onyx_data/deployment && $COMPOSE_CMD -f docker-compose.yml pull --quiet && cd ../.. || { print_error "Failed to pull images"; exit 1; } |
| print_step "Starting Onyx services" | ||
| print_info "Launching containers..." | ||
| echo "" | ||
| cd onyx_data/deployment && $COMPOSE_CMD -f docker-compose.yml up -d && cd ../.. |
Contributor
There was a problem hiding this comment.
logic: Same directory change issue as above
Suggested change
| cd onyx_data/deployment && $COMPOSE_CMD -f docker-compose.yml up -d && cd ../.. | |
| cd onyx_data/deployment && $COMPOSE_CMD -f docker-compose.yml up -d && cd ../.. || { print_error "Failed to start containers"; exit 1; } |
| # Check for restart loops | ||
| print_info "Checking container health status..." | ||
| RESTART_ISSUES=false | ||
| CONTAINERS=$(cd onyx_data/deployment && $COMPOSE_CMD -f docker-compose.yml ps -q) |
Contributor
There was a problem hiding this comment.
logic: Directory change without error handling
Suggested change
| CONTAINERS=$(cd onyx_data/deployment && $COMPOSE_CMD -f docker-compose.yml ps -q) | |
| CONTAINERS=$(cd onyx_data/deployment && $COMPOSE_CMD -f docker-compose.yml ps -q) || { print_error "Failed to list containers"; exit 1; } |
| # Enterprise Edition only | ||
| - NEXT_PUBLIC_THEME=${NEXT_PUBLIC_THEME:-} | ||
| # DO NOT TURN ON unless you have EXPLICIT PERMISSION from Onyx. | ||
| - NEXT_PUBLIC_DO_NOT_USE_TOGGLE_OFF_DANSWER_POWERED=${NEXT_PUBLIC_DO_NOT_USE_TOGGLE_OFF_DANSWER_POWERED:-false} |
Contributor
There was a problem hiding this comment.
style: According to custom rule, remove hardcoded boolean variables instead of setting to constant. This environment variable defaults to false and should be removed entirely
Suggested change
| - NEXT_PUBLIC_DO_NOT_USE_TOGGLE_OFF_DANSWER_POWERED=${NEXT_PUBLIC_DO_NOT_USE_TOGGLE_OFF_DANSWER_POWERED:-false} | |
| # - NEXT_PUBLIC_DO_NOT_USE_TOGGLE_OFF_DANSWER_POWERED=${NEXT_PUBLIC_DO_NOT_USE_TOGGLE_OFF_DANSWER_POWERED:-false} |
Context Used: Rule - When hardcoding a boolean variable to a constant value, remove the variable entirely and clean up all places where it's used rather than just setting it to a constant. (link)
Contributor
There was a problem hiding this comment.
23 issues found across 92 files
Prompt for AI agents (all 23 issues)
Understand the root cause of the following 23 issues and fix them.
<file name="backend/onyx/tools/tool_implementations/custom/custom_tool.py">
<violation number="1" location="backend/onyx/tools/tool_implementations/custom/custom_tool.py:209">
Avoid logging the full args_result_str; it may contain sensitive user or tool-argument data. Log only safe metadata (e.g., length) or explicitly redact the payload.</violation>
</file>
<file name="backend/onyx/configs/app_configs.py">
<violation number="1" location="backend/onyx/configs/app_configs.py:670">
Dropping the old env name breaks backward compatibility; add a fallback to LOG_DANSWER_MODEL_INTERACTIONS to honor existing deployments.</violation>
</file>
<file name="deployment/docker_compose/README.md">
<violation number="1" location="deployment/docker_compose/README.md:16">
Avoid executing an unpinned remote script; pin to a tag/commit and/or verify checksum before execution to reduce supply-chain risk.</violation>
<violation number="2" location="deployment/docker_compose/README.md:47">
Avoid recommending 'latest' for production; suggest pinning a specific version and using 'latest' only when automatic upgrades are desired.</violation>
</file>
<file name="deployment/docker_compose/install.sh">
<violation number="1" location="deployment/docker_compose/install.sh:593">
sed uses basic regex; `|` is not alternation. Use -E or separate expressions to correctly strip both prefixes.</violation>
</file>
<file name="deployment/helm/charts/onyx/Chart.yaml">
<violation number="1" location="deployment/helm/charts/onyx/Chart.yaml:2">
Renaming the chart changes .Chart.Name used in helper templates and selector labels, altering spec.selector.matchLabels for existing Deployments (an immutable field) and likely breaking helm upgrades for existing releases. Preserve backward compatibility (e.g., keep the old chart name or provide a stable nameOverride).</violation>
</file>
<file name="deployment/docker_compose/env.template">
<violation number="1" location="deployment/docker_compose/env.template:41">
Defaulting Postgres to password is insecure; use a strong placeholder or require explicit configuration.</violation>
<violation number="2" location="deployment/docker_compose/env.template:58">
MinIO credentials default to minioadmin/minioadmin; set non-trivial secrets for safer out-of-the-box behavior.</violation>
</file>
<file name="deployment/data/nginx/app.conf.template">
<violation number="1" location="deployment/data/nginx/app.conf.template:38">
Setting X-Forwarded-Proto from $scheme ignores upstream proto; behind TLS termination this will incorrectly report HTTP, breaking origin/cookie/security expectations.</violation>
<violation number="2" location="deployment/data/nginx/app.conf.template:40">
Setting X-Forwarded-Port from $server_port can misreport client port (e.g., 443→80) behind proxies, causing incorrect origin construction and SAML context.</violation>
</file>
<file name="deployment/helm/charts/onyx/templates/configmap.yaml">
<violation number="1" location="deployment/helm/charts/onyx/templates/configmap.yaml:8">
INTERNAL_URL references .Values.api.service.port, but the chart uses api.service.servicePort; this breaks custom port configurations.</violation>
</file>
<file name="backend/onyx/connectors/fireflies/connector.py">
<violation number="1" location="backend/onyx/connectors/fireflies/connector.py:172">
parsed_transcripts may be None if response['data']['transcripts'] is null; iterating over it and calling len() will raise TypeError. Use an explicit fallback to [].</violation>
</file>
<file name="deployment/docker_compose/docker-compose.model-server-test.yml">
<violation number="1" location="deployment/docker_compose/docker-compose.model-server-test.yml:1">
Hard-coding the Compose project name to 'onyx' in a test stack risks collisions with other stacks in this repo that also use 'name: onyx'. Prefer a unique project name here to avoid container/network/volume conflicts when multiple stacks run concurrently.</violation>
</file>
<file name="deployment/docker_compose/docker-compose.prod-no-letsencrypt.yml">
<violation number="1" location="deployment/docker_compose/docker-compose.prod-no-letsencrypt.yml:227">
nginx command points to a missing template file, causing startup failure for the web gateway.</violation>
</file>
<file name="deployment/helm/charts/onyx/templates_disabled/webserver-hpa.yaml">
<violation number="1" location="deployment/helm/charts/onyx/templates_disabled/webserver-hpa.yaml:12">
HPA scaleTargetRef name must match the Deployment name; missing "-web-server" suffix causes it to reference a non-existent target.</violation>
</file>
<file name="deployment/data/nginx/app.conf.template.prod">
<violation number="1" location="deployment/data/nginx/app.conf.template.prod:9">
Do not trust client-provided X-Forwarded-Proto by default; fall back to $scheme unless you implement a trusted proxy list.</violation>
<violation number="2" location="deployment/data/nginx/app.conf.template.prod:15">
Avoid trusting X-Forwarded-Host from clients; default to $host unless you restrict trusted sources.</violation>
<violation number="3" location="deployment/data/nginx/app.conf.template.prod:102">
Typo in comment: use "scheme" instead of "schema" for clarity.</violation>
</file>
<file name="backend/tests/regression/answer_quality/cli_utils.py">
<violation number="1" location="backend/tests/regression/answer_quality/cli_utils.py:190">
Cleanup prefix omits the hyphen, so it won’t match containers launched with "-p onyx-{env_name}"; resources may not be removed.</violation>
</file>
<file name="deployment/helm/charts/onyx/templates_disabled/background-hpa.yaml">
<violation number="1" location="deployment/helm/charts/onyx/templates_disabled/background-hpa.yaml:12">
HPA scaleTargetRef.name does not match the background Deployment name, so autoscaling will not attach to the intended Deployment.</violation>
</file>
<file name="backend/onyx/onyxbot/slack/utils.py">
<violation number="1" location="backend/onyx/onyxbot/slack/utils.py:126">
Condition deviates from docs; only 0 should disable the limit to match onyxbot_configs guidance and avoid masking misconfigurations.</violation>
<violation number="2" location="backend/onyx/onyxbot/slack/utils.py:141">
Shared counters are updated without synchronization, risking race conditions and incorrect rate limiting under concurrent threads.</violation>
</file>
<file name="deployment/docker_compose/docker-compose.yml">
<violation number="1" location="deployment/docker_compose/docker-compose.yml:170">
Conditional check uses "True" instead of lowercase "true"; DISABLE_MODEL_SERVER=true will not disable the service.</violation>
</file>
React with 👍 or 👎 to teach cubic. Mention @cubic-dev-ai to give feedback, ask questions, or re-run the review.
| # pretend like nothing happened if not parse-able | ||
| logger.error( | ||
| f"Failed to parse args for '{self.name}' tool. Recieved: {args_result_str}" | ||
| f"Failed to parse args for '{self.name}' tool. Received: {args_result_str}" |
Contributor
There was a problem hiding this comment.
Avoid logging the full args_result_str; it may contain sensitive user or tool-argument data. Log only safe metadata (e.g., length) or explicitly redact the payload.
Prompt for AI agents
Address the following comment on backend/onyx/tools/tool_implementations/custom/custom_tool.py at line 209:
<comment>Avoid logging the full args_result_str; it may contain sensitive user or tool-argument data. Log only safe metadata (e.g., length) or explicitly redact the payload.</comment>
<file context>
@@ -206,7 +206,7 @@ def get_args_for_non_tool_calling_llm(
# pretend like nothing happened if not parse-able
logger.error(
- f"Failed to parse args for '{self.name}' tool. Recieved: {args_result_str}"
+ f"Failed to parse args for '{self.name}' tool. Received: {args_result_str}"
)
return None
</file context>
Suggested change
| f"Failed to parse args for '{self.name}' tool. Received: {args_result_str}" | |
| f"Failed to parse args for '{self.name}' tool. Received length={len(args_result_str)}" |
| LOG_DANSWER_MODEL_INTERACTIONS = ( | ||
| os.environ.get("LOG_DANSWER_MODEL_INTERACTIONS", "").lower() == "true" | ||
| LOG_ONYX_MODEL_INTERACTIONS = ( | ||
| os.environ.get("LOG_ONYX_MODEL_INTERACTIONS", "").lower() == "true" |
Contributor
There was a problem hiding this comment.
Dropping the old env name breaks backward compatibility; add a fallback to LOG_DANSWER_MODEL_INTERACTIONS to honor existing deployments.
Prompt for AI agents
Address the following comment on backend/onyx/configs/app_configs.py at line 670:
<comment>Dropping the old env name breaks backward compatibility; add a fallback to LOG_DANSWER_MODEL_INTERACTIONS to honor existing deployments.</comment>
<file context>
@@ -666,8 +666,8 @@ def get_current_tz_offset() -> int:
-LOG_DANSWER_MODEL_INTERACTIONS = (
- os.environ.get("LOG_DANSWER_MODEL_INTERACTIONS", "").lower() == "true"
+LOG_ONYX_MODEL_INTERACTIONS = (
+ os.environ.get("LOG_ONYX_MODEL_INTERACTIONS", "").lower() == "true"
)
LOG_INDIVIDUAL_MODEL_TOKENS = (
</file context>
Suggested change
| os.environ.get("LOG_ONYX_MODEL_INTERACTIONS", "").lower() == "true" | |
| (os.environ.get("LOG_ONYX_MODEL_INTERACTIONS") or os.environ.get("LOG_DANSWER_MODEL_INTERACTIONS", "")).lower() == "true" |
|
|
||
| ## Deploy in a system with GPU support | ||
| ``` | ||
| curl -fsSL https://raw.githubusercontent.com/onyx-dot-app/onyx/main/deployment/docker_compose/install.sh > install.sh && chmod +x install.sh && ./install.sh |
Contributor
There was a problem hiding this comment.
Avoid executing an unpinned remote script; pin to a tag/commit and/or verify checksum before execution to reduce supply-chain risk.
Prompt for AI agents
Address the following comment on deployment/docker_compose/README.md at line 16:
<comment>Avoid executing an unpinned remote script; pin to a tag/commit and/or verify checksum before execution to reduce supply-chain risk.</comment>
<file context>
@@ -1,44 +1,47 @@
-## Deploy in a system with GPU support
+```
+curl -fsSL https://raw.githubusercontent.com/onyx-dot-app/onyx/main/deployment/docker_compose/install.sh > install.sh && chmod +x install.sh && ./install.sh
+```
</file context>
| 2. To shut down the deployment, run: | ||
| - To stop the containers: `docker compose -f docker-compose.gpu-dev.yml -p onyx-stack stop` | ||
| - To delete the containers: `docker compose -f docker-compose.gpu-dev.yml -p onyx-stack down` | ||
| IMAGE_TAG is the version of Onyx to run. It is recommended to leave it as latest to get all updates with each redeployment. |
Contributor
There was a problem hiding this comment.
Avoid recommending 'latest' for production; suggest pinning a specific version and using 'latest' only when automatic upgrades are desired.
Prompt for AI agents
Address the following comment on deployment/docker_compose/README.md at line 47:
<comment>Avoid recommending 'latest' for production; suggest pinning a specific version and using 'latest' only when automatic upgrades are desired.</comment>
<file context>
@@ -1,44 +1,47 @@
-2. To shut down the deployment, run:
- - To stop the containers: `docker compose -f docker-compose.gpu-dev.yml -p onyx-stack stop`
- - To delete the containers: `docker compose -f docker-compose.gpu-dev.yml -p onyx-stack down`
+IMAGE_TAG is the version of Onyx to run. It is recommended to leave it as latest to get all updates with each redeployment.
</file context>
Suggested change
| IMAGE_TAG is the version of Onyx to run. It is recommended to leave it as latest to get all updates with each redeployment. | |
| IMAGE_TAG is the version of Onyx to run. For production, pin to a specific version tag; use 'latest' only if you explicitly want automatic upgrades. |
| CONTAINERS=$(cd onyx_data/deployment && $COMPOSE_CMD -f docker-compose.yml ps -q) | ||
|
|
||
| for CONTAINER in $CONTAINERS; do | ||
| CONTAINER_NAME=$(docker inspect --format '{{.Name}}' "$CONTAINER" | sed 's/^\/\|^onyx_data_deployment_//g') |
Contributor
There was a problem hiding this comment.
sed uses basic regex; | is not alternation. Use -E or separate expressions to correctly strip both prefixes.
Prompt for AI agents
Address the following comment on deployment/docker_compose/install.sh at line 593:
<comment>sed uses basic regex; `|` is not alternation. Use -E or separate expressions to correctly strip both prefixes.</comment>
<file context>
@@ -0,0 +1,695 @@
+CONTAINERS=$(cd onyx_data/deployment && $COMPOSE_CMD -f docker-compose.yml ps -q)
+
+for CONTAINER in $CONTAINERS; do
+ CONTAINER_NAME=$(docker inspect --format '{{.Name}}' "$CONTAINER" | sed 's/^\/\|^onyx_data_deployment_//g')
+ RESTART_COUNT=$(docker inspect --format '{{.RestartCount}}' "$CONTAINER")
+ STATUS=$(docker inspect --format '{{.State.Status}}' "$CONTAINER")
</file context>
| if not env_name: | ||
| env_name = datetime.now().strftime("-%Y") | ||
| project_name = f"onyx-stack{env_name}" | ||
| project_name = f"onyx{env_name}" |
Contributor
There was a problem hiding this comment.
Cleanup prefix omits the hyphen, so it won’t match containers launched with "-p onyx-{env_name}"; resources may not be removed.
Prompt for AI agents
Address the following comment on backend/tests/regression/answer_quality/cli_utils.py at line 190:
<comment>Cleanup prefix omits the hyphen, so it won’t match containers launched with "-p onyx-{env_name}"; resources may not be removed.</comment>
<file context>
@@ -185,7 +187,7 @@ def cleanup_docker(env_name: str) -> None:
if not env_name:
env_name = datetime.now().strftime("-%Y")
- project_name = f"onyx-stack{env_name}"
+ project_name = f"onyx{env_name}"
containers_to_delete = [
c for c in containers if c["Names"].startswith(project_name)
</file context>
Suggested change
| project_name = f"onyx{env_name}" | |
| project_name = f"onyx-{env_name}" |
| apiVersion: apps/v1 | ||
| kind: Deployment | ||
| name: {{ include "onyx-stack.fullname" . }} | ||
| name: {{ include "onyx.fullname" . }} |
Contributor
There was a problem hiding this comment.
HPA scaleTargetRef.name does not match the background Deployment name, so autoscaling will not attach to the intended Deployment.
Prompt for AI agents
Address the following comment on deployment/helm/charts/onyx/templates_disabled/background-hpa.yaml at line 12:
<comment>HPA scaleTargetRef.name does not match the background Deployment name, so autoscaling will not attach to the intended Deployment.</comment>
<file context>
@@ -2,14 +2,14 @@
apiVersion: apps/v1
kind: Deployment
- name: {{ include "onyx-stack.fullname" . }}
+ name: {{ include "onyx.fullname" . }}
minReplicas: {{ .Values.background.autoscaling.minReplicas }}
maxReplicas: {{ .Values.background.autoscaling.maxReplicas }}
</file context>
Suggested change
| name: {{ include "onyx.fullname" . }} | |
| name: {{ include "onyx.fullname" . }}-background |
| the limit to be exceeded. | ||
| """ | ||
| if DANSWER_BOT_RESPONSE_LIMIT_PER_TIME_PERIOD == 0: | ||
| if ONYX_BOT_RESPONSE_LIMIT_PER_TIME_PERIOD <= 0: |
Contributor
There was a problem hiding this comment.
Condition deviates from docs; only 0 should disable the limit to match onyxbot_configs guidance and avoid masking misconfigurations.
Prompt for AI agents
Address the following comment on backend/onyx/onyxbot/slack/utils.py at line 126:
<comment>Condition deviates from docs; only 0 should disable the limit to match onyxbot_configs guidance and avoid masking misconfigurations.</comment>
<file context>
@@ -123,22 +123,22 @@ def check_message_limit() -> bool:
the limit to be exceeded.
"""
- if DANSWER_BOT_RESPONSE_LIMIT_PER_TIME_PERIOD == 0:
+ if ONYX_BOT_RESPONSE_LIMIT_PER_TIME_PERIOD <= 0:
return True
- global _DANSWER_BOT_MESSAGE_COUNT
</file context>
Suggested change
| if ONYX_BOT_RESPONSE_LIMIT_PER_TIME_PERIOD <= 0: | |
| if ONYX_BOT_RESPONSE_LIMIT_PER_TIME_PERIOD == 0: |
| ) | ||
| return False | ||
| _DANSWER_BOT_MESSAGE_COUNT += 1 | ||
| _ONYX_BOT_MESSAGE_COUNT += 1 |
Contributor
There was a problem hiding this comment.
Shared counters are updated without synchronization, risking race conditions and incorrect rate limiting under concurrent threads.
Prompt for AI agents
Address the following comment on backend/onyx/onyxbot/slack/utils.py at line 141:
<comment>Shared counters are updated without synchronization, risking race conditions and incorrect rate limiting under concurrent threads.</comment>
<file context>
@@ -123,22 +123,22 @@ def check_message_limit() -> bool:
)
return False
- _DANSWER_BOT_MESSAGE_COUNT += 1
+ _ONYX_BOT_MESSAGE_COUNT += 1
return True
</file context>
| # count: all | ||
| # capabilities: [gpu] | ||
| command: > | ||
| /bin/sh -c "if [ \"${DISABLE_MODEL_SERVER:-false}\" = \"True\" ]; then |
Contributor
There was a problem hiding this comment.
Conditional check uses "True" instead of lowercase "true"; DISABLE_MODEL_SERVER=true will not disable the service.
Prompt for AI agents
Address the following comment on deployment/docker_compose/docker-compose.yml at line 170:
<comment>Conditional check uses "True" instead of lowercase "true"; DISABLE_MODEL_SERVER=true will not disable the service.</comment>
<file context>
@@ -0,0 +1,346 @@
+ # count: all
+ # capabilities: [gpu]
+ command: >
+ /bin/sh -c "if [ \"${DISABLE_MODEL_SERVER:-false}\" = \"True\" ]; then
+ echo 'Skipping service...';
+ exit 0;
</file context>
|
33 Jobs Failed: Run Integration Tests v2 / integration-tests (connector_job_tests/google, connector-google) failed on "Pull Docker images"Run Integration Tests v2 / integration-tests (connector_job_tests/jira, connector-jira) failed on "Pull Docker images"Run Integration Tests v2 / integration-tests (connector_job_tests/sharepoint, connector-sharepoint) failed on "Pull Docker images"Run Integration Tests v2 / integration-tests (connector_job_tests/slack, connector-slack) failed on "Pull Docker images"Run Integration Tests v2 / integration-tests (tests/anonymous_user, tests-anonymous_user) failed on "Pull Docker images"Run Integration Tests v2 / integration-tests (tests/api_key, tests-api_key) failed on "Pull Docker images"Run Integration Tests v2 / integration-tests (tests/auth, tests-auth) failed on "Pull Docker images"Run Integration Tests v2 / integration-tests (tests/chat, tests-chat) failed on "Pull Docker images"Run Integration Tests v2 / integration-tests (tests/chat_retention, tests-chat_retention) failed on "Pull Docker images"Run Integration Tests v2 / integration-tests (tests/connector, tests-connector) failed on "Pull Docker images"Run Integration Tests v2 / integration-tests (tests/dev_apis, tests-dev_apis) failed on "Pull Docker images"Run Integration Tests v2 / integration-tests (tests/document_set, tests-document_set) failed on "Pull Docker images"Run Integration Tests v2 / integration-tests (tests/image_indexing, tests-image_indexing) failed on "Pull Docker images"Run Integration Tests v2 / integration-tests (tests/index_attempt, tests-index_attempt) failed on "Pull Docker images"Run Integration Tests v2 / integration-tests (tests/indexing, tests-indexing) failed on "Pull Docker images"16 more jobs failed (See summary below for more details) 2 jobs failed running on non-Blacksmith runners. Summary: 7 successful workflows, 3 failed workflows
Last updated: 2025-09-25 00:31:25 UTC |
This was referenced Sep 26, 2025
razvanMiu
pushed a commit
to eea/danswer
that referenced
this pull request
Oct 16, 2025
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
2 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.
Description
1 command install of Onyx
How Has This Been Tested?
Ran locally, some concerns:
Backporting (check the box to trigger backport action)
Note: You have to check that the action passes, otherwise resolve the conflicts manually and tag the patches.
Summary by cubic
Adds a one-command Docker Compose install with a guided script, a single compose file, and an env template to make setup fast and consistent. Also standardizes config names (DANSWER_BOT_* → ONYX_BOT_, LOG_DANSWER_ → LOG_ONYX_*) and cleans up nginx/web configs.
New Features
Migration