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

Better support local dev - local DB documentation & env vars #456

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

draknor
Copy link

@draknor draknor commented Jul 10, 2024

Update README to better document db setup for local dev, and update database.yml to support local username+password.

NOTE: This does change the default username + password for local (non-Docker) dev, so there's a potential compatibility issue there. Not sure if there's a good way around it?

…atabase.yml to support local username+password
@draknor
Copy link
Author

draknor commented Jul 10, 2024

Ha, I guess I could just NOT set defaults for username + password - seems so obvious in hindsight! I'll have to test that - I haven't setup my local dev DB to allow anonymous access.

@krschacht
Copy link
Contributor

krschacht commented Jul 10, 2024 via email

@krschacht
Copy link
Contributor

@draknor let me know what you think of just setting DATBASE_URL instead of this? The format is postgres://username:password@localhost/hostedgpt_development

(Password and/or username can be left off)

This is a rails convention and the production configs use this to set the database details.

@draknor
Copy link
Author

draknor commented Jul 11, 2024

Yep - I revised my approach to just focus on DATABASE_URL -- how's the new commit look?

@krschacht
Copy link
Contributor

krschacht commented Jul 11, 2024

@draknor Did you need to change anything in database.yml in order to get it to work? I could be wrong, but I thought that DATABASE_URL would just work as is. I think all we need to do is elaborate within the README

Oh, actually, I see a mistake in docker-compose.yml, on line 27 there is DATABASE_URL but I do not think we should be including a default value in there. If we just list DATABASE_URL with nothing afterwards then it will pass in whatever is set from the user's environment.

So, to summarize: (1) can you try reverting this docker-compose default, (2) confirm you can set DATABASE_URL locally and things work w/o any changes to database.yml, and (3) review my proposed edit of the README.

README.md Outdated
Comment on lines 231 to 235
- Postgres ([installation instructions](https://www.postgresql.org/download/))
- Use environment variable `DATABASE_URL` to specify the connection parameters
- *Make sure to set that environment variable in your setup!*
- Recommended (development): `DATABASE_URL=postgres://app:secret@localhost/hostedgpt_development`
- Recommended (test): `DATABASE_URL=postgres://app:secret@localhost/hostedgpt_test`
Copy link
Contributor

@krschacht krschacht Jul 11, 2024

Choose a reason for hiding this comment

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

Suggested change
- Postgres ([installation instructions](https://www.postgresql.org/download/))
- Use environment variable `DATABASE_URL` to specify the connection parameters
- *Make sure to set that environment variable in your setup!*
- Recommended (development): `DATABASE_URL=postgres://app:secret@localhost/hostedgpt_development`
- Recommended (test): `DATABASE_URL=postgres://app:secret@localhost/hostedgpt_test`
- Postgres (`brew install imagemagick` or other [install instructions](https://www.postgresql.org/download/))
- By default postgres will create a default user and following the instructions below to run the app should just work without any additional config, but if you have set a specific username and password for your database then set the environment `DATABASE_URL=postgres://username:password@localhost/hostedgpt_development` (replacing username & password with your values, optionally you can change the database name)

Copy link
Author

Choose a reason for hiding this comment

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

@krschacht I changed database.yml to be more explicit that we're using DATABASE_URL, per the recommendation in the Rails Guide 3.21 Connection Preference

(So yes - you are correct that DATABASE_URL works as-is; editing database.yml is just to reduce developer confusion).

New commit added!

Copy link
Contributor

Choose a reason for hiding this comment

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

@draknor Got it. Yes, you’re right, I think it’s good that we explicitly set url: as you’ve done. That makes it easier to discover. In fact, let’s even add a comment that gives an example of the format for the URL.

The thing I worry about is if any users have made use of the ENV which are being removed from database.yml, we might break their setup with this release. Reading the Rails Guide that you linked to, it sounds like details from DATABASE_URL will take precedence over any explicitly specified things. I’m just inclined to add back all the red removals from database.yml (the default host and the default database and what not).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants