-
Notifications
You must be signed in to change notification settings - Fork 0
Enable embedding service in Docker Compose and update profiles #6
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: qa/agent-appwrite-appwrite/pr-06-13429/base
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -715,6 +715,15 @@ | |
| 'question' => '', | ||
| 'filter' => '' | ||
| ], | ||
| [ | ||
| 'name' => '_APP_EMBEDDING', | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Shipwright · CRITICAL The default value for _APP_EMBEDDING is 'enabled' in app/config/variables.php, but the .env file shipped with the repository sets _APP_EMBEDDING=disabled. Impact: The default value for _APP_EMBEDDING is 'enabled' in app/config/variables.php, but the .env file shipped with the repository sets _APP_EMBEDDING=disabled. This creates a dangerous divergence: any deployment that does not explicitly copy the .env value (e.g., uses defaults, or an operator misses this line) will silently enable the embeddings API while the resource-heavy embedding container is NOT started (COMPOSE_PRO… Suggested fix: Review the cited evidence, fix the risk if confirmed, and rerun Shipwright. |
||
| 'description' => 'Enables the embeddings API, backed by the resource-heavy appwrite-embedding container. That container sits behind the "embedding" Compose profile, so add "embedding" to COMPOSE_PROFILES to start it. Set this to "disabled" to have the /v1/embeddings routes return a service disabled error instead of reaching for the container. Default value is: enabled.', | ||
| 'introduction' => '2.0.0', | ||
| 'default' => 'enabled', | ||
| 'required' => false, | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Shipwright · HIGH The .env comment says 'The embeddings API is on by default and is switched off below until then' but the actual line sets _APP_EMBEDDING=disabled. Impact: The .env comment says 'The embeddings API is on by default and is switched off below until then' but the actual line sets _APP_EMBEDDING=disabled. This contradicts the variables.php default of 'enabled'. An operator reading the .env comment may believe the API is off, while a fresh install using variables.php defaults will have it on. This documentation/behavior mismatch is a classic source of misconfiguration. Suggested fix: Review the cited evidence, fix the risk if confirmed, and rerun Shipwright. |
||
| 'question' => '', | ||
| 'filter' => '' | ||
| ], | ||
| ], | ||
| ], | ||
| [ | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -465,14 +465,18 @@ | |
| // installation deploys just the engine backing the platform, so neither is on | ||
| // until an operator provisions that engine and says so. Closed to everyone -- | ||
| // keys and privileged roles included -- rather than answering and then failing | ||
| // on the first write with the reason only in the logs. | ||
| // against an absent service with the reason only in the logs. Embeddings ran | ||
| // on every installation before it had a switch, so it stays on unless an | ||
| // operator turns it off; the resource-heavy container is what sits behind a | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Shipwright · HIGH The refactor of $products from string values to arrays with a default parameter ('documentsdb' => ['_APP_DOCUMENTSDB', 'disabled']) is non-obvious. Impact: The refactor of $products from string values to arrays with a default parameter ('documentsdb' => ['_APP_DOCUMENTSDB', 'disabled']) is non-obvious. A reader must understand that the second array element is the fallback default for System::getEnv. The spread operator System::getEnv(...$products[$namespace]) obscures the argument order and makes it easy to swap the env var name and default value in future edits. Th… Suggested fix: Review the cited evidence, fix the risk if confirmed, and rerun Shipwright. |
||
| // Compose profile. | ||
| $products = [ | ||
| 'documentsdb' => '_APP_DOCUMENTSDB', | ||
| 'vectorsdb' => '_APP_VECTORSDB', | ||
| 'documentsdb' => ['_APP_DOCUMENTSDB', 'disabled'], | ||
| 'vectorsdb' => ['_APP_VECTORSDB', 'disabled'], | ||
| 'embeddings' => ['_APP_EMBEDDING', 'enabled'], | ||
| ]; | ||
| if ( | ||
| isset($products[$namespace]) | ||
| && System::getEnv($products[$namespace], 'disabled') !== 'enabled' | ||
| && System::getEnv(...$products[$namespace]) !== 'enabled' | ||
| ) { | ||
| throw new Exception(Exception::GENERAL_SERVICE_DISABLED); | ||
| } | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shipwright · CRITICAL
The CI workflow conditionally appends _APP_EMBEDDING=enabled to GITHUB_ENV only when matrix.service.embedding is true.
Impact: The CI workflow conditionally appends _APP_EMBEDDING=enabled to GITHUB_ENV only when matrix.service.embedding is true. However, the .env file now hardcodes _APP_EMBEDDING=disabled. If the CI setup sources .env after GITHUB_ENV, or if the compose environment resolution prefers .env over the shell environment, the embedding tests will run against a disabled API and fail. Conversely, if GITHUB_ENV wins, the container p…
Suggested fix: Review the cited evidence, fix the risk if confirmed, and rerun Shipwright.