-
Notifications
You must be signed in to change notification settings - Fork 2.3k
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
Begin moving to postgres spilo + adding pgvector #8309
Changes from all commits
4fbb514
c88e176
9bf9be0
dfda7e9
6e70ce4
a5ccee3
edccbcf
2850fae
c8792a2
828d4c3
4af3211
e1b828e
9b24a8e
f13db8d
1fbcfaf
9035a09
fd5c301
64736a6
5912f91
bee8aab
5323a4a
541edde
e0b1aa9
832d609
0c37a83
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 |
---|---|---|
|
@@ -18,12 +18,19 @@ jobs: | |
NX_REJECT_UNKNOWN_LOCAL_CACHE: 0 | ||
services: | ||
postgres: | ||
image: twentycrm/twenty-postgres | ||
image: twentycrm/twenty-postgres-spilo | ||
env: | ||
POSTGRES_PASSWORD: postgres | ||
POSTGRES_USER: postgres | ||
PGUSER_SUPERUSER: postgres | ||
PGPASSWORD_SUPERUSER: twenty | ||
ALLOW_NOSSL: "true" | ||
SPILO_PROVIDER: "local" | ||
ports: | ||
- 5432:5432 | ||
options: >- | ||
--health-cmd pg_isready | ||
--health-interval 10s | ||
--health-timeout 5s | ||
--health-retries 5 | ||
redis: | ||
image: redis | ||
ports: | ||
|
@@ -63,6 +70,11 @@ jobs: | |
- name: Server / Write .env | ||
if: steps.changed-files.outputs.any_changed == 'true' | ||
run: npx nx reset:env twenty-server | ||
- name: Server / Create DB | ||
if: steps.changed-files.outputs.any_changed == 'true' | ||
run: | | ||
PGPASSWORD=twenty psql -h localhost -p 5432 -U postgres -d postgres -c 'CREATE DATABASE "default";' | ||
PGPASSWORD=twenty psql -h localhost -p 5432 -U postgres -d postgres -c 'CREATE DATABASE "test";' | ||
- name: Worker / Run | ||
if: steps.changed-files.outputs.any_changed == 'true' | ||
run: npx nx run twenty-server:worker:ci | ||
|
@@ -109,12 +121,19 @@ jobs: | |
needs: server-setup | ||
services: | ||
postgres: | ||
image: twentycrm/twenty-postgres | ||
image: twentycrm/twenty-postgres-spilo | ||
env: | ||
POSTGRES_PASSWORD: postgres | ||
POSTGRES_USER: postgres | ||
PGUSER_SUPERUSER: postgres | ||
PGPASSWORD_SUPERUSER: twenty | ||
ALLOW_NOSSL: "true" | ||
SPILO_PROVIDER: "local" | ||
ports: | ||
- 5432:5432 | ||
options: >- | ||
--health-cmd pg_isready | ||
--health-interval 10s | ||
--health-timeout 5s | ||
--health-retries 5 | ||
Comment on lines
122
to
+136
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. logic: Missing database creation step in server-integration-test job that's present in server-setup job |
||
redis: | ||
image: redis | ||
ports: | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -8,7 +8,7 @@ concurrency: | |
|
||
jobs: | ||
test: | ||
timeout-minutes: 10 | ||
timeout-minutes: 30 | ||
runs-on: ubuntu-latest | ||
steps: | ||
- name: Checkout | ||
|
@@ -36,32 +36,57 @@ jobs: | |
yq eval 'del(.services.db.image)' -i docker-compose.yml | ||
yq eval '.services.db.build.context = "../../"' -i docker-compose.yml | ||
yq eval '.services.db.build.dockerfile = "./packages/twenty-docker/twenty-postgres/Dockerfile"' -i docker-compose.yml | ||
yq eval '.services.db.build.dockerfile = "./packages/twenty-docker/twenty-postgres-spilo/Dockerfile"' -i docker-compose.yml | ||
echo "Setting up .env file..." | ||
cp .env.example .env | ||
echo "Generating secrets..." | ||
echo "# === Randomly generated secrets ===" >>.env | ||
echo "APP_SECRET=$(openssl rand -base64 32)" >>.env | ||
echo "POSTGRES_ADMIN_PASSWORD=$(openssl rand -base64 32)" >>.env | ||
echo "PGPASSWORD_SUPERUSER=$(openssl rand -base64 32)" >>.env | ||
echo "Starting server..." | ||
docker compose up -d | ||
echo "Docker compose up..." | ||
docker compose up -d || { | ||
echo "Docker compose failed to start" | ||
docker compose logs | ||
exit 1 | ||
} | ||
docker compose logs db server -f & | ||
pid=$! | ||
Comment on lines
54
to
55
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. logic: Background process pid is never killed if containers start successfully - potential resource leak |
||
echo "Waiting for database to start..." | ||
count=0 | ||
while [ ! $(docker inspect --format='{{.State.Health.Status}}' twenty-db-1) = "healthy" ]; do | ||
sleep 1; | ||
count=$((count+1)); | ||
if [ $(docker inspect --format='{{.State.Status}}' twenty-db-1) = "exited" ]; then | ||
echo "Database exited" | ||
docker compose logs db | ||
exit 1 | ||
fi | ||
if [ $count -gt 300 ]; then | ||
echo "Failed to start database after 5 minutes" | ||
docker compose logs db | ||
exit 1 | ||
fi | ||
echo "Still waiting for database... (${count}/60)" | ||
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. logic: Message shows '(${count}/60)' but timeout is 300 - inconsistent messaging |
||
done | ||
echo "Waiting for server to start..." | ||
count=0 | ||
while [ ! $(docker inspect --format='{{.State.Health.Status}}' twenty-server-1) = "healthy" ]; do | ||
sleep 1; | ||
count=$((count+1)); | ||
if [ $(docker inspect --format='{{.State.Status}}' twenty-server-1) = "exited" ]; then | ||
echo "Server exited" | ||
docker compose logs server | ||
exit 1 | ||
fi | ||
if [ $count -gt 300 ]; then | ||
echo "Failed to start server" | ||
echo "Failed to start server after 5 minutes" | ||
docker compose logs server | ||
exit 1 | ||
fi | ||
echo "Still waiting for server... (${count}/300s)" | ||
done | ||
working-directory: ./packages/twenty-docker/ |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -17,12 +17,19 @@ jobs: | |
runs-on: ubuntu-latest | ||
services: | ||
postgres: | ||
image: twentycrm/twenty-postgres | ||
image: twentycrm/twenty-postgres-spilo | ||
env: | ||
POSTGRES_PASSWORD: twenty | ||
POSTGRES_USER: twenty | ||
PGUSER_SUPERUSER: postgres | ||
PGPASSWORD_SUPERUSER: twenty | ||
ALLOW_NOSSL: "true" | ||
SPILO_PROVIDER: "local" | ||
ports: | ||
- 5432:5432 | ||
options: >- | ||
--health-cmd pg_isready | ||
--health-interval 10s | ||
--health-timeout 5s | ||
--health-retries 5 | ||
steps: | ||
- uses: actions/checkout@v4 | ||
with: | ||
|
@@ -37,16 +44,20 @@ jobs: | |
if: steps.changed-files.outputs.changed == 'true' | ||
uses: ./.github/workflows/actions/yarn-install | ||
|
||
- name: Server / Create DB | ||
if: steps.changed-files.outputs.any_changed == 'true' | ||
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. logic: Condition uses any_changed but previous steps use changed - this inconsistency could cause the database creation to run when it shouldn't or vice versa |
||
run: PGPASSWORD=twenty psql -h localhost -p 5432 -U postgres -d postgres -c 'CREATE DATABASE "default";' | ||
Comment on lines
+47
to
+49
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. style: Database creation step should include error handling in case database already exists |
||
|
||
- name: Website / Run migrations | ||
if: steps.changed-files.outputs.changed == 'true' | ||
run: npx nx database:migrate twenty-website | ||
env: | ||
DATABASE_PG_URL: postgres://twenty:twenty@localhost:5432/default | ||
DATABASE_PG_URL: postgres://postgres:twenty@localhost:5432/default | ||
- name: Website / Build Website | ||
if: steps.changed-files.outputs.changed == 'true' | ||
run: npx nx build twenty-website | ||
env: | ||
DATABASE_PG_URL: postgres://twenty:twenty@localhost:5432/default | ||
DATABASE_PG_URL: postgres://postgres:twenty@localhost:5432/default | ||
|
||
- name: Mark as VALID | ||
if: steps.changed-files.outputs.changed != 'true' # If no changes, mark as valid | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,12 +1,20 @@ | ||
postgres-on-docker: | ||
docker run \ | ||
--name twenty_postgres \ | ||
-e POSTGRES_USER=postgres \ | ||
-e POSTGRES_PASSWORD=postgres \ | ||
-e POSTGRES_DB=default \ | ||
-v twenty_db_data:/var/lib/postgresql/data \ | ||
docker run -d \ | ||
--name twenty_pg \ | ||
-e PGUSER_SUPERUSER=postgres \ | ||
-e PGPASSWORD_SUPERUSER=twenty \ | ||
Comment on lines
+4
to
+5
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. logic: PGUSER_SUPERUSER and PGPASSWORD_SUPERUSER values don't match the test instructions in PR description (which uses 'twenty' as the username) |
||
-e ALLOW_NOSSL=true \ | ||
-v twenty_db_data:/home/postgres/pgdata \ | ||
-p 5432:5432 \ | ||
twentycrm/twenty-postgres:latest | ||
twentycrm/twenty-postgres-spilo:latest | ||
@echo "Waiting for PostgreSQL to be ready..." | ||
@until PGPASSWORD=twenty psql -h localhost -p 5432 -U postgres -d postgres \ | ||
-c 'SELECT pg_is_in_recovery();' 2>/dev/null | grep -q 'f'; do \ | ||
sleep 1; \ | ||
done | ||
Comment on lines
+11
to
+14
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. style: Health check loop should have a timeout to prevent infinite waiting if database fails to start |
||
PGPASSWORD=twenty psql -h localhost -p 5432 -U postgres -d postgres \ | ||
-c "CREATE DATABASE \"default\" WITH OWNER postgres;" \ | ||
-c "CREATE DATABASE \"test\" WITH OWNER postgres;" | ||
Comment on lines
+15
to
+17
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. style: Database creation commands should check if databases already exist to prevent errors on container restart |
||
|
||
redis-on-docker: | ||
docker run -d --name twenty_redis -p 6379:6379 redis/redis-stack-server:latest |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,6 +1,7 @@ | ||
TAG=latest | ||
|
||
# POSTGRES_ADMIN_PASSWORD=replace_me_with_a_strong_password | ||
#PGUSER_SUPERUSER=postgres | ||
#PGPASSWORD_SUPERUSER=replace_me_with_a_strong_password | ||
Comment on lines
+3
to
+4
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. logic: These variables should not be commented out by default since they are required for Spilo to function properly |
||
|
||
PG_DATABASE_HOST=db:5432 | ||
REDIS_URL=redis://redis:6379 | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -4,9 +4,6 @@ prod-build: | |
prod-run: | ||
@docker run -d -p 3000:3000 --name twenty twenty | ||
|
||
prod-postgres-build: | ||
@cd ../.. && docker build -f ./packages/twenty-docker/twenty-postgres/Dockerfile --tag twenty-postgres . && cd - | ||
|
||
prod-postgres-run: | ||
@docker run -d -p 5432:5432 -e POSTGRES_USER=postgres -e POSTGRES_PASSWORD=postgres --name twenty-postgres twenty-postgres | ||
Comment on lines
7
to
8
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. logic: This target tries to run the twenty-postgres image which no longer exists after removing prod-postgres-build. Should be updated to use twenty-postgres-spilo with appropriate environment variables. |
||
|
||
|
@@ -15,11 +12,3 @@ prod-website-build: | |
|
||
prod-website-run: | ||
@docker run -d -p 3000:3000 --name twenty-website twenty-website | ||
|
||
release-postgres: | ||
@cd ../.. && docker buildx build \ | ||
--push \ | ||
--no-cache \ | ||
--platform linux/amd64,linux/arm64 \ | ||
-f ./packages/twenty-docker/twenty-postgres/Dockerfile -t twentycrm/twenty-postgres:$(version) -t twentycrm/twenty-postgres:latest . \ | ||
&& cd - |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -22,10 +22,10 @@ services: | |
- "3000:3000" | ||
environment: | ||
PORT: 3000 | ||
PG_DATABASE_URL: postgres://twenty:twenty@${PG_DATABASE_HOST}/default | ||
PG_DATABASE_URL: postgres://${PGUSER_SUPERUSER:-postgres}:${PGPASSWORD_SUPERUSER:-twenty}@${PG_DATABASE_HOST:-db:5432}/default | ||
SERVER_URL: ${SERVER_URL} | ||
FRONT_BASE_URL: ${FRONT_BASE_URL:-$SERVER_URL} | ||
REDIS_URL: ${REDIS_URL:-redis://localhost:6379} | ||
REDIS_URL: ${REDIS_URL:-redis://redis:6379} | ||
|
||
ENABLE_DB_MIGRATIONS: "true" | ||
|
||
|
@@ -52,10 +52,10 @@ services: | |
image: twentycrm/twenty:${TAG} | ||
command: ["yarn", "worker:prod"] | ||
environment: | ||
PG_DATABASE_URL: postgres://twenty:twenty@${PG_DATABASE_HOST}/default | ||
PG_DATABASE_URL: postgres://${PGUSER_SUPERUSER:-postgres}:${PGPASSWORD_SUPERUSER:-twenty}@${PG_DATABASE_HOST:-db:5432}/default | ||
SERVER_URL: ${SERVER_URL} | ||
FRONT_BASE_URL: ${FRONT_BASE_URL:-$SERVER_URL} | ||
REDIS_URL: ${REDIS_URL:-redis://localhost:6379} | ||
REDIS_URL: ${REDIS_URL:-redis://redis:6379} | ||
|
||
ENABLE_DB_MIGRATIONS: "false" # it already runs on the server | ||
|
||
|
@@ -73,13 +73,16 @@ services: | |
restart: always | ||
|
||
db: | ||
image: twentycrm/twenty-postgres:${TAG} | ||
image: twentycrm/twenty-postgres-spilo:${TAG} | ||
volumes: | ||
- db-data:/bitnami/postgresql | ||
- db-data:/home/postgres/pgdata | ||
environment: | ||
POSTGRES_PASSWORD: ${POSTGRES_ADMIN_PASSWORD} | ||
PGUSER_SUPERUSER: ${PGUSER_SUPERUSER:-postgres} | ||
PGPASSWORD_SUPERUSER: ${PGPASSWORD_SUPERUSER:-twenty} | ||
ALLOW_NOSSL: "true" | ||
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. style: consider security implications of ALLOW_NOSSL=true in production environments |
||
SPILO_PROVIDER: "local" | ||
healthcheck: | ||
test: pg_isready -U twenty -d default | ||
test: pg_isready -U ${PGUSER_SUPERUSER:-postgres} -h localhost -d postgres | ||
interval: 5s | ||
timeout: 5s | ||
retries: 10 | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -30,10 +30,10 @@ spec: | |
image: twentycrm/twenty-postgres:latest | ||
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. logic: image still references twentycrm/twenty-postgres:latest but should be twentycrm/twenty-postgres-spilo:latest based on PR description |
||
imagePullPolicy: Always | ||
env: | ||
- name: POSTGRES_PASSWORD | ||
- name: PGUSER_SUPERUSER | ||
value: "postgres" | ||
- name: PGPASSWORD_SUPERUSER | ||
value: "twenty" | ||
Comment on lines
+33
to
36
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. style: consider making these credentials configurable via secrets rather than hardcoding them in the deployment manifest |
||
- name: BITNAMI_DEBUG | ||
value: "true" | ||
ports: | ||
- containerPort: 5432 | ||
name: tcp | ||
|
@@ -48,7 +48,7 @@ spec: | |
stdin: true | ||
tty: true | ||
volumeMounts: | ||
- mountPath: /bitnami/postgresql | ||
- mountPath: /home/postgres/pgdata | ||
name: twentycrm-db-data | ||
dnsPolicy: ClusterFirst | ||
restartPolicy: Always |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -40,7 +40,7 @@ spec: | |
- name: FRONT_BASE_URL | ||
value: "https://crm.example.com:443" | ||
- name: "PG_DATABASE_URL" | ||
value: "postgres://twenty:[email protected]/default" | ||
value: "postgres://postgres:[email protected]/default" | ||
Comment on lines
42
to
+43
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. style: Consider using a less privileged database user instead of the postgres superuser for the application connection |
||
- name: "REDIS_URL" | ||
value: "redis://twentycrm-redis.twentycrm.svc.cluster.local:6379" | ||
- name: ENABLE_DB_MIGRATIONS | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -31,7 +31,7 @@ spec: | |
- name: FRONT_BASE_URL | ||
value: "https://crm.example.com:443" | ||
- name: PG_DATABASE_URL | ||
value: "postgres://twenty:[email protected]/default" | ||
value: "postgres://postgres:[email protected]/default" | ||
Comment on lines
33
to
+34
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. logic: Ensure this connection string matches the credentials configured in the Spilo database deployment. The superuser password should be securely stored and accessed via a secret rather than hardcoded. |
||
- name: ENABLE_DB_MIGRATIONS | ||
value: "false" # it already runs on the server | ||
- name: STORAGE_TYPE | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -51,7 +51,7 @@ To make configuration changes to how this doc is generated, see `./.terraform-do | |
| <a name="input_twentycrm_app_hostname"></a> [twentycrm\_app\_hostname](#input\_twentycrm\_app\_hostname) | The protocol, DNS fully qualified hostname, and port used to access TwentyCRM in your environment. Ex: https://crm.example.com:443 | `string` | n/a | yes | | ||
| <a name="input_twentycrm_pgdb_admin_password"></a> [twentycrm\_pgdb\_admin\_password](#input\_twentycrm\_pgdb\_admin\_password) | TwentyCRM password for postgres database. | `string` | n/a | yes | | ||
Comment on lines
51
to
52
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. style: check if pgdb_admin_password variable needs to be renamed to match new Spilo environment variable PGPASSWORD_SUPERUSER |
||
| <a name="input_twentycrm_app_name"></a> [twentycrm\_app\_name](#input\_twentycrm\_app\_name) | A friendly name prefix to use for every component deployed. | `string` | `"twentycrm"` | no | | ||
| <a name="input_twentycrm_db_image"></a> [twentycrm\_db\_image](#input\_twentycrm\_db\_image) | TwentyCRM image for database deployment. This defaults to latest. | `string` | `"twentycrm/twenty-postgres:latest"` | no | | ||
| <a name="input_twentycrm_db_image"></a> [twentycrm\_db\_image](#input\_twentycrm\_db\_image) | TwentyCRM image for database deployment. This defaults to latest. | `string` | `"twentycrm/twenty-postgres-spilo:latest"` | no | | ||
| <a name="input_twentycrm_db_pv_capacity"></a> [twentycrm\_db\_pv\_capacity](#input\_twentycrm\_db\_pv\_capacity) | Storage capacity provisioned for database persistent volume. | `string` | `"10Gi"` | no | | ||
| <a name="input_twentycrm_db_pv_path"></a> [twentycrm\_db\_pv\_path](#input\_twentycrm\_db\_pv\_path) | Local path to use to store the physical volume if using local storage on nodes. | `string` | `""` | no | | ||
| <a name="input_twentycrm_db_pvc_requests"></a> [twentycrm\_db\_pvc\_requests](#input\_twentycrm\_db\_pvc\_requests) | Storage capacity reservation for database persistent volume claim. | `string` | `"10Gi"` | no | | ||
|
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.
logic: Database creation step should be moved before worker run but after environment setup to ensure proper initialization order