Skip to content
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

Improve Docker-Compose Install Experience #9781

Merged
merged 12 commits into from
Jan 24, 2025

Conversation

john-s4d
Copy link
Contributor

@john-s4d john-s4d commented Jan 21, 2025

This PR updates the docker-compose installation documentation and env sample to improve the setup experience.

  • Updates the URLs for raw files to reference main branch, which is likely where new users will be pulling from initially. This seems to be the most straightforward option; assume that advanced users who want to retrieve it from a particular tag will know to change the URL for their scenario.
  • Fixes an improperly stated curl command.
  • Adds a note that the PGPASSWORD_SUPERUSER should be URL-safe. This is required since the value is later concat into a PG_DATABASE_URL as a URL, and expected to be in proper URL format. Touches on PostgreSQL Password with Special Characters Leads to isUrl Validation Failure #8597.

Copy link
Contributor

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PR Summary

This PR enhances the Docker Compose installation documentation and environment configuration, focusing on PostgreSQL connection settings and user guidance.

  • Added port specification (5432) to PG_DATABASE_HOST in /packages/twenty-docker/.env.example to prevent connection issues
  • Added explicit warning about URL-safe PostgreSQL passwords in documentation to prevent validation failures
  • Updated raw file URLs in documentation to reference main branch for better initial setup experience
  • Fixed curl command syntax in Docker Compose installation guide for proper file retrieval

💡 (1/5) You can manually trigger the bot by mentioning @greptileai in a comment!

2 file(s) reviewed, no comment(s)
Edit PR Review Bot Settings | Greptile

Copy link
Member

@FelixMalfait FelixMalfait left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @john-s4d, thanks a lot for helping others self-host more easily! Made minor comments

#PGPASSWORD_SUPERUSER=replace_me_with_a_strong_password
#PG_DATABASE_HOST=db
#PGPASSWORD_SUPERUSER=replace_me_with_a_url-safe_strong_password
#PG_DATABASE_HOST=db:5432
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the suggestion in the issue was rather to keep PG_DATABASE_HOST=db but enforce port 5432 in the docker-compose, I think this make more sense (people don't expect to put the port in the host variable)

Copy link
Member

@FelixMalfait FelixMalfait Jan 22, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just saw the PR description, If you want port to be configurable in .env then you can add it as a new variable (PG_DATABASE_PORT). But I don't know if there is that much use for it since this is an internal port and it is not exposed publicly on that port, it could confuse users

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think there are scenarios where self-hosted will want to change the DB port, and I agree the ideal solution is to create a var PG_DATABASE_PORT. However, it's a bigger change, and I wasn't able to actually reproduce the original complaint in #9615 . So I think it's better to handle that in a new PR. I'll revert this part.

@@ -21,7 +21,7 @@ services:
- "3000:3000"
environment:
PORT: 3000
PG_DATABASE_URL: postgres://${PGUSER_SUPERUSER:-postgres}:${PGPASSWORD_SUPERUSER:-postgres}@${PG_DATABASE_HOST:-db:5432}/default
PG_DATABASE_URL: "postgres://${PGUSER_SUPERUSER:-postgres}:${PGPASSWORD_SUPERUSER:-postgres}@${PG_DATABASE_HOST:-db:5432}/default"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(as per previous comment) I think ${PG_DATABASE_HOST:-db}:5432 would be best

Copy link
Member

@FelixMalfait FelixMalfait left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks a lot! I made a commit on your branch to ship to fix asap but open to discussion if you disagree!

@FelixMalfait FelixMalfait enabled auto-merge (squash) January 22, 2025 09:33
@@ -1,7 +1,7 @@
TAG=latest

#PGUSER_SUPERUSER=postgres
#PGPASSWORD_SUPERUSER=replace_me_with_a_strong_password
#PGPASSWORD_SUPERUSER=replace_me_with_a_url-safe_strong_password
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

case: url-safe => url_safe

  • to me "url safe" is not a standard wording, I googled it and got no result. I did not understand it upon reading the first time
    url escaped? not sure it's super clear either

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

More specifically these characters must be avoided: % @ ! # $ &
URL-Escaped (ie: @ -> %40) also doesn't work, which is surprising to me, but perhaps a deeper issue.
The phrase "without special characters" seems like a reasonable fix.

# Issue with Postgres spilo?
#echo "" >>.env
#echo "PGPASSWORD_SUPERUSER=$(openssl rand -hex 16)" >>.env
echo "" >>.env
Copy link
Member

@charlesBochet charlesBochet Jan 22, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

>> .env

#echo "" >>.env
#echo "PGPASSWORD_SUPERUSER=$(openssl rand -hex 16)" >>.env
echo "" >>.env
echo "PGPASSWORD_SUPERUSER=$(openssl rand -hex 16)" >>.env
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same here :)

@@ -65,7 +65,7 @@ Follow these steps for a manual setup.

4. **Set the Postgres Password**

Update the `PGPASSWORD_SUPERUSER` value in the .env file with a strong password.
Update the `PGPASSWORD_SUPERUSER` value in the .env file with a strong password that is URL-safe.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same note here on "URL-safe" wording

Copy link
Member

@charlesBochet charlesBochet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @john-s4d! I have left a few comments too

@FelixMalfait FelixMalfait disabled auto-merge January 22, 2025 10:00
@FelixMalfait FelixMalfait enabled auto-merge (squash) January 22, 2025 10:10
auto-merge was automatically disabled January 22, 2025 14:36

Head branch was pushed to by a user without write access

@FelixMalfait FelixMalfait merged commit 25cb909 into twentyhq:main Jan 24, 2025
30 checks passed
DeepaPrasanna pushed a commit to DeepaPrasanna/twenty that referenced this pull request Jan 27, 2025
This PR updates the docker-compose installation documentation and env
sample to improve the setup experience.

- Updates the URLs for raw files to reference main branch, which is
likely where new users will be pulling from initially. This seems to be
the most straightforward option; assume that advanced users who want to
retrieve it from a particular tag will know to change the URL for their
scenario.
- Fixes an improperly stated curl command.
- Adds a note that the PGPASSWORD_SUPERUSER should be URL-safe. This is
required since the value is later concat into a PG_DATABASE_URL as a
URL, and expected to be in proper URL format. Touches on twentyhq#8597.

---------

Co-authored-by: Félix Malfait <[email protected]>
Co-authored-by: Félix Malfait <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants