-
Notifications
You must be signed in to change notification settings - Fork 4.9k
feat: helm chart #16808
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
feat: helm chart #16808
Conversation
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.
Your free trial has ended. If you'd like to continue receiving code reviews, you can add a payment method here.
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.
20 issues found across 29 files
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="packages/twenty-docker/helm/twenty/templates/_helpers.tpl">
<violation number="1" location="packages/twenty-docker/helm/twenty/templates/_helpers.tpl:62">
P1: Hardcoded database credentials `postgres:postgres` embedded directly in connection URL. This exposes the password in plaintext in rendered templates, Deployment manifests, and `kubectl describe` output. The chart already uses `secretKeyRef` for `APP_SECRET` - the same pattern should be applied for database credentials.</violation>
<violation number="2" location="packages/twenty-docker/helm/twenty/templates/_helpers.tpl:64">
P1: SSL configuration for external database is ineffective. Changing URI scheme from 'postgres' to 'postgresql' does not enable SSL - PostgreSQL requires query parameters like `?sslmode=require` to enable SSL connections. Users enabling `db.external.ssl: true` will have a false sense of security.</violation>
<violation number="3" location="packages/twenty-docker/helm/twenty/templates/_helpers.tpl:153">
P1: YAML syntax error: Tab characters used for indentation will cause parsing failures. YAML only allows spaces for indentation. Replace tabs with spaces (typically 2 spaces for Helm templates).</violation>
</file>
<file name="packages/twenty-docker/helm/twenty/templates/secret-db-user.yaml">
<violation number="1" location="packages/twenty-docker/helm/twenty/templates/secret-db-user.yaml:12">
P1: Random password regeneration on each `helm upgrade` will cause database authentication failures. The `randAlphaNum` function generates a new password on every template render, updating the Secret, but the database user's password remains unchanged from initial creation. The codebase already has an established pattern using `lookup` to preserve existing secrets (see `_helpers.tpl` line 116). This template should follow the same pattern to check for existing secrets before generating new passwords.</violation>
</file>
<file name="packages/twenty-docker/helm/twenty/templates/deployment-db-internal.yaml">
<violation number="1" location="packages/twenty-docker/helm/twenty/templates/deployment-db-internal.yaml:17">
P1: Using `RollingUpdate` strategy with `maxUnavailable: 1` for a single-replica database with persistent storage is problematic. With ReadWriteOnce PVCs (typical default), the new pod cannot mount the volume until the old pod terminates, but RollingUpdate may attempt to start the new pod first. Consider using `Recreate` strategy instead for stateful single-replica workloads.</violation>
<violation number="2" location="packages/twenty-docker/helm/twenty/templates/deployment-db-internal.yaml:42">
P1: Database password `PGPASSWORD_SUPERUSER` is exposed as a plain text environment variable. This makes it visible in pod specs and `kubectl describe` output. Consider using a Kubernetes Secret and referencing it with `valueFrom.secretKeyRef` for sensitive credentials.</violation>
</file>
<file name="packages/twenty-docker/helm/twenty/templates/deployment-worker.yaml">
<violation number="1" location="packages/twenty-docker/helm/twenty/templates/deployment-worker.yaml:38">
P1: Invalid YAML: `command:` with inline `toYaml` produces broken YAML when the value is an array. The `toYaml` function outputs a multi-line YAML list, which cannot be placed inline after the key. The key should be on its own line with the list output below it.</violation>
</file>
<file name="packages/twenty-docker/helm/twenty/values.schema.json">
<violation number="1" location="packages/twenty-docker/helm/twenty/values.schema.json:163">
P2: Conditional validation for S3 storage is incomplete. When `type: "s3"`, the schema requires fields inside the `s3` object but doesn't require the `s3` object itself. Users could configure `storage.type: s3` without providing any S3 configuration, bypassing validation. Add `"required": ["s3"]` to the `then` clause.</violation>
</file>
<file name="packages/twenty-docker/helm/twenty/tests/storage_env_test.yaml">
<violation number="1" location="packages/twenty-docker/helm/twenty/tests/storage_env_test.yaml:15">
P2: Test sets S3 credentials (accessKeyId, secretAccessKey) but does not assert they are injected as environment variables `S3_ACCESS_KEY_ID` and `S3_SECRET_ACCESS_KEY`. The template injects these values, and the schema marks them as required when storage.type is s3, but the test would pass even if credential injection was broken.</violation>
</file>
<file name="packages/twenty-docker/helm/twenty/templates/NOTES.txt">
<violation number="1" location="packages/twenty-docker/helm/twenty/templates/NOTES.txt:6">
P2: Unsafe array access - `index` will panic if `server.ingress.hosts` is empty or nil. The `| default` pipe only handles empty `.host` values, not failed index operations. Consider wrapping in a conditional or using a helper function.</violation>
</file>
<file name="packages/twenty-docker/helm/twenty/templates/job-run-migrations.yaml">
<violation number="1" location="packages/twenty-docker/helm/twenty/templates/job-run-migrations.yaml:40">
P1: Database URL containing credentials is exposed as plain text environment variable instead of using Kubernetes Secret reference. The `twenty.dbUrl` template embeds username/password directly in the connection string, making credentials visible in Job manifests, potentially in logs, and stored in etcd without encryption-at-rest protection. Consider using `valueFrom.secretKeyRef` similar to how `APP_SECRET` is handled in deployment-server.yaml.</violation>
</file>
<file name="packages/twenty-docker/helm/twenty/templates/deployment-server.yaml">
<violation number="1" location="packages/twenty-docker/helm/twenty/templates/deployment-server.yaml:89">
P1: Missing `livenessProbe` and `readinessProbe` configuration. Health probes are essential for production Kubernetes deployments to ensure traffic is only routed to healthy pods and failed pods are restarted. Consider adding HTTP probes on the server's health endpoint (e.g., `/healthz` or `/`).</violation>
</file>
<file name="packages/twenty-docker/helm/twenty/templates/pvc-server.yaml">
<violation number="1" location="packages/twenty-docker/helm/twenty/templates/pvc-server.yaml:12">
P1: The `accessModes` templating will produce invalid YAML. When `toYaml` renders an array on the same line as the key, it creates malformed output like `accessModes: - ReadWriteOnce`. Move the template output to a new line with correct indentation.</violation>
</file>
<file name="packages/twenty-docker/helm/twenty/values.yaml">
<violation number="1" location="packages/twenty-docker/helm/twenty/values.yaml:4">
P2: Using `latest` tag makes deployments non-reproducible and can cause unexpected issues when images are updated. Consider pinning to specific version tags (e.g., `v0.40.0`) for production stability and reproducible deployments.</violation>
</file>
<file name="packages/twenty-docker/helm/twenty/templates/job-create-database.yaml">
<violation number="1" location="packages/twenty-docker/helm/twenty/templates/job-create-database.yaml:39">
P1: Database password is exposed in plaintext in the Job spec environment variables instead of using Kubernetes Secrets. This is inconsistent with how other secrets (APP_SECRET) are handled in this same chart using `secretKeyRef`. Plaintext passwords are visible via `kubectl describe job` and stored in etcd.</violation>
<violation number="2" location="packages/twenty-docker/helm/twenty/templates/job-create-database.yaml:54">
P1: Shell variable `$DBNAME` is unquoted, causing potential word splitting. The variable should remain inside the double-quoted string to prevent issues if the database name contains spaces or special characters.</violation>
</file>
<file name="packages/twenty-docker/helm/twenty/templates/deployment-redis-internal.yaml">
<violation number="1" location="packages/twenty-docker/helm/twenty/templates/deployment-redis-internal.yaml:37">
P1: The `PORT` environment variable is not used by `redis-stack-server`. To make the port configurable, add `--port` to the command args. Otherwise, Redis will always listen on its default port 6379 regardless of the configured `service.port`.</violation>
</file>
<file name="packages/twenty-docker/helm/twenty/templates/job-create-db-user.yaml">
<violation number="1" location="packages/twenty-docker/helm/twenty/templates/job-create-db-user.yaml:57">
P1: SQL injection vulnerability: string interpolation of USERNAME in SQL query allows injection if the value contains single quotes. Use psql's `-v` variable binding with `:'varname'` syntax for safe string literals.</violation>
<violation number="2" location="packages/twenty-docker/helm/twenty/templates/job-create-db-user.yaml:60">
P1: SQL syntax error/injection risk: Password is directly interpolated into CREATE ROLE statement without escaping single quotes. If the password from the secret contains a single quote, it will break the SQL syntax or could enable SQL injection.</violation>
</file>
<file name="packages/twenty-docker/helm/twenty/README.md">
<violation number="1" location="packages/twenty-docker/helm/twenty/README.md:76">
P2: Avoid documenting sensitive credentials passed via `--set` flags. This pattern exposes secrets in shell history and process listings. Consider recommending a values file approach:
```bash
# Create values-secrets.yaml (do not commit to git)
helm install my-twenty ./packages/twenty-docker/helm/twenty \
-f values-secrets.yaml
Or reference existing Kubernetes secrets.
</details>
---
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
<sub>Reply to cubic to teach it or ask questions. Re-run a review with `@cubic-dev-ai review this PR`</sub>
packages/twenty-docker/helm/twenty/templates/secret-db-user.yaml
Outdated
Show resolved
Hide resolved
packages/twenty-docker/helm/twenty/templates/deployment-db-internal.yaml
Show resolved
Hide resolved
packages/twenty-docker/helm/twenty/templates/deployment-db-internal.yaml
Show resolved
Hide resolved
packages/twenty-docker/helm/twenty/templates/deployment-worker.yaml
Outdated
Show resolved
Hide resolved
packages/twenty-docker/helm/twenty/templates/job-create-db-user.yaml
Outdated
Show resolved
Hide resolved
packages/twenty-docker/helm/twenty/templates/job-create-database.yaml
Outdated
Show resolved
Hide resolved
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.
Pull request overview
This PR introduces a comprehensive Helm chart for deploying Twenty CRM on Kubernetes. The chart provides flexible configuration options for deploying the server and worker components with support for both internal and external PostgreSQL/Redis dependencies.
Key Changes:
- Complete Helm chart structure with configurable deployments, services, ingress, and persistence
- Support for internal minimal databases (Spilo Postgres, Redis Stack) or Bitnami subcharts
- Automated database creation, user provisioning, and migration jobs
- TLS configuration via cert-manager with Let's Encrypt integration
Reviewed changes
Copilot reviewed 26 out of 29 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| packages/twenty-docker/k8s/README.md | Updated to reference new Helm chart as recommended deployment method |
| packages/twenty-docker/helm/twenty/Chart.yaml | Chart metadata with optional Bitnami dependencies |
| packages/twenty-docker/helm/twenty/values.yaml | Comprehensive configuration values for all components |
| packages/twenty-docker/helm/twenty/values.schema.json | JSON schema for values validation |
| packages/twenty-docker/helm/twenty/templates/_helpers.tpl | Template helper functions for image resolution, URL composition |
| packages/twenty-docker/helm/twenty/templates/deployment-*.yaml | Deployment manifests for server, worker, database, and Redis |
| packages/twenty-docker/helm/twenty/templates/service-*.yaml | Service definitions for all components |
| packages/twenty-docker/helm/twenty/templates/ingress.yaml | Ingress configuration with TLS support |
| packages/twenty-docker/helm/twenty/templates/pvc-*.yaml | PersistentVolumeClaim templates for storage |
| packages/twenty-docker/helm/twenty/templates/job-*.yaml | Post-install hooks for DB setup and migrations |
| packages/twenty-docker/helm/twenty/templates/secret-*.yaml | Secret templates for tokens and credentials |
| packages/twenty-docker/helm/twenty/tests/*.yaml | Helm unit tests for URL derivation and storage configuration |
| packages/twenty-docker/helm/twenty/README.md | Comprehensive chart documentation |
| packages/twenty-docker/helm/twenty/QUICKSTART.md | Quick installation guide |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
🚀 Preview Environment Ready! Your preview environment is available at: http://bore.pub:38520 This environment will automatically shut down when the PR is closed or after 5 hours. |
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.
6 issues found across 24 files (changes from recent commits).
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="packages/twenty-docker/helm/twenty/templates/job-create-db-user.yaml">
<violation number="1" location="packages/twenty-docker/helm/twenty/templates/job-create-db-user.yaml:67">
P1: SQL syntax error: `:'username'` produces a string literal, but `CREATE ROLE` expects an identifier. Use `:\"username\"` (double-quoted identifier syntax) for the role name.</violation>
<violation number="2" location="packages/twenty-docker/helm/twenty/templates/job-create-db-user.yaml:72">
P1: SQL syntax error: `:'db'` and `:'username'` produce string literals, but `GRANT` expects identifiers for database and role names. Use `:\"db\"` and `:\"username\"` instead.</violation>
</file>
<file name="packages/twenty-docker/helm/twenty/tests/pvc_test.yaml">
<violation number="1" location="packages/twenty-docker/helm/twenty/tests/pvc_test.yaml:86">
P1: Test references wrong template and sets mismatched values. The test "redis-internal pvc renders when enabled" references `templates/pvc-docker-data.yaml` but sets `redisInternal.*` values. The actual `pvc-docker-data.yaml` template uses `server.dockerDataPersistence.*` values. Also, `templates/pvc-redis-internal.yaml` in the templates list (line 5) doesn't exist. Either create a proper redis PVC template or update this test to test the correct template with matching values.</violation>
</file>
<file name="packages/twenty-docker/helm/twenty/templates/secret-db-superuser.yaml">
<violation number="1" location="packages/twenty-docker/helm/twenty/templates/secret-db-superuser.yaml:5">
P1: Insecure default password. Using `"postgres"` as the default superuser password is a security risk. Consider either requiring an explicit password value or generating a random password similar to how the access token is handled (as mentioned in the README: "Access token auto-generated (32 chars) if not provided").</violation>
</file>
<file name="packages/twenty-docker/helm/twenty/templates/job-create-database.yaml">
<violation number="1" location="packages/twenty-docker/helm/twenty/templates/job-create-database.yaml:64">
P1: Invalid psql variable syntax for CREATE DATABASE. Using `:'db'` produces a string literal, but PostgreSQL's CREATE DATABASE requires an identifier. This will cause a syntax error when the job tries to create the database. Use `:"db"` instead to produce a properly quoted identifier.</violation>
</file>
<file name="packages/twenty-docker/helm/twenty/templates/secret-db-url.yaml">
<violation number="1" location="packages/twenty-docker/helm/twenty/templates/secret-db-url.yaml:30">
P1: Password and username should be URL-encoded to prevent connection failures when credentials contain special characters. Use Helm's `urlquery` function to properly escape these values.</violation>
</file>
Reply to cubic to teach it or ask questions. Re-run a review with @cubic-dev-ai review this PR
packages/twenty-docker/helm/twenty/templates/job-create-db-user.yaml
Outdated
Show resolved
Hide resolved
packages/twenty-docker/helm/twenty/templates/job-create-db-user.yaml
Outdated
Show resolved
Hide resolved
packages/twenty-docker/helm/twenty/templates/secret-db-superuser.yaml
Outdated
Show resolved
Hide resolved
packages/twenty-docker/helm/twenty/templates/job-create-database.yaml
Outdated
Show resolved
Hide resolved
packages/twenty-docker/helm/twenty/templates/secret-db-url.yaml
Outdated
Show resolved
Hide resolved
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.
6 issues found across 32 files (changes from recent commits).
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="packages/twenty-docker/helm/twenty/templates/deployment-server.yaml">
<violation number="1" location="packages/twenty-docker/helm/twenty/templates/deployment-server.yaml:51">
P2: The `-U postgres` is hardcoded for `pg_isready`, but external databases may use a different user. Consider using `.Values.db.external.user` when `db.enabled` is false, consistent with how host and port are already conditionally handled.</violation>
</file>
<file name="packages/twenty-docker/helm/twenty/README.md">
<violation number="1" location="packages/twenty-docker/helm/twenty/README.md:11">
P3: This bullet point uses an en-dash (–) instead of a hyphen (-), which is inconsistent with all other bullet points in the Features section. This will render incorrectly as a bullet in most Markdown parsers.</violation>
</file>
<file name="packages/twenty-docker/helm/twenty/templates/secret-db-url.yaml">
<violation number="1" location="packages/twenty-docker/helm/twenty/templates/secret-db-url.yaml:20">
P1: Password and username should be URL-encoded when embedded in the PostgreSQL connection URL. If a user provides a password containing special characters (e.g., `@`, `:`, `/`), the URL will be malformed and connections will fail.</violation>
</file>
<file name="packages/twenty-docker/helm/twenty/templates/deployment-worker.yaml">
<violation number="1" location="packages/twenty-docker/helm/twenty/templates/deployment-worker.yaml:33">
P2: Hardcoded init container image `postgres:16-alpine` should be configurable via values. Organizations using private registries or air-gapped environments cannot override this image. Consider adding a value like `initContainers.waitForDb.image` or at minimum use `.Values.image.registry` as a prefix.</violation>
</file>
<file name="packages/twenty-docker/helm/twenty/values-old.yaml">
<violation number="1" location="packages/twenty-docker/helm/twenty/values-old.yaml:175">
P1: Hardcoded default password `postgres` is a security risk. Consider leaving this empty with a comment indicating it must be set, or generating a random password like the access token.</violation>
<violation number="2" location="packages/twenty-docker/helm/twenty/values-old.yaml:189">
P2: Using `latest` tag for production images makes deployments non-reproducible. Consider pinning to a specific version or using the chart's appVersion pattern like the main image.</violation>
</file>
Reply to cubic to teach it or ask questions. Tag @cubic-dev-ai to re-run a review.
packages/twenty-docker/helm/twenty/templates/secret-db-url.yaml
Outdated
Show resolved
Hide resolved
packages/twenty-docker/helm/twenty/templates/deployment-server.yaml
Outdated
Show resolved
Hide resolved
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.
1 issue found across 6 files (changes from recent commits).
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="packages/twenty-docker/helm/twenty/templates/deployment-server.yaml">
<violation number="1" location="packages/twenty-docker/helm/twenty/templates/deployment-server.yaml:83">
P1: Incorrect shell quoting produces wrong psql variable syntax for password. The current pattern produces `:""app_password""` (double quotes/identifier syntax) instead of `:'app_password'` (single quotes/string literal syntax). PostgreSQL CREATE USER PASSWORD requires a string literal, causing this command to fail.</violation>
</file>
Reply to cubic to teach it or ask questions. Tag @cubic-dev-ai to re-run a review.
packages/twenty-docker/helm/twenty/templates/deployment-server.yaml
Outdated
Show resolved
Hide resolved
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.
Pull request overview
Copilot reviewed 31 out of 31 changed files in this pull request and generated 19 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| { | ||
| "if": { "properties": { "type": { "const": "s3" } }, "required": ["type"] }, | ||
| "then": { | ||
| "required": ["s3"], |
Copilot
AI
Dec 25, 2025
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.
There's improper indentation on line 164. The "required" key appears to be at the same level as the conditional "if" statement, but based on JSON-schema conditional semantics, it should be inside the "then" clause. This will cause the schema to always require the "s3" property regardless of the storage type.
|
|
||
| - it: supports local storage configuration | ||
| set: | ||
| server.storage.type: local |
Copilot
AI
Dec 25, 2025
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.
The test references "server.storage.type" but according to values.yaml, the correct path is "storage.type" (top-level, not under server). This test will not work correctly and should be updated to use the correct values path.
| server.storage.type: local | |
| storage.type: local |
| {{- with .Values.storage.s3.accessKeyId }} | ||
| - name: STORAGE_S3_ACCESS_KEY_ID | ||
| value: {{ . | quote }} | ||
| {{- end }} | ||
| {{- with .Values.storage.s3.secretAccessKey }} | ||
| - name: STORAGE_S3_SECRET_ACCESS_KEY | ||
| value: {{ . | quote }} |
Copilot
AI
Dec 25, 2025
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.
S3 credentials (accessKeyId and secretAccessKey) are being injected directly as environment variables in plain text. This is a security risk as these values will be visible in pod specifications and logs. Consider using Kubernetes secrets and referencing them via secretKeyRef instead of passing the values directly from values.yaml.
| {{- with .Values.storage.s3.accessKeyId }} | |
| - name: STORAGE_S3_ACCESS_KEY_ID | |
| value: {{ . | quote }} | |
| {{- end }} | |
| {{- with .Values.storage.s3.secretAccessKey }} | |
| - name: STORAGE_S3_SECRET_ACCESS_KEY | |
| value: {{ . | quote }} | |
| {{- if and .Values.storage.s3.secretName .Values.storage.s3.accessKeyIdKey }} | |
| - name: STORAGE_S3_ACCESS_KEY_ID | |
| valueFrom: | |
| secretKeyRef: | |
| name: {{ .Values.storage.s3.secretName | quote }} | |
| key: {{ .Values.storage.s3.accessKeyIdKey | quote }} | |
| {{- end }} | |
| {{- if and .Values.storage.s3.secretName .Values.storage.s3.secretAccessKeyKey }} | |
| - name: STORAGE_S3_SECRET_ACCESS_KEY | |
| valueFrom: | |
| secretKeyRef: | |
| name: {{ .Values.storage.s3.secretName | quote }} | |
| key: {{ .Values.storage.s3.secretAccessKeyKey | quote }} |
| app.kubernetes.io/component: server | ||
| strategy: | ||
| type: RollingUpdate | ||
| rollingUpdate: | ||
| maxSurge: 1 | ||
| maxUnavailable: 1 |
Copilot
AI
Dec 25, 2025
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.
The server deployment uses a RollingUpdate strategy with ReadWriteOnce volumes. During a rolling update, the new pod cannot start until the old pod releases the PVC, which means there will always be downtime. Consider either documenting this limitation, switching to ReadWriteMany for shared storage scenarios, or recommending Recreate strategy when using local storage.
| app.kubernetes.io/component: server | |
| strategy: | |
| type: RollingUpdate | |
| rollingUpdate: | |
| maxSurge: 1 | |
| maxUnavailable: 1 | |
| app.kubernetes.io/component: server | |
| {{- if or .Values.server.dockerDataPersistence.enabled .Values.server.persistence.enabled }} | |
| strategy: | |
| type: Recreate | |
| {{- else }} | |
| strategy: | |
| type: RollingUpdate | |
| rollingUpdate: | |
| maxSurge: 1 | |
| maxUnavailable: 1 | |
| {{- end }} |
| "type": "object", | ||
| "properties": { | ||
| "enabled": { "type": "boolean" }, | ||
| "replicaCount": { "type": "integer", "minimum": 1 }, |
Copilot
AI
Dec 25, 2025
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.
The schema defines a minimum value of 1 for replicaCount, but when using ReadWriteOnce (RWO) persistent volumes (the default access mode in values.yaml), multiple replicas cannot share the same PVC. If server.replicaCount > 1 with local storage and RWO PVCs, pods will fail to schedule. Consider adding validation logic or documentation warning about this constraint, or requiring storage.type to be s3 when replicaCount > 1.
| "replicaCount": { "type": "integer", "minimum": 1 }, | |
| "replicaCount": { | |
| "type": "integer", | |
| "minimum": 1, | |
| "description": "Number of server replicas. When using local storage with ReadWriteOnce (RWO) PersistentVolumeClaims (the default), multiple replicas cannot share the same PVC and pods will fail to schedule if replicaCount > 1. Use replicaCount = 1 for local/RWO storage, or configure a shared storage backend such as S3 before increasing this value." | |
| }, |
| app.kubernetes.io/instance: {{ .Release.Name }} | ||
| app.kubernetes.io/component: server | ||
| spec: | ||
| type: ClusterIP |
Copilot
AI
Dec 25, 2025
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.
The service type is hardcoded to "ClusterIP". While this is a reasonable default, it should be configurable via values.yaml to allow users to easily switch to NodePort or LoadBalancer when needed. This is particularly important for environments where Ingress is not available or desired.
| type: ClusterIP | |
| type: {{ .Values.server.service.type | default "ClusterIP" }} |
| psql -h {{ include "twenty.fullname" . }}-db -p 5432 -U postgres -d postgres -v app_user="${APP_USER}" -Atc "SELECT 1 FROM pg_roles WHERE rolname = :'app_user'" | grep -q 1 || \ | ||
| psql -h {{ include "twenty.fullname" . }}-db -p 5432 -U postgres -d postgres -v app_user="${APP_USER}" -v app_password="${APP_PASSWORD}" -c 'CREATE USER :"app_user" WITH PASSWORD :'"'"'"'"'app_password'"'"'"'"';' |
Copilot
AI
Dec 25, 2025
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.
The ensure-database-exists init container uses the postgres superuser to create the application user and schema. If the APP_PASSWORD environment variable contains special characters like quotes or dollar signs, the shell command on line 83 could break or create SQL injection vulnerabilities. The password should be passed more safely, possibly using a file-based approach or ensuring proper escaping.
| psql -h {{ include "twenty.fullname" . }}-db -p 5432 -U postgres -d postgres -v app_user="${APP_USER}" -Atc "SELECT 1 FROM pg_roles WHERE rolname = :'app_user'" | grep -q 1 || \ | |
| psql -h {{ include "twenty.fullname" . }}-db -p 5432 -U postgres -d postgres -v app_user="${APP_USER}" -v app_password="${APP_PASSWORD}" -c 'CREATE USER :"app_user" WITH PASSWORD :'"'"'"'"'app_password'"'"'"'"';' | |
| psql -h {{ include "twenty.fullname" . }}-db -p 5432 -U postgres -d postgres -v app_user="${APP_USER}" -v app_password="${APP_PASSWORD}" <<'EOSQL' | |
| DO | |
| $do$ | |
| BEGIN | |
| IF NOT EXISTS (SELECT 1 FROM pg_roles WHERE rolname = :'app_user') THEN | |
| EXECUTE format('CREATE USER %I WITH PASSWORD %L', :'app_user', :'app_password'); | |
| END IF; | |
| END | |
| $do$; | |
| EOSQL |
| livenessProbe: | ||
| httpGet: | ||
| path: / | ||
| port: {{ include "twenty.server.containerPort" . }} | ||
| initialDelaySeconds: 60 | ||
| periodSeconds: 10 | ||
| timeoutSeconds: 5 | ||
| failureThreshold: 5 | ||
| readinessProbe: | ||
| httpGet: | ||
| path: / | ||
| port: {{ include "twenty.server.containerPort" . }} | ||
| initialDelaySeconds: 40 | ||
| periodSeconds: 5 | ||
| timeoutSeconds: 5 | ||
| failureThreshold: 5 |
Copilot
AI
Dec 25, 2025
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.
The liveness and readiness probes both use the root path "/" which might not be ideal for health checking. Modern applications typically expose dedicated health check endpoints like "/health" or "/readyz" and "/livez". The root path may perform unnecessary work or not accurately reflect the application's health status. Consider documenting what the expected behavior is or using more specific health check endpoints if available in the Twenty application.
packages/twenty-docker/helm/twenty/tests/redis_db_switching_test.yaml
Outdated
Show resolved
Hide resolved
charlesBochet
left a comment
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.
Thank you @dnplkndll. I could not test it but as this will be community maintained, I'm happy to merge it.
It's likely that the core team will maintain it at some point seems we are using helm internally and we will publish our own charts at some point so everybody can benefit for it (but right now they are not abstract enough, so this should be a good base)
|
Hey @charlesBochet! After you've done the QA of your Pull Request, you can mark it as done here. Thank you! |
1 similar comment
|
Hey @charlesBochet! After you've done the QA of your Pull Request, you can mark it as done here. Thank you! |
Add Helm Chart
Feedback requested